Enforce unconditional attributes within method bodies by jcouv · Pull Request #40789 · dotnet/roslyn

jcouv · 2020-01-07T23:01:17Z

Addresses part of #36039
MaybeNullWhen and NotNullWhen are treated unconditionally to produce fewer warnings.
DoesNotReturn and DoesNotReturnIf are not yet enforced.
OHI will come later.

Fixes #40139 ([DisallowNull] T parameter has not-null initial value)
Fixes #39922

Relates to discussion in LDM on Jan 6th 2020.

jcouv · 2020-01-13T20:31:55Z


-            if (_expressionIsRead)
-            {
-                ReportMaybeNullFromTypeParameterValueIfNeeded(node, resultType, memberAnnotations);


📝 removed empty method #Resolved

jcouv · 2020-01-13T20:32:50Z

@dotnet/roslyn-compiler @cston for review. Thanks

I'll add a note to breaking changes. #Closed

cston · 2020-01-13T22:31:42Z

        /// Note this does not work for nullable value types, so an additional check with <see cref="CheckDisallowedNullAssignment"/> may be required.
        /// </summary>
-        private static TypeWithAnnotations ApplyLValueAnnotations(TypeWithAnnotations declaredType, FlowAnalysisAnnotations flowAnalysisAnnotations)
+        internal static TypeWithAnnotations ApplyLValueAnnotations(TypeWithAnnotations declaredType, FlowAnalysisAnnotations flowAnalysisAnnotations)


internal [](start = 8, length = 8)

Is this change needed? #Closed

cston · 2020-01-13T22:35:42Z

+                // [NotNull]TNullable -> X
+                // [NotNull]TStruct? -> X
+                // [NotNull]TOpen -> X
+                return type is null || (type.Kind == SymbolKind.TypeParameter && !type.IsReferenceType) || type.IsNullableTypeOrTypeParameter();


type is null [](start = 23, length = 12)

When do we hit this, and is it necessary to report a warning in that case? #Resolved

cston · 2020-01-13T22:37:26Z

            }
        }

+        FlowAnalysisAnnotations ToInwardAnnotations(FlowAnalysisAnnotations outwardAnnotations)


private static #Closed

cston · 2020-01-13T22:57:01Z

  </data>
  <data name="WRN_DisallowNullAttributeForbidsMaybeNullAssignment" xml:space="preserve">
-    <value>A possible null value may not be assigned to a target marked with the [DisallowNull] attribute</value>
+    <value>A possible null value may not be assigned to a target marked with the [DisallowNull] attribute or output to a target marked with the [NotNull] attribute</value>


output to a target marked with [](start = 109, length = 30)

Is this qualifier necessary? Do we need to differentiate "assigned to" and "output to", or can we find one phrase that captures both?

How about: "A possible null value may not be used for a type marked with [NotNull] or [DisallowNull]"? #Closed

cston · 2020-01-13T23:19:28Z

+    [DisallowNull] string? P1 { get; set; } = """";
+    [AllowNull] string P2 { get; set; } = """";
+    [MaybeNull] string P3 { get; set; } = """";
+    [NotNull] string? P4 { get; set; } = """";


It's not clear we're that the attributes affect the property bodies since there are no warnings. Should the assignments be changed to = null' to at least test two of the four? #Closed

💡 The previous test has good coverage of these, so a starting point could model after it. #Resolved

cston · 2020-01-13T23:29:36Z

+
+        [Fact]
+        [WorkItem(36039, "https://github.com/dotnet/roslyn/issues/36039")]
+        public void NontNullOutParameterWithVariousTypes()


Nont [](start = 20, length = 4)

Typo #Closed

sharwell · 2020-01-13T23:37:38Z

+        x.ToString(); // 1
+    }
+
+    void M2([DisallowNull]T x)


📝 Should have a test showing that x = default is allowed within a method like this. #Closed

@sharwell I disagree with this one. x = default is disallowed (ie. a warning) in absence of [DisallowNull], so should still be disallowed. #Closed

That makes sense to me. Consider adding a line showing this diagnostic is reported for the case. #Resolved

I think that case is already covered. See DefaultLiteralInConditional #Resolved

sharwell · 2020-01-13T23:39:28Z

+    [DisallowNull] string? P1 { get; set; } = """";
+    [AllowNull] string P2 { get; set; } = """";
+    [MaybeNull] string P3 { get; set; } = """";
+    [NotNull] string? P4 { get; set; } = """";


💡 The previous test has good coverage of these, so a starting point could model after it. #Resolved

sharwell · 2020-01-13T23:48:16Z

+                // (4,43): warning CS8607: A possible null value may not be assigned to a target marked with the [DisallowNull] attribute or output to a target marked with the [NotNull] attribute
+                //     [AllowNull, NotNull]public TOpen P1 { get; set; } = default!;
+                Diagnostic(ErrorCode.WRN_DisallowNullAttributeForbidsMaybeNullAssignment, "get;").WithLocation(4, 43),


😟 This makes sense if you have { get; }, but I don't see why this would be a warning if the property has a setter and has [AllowNull], especially since it uses the suppression in default!. #Closed

We analyze a body that says return backingField;. But that backing field is a TOpen and lacks attribute annotations. So we warn.
I'll take a look to see if we can refine that. #Closed

I find out that some warnings involving backing fields and other compiler-generated values were suppressed. I removed that, which not only keeps this warning on P1, but also adds a warning on P3.
@sharwell @cston Let me know what you think about those. In my opinion, those are bad usages of the attributes, and so I think it's actually desirable to warn. #Resolved

Both cases can be resolved (in user code) by removing the unnecessary initializing expression. If users find this confusing, we can iterate on it. #Resolved

From discussion with @cston I'm going to restore the logic to skip analyzing bodies of auto-prop getters and setters. We'll lose some warning about misused attributes, but that's okay.

One reason is that there is a scenario where that would produce an unwanted warning: [MaybeNull, AllowNull] T { get; set; } which the user can't fix. #Resolved

sharwell · 2020-01-14T00:01:23Z

+                Diagnostic(ErrorCode.WRN_DisallowNullAttributeForbidsMaybeNullAssignment, "get;").WithLocation(4, 32),
+                // (14,35): warning CS8607: A possible null value may not be assigned to a target marked with the [DisallowNull] attribute or output to a target marked with the [NotNull] attribute
+                //     [NotNull]public TStruct? P5 { get; set; }
+                Diagnostic(ErrorCode.WRN_DisallowNullAttributeForbidsMaybeNullAssignment, "get;").WithLocation(14, 35)


💡 It would be good to have a test showing that the value can be assigned in a constructor to eliminate the warning (may already exist as a test). #Resolved

❔ I didn't see a test that used the following in a constructor:

P5 = new TStruct();

Unlike the newly-added NotNull_Property_InitializedInConstructor cases, I would expect the above to not produce a warning and resolve the warning reported on the property definition in the two tests here. #Pending

jcouv · 2020-01-15T15:33:56Z


            static bool hasNoNonNullableCounterpart(TypeSymbol type)
            {
+                if (type is null)


📝 not sure if we can hit this. Just included for safety. #Resolved

cston · 2020-01-16T04:41:57Z

-            if (((annotations & FlowAnalysisAnnotations.DisallowNull) != 0) && state.Type.IsNullableTypeOrTypeParameter() && state.MayBeNull)
+            if (boundValueOpt is object && boundValueOpt.WasCompilerGenerated)
+            {
+                // We need to skip `return backingField;` in auto-prop getters


// We need to skip return backingField; in auto-prop getters [](start = 16, length = 62)

Can we simply avoid running NullableWalker on auto-property accessors? #Resolved

We could, but I'd prefer to contain the scope of this PR given timeline.
Also, even if we skip auto-prop accessors, we'll still need some logic to skip compiler-generated nodes because of field = initializerValue; that is injected to constructor body and analyzed.

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

Please consider logging a bug to skip auto-property accessors.

In reply to: 367632535 [](ancestors = 367632535,367228167)

Filed #41017 #Resolved

cston · 2020-01-16T04:42:10Z

+        private void CheckDisallowedNullAssignment(TypeWithState state, FlowAnalysisAnnotations annotations, Location location, BoundExpression boundValueOpt = null)
        {
-            if (((annotations & FlowAnalysisAnnotations.DisallowNull) != 0) && state.Type.IsNullableTypeOrTypeParameter() && state.MayBeNull)
+            if (boundValueOpt is object && boundValueOpt.WasCompilerGenerated)


boundValueOpt is object && boundValueOpt.WasCompilerGenerated [](start = 16, length = 61)

Consider using ?. #Resolved

jcouv · 2020-01-16T21:44:53Z

@dotnet/roslyn-compiler for a second review please.
Thanks @cston! #Resolved

RikkiGibson · 2020-01-17T22:23:41Z

        }

-        private void CheckDisallowedNullAssignment(TypeWithState state, FlowAnalysisAnnotations annotations, Location location)
+        private void CheckDisallowedNullAssignment(TypeWithState state, FlowAnalysisAnnotations annotations, Location location, BoundExpression boundValueOpt = null)


Consider naming CheckDisallowNullAssignment to more strongly denote that this is about DisallowNullAttribute #Resolved

I think that "check disallowed null assignment" rings better as an action. I'll keep as-is

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

RikkiGibson · 2020-01-17T22:23:52Z

            }
        }

+        private static FlowAnalysisAnnotations ToInwardAnnotations(FlowAnalysisAnnotations outwardAnnotations)


ToInwardAnnotations [](start = 47, length = 19)

Consider naming ToInboundAnnotations to resemble similar terminology used in VisitArguments

RikkiGibson · 2020-01-17T22:23:57Z

+                // MaybeNull and MaybeNullWhen count as MaybeNull
+                annotations |= FlowAnalysisAnnotations.AllowNull;
+            }
+            if ((outwardAnnotations & FlowAnalysisAnnotations.NotNull) == FlowAnalysisAnnotations.NotNull)


Style between this and the above if feels inconsistent #Closed

Yes, that's on purpose. MaybeNull is two bits, either of which should count (see comment above).
NotNull is also two bits, but we are lenient (we ignore NotNullWhenTrue and NotNullWhenFalse on their own here).

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

I'll add a comment

In reply to: 368183067 [](ancestors = 368183067,368161275)

RikkiGibson · 2020-01-17T23:27:11Z

-                //     static void F4<T>(   [AllowNull]T x,    [MaybeNull]ref T y) { y = x; }
-                Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "x").WithLocation(8, 71),
                // (9,71): warning CS8601: Possible null reference assignment.
                //     static void F5<T>(   [AllowNull]T x,      [NotNull]ref T y) { y = x; } // 4


T x [](start = 55, length = 3)

I think this is maybe innate to the problem, but it feels odd to have a different message here depending on whether AllowNull was used. In F5, T is "definitely maybe null", but in FA, T is "possibly maybe null". #WontFix

We could have the new check (from CheckDisallowedNullAssignment) report the same diagnostic as the regular checks that VisitConversion can report. But since we have more context/information here, I figured out that some hint would be helpful, when possible. But it is a bit inconsistent, I agree.

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

RikkiGibson · 2020-01-17T23:27:32Z

static void F2<T>(   [AllowNull]T x,    [AllowNull]out T y) { y = x; } // 2

It feels like AllowNull/DisallowNull on out parameters is something that could never be what the user intended. Related to #36073 #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:119870 in 501855b. [](commit_id = 501855b, deletion_comment = True)

RikkiGibson · 2020-01-17T23:28:00Z

static void F8<T>(              T x, [DisallowNull]T y) { y = x; }

It feels like if assigning maybe-null to a [NotNull]T parameter gives a warning, assigning maybe-null to a [DisallowNull]T parameter inside the method should give a warning. #WontFix

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:119847 in 501855b. [](commit_id = 501855b, deletion_comment = True)

RikkiGibson · 2020-01-17T23:28:09Z

static void FA<T>(              T x,      [NotNull]T y) { y = x; }

Also consider a // 1 comment here.

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:120557 in 501855b. [](commit_id = 501855b, deletion_comment = False)

RikkiGibson · 2020-01-17T23:28:14Z

[return: NotNull] public override T F1<T>(T t) => t;

Consider adding a // 1 comment here #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:33885 in 501855b. [](commit_id = 501855b, deletion_comment = False)

RikkiGibson · 2020-01-17T23:28:19Z

+@"using System.Diagnostics.CodeAnalysis;
+public class COpen<TOpen> where TOpen : new()
+{
+    [NotNull]public TOpen P1 { get; set; }


Is this just additional coverage, or did your change affect behavior of these scenarios? It doesn't seem like enforcing unconditional attributes inside the members using the attributes affects this? #Resolved

For a while, this PR include analyzing the body of the auto-prop accessors. So those tests were useful.
Since I added the tests, I'd rather keep them even though they are less useful now :-)

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

RikkiGibson · 2020-01-17T23:28:29Z

 class Program
 {
-    [return: MaybeNull, NotNull] static T F1<T>(T t) => t;
+    [return: MaybeNull, NotNull] static T F1<T>(T t) => t; //


// [](start = 59, length = 2)

Was typing // -1 a bridge too far? :) #Resolved

I could not bring myself to do it :-P

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

RikkiGibson · 2020-01-17T23:28:36Z

 }";
            var comp = CreateCompilation(new[] { DisallowNullAttributeDefinition, source }, options: WithNonNullTypesTrue());
            comp.VerifyDiagnostics(
+                // (11,12): warning CS8607: A possible null value may not be assigned to a target marked with the [DisallowNull] attribute


nit: Consider updating the diagnosic message in this comment. I had started to comment about how this message could be better but it turned out you already changed it for the better :)

RikkiGibson

jcouv · 2020-01-18T00:09:29Z

static void F2<T>(   [AllowNull]T x,    [AllowNull]out T y) { y = x; } // 2

Per our discussion and spec, NotNull is definitely useful on inputs.
I can't think of a scenario where AllowNull or DisallowNull would be useful on an output. But I don't want to produce individual warnings for those situations until we've decided that we want to warn on misused attributes as a whole (there are many more cases that could warrant a warning).

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:119870 in 501855b. [](commit_id = 501855b, deletion_comment = True)

jcouv · 2020-01-18T00:45:17Z

FYI @cston, I took Rikki's suggestion so that void M([DisallowNull]T x) { x = null; } now produces a warning and void M([AllowNull]string x) { x = null; } no longer does.

jcouv · 2020-01-18T01:22:40Z

FYI @cston, I took Rikki's suggestion so that void M([DisallowNull]T x) { x = null; } now produces a warning and void M([AllowNull]string x) { x = null; } no longer does.

Never mind, that won't work because of void M([DisallowNull] ref T x) { x = null; } scenario. I'll revert.

Enforce unconditional attributes within method bodies

3e6559d

jcouv added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jan 7, 2020

jcouv added this to the 16.5 milestone Jan 7, 2020

jcouv self-assigned this Jan 7, 2020

Mitigate cycle issue

c688ccb

jcouv marked this pull request as ready for review January 8, 2020 01:53

jcouv requested a review from a team as a code owner January 8, 2020 01:53

jcouv added 2 commits January 7, 2020 22:19

Treat conditional attributes as unconditional

ac90c31

Add a couple more tests to close issues

db6d529

jcouv requested a review from cston January 8, 2020 23:18

jcouv commented Jan 13, 2020

View reviewed changes

cston reviewed Jan 13, 2020

View reviewed changes

sharwell reviewed Jan 14, 2020

View reviewed changes

jcouv modified the milestones: 16.5, 16.6, 16.5.P3 Jan 14, 2020

jcouv mentioned this pull request Jan 15, 2020

Should not warn on null default value on [AllowNull] parameter #40975

Closed

jcouv added 2 commits January 15, 2020 06:04

Address feedback

8f58732

Also check compiler-generated values

efad142

jcouv commented Jan 15, 2020

View reviewed changes

jcouv added 2 commits January 15, 2020 07:36

remove unnecessary type argument

9e6927f

Document breaking change

c1bbd61

cston reviewed Jan 16, 2020

View reviewed changes

Couple more tests

501855b

cston approved these changes Jan 16, 2020

View reviewed changes

This was referenced Jan 16, 2020

NullableWalker should not inspect bodies of auto-prop accessors #41017

Open

CS8601 false positive #41040

Closed

RikkiGibson reviewed Jan 17, 2020

View reviewed changes

RikkiGibson approved these changes Jan 17, 2020

View reviewed changes

Address feedback from Rikki

342f856

Revert some

8cf4b46

jcouv merged commit a8b5e42 into dotnet:master Jan 18, 2020

jcouv deleted the method-bodies branch January 18, 2020 06:24

This was referenced Jan 18, 2020

Should nullability attributes affect method bodies and OHI? #36039

Closed

Enforce conditional attributes within method bodies #41098

Merged

v-zbsail mentioned this pull request Jan 22, 2020

LOC CHECKIN | dotnet/roslyn master | 20200116 #41005

Merged

jcouv mentioned this pull request Mar 31, 2020

Enforce NotNull on method exit #42827

Merged

Conversation

jcouv commented Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Jan 13, 2020 • 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 commented Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Jan 13, 2020 • 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!

cston Jan 13, 2020 • edited by jcouv 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 Jan 13, 2020 • 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!

cston Jan 13, 2020 • 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!

cston Jan 13, 2020 • 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!

sharwell Jan 13, 2020 • edited by jcouv 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 Jan 13, 2020 • 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!

Uh oh!

Uh oh!

sharwell Jan 13, 2020 • edited by jcouv 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 Jan 15, 2020 • 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!

sharwell Jan 15, 2020 • edited by jcouv 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 Jan 15, 2020 • 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!

sharwell Jan 13, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Jan 13, 2020 • edited by jcouv 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 Jan 15, 2020 • 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 Jan 15, 2020 • 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!

sharwell Jan 15, 2020 • edited by jcouv 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 Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

jcouv commented Jan 7, 2020 •

edited

Loading

jcouv Jan 13, 2020 •

edited

Loading

jcouv commented Jan 13, 2020 •

edited

Loading

cston Jan 13, 2020 •

edited

Loading

cston Jan 13, 2020 •

edited by jcouv

Loading

cston Jan 13, 2020 •

edited

Loading

cston Jan 13, 2020 •

edited

Loading

cston Jan 13, 2020 •

edited

Loading

sharwell Jan 13, 2020 •

edited by jcouv

Loading

cston Jan 13, 2020 •

edited

Loading

sharwell Jan 13, 2020 •

edited by jcouv

Loading

jcouv Jan 15, 2020 •

edited

Loading

sharwell Jan 15, 2020 •

edited by jcouv

Loading

jcouv Jan 15, 2020 •

edited

Loading

sharwell Jan 13, 2020 •

edited by jcouv

Loading

sharwell Jan 13, 2020 •

edited by jcouv

Loading

jcouv Jan 15, 2020 •

edited

Loading

jcouv Jan 15, 2020 •

edited

Loading

sharwell Jan 15, 2020 •

edited by jcouv

Loading

jcouv Jan 15, 2020 •

edited

Loading

sharwell Jan 14, 2020 •

edited by jcouv

Loading

sharwell Jan 15, 2020 •

edited by jcouv

Loading

jcouv Jan 15, 2020 •

edited

Loading

cston Jan 16, 2020 •

edited by jcouv

Loading

jcouv Jan 16, 2020 •

edited

Loading

cston Jan 16, 2020 •

edited by jcouv

Loading

jcouv commented Jan 16, 2020 •

edited

Loading

RikkiGibson Jan 17, 2020 •

edited by jcouv

Loading

RikkiGibson Jan 17, 2020 •

edited

Loading

RikkiGibson Jan 17, 2020 •

edited by jcouv

Loading