Enforce NotNull on method exit #42827

jcouv · 2020-03-27T03:44:36Z

Relates to PR #40789 (enforce unconditional attributes in method bodies)

jcouv · 2020-03-31T14:51:00Z

@dotnet/roslyn-compiler for review. Thanks

cston · 2020-03-31T21:45:23Z

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

+                var slot = GetOrCreateSlot(parameter);
+                if (slot > 0)
+                {
+                    var parameterState = state[slot];


Consider inlining this variable to avoid calculating value in most cases.

cston · 2020-03-31T21:48:01Z

src/Compilers/Core/Portable/InternalUtilities/Debug.cs

+            if (value is null)
+            {
+                throw new InvalidOperationException("This program location is unreachable.");
+            }


Is the throw needed since the assert above will have failed?

The assert above is not necessarily annotated, in some of our projects. The stricter new enforcement implemented in this PR caused bootstrap build to fail some projects...

Could you implement in terms RoslynDebug.Assert instead of Debug.Assert to solve this?

Ah, indeed :-) I'll try using RoslynDebug.Assert

cston · 2020-03-31T21:56:09Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -5506,6 +5506,12 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="WRN_ParameterConditionallyDisallowsNull_Title" xml:space="preserve">
    <value>Parameter may not have a null value when exiting in some condition.</value>
  </data>
+  <data name="WRN_ParameterDisallowsNull" xml:space="preserve">
+    <value>Parameter '{0}' may not have a null value when exiting.</value>
+  </data>


"Parameter may not have a null value when exiting" could imply the parameter must have a null value but might not. How about "Parameter must have a non-null value when exiting"? Similar comment for WRN_ParameterConditionallyDisallowsNull.

RikkiGibson · 2020-03-31T22:35:16Z

interesting how the approach we came up with specifically to solve NotNullWhenAttribute ended up being useful for NotNullAttribute as well. 😄

cston · 2020-04-01T03:38:02Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+
+class C<TStruct, TClass, TNotNull>
+    where TStruct : struct
+    where TClass : class


TClass is not used.

jcouv · 2020-04-01T14:23:17Z

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

RikkiGibson · 2020-04-01T21:54:30Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

-    }
+        x = null;
+        (x, _) = (null, 1);
+    } // 2


hmm. this makes it start to feel like a tradeoff for more complex methods. Imagine a complex method with many return points and many writes to some [NotNull] out param. Would you feel lost if the nullability warnings were on the returns instead of the original assignments?

Are we also trying to improve the experience in scenarios where users write a value that might be null to the out param, and if it is null, they write some fallback value that's known to be non-null?

Yes, there is a bit of trade-off. Warning sooner points to the location of the problematic assignment, but it also means that we flag temporary assignments (annoying).
For nullability feature in general, we lean away from anything that could be annoying and could hold you back from annotating your correct code as it already exists.

RikkiGibson · 2020-04-01T21:56:35Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+                //         (x, _) = (null, 1); // 5
+                Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "null").WithLocation(41, 19),
+                // (42,5): warning CS8777: Parameter 'x' must have a non-null value when exiting.
+                //     } // 6


Why enforce this on parameters with RefKind.None? Isn't the method just asserting about the value originally passed as an argument in that case?

On the other hand, it doesn't feel like there's a use case for assigning maybe-null to a NotNull, non-ref parameter, so it probably isn't very important to remove this warning.

Thinking about it for a moment, it seems like unnecessary effort to attempt to remove this warning--but it might not be what we want to prevent this warning by just assigning not-null to the parameter. e.g.

void MyAssert1([NotNull] object? obj) { // hooray, we know the argument passed by the caller is non-null after the method returns if (obj is null) throw new ArgumentNullException(nameof(obj)); } void MyAssert2([NotNull] object? obj) { obj = new object(); // we fixed the warning, hooray! // but caller still might be holding a null.. }

Good point. I don't have a strategy to distinguish between those two cases :-(

RikkiGibson · 2020-04-01T22:09:05Z

src/Compilers/Core/Portable/InternalUtilities/Debug.cs

            => Debug.Assert(b, message);

        [Conditional("DEBUG")]
-        public static void AssertNotNull<T>([NotNull]T value) where T : class?
-            => Debug.Assert(value is object, "Unexpected null reference");
+        public static void AssertNotNull<T>([NotNull] T value) where T : class?


This is not really something to fix in this PR, but I don't understand why this method is generic.

I suspect it is so that it cannot be called with a struct parameter.
If it just took an object parameter, then structs would box, rather than produce an error.

jcouv added Area-Compilers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. Feature - Nullable Reference Types Nullable Reference Types labels Mar 27, 2020

jcouv self-assigned this Mar 27, 2020

jcouv force-pushed the enforce-notnull branch 2 times, most recently from a69138b to 68f2edd Compare March 30, 2020 22:26

jcouv marked this pull request as ready for review March 31, 2020 01:24

jcouv requested review from a team as code owners March 31, 2020 01:24

Enforce NotNull on method exit

a04952a

jcouv force-pushed the enforce-notnull branch from 68f2edd to a04952a Compare March 31, 2020 01:25

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 31, 2020

Resolve merge conflict in test

d973fd9

jasonmalinowski removed the request for review from a team March 31, 2020 18:26

cston reviewed Mar 31, 2020

View reviewed changes

jcouv added 4 commits March 31, 2020 17:14

Adjust diagnostic message

7257d02

Adjust formatting and quotes on true/false

9fc1599

Inline variable

7c35151

Add test with various types

7f8516e

cston approved these changes Apr 1, 2020

View reviewed changes

cston reviewed Apr 1, 2020

View reviewed changes

Merge remote-tracking branch 'dotnet/master' into enforce-notnull

cd42d73

jcouv mentioned this pull request Apr 1, 2020

Roslyn doesn't verify variables' nullable states on exits through finally blocks #42981

Closed

Add test

6d19cb6

RikkiGibson reviewed Apr 1, 2020

View reviewed changes

RikkiGibson approved these changes Apr 1, 2020

View reviewed changes

jcouv added 2 commits April 1, 2020 16:10

Use RoslynDebug.Assert

386865a

Update generated files

13a0855

jcouv merged commit 7be257a into dotnet:master Apr 2, 2020

ghost added this to the Next milestone Apr 2, 2020

jcouv deleted the enforce-notnull branch April 2, 2020 15:27

sharwell modified the milestones: Next, temp, 16.6.P3 Apr 6, 2020

jakobbotsch mentioned this pull request Nov 16, 2021

Nullability for [NotNull] input arguments that are assigned #57793

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce NotNull on method exit #42827

Enforce NotNull on method exit #42827

jcouv commented Mar 27, 2020 •

edited

Loading

jcouv commented Mar 31, 2020

cston Mar 31, 2020

cston Mar 31, 2020

jcouv Mar 31, 2020 •

edited

Loading

RikkiGibson Apr 1, 2020

jcouv Apr 1, 2020

cston Mar 31, 2020 •

edited

Loading

RikkiGibson commented Mar 31, 2020

cston Apr 1, 2020

jcouv commented Apr 1, 2020

RikkiGibson Apr 1, 2020

jcouv Apr 1, 2020

RikkiGibson Apr 1, 2020

RikkiGibson Apr 1, 2020

jcouv Apr 1, 2020

RikkiGibson Apr 1, 2020

jcouv Apr 1, 2020

Enforce NotNull on method exit #42827

Enforce NotNull on method exit #42827

Conversation

jcouv commented Mar 27, 2020 • edited Loading

jcouv commented Mar 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

RikkiGibson commented Mar 31, 2020

Choose a reason for hiding this comment

jcouv commented Apr 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Mar 27, 2020 •

edited

Loading

jcouv Mar 31, 2020 •

edited

Loading

cston Mar 31, 2020 •

edited

Loading