Use [NonNullTypes] to interpret unannotated types in source #27807

jcouv · 2018-06-13T23:04:04Z

In a context where NonNullTypes is true, an unannotated type is interpreted as non-null. But in a context where it is false, then such type is interpreted as null-oblivious.
Annotated types (ie. with ?) are unaffected.

The main parts of this change

revive a property on Symbol to indicate the presence of the attribute (on symbol or containing symbol, with module symbols terminating the chain)
add a similar property in Binder (generally look for the containing symbol's context, except if a binding flag was set to break cycles)
the logic to copy modifiers by re-applying transforms now takes the NonNullTypes context into account
the [NonNullTypes] attribute is currently recognized syntactically, rather that properly bound. That is a temporary solution to avoid the numerous code cycles that arose. I'll work on breaking those cycles next.

What is not yet done:

add more verification of symbols
[NonNullTypes] targeting a return value or a parameter (will be a follow-up PR)
add more verification of emitted metadata (for those transformed) (will be a follow-up PR)
I intend to unskip some more existing tests related to this attribute (will be a follow-up PR)
there's one difficult cycle to break in SourceNamedTypeSymbol.MakeOneDeclaredBases which I will investigate and fix in a separate PR
this PR focuses on the NonNullTypes attribute in source. The handling of this attribute in metadata will be a follow-up PR

jcouv · 2018-06-13T23:14:05Z

src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs

@@ -803,7 +813,7 @@ private NamespaceOrTypeSymbolWithAnnotations UnwrapAlias(NamespaceOrTypeOrAliasS
            if (symbol.IsAlias)
            {
                AliasSymbol discarded;
-                return NamespaceOrTypeSymbolWithAnnotations.Create(Compilation, (NamespaceOrTypeSymbol)UnwrapAlias(symbol.Symbol, out discarded, diagnostics, syntax, basesBeingResolved));
+                return NamespaceOrTypeSymbolWithAnnotations.CreateNonNull(NonNullTypes, (NamespaceOrTypeSymbol)UnwrapAlias(symbol.Symbol, out discarded, diagnostics, syntax, basesBeingResolved));


📝 I will check whether this and the similar change below are covered by test. #Resolved

jcouv · 2018-06-13T23:16:17Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -588,6 +593,11 @@ private bool IsByRefTarget(int slot)
            return false;
        }

+        private void ReportWarningW(SyntaxNode syntax)
+        {
+            ReportStaticNullCheckingDiagnostics(ErrorCode.WRN_ConvertingNullableToNonNullable, syntax);


ReportStaticNullCheckingDiagnostics [](start = 12, length = 35)

📝 This makes it easy to set a breakpoint to catch all W warnings. #Closed

jcouv · 2018-06-14T05:01:49Z

@AlekseyTs @cston I've run into some more non-trivial cycles. It got me thinking, does binding really need to care about merging the information from [NonNullTypes] and from annotations? What if we kept the two separate during initial binding?

On one side, we'd have the [NonNullTypes] information (which mostly comes from attributes, but they are regular well-known attributes if they are not used elsewhere in binding).
On the other side, we'd have the information about "annotated vs. unannotated" (true/false).
To decide if a type is non-null or null-oblivious, you need to put the two pieces of information together (NonNullTypes determines the meaning of unannotated). #Closed

cston · 2018-06-14T13:57:24Z

                AttributeTargets.Struct,

AttributeTargets.All #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/StaticNullChecking.cs:932 in e83865f. [](commit_id = e83865f, deletion_comment = False)

cston · 2018-06-14T13:59:41Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -588,6 +593,11 @@ private bool IsByRefTarget(int slot)
            return false;
        }

+        private void ReportWarningW(SyntaxNode syntax)


ReportWarningW [](start = 21, length = 14)

Consider naming ReportWWarning. #Resolved

cston · 2018-06-14T14:33:10Z

        s /*T:string*/ .ToString(); // PROTOTYPE(NullableReferenceTypes): Expecting a string! state

Should be string~. #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/StaticNullChecking.cs:1699 in e83865f. [](commit_id = e83865f, deletion_comment = False)

cston · 2018-06-14T19:22:35Z

public struct C<T, NT> where T : S

Type parameter is not used here or in D<,>. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/StaticNullChecking.cs:2174 in e83865f. [](commit_id = e83865f, deletion_comment = False)

cston · 2018-06-14T19:26:06Z

        compilation.VerifyDiagnostics();

Consider checking the [NonNullTypes] attributes had an effect. Also consider adding delegate tests with ?. #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/StaticNullChecking.cs:2240 in e83865f. [](commit_id = e83865f, deletion_comment = False)

cston · 2018-06-14T19:27:58Z

        compilation.VerifyDiagnostics();

Consider testing C(nullableField) and D(nullableField) so it clear the [NonNullTypes] attributes had an effect. #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/StaticNullChecking.cs:2277 in e83865f. [](commit_id = e83865f, deletion_comment = False)

cston · 2018-06-14T21:07:46Z

        comp.VerifyDiagnostics();

There are several tests where there is only [NonNullTypes(false)] but no [NonNullTypes(true)] and where no diagnostics are expected. Will we know to update these tests if and when the default is switched to [NonNullTypes(false)]? #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/StaticNullChecking.cs:3489 in e83865f. [](commit_id = e83865f, deletion_comment = False)

cston · 2018-06-14T21:23:33Z

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

            Debug.Assert(!info.HasValue || info.SignatureIndex == 0);

            if (info.HasValue && TryExtractValueFromAttribute(info.Handle, out value, s_attributeBooleanValueExtractor))
            {
                return true;
            }

-            value = false;
+            value = default;


default [](start = 20, length = 7)

Consider using false instead. #Closed

cston · 2018-06-14T21:33:18Z

src/Compilers/Core/Portable/Symbols/Attributes/CommonFieldWellKnownAttributeData.cs

+        #region NonNullTypesAttribute
+
+        private bool? _nonNullTypes;
+        public bool? NonNullTypes


public bool? NonNullTypes [](start = 8, length = 25)

Should these NonNullTypes properties be on C# attribute data types only: FieldEarlyWellKnownAttributeData, etc.? #Closed

cston · 2018-06-14T21:37:15Z

src/Compilers/CSharp/Portable/Binder/BinderFlags.cs

@@ -8,7 +8,7 @@ namespace Microsoft.CodeAnalysis.CSharp
    /// A specific location for binding.
    /// </summary>
    [Flags]
-    internal enum BinderFlags : uint
+    internal enum BinderFlags : ulong


ulong [](start = 32, length = 5)

Is this change needed? #Resolved

I have to say I was surprised by that too (I thought I'd be using the last bit, but not going over). The error I got on 1 << 31 is: Error CS0031 Constant value '-2147483648' cannot be converted to a 'uint'

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

Try 1u << 31

In reply to: 195607839 [](ancestors = 195607839,195579166)

cston · 2018-06-14T22:12:23Z

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

@@ -670,6 +681,13 @@ public override TypeSymbolWithAnnotations AsNotNullableReferenceType()
                    new NonLazyType(_typeSymbol, isNullable: false, _customModifiers);
            }

+            public override TypeSymbolWithAnnotations AsObliviousReferenceType()
+            {


{ [](start = 12, length = 1)

Debug.Assert(_isNullable != true); #Resolved

cston · 2018-06-14T22:18:09Z

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

        {
            get
            {
+                Debug.Assert(false); // PROTOTYPE(NullableReferenceTypes): Can we reach this?


// PROTOTYPE(NullableReferenceTypes): Can we reach this? [](start = 37, length = 56)

Perhaps: F([NotNullTypes]List<object?> arg = default(List<object>)) #Resolved

I still didn't manage to reach this. I'll keep PROTOTYPE for a follow-up.

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

cston · 2018-06-14T22:19:45Z

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

@@ -832,13 +832,15 @@ public virtual bool HasUnsupportedMetadata
        }

        /// <summary>
-        /// Is module/type/method/field/property/event/parameter definition opted out of nullable warnings. Not valid to call on non-definitions.
+        /// Is module/type/method/field/property/event/parameter definition opted-in/out of treating un-annotated types as non-null.
+        /// This is determined by the presence of the `[NonNullTypes]` attribute and whether the containing module utilizes nullable reference types.


and whether the containing module utilizes nullable reference types [](start = 81, length = 67)

Isn't this also [NonNullTypes]? #Resolved

There is also defaulting logic on modules (currently based on language version). I'll update comment.

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

cston · 2018-06-14T22:21:16Z

...ers/CSharp/Portable/Symbols/Attributes/WellKnownAttributeData/EventWellKnownAttributeData.cs

@@ -6,25 +6,25 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols
 {
    internal sealed class EventWellKnownAttributeData : CommonEventWellKnownAttributeData
    {
-        #region NullableOptOutAttribute
+        #region NonNullTypesAttribute


NonNullTypesAttribute [](start = 16, length = 21)

Seems surprising that some AttributeData cases are early, others are not. #Closed

cston · 2018-06-14T22:28:22Z

...rs/CSharp/Portable/Symbols/Attributes/WellKnownAttributeData/MethodWellKnownAttributeData.cs

@@ -6,25 +6,25 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols
 {
    internal sealed class MethodWellKnownAttributeData : CommonMethodWellKnownAttributeData
    {
-        #region NullableOptOutAttribute
+        #region NonNullTypesAttribute


NonNullTypesAttribute [](start = 16, length = 21)

This is also on CommonMethodEarlyWellKnownAttributeData. Is that correct? #Closed

cston · 2018-06-14T22:35:31Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs

@@ -28,6 +28,7 @@ internal sealed class SourceOrdinaryMethodSymbol : SourceMemberMethodSymbol
        private ImmutableArray<ParameterSymbol> _lazyParameters;
        private TypeSymbolWithAnnotations _lazyReturnType;
        private bool _lazyIsVararg;
+        private bool? _nonNullTypesFromAttributes;


_nonNullTypesFromAttributes [](start = 22, length = 27)

_lazyNonNullTypesFromAttributes #Closed

cston · 2018-06-26T18:53:15Z

    [Fact(Skip = "NonNullTypes does not control warnings")] // PROTOTYPE(NullableReferenceTypes): Update or remove test.

Let's remove these tests that expect the attribute to control warnings. Perhaps in a separate PR. #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/StaticNullChecking.cs:24755 in 31429cd. [](commit_id = 31429cd88ba99d331ca9c3b96119f33e26461979, deletion_comment = False)

jcouv · 2018-06-26T19:48:16Z

    [Fact(Skip = "NonNullTypes does not control warnings")] // PROTOTYPE(NullableReferenceTypes): Update or remove test.

I'm expecting to each unskip or remove them, in a follow-up PR.

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/StaticNullChecking.cs:24755 in 31429cd. [](commit_id = 31429cd88ba99d331ca9c3b96119f33e26461979, deletion_comment = False)

jcouv · 2018-06-26T23:39:40Z

@dotnet/roslyn-compiler for a second review. Thanks

jcouv · 2018-06-26T23:41:16Z

        compilation.VerifyDiagnostics();

@cston From our discussion, I'm disallowing [NonNullTypes] on return value and parameters.

In reply to: 399260480 [](ancestors = 399260480,397410688)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/StaticNullChecking.cs:2240 in e83865f. [](commit_id = e83865f, deletion_comment = False)

jcouv · 2018-06-28T16:50:48Z

@333fred for a second review when you get the chance. Thanks #Resolved

333fred · 2018-06-28T21:09:32Z

docs/features/nullable-reference-types.md

-    [AttributeUsage(
-        AttributeTargets.All,
-        AllowMultiple = false)]
+    [AttributeUsage(AttributeTargets.Class |


Why not assembly? #Resolved

From our discussion, I added a note to work items (module: target is indeed unfamiliar to developers). #Resolved

333fred · 2018-06-28T21:35:02Z

src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs

-                    receiverArgument = CreateConversion(receiver, methodResult.Result.ConversionForArg(0), 
-                                                                     GetTypeOrReturnTypeWithAdjustedNullableAnnotations(receiverParameter).TypeSymbol, 
+                    receiverArgument = CreateConversion(receiver, methodResult.Result.ConversionForArg(0),
+                                                                     receiverParameter.Type.TypeSymbol,


[](start = 56, length = 13)

Nit: consider reformatting to have consistent indentation with the previous line. Looks like this is a victim of past renames.

333fred

LGTM. One small formatting nits.

jcouv · 2018-10-03T05:37:12Z

src/Compilers/CSharp/Portable/Symbols/Source/FieldSymbolWithAttributesAndModifiers.cs

+            get
+            {
+                // PROTOTYPE(NullableReferenceTypes): temporary solution to avoid cycle
+                return SyntaxBasedNonNullTypes(this.AttributeDeclarationSyntaxList) ?? base.NonNullTypes; 


@AlekseyTs In case this helps, this is the approach we had previously used for syntax-based detection of NonNullTypes.
The specific SyntaxBasedNonNullTypes in this PR was very inefficient, but we could use that model and make the check cheaper (ie. avoid .ToString()).

jcouv added Area-Compilers New Language Feature - Nullable Reference Types Nullable Reference Types labels Jun 13, 2018

jcouv added this to the 16.0 milestone Jun 13, 2018

jcouv self-assigned this Jun 13, 2018

jcouv requested a review from a team as a code owner June 13, 2018 23:04

jcouv commented Jun 13, 2018

View reviewed changes

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jun 14, 2018

cston reviewed Jun 14, 2018

View reviewed changes

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jun 14, 2018

cston reviewed Jun 14, 2018

View reviewed changes

jcouv force-pushed the urtann branch 2 times, most recently from 4accd68 to ba396d5 Compare June 15, 2018 05:56

jcouv requested a review from a team as a code owner June 15, 2018 05:56

jcouv force-pushed the urtann branch from 538124b to 3417d47 Compare June 16, 2018 03:06

cston approved these changes Jun 26, 2018

View reviewed changes

jcouv force-pushed the urtann branch from 47b0756 to 93c0833 Compare June 26, 2018 23:40

333fred reviewed Jun 28, 2018

View reviewed changes

333fred approved these changes Jun 28, 2018

View reviewed changes

jcouv and others added 16 commits June 28, 2018 19:40

Rename to NonNullTypes and fix one scenario

b791bfa

Propagate change and fix cycles

d6c2c93

Cleanup from self-review

6843fab

Add direct tests for NonNullTypes and unblock some cycles

dad4fba

Address PR feedback and shortcut some cycles

59435b3

Bypasses cycles by recognizing NonNullTypes syntactically

3fd345d

Remove assertion

df3af4d

Address PR feedback (part 1)

5cabb24

Address PR feedback (part 2)

e372cf0

Remove GetTypeOrReturnTypeWithAdjustedNullableAnnotations

6f8f3c9

Remove unused binder flags

7b04c23

Remove GetFieldTypeWithAdjustedNullableAnnotations

872cf98

Address PR feedback from Chuck

d44684c

Remove some attribute data types

edbc970

NonNullTypes attribute should not target parameter or return value

45d945b

Indent

6f2cd49

jcouv force-pushed the urtann branch from 6acdcbd to 6f2cd49 Compare June 29, 2018 02:43

jcouv merged commit 45c76ee into dotnet:features/NullableReferenceTypes Jun 29, 2018

jcouv deleted the urtann branch June 29, 2018 03:48

jcouv commented Oct 3, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use [NonNullTypes] to interpret unannotated types in source #27807

Use [NonNullTypes] to interpret unannotated types in source #27807

jcouv commented Jun 13, 2018 •

edited

Loading

jcouv Jun 13, 2018 •

edited

Loading

jcouv Jun 13, 2018 •

edited

Loading

jcouv commented Jun 14, 2018 •

edited

Loading

cston commented Jun 14, 2018 •

edited by jcouv

Loading

cston Jun 14, 2018 •

edited by jcouv

Loading

cston commented Jun 14, 2018 •

edited by jcouv

Loading

cston commented Jun 14, 2018 •

edited by jcouv

Loading

cston commented Jun 14, 2018 •

edited by jcouv

Loading

cston commented Jun 14, 2018 •

edited by jcouv

Loading

cston commented Jun 14, 2018 •

edited by jcouv

Loading

cston Jun 14, 2018 •

edited by jcouv

Loading

cston Jun 14, 2018 •

edited by jcouv

Loading

cston Jun 14, 2018 •

edited by jcouv

Loading

jcouv Jun 15, 2018

cston Jun 15, 2018

cston Jun 14, 2018 •

edited by jcouv

Loading

cston Jun 14, 2018 •

edited by jcouv

Loading

jcouv Jun 21, 2018

cston Jun 14, 2018 •

edited by jcouv

Loading

jcouv Jun 15, 2018

cston Jun 14, 2018 •

edited by jcouv

Loading

cston Jun 14, 2018 •

edited by jcouv

Loading

cston Jun 14, 2018 •

edited by jcouv

Loading

cston commented Jun 26, 2018 •

edited by jcouv

Loading

jcouv commented Jun 26, 2018

jcouv commented Jun 26, 2018

jcouv commented Jun 26, 2018

jcouv commented Jun 28, 2018 •

edited

Loading

333fred Jun 28, 2018 •

edited by jcouv

Loading

jcouv Jun 28, 2018 •

edited

Loading

333fred Jun 28, 2018

333fred left a comment

jcouv Oct 3, 2018

Use [NonNullTypes] to interpret unannotated types in source #27807

Use [NonNullTypes] to interpret unannotated types in source #27807

Conversation

jcouv commented Jun 13, 2018 • edited Loading

jcouv Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv commented Jun 14, 2018 • edited Loading

cston commented Jun 14, 2018 • edited by jcouv Loading

cston Jun 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston commented Jun 14, 2018 • edited by jcouv Loading

cston commented Jun 14, 2018 • edited by jcouv Loading

cston commented Jun 14, 2018 • edited by jcouv Loading

cston commented Jun 14, 2018 • edited by jcouv Loading

cston commented Jun 14, 2018 • edited by jcouv Loading

cston Jun 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Jun 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Jun 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Jun 15, 2018

Choose a reason for hiding this comment

cston Jun 15, 2018

Choose a reason for hiding this comment

cston Jun 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Jun 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Jun 21, 2018

Choose a reason for hiding this comment

cston Jun 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Jun 15, 2018

Choose a reason for hiding this comment

cston Jun 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Jun 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Jun 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston commented Jun 26, 2018 • edited by jcouv Loading

jcouv commented Jun 26, 2018

jcouv commented Jun 26, 2018

jcouv commented Jun 26, 2018

jcouv commented Jun 28, 2018 • edited Loading

333fred Jun 28, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

333fred Jun 28, 2018

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

jcouv Oct 3, 2018

Choose a reason for hiding this comment

jcouv commented Jun 13, 2018 •

edited

Loading

jcouv Jun 13, 2018 •

edited

Loading

jcouv Jun 13, 2018 •

edited

Loading

jcouv commented Jun 14, 2018 •

edited

Loading

cston commented Jun 14, 2018 •

edited by jcouv

Loading

cston Jun 14, 2018 •

edited by jcouv

Loading

cston commented Jun 14, 2018 •

edited by jcouv

Loading

cston commented Jun 14, 2018 •

edited by jcouv

Loading

cston commented Jun 14, 2018 •

edited by jcouv

Loading

cston commented Jun 14, 2018 •

edited by jcouv

Loading

cston commented Jun 14, 2018 •

edited by jcouv

Loading

cston Jun 14, 2018 •

edited by jcouv

Loading

cston Jun 14, 2018 •

edited by jcouv

Loading

cston Jun 14, 2018 •

edited by jcouv

Loading

cston Jun 14, 2018 •

edited by jcouv

Loading

cston Jun 14, 2018 •

edited by jcouv

Loading

cston Jun 14, 2018 •

edited by jcouv

Loading

cston Jun 14, 2018 •

edited by jcouv

Loading

cston Jun 14, 2018 •

edited by jcouv

Loading

cston Jun 14, 2018 •

edited by jcouv

Loading

cston commented Jun 26, 2018 •

edited by jcouv

Loading

jcouv commented Jun 28, 2018 •

edited

Loading

333fred Jun 28, 2018 •

edited by jcouv

Loading

jcouv Jun 28, 2018 •

edited

Loading