Update argument state when parameter has not-null type #46072

RikkiGibson · 2020-07-17T00:31:50Z

🦔
Fixes #43383

jcouv · 2020-07-19T15:32:28Z

Could you add a couple more scenarios? I think those will work:

void M([DisallowNull] int? i)
bool M([MaybeNullWhen(true)] string s)
bool M([MaybeNullWhen(true), MaybeNullWhen(false)] string s)

I'm not completely sure about this one:

void M(T t) where T is constained with notnull #Closed

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

jcouv

Done with review pass (iteration 1). Looks nice, thanks!

RikkiGibson · 2020-07-20T17:04:52Z

MaybeNullWhen attribute cannot be applied multiple times, and seems a bit esoteric to test with a specially-crafted attribute, but will add the other test cases.

RikkiGibson · 2020-07-21T23:53:36Z

I have added the requested tests. Please have another look at your convenience @jcouv. /cc @dotnet/roslyn-compiler

jcouv · 2020-07-24T03:28:02Z

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

+                    => !argument.IsSuppressed
+                    && ((!parameterType.Type.IsTypeParameterDisallowingAnnotation()
+                            && parameterType.NullableAnnotation.IsNotAnnotated())
+                        || parameterOriginalDefinitionType.Type is TypeParameterSymbol { HasNotNullConstraint: true });


parameterOriginalDefinitionType [](start = 27, length = 31)

I didn't understand why we need to use the original definition here.
If we're dealing with an unsubstituted method, then the parameter is TNotNull which we can check here (1).
If we're dealing with a substituted method, then the parameter was substituted with an unannotated type (2) or a safety diagnostic was reported on the substitution (3). Is scenario (3) what prompted you to use the original definition here? #Closed

In scenario (3) adding this check allows us to update argument state to not-null which reduces cascading diagnostics in a similar fashion to updating argument state when the parameter type disallows null.

class C { void M<T>(T t) where T : notnull { } void M1(string? s) { M(s); // warning: 'string?' does not meet the 'notnull' constraint s.ToString(); // in master we get another warning here, but this is avoidable. } }

It occurs to me that if we do this here, we should probably do something similar for the 'class' constraint. #Closed

I'd be okay if this we didn't handle this cascading diagnostic at all, or if we split that to a different PR. We would report a warning on the constraint, then one warning on passing a bad value to M(s) but no warning on s.ToString(). #Closed

@RikkiGibson From discussion with @cston, it feels like the second part of the PR should be reverted and not even addressed in a follow-up PR, as it is addressing a different scenario than the original point of this PR and what was discussed in LDM.
The main rationale from LDM is that we should assume that methods with non-nullable parameters really cannot handle null (one doesn't need to add an attribute) to express that. There was no discussion of avoiding cascades from violated constraints. #Closed

I agree, it seems like this part of the change needs to be considered separately. I think I just misinterpreted the suggestion to add a T : notnull test. #Closed

The main rationale from LDM is that we should assume that methods with non-nullable parameters really cannot handle null

It does feel like this is also the case for type parameters constrained to notnull or class. However, whether to change anything in nullable analysis for this scenario is up to the LDM. #Closed

…state

RikkiGibson · 2020-07-24T17:43:35Z

Have resolved merge conflicts, but have not fixed a semantic break related to T : default.

jcouv

LGTM Thanks (iteration 6)

RikkiGibson · 2020-07-24T22:24:54Z

@dotnet/roslyn-compiler Please review

jcouv · 2020-07-24T22:55:55Z

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

    }
 }
 ";
            var comp = CreateNullableCompilation(source);
            comp.VerifyDiagnostics(
+                // (9,12): warning CS8604: Possible null reference argument for parameter 't' in 'void Program<T>.M0(T t)'.
+                //         M0(t); // 2


Thanks for catching/fixing that scenario #Closed

jcouv

LGTM Thanks (iteration 7)

RikkiGibson · 2020-07-24T23:33:37Z

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

+    void M1()
+    {
+        T t = default;
+        M0(t); // 1


It feels like it would be illustrative here to try calling M0(t) a second time. I expect the state to get upgraded from "maybe-default" to "maybe-null" and for there to be no diagnostic on the second call. Does that match your expectation @jcouv @cston?

I expect the state to get upgraded from "maybe-default" to "maybe-null"

I didn't follow.
I could see that either (1) the state is unchanged (which I believe is the case with this PR), or (2) the state would be set to not-null (ie. assuming that the invocation throws).

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

Hmm. I think I inferred a principle that doesn't quite apply to unconstrained generics. My thinking is that if you call a method with a value it's not meant to accept, we update the argument state in a way that avoids warnings on subsequent calls. Thus if you call some method with an unconstrained generic parameter (requiring a maybe-null or not-null state), and you pass an argument with a maybe-default state, we change the argument state to maybe-null.

This is based on a conception of the flow states where:

"maybe-default" means a variable of a generic type may contain null, even if the type argument disallows it.

"maybe-null" means a variable of a generic type may contain null if the type argument allows it.

"not-null" means the variable doesn't contain null.

I think this is not something to change for this PR, but I will modify the test to record this behavior.

…state

cston · 2020-07-27T17:51:17Z

void M1(string? s1)

Please test M1<T>(T? t) where T : class and M1<T>(T t) where T : class?. This test could be replaced by the where T : class case.

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

cston · 2020-07-27T18:08:24Z

}

Please add a similar test for an unconstrained type parameter.

static void M<T>([DisallowNull] T t) { }
static void M1<T>(T t)
{
    M(t);
    t.ToString();
}

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

cston · 2020-07-27T18:10:50Z

    M0(s1); // 1

Should this be M0<string>(s1) instead, in which case we should update the state of s1.

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

cston · 2020-07-27T18:18:22Z

        b ? y : null, // 1

Consider using b ? y : x or b ? y : new object() in each of these cases in this test.

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

RikkiGibson · 2020-07-27T18:52:10Z

        b ? y : null, // 1
Consider using b ? y : x or b ? y : new object() in each of these cases in this test.

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

I decided to change null to null! here.

cston · 2020-07-27T19:44:14Z

void M1(T? t)

The ? are not needed.

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:34746 in 790899e. [](commit_id = 790899e, deletion_comment = False)

cston · 2020-07-27T19:47:29Z

void M0<T>(T t) where T : class { }

Sorry, I meant for the constraint on M1 instead:

void M0<T>(T t) { }

void M1<T>(T? t) where T : class
{
    // ...
}

And:

void M0<T>(T t) { }

void M1<T>(T t) where T : class?
{
    // ...
}

The M1(string? s1) can be replaced by the first of these.

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:34495 in 790899e. [](commit_id = 790899e, deletion_comment = False)

cston · 2020-07-27T19:51:53Z

void M0<T>(params T[]? s0) { }

Please consider keeping where T : notnull constraint if possible.

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:34898 in 790899e. [](commit_id = 790899e, deletion_comment = False)

…-pointers * upstream/master: (207 commits) Update argument state when parameter has not-null type (dotnet#46072) Fix TypeWithAnnotations.ToTypeWithState() for (untyped) null literal (dotnet#46344) Update README (dotnet#46136) Revert "Revert "Support nullable annotations on unconstrained type parameters"" Revert "Support nullable annotations on unconstrained type parameters (dotnet#45993)" Fix type in publish data Update VSIXExpInstaller version to one available on ADO Update publish data for 16.8 Update version of RichCodeNav.EnvVarDump A fixed initializer must be bound to its natural type (dotnet#46293) Update features merged into 16.7p4 (dotnet#46229) Async-streams: disposal should continue without jump within a finally (dotnet#46188) Recommend default in type constraint, but not record (dotnet#46311) Add use site diagnostics to IsUnmanaged (dotnet#46114) Add another flaky test. Ensure NuGet connections use TLS 1.2 Update to Microsoft.CodeAnalysis.Testing 1.0.1-beta1.20374.2 Skip flaky test. Fix build break. (dotnet#46303) Skip a flaky test Relates to dotnet#46304 ...

jaredpar · 2020-07-28T13:53:57Z

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

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


Consider making this test a [Theory] and using an argument to enumerate all the different constraint combinations where this is now valid.

jaredpar · 2020-07-28T13:56:45Z

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

+                );
+        }
+
+        [Fact]


Consider adding a test here there is an override that removes the nullability annotation and calls through both the base and derived type and shows the outcome on the null state of the argument. Basically making sure we are considering the base and derived method annotation as appropriate.

I actually can't remember off hand if we look at the declaration method or the override when considering annotations hence I can't remember what the correct behavior is here. Hence a test 😄

…to function-pointer-type-lookup * upstream/features/function-pointers: (212 commits) Correct public API number. Update argument state when parameter has not-null type (dotnet#46072) Fix TypeWithAnnotations.ToTypeWithState() for (untyped) null literal (dotnet#46344) Update README (dotnet#46136) Revert "Revert "Support nullable annotations on unconstrained type parameters"" Revert "Support nullable annotations on unconstrained type parameters (dotnet#45993)" Fix type in publish data Update VSIXExpInstaller version to one available on ADO Update publish data for 16.8 Update version of RichCodeNav.EnvVarDump A fixed initializer must be bound to its natural type (dotnet#46293) Update features merged into 16.7p4 (dotnet#46229) Async-streams: disposal should continue without jump within a finally (dotnet#46188) Recommend default in type constraint, but not record (dotnet#46311) Add use site diagnostics to IsUnmanaged (dotnet#46114) Add another flaky test. Ensure NuGet connections use TLS 1.2 Update to Microsoft.CodeAnalysis.Testing 1.0.1-beta1.20374.2 Skip flaky test. Fix build break. (dotnet#46303) ...

RikkiGibson added 2 commits July 16, 2020 17:07

Update argument state when parameter has not-null type

979b215

Fix tests

a298c6f

RikkiGibson marked this pull request as ready for review July 17, 2020 20:38

RikkiGibson requested a review from a team as a code owner July 17, 2020 20:38

jcouv added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jul 17, 2020

jcouv self-assigned this Jul 17, 2020

jcouv reviewed Jul 19, 2020

View reviewed changes

RikkiGibson and others added 2 commits July 20, 2020 12:33

Address 'notnull' constraint. Add more tests.

35b2049

Fix baseline

21fb29e

jcouv reviewed Jul 24, 2020

View reviewed changes

RikkiGibson added 2 commits July 24, 2020 10:20

Revert parameterOriginalDefinition change

fe67bd0

Merge branch 'master' of github.com:dotnet/roslyn into notnull-param-…

3b1347c

…state

jcouv approved these changes Jul 24, 2020

View reviewed changes

Fix tests

0454260

RikkiGibson requested a review from a team July 24, 2020 22:23

jcouv reviewed Jul 24, 2020

View reviewed changes

jcouv approved these changes Jul 24, 2020

View reviewed changes

RikkiGibson commented Jul 24, 2020

View reviewed changes

RikkiGibson added 2 commits July 26, 2020 16:00

Merge branch 'master' of github.com:dotnet/roslyn into notnull-param-…

f602045

…state

Update test

c2c725f

Address feedback

790899e

Address feedback

8070a25

cston approved these changes Jul 27, 2020

View reviewed changes

RikkiGibson merged commit 53b2957 into dotnet:master Jul 28, 2020

ghost added this to the Next milestone Jul 28, 2020

RikkiGibson deleted the notnull-param-state branch July 28, 2020 01:12

jaredpar reviewed Jul 28, 2020

View reviewed changes

RikkiGibson mentioned this pull request Jul 28, 2020

Follow-up testing for notnull-param-state #46384

Open

RikkiGibson modified the milestones: Next, 16.8.P2 Aug 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update argument state when parameter has not-null type #46072

Update argument state when parameter has not-null type #46072

RikkiGibson commented Jul 17, 2020 •

edited

Loading

jcouv commented Jul 19, 2020 •

edited

Loading

jcouv left a comment

RikkiGibson commented Jul 20, 2020

RikkiGibson commented Jul 21, 2020

jcouv Jul 24, 2020 •

edited

Loading

RikkiGibson Jul 24, 2020 •

edited by jcouv

Loading

jcouv Jul 24, 2020 •

edited

Loading

jcouv Jul 24, 2020 •

edited

Loading

RikkiGibson Jul 24, 2020 •

edited by jcouv

Loading

RikkiGibson Jul 24, 2020 •

edited by jcouv

Loading

RikkiGibson commented Jul 24, 2020

jcouv left a comment

RikkiGibson commented Jul 24, 2020

jcouv Jul 24, 2020 •

edited

Loading

jcouv left a comment

RikkiGibson Jul 24, 2020 •

edited

Loading

jcouv Jul 25, 2020

RikkiGibson Jul 26, 2020

cston commented Jul 27, 2020 •

edited

Loading

cston commented Jul 27, 2020

cston commented Jul 27, 2020

cston commented Jul 27, 2020

RikkiGibson commented Jul 27, 2020

cston commented Jul 27, 2020

cston commented Jul 27, 2020

cston commented Jul 27, 2020

jaredpar Jul 28, 2020

jaredpar Jul 28, 2020

+                              );
+                      }
+                      [Fact]

Update argument state when parameter has not-null type #46072

Update argument state when parameter has not-null type #46072

Conversation

RikkiGibson commented Jul 17, 2020 • edited Loading

jcouv commented Jul 19, 2020 • edited Loading

jcouv left a comment

Choose a reason for hiding this comment

RikkiGibson commented Jul 20, 2020

RikkiGibson commented Jul 21, 2020

jcouv Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Jul 24, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Jul 24, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

RikkiGibson Jul 24, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

RikkiGibson commented Jul 24, 2020

jcouv left a comment

Choose a reason for hiding this comment

RikkiGibson commented Jul 24, 2020

jcouv Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

RikkiGibson Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 25, 2020

Choose a reason for hiding this comment

RikkiGibson Jul 26, 2020

Choose a reason for hiding this comment

cston commented Jul 27, 2020 • edited Loading

cston commented Jul 27, 2020

cston commented Jul 27, 2020

cston commented Jul 27, 2020

RikkiGibson commented Jul 27, 2020

cston commented Jul 27, 2020

cston commented Jul 27, 2020

cston commented Jul 27, 2020

jaredpar Jul 28, 2020

Choose a reason for hiding this comment

jaredpar Jul 28, 2020

Choose a reason for hiding this comment

RikkiGibson commented Jul 17, 2020 •

edited

Loading

jcouv commented Jul 19, 2020 •

edited

Loading

jcouv Jul 24, 2020 •

edited

Loading

RikkiGibson Jul 24, 2020 •

edited by jcouv

Loading

jcouv Jul 24, 2020 •

edited

Loading

jcouv Jul 24, 2020 •

edited

Loading

RikkiGibson Jul 24, 2020 •

edited by jcouv

Loading

RikkiGibson Jul 24, 2020 •

edited by jcouv

Loading

jcouv Jul 24, 2020 •

edited

Loading

RikkiGibson Jul 24, 2020 •

edited

Loading

cston commented Jul 27, 2020 •

edited

Loading