Do not include value types in NullableAttribute byte[] by cston · Pull Request #36519 · dotnet/roslyn

cston · 2019-06-17T22:41:44Z

Do not include a 0 entry in the byte[] for non-generic value types. The 0 is still included for generic value types, tuples, and type parameters constrained to value types.

cston · 2019-06-18T15:29:27Z

@dotnet/roslyn-compiler please review.

AlekseyTs · 2019-06-18T17:16:17Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

        private NamedTypeSymbol _lazyBaseType = ErrorTypeSymbol.UnknownResultType;
        private ImmutableArray<NamedTypeSymbol> _lazyInterfaces = default(ImmutableArray<NamedTypeSymbol>);
-        private NamedTypeSymbol _lazyDeclaredBaseType = ErrorTypeSymbol.UnknownResultType;
+        private NamedTypeSymbol _lazyDeclaredBaseTypeWithoutNullability = ErrorTypeSymbol.UnknownResultType;


private NamedTypeSymbol _lazyDeclaredBaseTypeWithoutNullability = ErrorTypeSymbol.UnknownResultType; [](start = 8, length = 100)

It doesn't look like adding this cache is necessary. Inside TypeKind.get we could check if we already have _lazyDeclaredBaseType and use that, otherwise we could resolve the type from metadata without applying any transforms, including tuple and dynamic, and use that type without caching it. #Closed

Even better might be to bundle resolution of base type with calculating type kind for non-interfaces. Before we apply any transforms to the base type, calculate and set the type kind, then proceed with transforms.

In reply to: 294935242 [](ancestors = 294935242)

jcouv · 2019-06-18T17:36:55Z

docs/features/nullable-metadata.md

 - Nullable value type: the representation of the type argument only
- Value type: the representation of the type arguments in order including containing types
+- Non-generic value type: skipped
+- Generic value type: 0, followed by the representation of the type arguments in order including containing types


0, [](start = 21, length = 4)

just curious: why do we want to encode an annotation for a value type? Is this due to a concern for cycles when loading metadata? (if so, may be good to record rationale) #Resolved

Yes, the reason that generic value types (and type parameters constrained to struct) include a 0 byte is to avoid cycles when loading types from metadata - in particular, when loading type parameters with constraint types. Simple (non-generic) value types are less of a concern.

I'll add a note to the document.

In reply to: 294943765 [](ancestors = 294943765)

AlekseyTs · 2019-06-18T17:45:07Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Nullable.cs

+            if (!expectedPreviously.SequenceEqual(expectedNow))
+            {
+                position = 0;
+                Assert.True(underlyingType.ApplyNullableTransforms(0, ImmutableArray.Create(expectedPreviously), ref position, out updated) || position < expectedPreviously.Length);


underlyingType.ApplyNullableTransforms(0, ImmutableArray.Create(expectedPreviously), ref position, out updated) [](start = 28, length = 111)

Should this condition be negated? #Closed

Or perhaps removed?

In reply to: 294947325 [](ancestors = 294947325)

AlekseyTs · 2019-06-18T17:51:28Z

src/Compilers/CSharp/Portable/Symbols/TypeWithAnnotations.cs

+                return type.IsNullableType();
            }
-            else
+            return type.IsValueType && !type.IsTupleType;


type.IsValueType [](start = 19, length = 16)

Are we testing scenarios when concrete value type references within another type cannot be resolved from metadata (an assembly reference is missing, for example)? It looks like this might cause a confusion during ApplyNullableTransforms causing us to apply flags to wrong places.

AlekseyTs · 2019-06-18T17:52:34Z

Done with review pass (iteration 4) #Closed

jcouv · 2019-06-18T18:01:37Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Nullable.cs

+                VerifyBytes(globalNamespace.GetMember<FieldSymbol>("Program.F25").TypeWithAnnotations, new byte[] { 0, 1 }, new byte[] { 0, 1 }, "S<U!>");
+                VerifyBytes(globalNamespace.GetMember<FieldSymbol>("Program.F31").TypeWithAnnotations, new byte[] { 0 }, new byte[] { 0 }, "V");
+                VerifyBytes(globalNamespace.GetMember<FieldSymbol>("Program.F32").TypeWithAnnotations, new byte[] { 0, 0 }, new byte[] { 0 }, "V?");
+                VerifyBytes(globalNamespace.GetMember<FieldSymbol>("Program.F33").TypeWithAnnotations, new byte[] { 1, 0 }, new byte[] { 1, 0 }, "V[]!");


new byte[] { 1, 0 } [](start = 103, length = 19)

nit: consider using null for expectedPreviously when it is equals to expectedNow, as the actual value will be ignored.

jcouv

LGTM modulo Aleksey's feedback on removing cache. Thanks (iteration 4)

AlekseyTs · 2019-06-18T21:26:06Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

+                    {
+                        // Decoding nullability might lead to a cycle calculating the base type.
+                        // Instead, return early without updating the field.
+                        return baseType;


return baseType; [](start = 24, length = 16)

No need to do dynamic and tuple transforms for this code path. #Closed

AlekseyTs · 2019-06-19T16:49:23Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Nullable.cs

+        }
+
+        [Fact]
+        public void EmitAttribute_ValueTypes_08()


EmitAttribute_ValueTypes_08 [](start = 20, length = 27)

I don't think this test covers the error scenario that I was interested in. I was thinking about the tests that deals with types coming from metadata, this test verifies types coming from source. #Resolved

AlekseyTs

LGTM (iteration 7), the tests can be followed up on in a separate PR.

* dotnet/master: (85 commits) Don't complete statement when typing semicolon inside comments in an argument list (dotnet#36521) Set focus to editor before finding text Add a bunch of nullability support to some code generation helpers Add 'annotations' and 'warnings' support to nullable directive (dotnet#36464) Fixed IDE services touching `notnull` constraint (dotnet#36508) Update compiler toolset to arcade version (dotnet#36549) Fix for 923157 Do not include value types in NullableAttribute byte[] (dotnet#36519) Fix a deadlock in InvokeOnUIThread Apply a hang mitigating timeout to UI thread operations Move to a different lowering from for nullable value types to work around a bug in TransformCompoundAssignmentLHS. Addressed PR feedback. Fix stack overflow in requesting syntax directives (dotnet#36347) crash on ClassifyUpdate for EventFields (dotnet#35962) fixing bad merge with refactoring that was checked in later added basic completion statement telemetry Remove duplication in AbstractSymbolCompletionProvider.CreateItems Disable move type when the options service isn't present (dotnet#36334) Fix crash where type inference doing method inference needs to drop nullability Revert "Use IVsSolution to look up IVsHierarchy by project GUID (dotnet#35746)" Fix parsing bug in invalid using statements (dotnet#36428) ...

cston added 2 commits June 17, 2019 15:38

Do not include value types in NullableAttribute byte[]

e85c50b

Revert 3dc7655

797af4d

cston requested a review from a team as a code owner June 17, 2019 22:41

Update nullable-metadata.md

8b55cf9

jcouv added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jun 18, 2019

jcouv self-assigned this Jun 18, 2019

jcouv added this to the 16.2.P4 milestone Jun 18, 2019

Add test for System.Void

a059981

AlekseyTs reviewed Jun 18, 2019

View reviewed changes

jcouv reviewed Jun 18, 2019

View reviewed changes

AlekseyTs reviewed Jun 18, 2019

View reviewed changes

jcouv reviewed Jun 18, 2019

View reviewed changes

jcouv approved these changes Jun 18, 2019

View reviewed changes

jaredpar mentioned this pull request Jun 18, 2019

Nullable Reference Type Changes #35816

Closed

23 tasks

cston added 2 commits June 18, 2019 13:31

Revert revert

f0946c2

Ignore nullability in PENamedTypeSymbol.TypeKind

d0009ba

AlekseyTs reviewed Jun 18, 2019

View reviewed changes

PR feedback

ffc6a39

AlekseyTs reviewed Jun 19, 2019

View reviewed changes

AlekseyTs approved these changes Jun 19, 2019

View reviewed changes

cston merged commit 055ef1c into dotnet:master Jun 19, 2019

cston deleted the skip-value-type branch June 19, 2019 17:06

cston mentioned this pull request Jun 25, 2019

Reduce the impact of nullable attributes in metadata #35888

Closed

Conversation

cston commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Jun 18, 2019

Uh oh!

AlekseyTs Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 18, 2019 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 19, 2019 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cston commented Jun 17, 2019 •

edited

Loading

AlekseyTs Jun 18, 2019 •

edited

Loading

jcouv Jun 18, 2019 •

edited by cston

Loading

AlekseyTs Jun 18, 2019 •

edited

Loading

AlekseyTs Jun 18, 2019 •

edited

Loading

AlekseyTs commented Jun 18, 2019 •

edited

Loading

AlekseyTs Jun 18, 2019 •

edited

Loading

AlekseyTs Jun 19, 2019 •

edited by cston

Loading