MaybeNullWhen and NotNullWhen attribute affects callers by jcouv · Pull Request #36172 · dotnet/roslyn

jcouv · 2019-06-05T00:08:25Z

Design notes:

In an invocation, we first visit the arguments and save the results. We unsplit after each argument, except for those marked with AssertsTrue/AssertsFalse (which will be renamed to DoesNotReturnIf(bool) later on) where we drop half of the state.
We then do type inference.
We then validate inbound values and conversions, considering pre-condition attributes. This all happens in an unsplit state.
We then validate outbound values and apply post-condition attributes:
- for ref and out arguments, we track an assignment from parameter to argument
- for in/by-value, we don't do any assignment, but we can still learn from various attributes

Relates to #35816
LDM notes: https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-05-15.md

Fixes #35949 (complexity issue with lambdas in invocations with nullability annotation attributes) because VisitArgumentsEvaluateHonoringAnnotations used to visit arguments more, but that was removed. We now visit lambdas as often as in initial binding.

jcouv · 2019-06-06T15:58:37Z

@dotnet/roslyn-compiler for review. ~~I'm still adding a few tests~~, but the change should be ready otherwise. I want to merge this before the weekend (6/10 deadline for corefx). Thanks #Resolved

gafter · 2019-06-06T22:57:42Z

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

            {
-                builder.Add(VisitArgumentEvaluate(arguments[i], GetRefKind(refKindsOpt, i), preserveConditionalState: false));
+                (_, _, FlowAnalysisAnnotations parameterAnnotations) = GetCorrespondingParameter(i, parameters, argsToParamsOpt, expanded);
+


Perhaps simpler

FlowAnalysisAnnotations parameterAnnotations = GetCorrespondingParameter(i, parameters, argsToParamsOpt, expanded).Annotations;

#Resolved

gafter · 2019-06-06T23:07:16Z

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

+                        // check whether parameter would unsafely let a null out in the worse case
+                        if (!argument.IsSuppressed)
+                        {
+                            ReportNullableAssignmentIfNecessary(parameterValue, lValueType, applyPostConditionsUnconditionally(parameterWithState, parameterAnnotations), useLegacyWarnings: false);


false [](start = 189, length = 5)

Should this be UseLegacyWarnings(argument, result.LValueType)? In case the parameter is a local, I would expect it to be a legacy warning. (I'm not sure why we call it a legacy warning. I think the meaning is that legacy is a non-safety warning) #Resolved

gafter · 2019-06-06T23:11:14Z

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

+
+            void trackNullableStateForAssignment(BoundExpression parameterValue, TypeWithAnnotations lValueType, int targetSlot, TypeWithState parameterWithState, bool isSuppressed, FlowAnalysisAnnotations parameterAnnotations)
+            {
+                if (!IsConditionalState && !hasConditionalPostCondition(parameterAnnotations))


IsConditionalState [](start = 21, length = 18)

Is there a risk of duplicate diagnostics on following parameters? #Resolved

I don't think there is. Tracking assignments affects state, but does not produce diagnostics. #Resolved

gafter · 2019-06-06T23:15:04Z

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

+
+                return typeWithState;
+            }
+


These could be collapsed into a single function if you pass the whenTrue and whenFalse enum bits in as a parameter. #WontFix

I'm tempted to leave this as-is. I agree the code could be factored more, but I think it would become less readable. #Resolved

gafter

jcouv · 2019-06-07T17:36:01Z

Thanks Neal!
@dotnet/roslyn-compiler for a second review. I'm trying to merge this today so that corefx can consume it on Monday. Thanks #Resolved

RikkiGibson · 2019-06-07T20:56:10Z

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

+    {
+        _ = F1(t1, out var s2)
+            ? s2.ToString() // 1
+            : s2.ToString(); // 2


s2.ToString(); // 2 [](start = 14, length = 19)

I don't understand why there would be a warning here? #Resolved

Imagine that M1 is invoked with T = string?. Then we're looking at F1<string?>(t1, out string? t2) which yields a maybe-null t2 in both branches.

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

agocke

LGTM, with the assumption that design changes will come in later

agocke · 2019-06-08T02:26:59Z

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

-                        UseRvalueOnly(argument); // force use of flow result
-                    }
-                    else
+                    switch (annotations & (FlowAnalysisAnnotations.AssertsTrue | FlowAnalysisAnnotations.AssertsFalse))


Could annotations have both AssertsTrue and AssertsFalse? #Resolved

Thanks. It looks like I unintentionally changed the behavior for that scenario. Added a test and fixed it. #Resolved

agocke · 2019-06-08T02:40:20Z

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

+                if ((annotations & FlowAnalysisAnnotations.MaybeNull) != 0)
+                {
+                    // MaybeNull and MaybeNullWhen
+                    return TypeWithState.Create(typeWithState.Type, NullableFlowState.MaybeNull);


As mentioned offline, this should produce a nullable type, if one exists, right?

I added a note to the work items list (#35816) and started thread with LDM.

agocke · 2019-06-08T02:40:26Z

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

+                if ((annotations & FlowAnalysisAnnotations.NotNull) == FlowAnalysisAnnotations.NotNull)
+                {
+                    // NotNull
+                    return TypeWithState.Create(typeWithState.Type, NullableFlowState.NotNull);


Here as well?

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 Jun 5, 2019

jcouv added this to the 16.2.P3 milestone Jun 5, 2019

jcouv self-assigned this Jun 5, 2019

jcouv force-pushed the cond-attributes branch 6 times, most recently from 07af513 to 9bef0fb Compare June 5, 2019 22:33

jcouv marked this pull request as ready for review June 5, 2019 22:33

jcouv requested a review from a team as a code owner June 5, 2019 22:33

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

jcouv force-pushed the cond-attributes branch 2 times, most recently from 4115162 to 2f86a27 Compare June 6, 2019 14:45

MaybeNullWhen and NotNullWhen attribute affects callers

de944d1

jcouv force-pushed the cond-attributes branch from 2f86a27 to de944d1 Compare June 6, 2019 17:22

jaredpar mentioned this pull request Jun 6, 2019

Nullable Reference Type Changes #35816

Closed

23 tasks

gafter self-requested a review June 6, 2019 19:55

jcouv mentioned this pull request Jun 6, 2019

Nullable reference types cause exponential compile / analysis time even when disabled #35949

Closed

gafter reviewed Jun 6, 2019

View reviewed changes

gafter approved these changes Jun 6, 2019

View reviewed changes

Address feedback from Neal

a8e5cd5

RikkiGibson reviewed Jun 7, 2019

View reviewed changes

Update test baselines

0e5091e

agocke approved these changes Jun 8, 2019

View reviewed changes

Handle both AssertsTrue/False set

3b4c8a5

jcouv merged commit 9bb1b37 into dotnet:master Jun 8, 2019

jcouv deleted the cond-attributes branch June 8, 2019 19:51

Conversation

jcouv commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter Jun 6, 2019 • 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!

gafter Jun 6, 2019 • 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!

gafter Jun 6, 2019 • 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 Jun 7, 2019 • 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!

gafter Jun 6, 2019 • 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 Jun 7, 2019 • 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!

gafter left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

RikkiGibson Jun 7, 2019 • 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 Jun 7, 2019

Choose a reason for hiding this comment

Uh oh!

agocke left a comment

Choose a reason for hiding this comment

Uh oh!

agocke Jun 8, 2019 • 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 Jun 8, 2019 • 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!

agocke Jun 8, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 8, 2019

Choose a reason for hiding this comment

Uh oh!

agocke Jun 8, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcouv commented Jun 5, 2019 •

edited

Loading

jcouv commented Jun 6, 2019 •

edited

Loading

gafter Jun 6, 2019 •

edited by jcouv

Loading

gafter Jun 6, 2019 •

edited by jcouv

Loading

gafter Jun 6, 2019 •

edited by jcouv

Loading

jcouv Jun 7, 2019 •

edited

Loading

gafter Jun 6, 2019 •

edited by jcouv

Loading

jcouv Jun 7, 2019 •

edited

Loading

jcouv commented Jun 7, 2019 •

edited

Loading

RikkiGibson Jun 7, 2019 •

edited by jcouv

Loading

agocke Jun 8, 2019 •

edited by jcouv

Loading

jcouv Jun 8, 2019 •

edited

Loading