Adjust the way nullable annotations are represented in metadata by AlekseyTs · Pull Request #31212 · dotnet/roslyn

AlekseyTs · 2018-11-16T00:49:11Z

Closes #30075.
Closes #30065.
Closes #29594.

Closes dotnet#30075. Closes dotnet#30065. Closes dotnet#29594.

…nsInMetadata

AlekseyTs · 2018-11-16T17:41:48Z

jcouv · 2018-11-16T21:52:22Z

docs/features/nullable-reference-types.md

@@ -22,54 +22,43 @@ namespace System.Runtime.CompilerServices
        AllowMultiple = false)]
    public sealed class NullableAttribute : Attribute


NullableAttribute [](start = 24, length = 17)

Not blocking this PR: we'll need to think about how the values of the parameters will be stored in the fields of NullableAttribute. Jared mentioned we need to store those values so that they are accessible from reflection.

Tracked by #30143 #Resolved

jcouv · 2018-11-16T22:18:54Z

docs/features/nullable-reference-types.md

+
+NullableAttribute(1) should be placed on a type parameter definition that has a `class!` constraint.
+NullableAttribute(2) should be placed on a type parameter definition that has a `class?` constraint.
+Other forms of NullableAttribute ar not emitted on type parameter definitions and are not specially recognized on them.


ar [](start = 33, length = 2)

typo: are #Resolved

jcouv · 2018-11-16T22:21:56Z

docs/features/nullable-reference-types.md

 string OptString; // string?
-[Nullable(new[] { true, false, true })]
+[Nullable(new[] { 2, 1, 2 })]
 Dictionary<string, object> OptDictionaryOptValues; // Dictionary<string!, object?>?


object> [](start = 19, length = 7)

~~Should this be Dictionary<..., object?>? in source as well?~~ Never mind. The type here is from PE. #Closed

jcouv · 2018-11-16T22:35:37Z

src/Compilers/Core/Portable/MetadataReader/PEModule.cs

            AttributeInfo info = FindTargetAttribute(token, AttributeDescription.NullableAttribute);
            Debug.Assert(!info.HasValue || info.SignatureIndex == 0 || info.SignatureIndex == 1);

+            defaultTransform = 0;


0; [](start = 31, length = 2)

Consider using another value to represent "not a value" and only set to 0 at line 2437 (from TryExtractValueFromAttribute). #Resolved

Consider using another value to represent "not a value" and only set to 0 at line 2437 (from TryExtractValueFromAttribute).

I see no benefit in inventing new "defaults" when this value just works, this is a well established pattern for functions that return information via out parameters and return boolean value indicating success of the operation. Consumers that really depend on the fact whether the attribute was found and cracked successfully should simply check return value of this method.

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

gafter · 2018-11-17T00:33:56Z

docs/features/nullable-reference-types.md

 Dictionary<string, object?>? OptDictionaryOptValues; // dictionary may be null, values may be null
 ```
-A warning is reported when annotating a reference type or unconstrained generic type with `?` outside a `NonNullTypes(true)` context.
+A warning is reported when annotating a reference type or unconstrained generic type with `?` outside a `#nullable` context.


I thought it was an error to annotate an unconstrained generic type. #Resolved

cston · 2018-11-17T00:40:08Z

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

+                    break;
+
+                case NullableAnnotation.Unknown:
+                    Debug.Assert((NullableAnnotation)transformFlag == NullableAnnotation.Unknown);


Debug.Assert((NullableAnnotation)transformFlag == NullableAnnotation.Unknown); [](start = 20, length = 78)

The assert looks redundant. #Closed

cston · 2018-11-17T06:21:45Z

These checks are repeated many times. Please consider extracting a helper method: CheckReferenceTypeConstraint(t11, expectedValue: false);.

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:41929 in 60ce669. [](commit_id = 60ce669, deletion_comment = False)

jcouv · 2018-11-19T18:49:32Z

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

+
+                case NullableAnnotation.Unknown:
+                    Debug.Assert((NullableAnnotation)transformFlag == NullableAnnotation.Unknown);
+                    if (result.NullableAnnotation != NullableAnnotation.Unknown && (!result.NullableAnnotation.IsAnyNullable() || !oldTypeSymbol.IsNullableType()))


(!result.NullableAnnotation.IsAnyNullable() || !oldTypeSymbol.IsNullableType()) [](start = 83, length = 79)

nit: took me a while to understand this check. Inverting this last block helps: .. && !(result.NullableAnnotation.IsAnyNullable() && oldTypeSymbol.IsNullableType())
Maybe add a comment too: "// keep nullable annotation on Nullable<T> types" #Resolved

jcouv · 2018-11-19T18:52:16Z

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

-            var type = comp.GetWellKnownType(WellKnownType.Microsoft_CodeAnalysis_EmbeddedAttribute);
-            Assert.True(type.IsErrorType());
-
-            // https://github.com/dotnet/roslyn/issues/29683 CSharpCompilation.AbstractSymbolSearcher needs to inject namespaces and types too


#29683 [](start = 15, length = 45)

can be closed? #Resolved

can be closed?

I think it can be, I'll add to the list.

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

jcouv · 2018-11-19T18:54:37Z

    public void ExplicitAttribute_MissingParameterlessConstructor()

This test requires both constructors (because we have simple and mixed case of nullability). I assume we produce the same error even if we only need one constructor and it exists. #Resolved

Refers to: src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Nullable.cs:74 in 60ce669. [](commit_id = 60ce669, deletion_comment = False)

AlekseyTs · 2018-11-19T18:56:57Z

    public void ExplicitAttribute_MissingParameterlessConstructor()

I assume we produce the same error even if we only need one constructor and it exists.

Yes, this change didn't change the number of constructors and didn't change the way we complain about one of them is missing.

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

Refers to: src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Nullable.cs:74 in 60ce669. [](commit_id = 60ce669, deletion_comment = False)

jcouv · 2018-11-19T18:59:28Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs


            if (format.MiscellaneousOptions.IncludesOption(SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier) &&
-                !typeOpt.IsNullableType() &&
+                !typeOpt.IsNullableType() && !typeOpt.IsValueType &&


!typeOpt.IsValueType && [](start = 45, length = 23)

I didn't understand what's a scenario were we put a nullable annotation on a value type, but it's not Nullable<T>.
Is the when loading bad metadata? #Closed

I didn't understand what's a scenario were we put a nullable annotation on a value type, but it's not Nullable.
Is the when loading bad metadata?

On the contrary, binding from source does that, always did.

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

From our discussion, some of the weird combinations (int marked as nullable) only come from PE.
You mentioned that one reason for letting such combinations into the compiler is to avoid cycles. Do you remember what's a scenario?
In particular, I wonder why is that problem unique to nullability, but was ok for decoding dynamic or tuple names?

If it were possible, I still think it would be better for metadata import to ignore, rather than to propagate, bad metadata combinations. I'd expect all value types (except Nullable<T>) should be imported as oblivious. I believe that is what existing metadata import tends to do.

Along those lines, why encode a nullable annotation for value types at all? We don't encode a dynamic flag for types that are not object, and we don't encode tuple names for types that are not tuples.

In reply to: 234742698 [](ancestors = 234742698,234741474)

You mentioned that one reason for letting such combinations into the compiler is to avoid cycles. Do you remember what's a scenario?

For example, when we are loading type parameter constraints, we cannot ask questions whether the type parameter is a value type or a reference type

In particular, I wonder why is that problem unique to nullability, but was ok for decoding dynamic or tuple names?

In order to decode dynamic or tuple names we do not check whether targets are value or reference types either.

If it were possible, I still think it would be better for metadata import to ignore, rather than to propagate, bad metadata combinations. I'd expect all value types (except Nullable) should be imported as oblivious. I believe that is what existing metadata import tends to do.

I do not think this is something we should pursue. In any case, I think this PR doesn't change the way we import metadata in that specific regard, and it wasn't one of the goals to change this.

Along those lines, why encode a nullable annotation for value types at all? We don't encode a dynamic flag for types that are not object, and we don't encode tuple names for types that are not tuples.

Compiler always marks value types as oblivious in metadata, but we should be prepared to deal with malformed cases. This particular line of code makes sure that display doesn't confuse the reader that it deals with Nullable<T> case.

In reply to: 234772025 [](ancestors = 234772025,234742698,234741474)

jcouv · 2018-11-19T19:05:15Z

    [Fact]

Not related to a file in this PR: since we're undoing the injection of NNT attribute, two EnC tests can be re-enabled. #29662
Considering re-enabling along with this change #Resolved

Refers to: src/Workspaces/CoreTest/SymbolKeyTests.cs:68 in 60ce669. [](commit_id = 60ce669, deletion_comment = False)

jcouv · 2018-11-19T19:18:05Z

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

                            }
                        }

-                        // https://github.com/dotnet/roslyn/issues/30075: Test different [NonNullTypes] on method and containing type.


#30075 [](start = 27, length = 45)

Can be resolved? #Resolved

Can be resolved?

#30075 is already listed as addressed by this change.

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

jcouv

LGTM Thanks (iteration 4)

jcouv · 2018-11-19T20:37:30Z

docs/features/nullable-reference-types.md

+The `NullableAttribute` type declaration is synthesized by the compiler if it is not included in the compilation, but is needed to produce the output.
+
 ```c#
 // C# representation of metadata


It would be good to expand this section with an example involving value types (int and Nullable<int>). #Resolved

Closes dotnet#29683 Closes dotnet#29662

AlekseyTs added 4 commits November 14, 2018 21:38

Adjust the way nullable annotations are represented in metadata.

934f8da

Closes dotnet#30075. Closes dotnet#30065. Closes dotnet#29594.

Merge remote-tracking branch 'dotnet/dev16.0-preview2' into Annotatio…

68101a3

…nsInMetadata

Follow-up on merge from dev16.0-preview2

89ad74c

Tests, adjustments, speclet update.

60ce669

AlekseyTs added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Nov 16, 2018

AlekseyTs requested review from cston and jcouv November 16, 2018 00:49

AlekseyTs requested review from a team as code owners November 16, 2018 00:49

jcouv self-assigned this Nov 16, 2018

jcouv added this to the 16.0.P2 milestone Nov 16, 2018

jasonmalinowski changed the base branch from dev16.0-preview2 to master November 16, 2018 19:39

jcouv reviewed Nov 16, 2018

View reviewed changes

gafter reviewed Nov 17, 2018

View reviewed changes

cston reviewed Nov 17, 2018

View reviewed changes

cston approved these changes Nov 17, 2018

View reviewed changes

jcouv reviewed Nov 19, 2018

View reviewed changes

jcouv approved these changes Nov 19, 2018

View reviewed changes

jcouv reviewed Nov 19, 2018

View reviewed changes

AlekseyTs added 3 commits November 19, 2018 13:35

Merge remote-tracking branch 'dotnet/master' into AnnotationsInMetadata

6fa2447

Follow-up on merge from master

98df03c

PR feedback.

6160520

Closes dotnet#29683 Closes dotnet#29662

AlekseyTs merged commit 19b50a4 into dotnet:master Nov 19, 2018

cston mentioned this pull request Nov 28, 2018

[Work items] Nullable reference types #22152

Closed

		@@ -22,54 +22,43 @@ namespace System.Runtime.CompilerServices
		AllowMultiple = false)]
		public sealed class NullableAttribute : Attribute

Conversation

AlekseyTs commented Nov 16, 2018

Uh oh!

AlekseyTs commented Nov 16, 2018

Uh oh!

jcouv Nov 16, 2018 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 16, 2018 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 16, 2018 • 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!

jcouv Nov 16, 2018 • edited by AlekseyTs 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 Nov 16, 2018

Choose a reason for hiding this comment

Uh oh!

gafter Nov 17, 2018 • edited by AlekseyTs 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 Nov 17, 2018 • edited by AlekseyTs 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 commented Nov 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Nov 19, 2018 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 19, 2018 • edited by AlekseyTs 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 Nov 19, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv commented Nov 19, 2018 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Nov 19, 2018

Uh oh!

jcouv Nov 19, 2018 • 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 Nov 19, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 19, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 19, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv commented Nov 19, 2018 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Nov 19, 2018 • edited by AlekseyTs 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 Nov 19, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 19, 2018 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

jcouv Nov 16, 2018 •

edited by AlekseyTs

Loading

jcouv Nov 16, 2018 •

edited by AlekseyTs

Loading

jcouv Nov 16, 2018 •

edited

Loading

jcouv Nov 16, 2018 •

edited by AlekseyTs

Loading

gafter Nov 17, 2018 •

edited by AlekseyTs

Loading

cston Nov 17, 2018 •

edited by AlekseyTs

Loading

cston commented Nov 17, 2018 •

edited

Loading

jcouv Nov 19, 2018 •

edited by AlekseyTs

Loading

jcouv Nov 19, 2018 •

edited by AlekseyTs

Loading

jcouv commented Nov 19, 2018 •

edited by AlekseyTs

Loading

jcouv Nov 19, 2018 •

edited

Loading

jcouv commented Nov 19, 2018 •

edited by AlekseyTs

Loading

jcouv Nov 19, 2018 •

edited by AlekseyTs

Loading

jcouv Nov 19, 2018 •

edited by AlekseyTs

Loading