Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MaybeNullWhen and NotNullWhen attribute affects callers #36172

Merged
merged 4 commits into from Jun 8, 2019

Conversation

@jcouv
Copy link
Member

commented Jun 5, 2019

Design notes:

  1. 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.

  2. We then do type inference.

  3. We then validate inbound values and conversions, considering pre-condition attributes. This all happens in an unsplit state.

  4. 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 jcouv added this to the 16.2.P3 milestone Jun 5, 2019

@jcouv jcouv self-assigned this Jun 5, 2019

@jcouv jcouv force-pushed the jcouv:cond-attributes branch 6 times, most recently from 07af513 to 9bef0fb Jun 5, 2019

@jcouv jcouv marked this pull request as ready for review Jun 5, 2019

@jcouv jcouv requested a review from dotnet/roslyn-compiler as a code owner Jun 5, 2019

@jcouv jcouv force-pushed the jcouv:cond-attributes branch 2 times, most recently from 4115162 to 2f86a27 Jun 5, 2019

@jcouv

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

@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

@jcouv jcouv force-pushed the jcouv:cond-attributes branch from 2f86a27 to de944d1 Jun 6, 2019

@jaredpar jaredpar referenced this pull request Jun 6, 2019

Open

Nullable Reference Type Changes #35816

15 of 23 tasks complete

@gafter gafter self-requested a review Jun 6, 2019

@gafter gafter added this to In Review in Compiler: Gafter Jun 6, 2019

@@ -3077,37 +3017,50 @@ private ImmutableArray<VisitArgumentResult> VisitArgumentsEvaluate(ImmutableArra
var builder = ArrayBuilder<VisitArgumentResult>.GetInstance(n);
for (int i = 0; i < n; i++)
{
builder.Add(VisitArgumentEvaluate(arguments[i], GetRefKind(refKindsOpt, i), preserveConditionalState: false));
(_, _, FlowAnalysisAnnotations parameterAnnotations) = GetCorrespondingParameter(i, parameters, argsToParamsOpt, expanded);

This comment has been minimized.

Copy link
@gafter

gafter Jun 6, 2019

Member

Perhaps simpler

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

#Resolved

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

This comment has been minimized.

Copy link
@gafter

gafter Jun 6, 2019

Member

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


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

This comment has been minimized.

Copy link
@gafter

gafter Jun 6, 2019

Member

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

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

This comment has been minimized.

Copy link
@jcouv

jcouv Jun 7, 2019

Author Member

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


return typeWithState;
}

This comment has been minimized.

Copy link
@gafter

gafter Jun 6, 2019

Member

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

This comment has been minimized.

Copy link
@jcouv

jcouv Jun 7, 2019

Author Member

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

gafter approved these changes Jun 6, 2019

Copy link
Member

left a comment

:shipit:

@gafter gafter removed this from In Review in Compiler: Gafter Jun 7, 2019

@jcouv

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

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

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

This comment has been minimized.

Copy link
@RikkiGibson

RikkiGibson Jun 7, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@jcouv

jcouv Jun 7, 2019

Author Member

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

agocke approved these changes Jun 8, 2019

Copy link
Contributor

left a comment

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

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

This comment has been minimized.

Copy link
@agocke

agocke Jun 8, 2019

Contributor

Could annotations have both AssertsTrue and AssertsFalse? #Resolved

This comment has been minimized.

Copy link
@jcouv

jcouv Jun 8, 2019

Author Member

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

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

This comment has been minimized.

Copy link
@agocke

agocke Jun 8, 2019

Contributor

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

This comment has been minimized.

Copy link
@jcouv

jcouv Jun 8, 2019

Author Member

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

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

This comment has been minimized.

Copy link
@agocke

agocke Jun 8, 2019

Contributor

Here as well?

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

16 checks passed

WIP Ready for review
Details
license/cla All CLA requirements met.
Details
roslyn-CI Build #20190607.61 succeeded
Details
roslyn-CI (Linux_Test coreclr) Linux_Test coreclr succeeded
Details
roslyn-CI (Linux_Test mono) Linux_Test mono succeeded
Details
roslyn-CI (MacOs_Test) MacOs_Test succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests debug) Windows_CoreClr_Unit_Tests debug succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests release) Windows_CoreClr_Unit_Tests release succeeded
Details
roslyn-CI (Windows_Correctness_Test) Windows_Correctness_Test succeeded
Details
roslyn-CI (Windows_Desktop_Spanish_Unit_Tests) Windows_Desktop_Spanish_Unit_Tests succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_32) Windows_Desktop_Unit_Tests debug_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_64) Windows_Desktop_Unit_Tests debug_64 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_32) Windows_Desktop_Unit_Tests release_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_64) Windows_Desktop_Unit_Tests release_64 succeeded
Details
roslyn-CI (Windows_Determinism_Test) Windows_Determinism_Test succeeded
Details
roslyn-integration-CI Build #20190607.61 succeeded
Details

@jcouv jcouv deleted the jcouv:cond-attributes branch Jun 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.