Typeless expressions should contribute nullability to lambda return #47581

jcouv · 2020-09-10T06:08:50Z

NullableWalker assumed that we would not encounter unconverted conditional or switch expressions. This resulted in a crash because such expressions can lack type.
We can encounter such expressions when those expressions are target-typed and we're in a lambda while trying to figure out the return type of the lambda (which will then become the target-type).

So we need to analyze such typeless expressions before they become target-typed (switch, conditional, null, default, lambdas, ...). They should contribute nullability information, but no type (so we should avoid giving them an error type for this purpose).

RikkiGibson

Done with review pass (iteration 1).

RikkiGibson · 2020-09-10T15:51:43Z

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

@@ -6477,12 +6473,10 @@ private bool HasTopLevelNullabilityConversion(TypeWithAnnotations source, TypeWi
                    resultState = NullableFlowState.NotNull;
                    break;

-                case ConversionKind.ObjectCreation:
                case ConversionKind.SwitchExpression:


Is there any significance to the reordering of these case labels? #Resolved

No significance. I'll revert if any other change.

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

RikkiGibson · 2020-09-10T15:53:46Z

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

-            SetResultType(node, TypeWithState.Create(expr.Type, NullableFlowState.NotNull));
+            // This method is only involved in method inference with unbound lambdas.
+            // The diagnostics on arguments are reported by VisitObjectCreationExpression.
+            SetResultType(node, TypeWithState.Create(null, NullableFlowState.NotNull));


Do we need to make a pass over usages of ResultType to determine if we might try to access a null Type off of it? #Resolved

TypeWithState can handle null type well (TypeSymbol? Type already).
But TypeWithAnnotations is a mixed bag. Will see if I can annotate it.

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

I tried annotating TypeWithAnnotations (and in particular its .Type), but that cascades to a lot of binding code. That will be for a separate PR ;-)
So instead, I reviewed the uses of TypeWithAnnotations.Type in NullableWalker and also traced the uses of TypeWithAnnotations coming from _visitResult. Most of those types come straight out of symbols (ie. have types) and the rest handle or check for null.

In reply to: 486561434 [](ancestors = 486561434,486453187)

RikkiGibson · 2020-09-10T16:03:38Z

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

@@ -136114,27 +136536,31 @@ static void F<T>(Func<T> f)
    {
        F(() =>
        {
-            if (b) return default; // 1
+            if (b) return default;


Does your change fix #44339? If so, consider adding a WorkItem to this test linking to that issue, and consider adding a test for the specific in the issue.

Also consider checking if this change resolves #43536. #Resolved

Indeed this resolves the first issue (since it involves unbound lambdas during method type inference).
But it does not resolve the second (since it doesn't involve lambdas). Solving that second issue needs to go MethodTypeInferer. The method IsReallyAType used in MakeExplicitParameterTypeInferences discards typeless symbols. (note that there is a bug whereby default is not treated as typeless during re-inference, so we hit this bug for null but not for default)

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

cston · 2020-09-10T16:14:34Z

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

-            SetResultType(node, TypeWithState.Create(expr.Type, NullableFlowState.NotNull));
+            // This method is only involved in method inference with unbound lambdas.
+            // The diagnostics on arguments are reported by VisitObjectCreationExpression.
+            SetResultType(node, TypeWithState.Create(null, NullableFlowState.NotNull));


TypeWithState.Create(null, NullableFlowState.NotNull) [](start = 32, length = 53)

Does this create a TypeWithState with Type == null and State == NotNull? Should we pass defaultState: MaybeNull instead? #Resolved

Yes, that creates a TypeWithState with no type but NoteNull state.
Here we want to pass NotNull because we know the value is not null (object creation always results in an instance) even if we don't know the type.

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

cston · 2020-09-15T16:18:57Z

src/Compilers/CSharp/Portable/Binder/Semantics/BestTypeInferrer.cs

@@ -14,11 +15,17 @@ internal static class BestTypeInferrer
    {
        public static NullableAnnotation GetNullableAnnotation(ArrayBuilder<TypeWithAnnotations> types)
        {
+#if DEBUG
+            var example = types.Where(t => t.HasType).FirstOrDefault();


types.Where(t => t.HasType).FirstOrDefault() [](start = 26, length = 44)

Can use types.FirstOrDefault(t => t.HasType). #Pending

cston · 2020-09-15T16:19:41Z

src/Compilers/CSharp/Portable/Binder/Semantics/BestTypeInferrer.cs

-                Debug.Assert(type.HasType);
-                Debug.Assert(type.Equals(types[0], TypeCompareKind.AllIgnoreOptions));
+#if DEBUG
+                Debug.Assert(!type.HasType || (example.HasType && type.Equals(example, TypeCompareKind.AllIgnoreOptions)));


example.HasType && [](start = 47, length = 19)

example.HasType is not needed. #Pending

alrz · 2020-09-15T16:26:14Z

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

-            Visit(expr);
-            SetResultType(node, TypeWithState.Create(expr.Type, NullableFlowState.NotNull));
+            // This method is only involved in method inference with unbound lambdas.
+            // The diagnostics on arguments are reported by VisitObjectCreationExpression.


this comment worth a few days reading the nullable walker

Your comment made my day! :-)

cston · 2020-09-15T16:43:01Z

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

+        map.GetOrAdd("""", _ => null); // 2
+
+        map.GetOrAdd("""", _ => { if (b) return default; return """"; }); // 3
+        map.GetOrAdd("""", _ => { if (b) return null; return """"; }); // 4


map.GetOrAdd("""", _ => { if (b) return null; return """"; }); // 4 [](start = 8, length = 67)

Consider also testing with _ => { if (b) return ""; return null; }

cston

Minor comments only.

* upstream/master: (114 commits) Remove langversion restriction for source generators. (dotnet#47714) Adjust disambiguation rules for type pattern in a switch expression. (dotnet#47756) Delete decommissioned benchview tool scripts (dotnet#47752) Emit conversions between native integers and pointers directly (dotnet#47708) Typeless expressions should contribute nullability to lambda return (dotnet#47581) Use a distinct diagnostic ID when an exhaustiveness report uses an unnamed enum value. (dotnet#47693) [master] Update dependencies from dotnet/arcade (dotnet#46586) Change `Location` of record's primary constructor to point to record's identifier. (dotnet#47715) Add public API test for extended partial methods (dotnet#47727) Rename in CheckValidNullableMethodOverride (dotnet#47718) Update docs Add more doc comments Add comments and doc comments for ExternalErrorDiagnosticUpdateSource Add documentation remarks for syntax kinds (dotnet#43646) Disable TestCancellation (dotnet#47725) Classify function pointer punctuation (dotnet#47668) Disable flaky optprof test Handle NotNullIfNotNull in delegate creation and overrides (dotnet#47572) Adjust QuickInfo on record BaseType syntax (dotnet#47656) Don't exclude events for NameOf context ...

jcouv added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Sep 10, 2020

jcouv self-assigned this Sep 10, 2020

jcouv force-pushed the cond-crash branch 2 times, most recently from b4be2e5 to 3de7f21 Compare September 10, 2020 06:20

Typeless expressions should contribute nullability to lambda return

3657e17

jcouv force-pushed the cond-crash branch from 3de7f21 to 3657e17 Compare September 10, 2020 14:54

jcouv marked this pull request as ready for review September 10, 2020 15:14

jcouv requested a review from a team as a code owner September 10, 2020 15:14

RikkiGibson reviewed Sep 10, 2020

View reviewed changes

cston reviewed Sep 10, 2020

View reviewed changes

Address feedback

d469230

RikkiGibson approved these changes Sep 11, 2020

View reviewed changes

jcouv requested a review from cston September 11, 2020 17:15

cston reviewed Sep 15, 2020

View reviewed changes

alrz reviewed Sep 15, 2020

View reviewed changes

cston reviewed Sep 15, 2020

View reviewed changes

cston approved these changes Sep 15, 2020

View reviewed changes

Address feedback from Chuck

a0e1311

jcouv merged commit 87f4c9d into dotnet:master Sep 16, 2020

ghost added this to the Next milestone Sep 16, 2020

jcouv deleted the cond-crash branch September 16, 2020 17:01

RikkiGibson mentioned this pull request Sep 18, 2020

Bogus nullability warning (recent regression) #46461

Closed

dibarbet modified the milestones: Next, 16.8.P4 Sep 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typeless expressions should contribute nullability to lambda return #47581

Typeless expressions should contribute nullability to lambda return #47581

jcouv commented Sep 10, 2020 •

edited

Loading

RikkiGibson left a comment

RikkiGibson Sep 10, 2020 •

edited by jcouv

Loading

jcouv Sep 10, 2020

RikkiGibson Sep 10, 2020 •

edited by jcouv

Loading

jcouv Sep 10, 2020

jcouv Sep 11, 2020

RikkiGibson Sep 10, 2020 •

edited by jcouv

Loading

jcouv Sep 11, 2020

cston Sep 10, 2020 •

edited by jcouv

Loading

jcouv Sep 10, 2020 •

edited

Loading

cston Sep 15, 2020 •

edited by jcouv

Loading

cston Sep 15, 2020 •

edited by jcouv

Loading

alrz Sep 15, 2020

jcouv Sep 16, 2020 •

edited

Loading

cston Sep 15, 2020

cston left a comment

Typeless expressions should contribute nullability to lambda return #47581

Typeless expressions should contribute nullability to lambda return #47581

Conversation

jcouv commented Sep 10, 2020 • edited Loading

RikkiGibson left a comment

Choose a reason for hiding this comment

RikkiGibson Sep 10, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Sep 10, 2020

Choose a reason for hiding this comment

RikkiGibson Sep 10, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Sep 10, 2020

Choose a reason for hiding this comment

jcouv Sep 11, 2020

Choose a reason for hiding this comment

RikkiGibson Sep 10, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Sep 11, 2020

Choose a reason for hiding this comment

cston Sep 10, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

cston Sep 15, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Sep 15, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

alrz Sep 15, 2020

Choose a reason for hiding this comment

jcouv Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

cston Sep 15, 2020

Choose a reason for hiding this comment

cston left a comment

Choose a reason for hiding this comment

jcouv commented Sep 10, 2020 •

edited

Loading

RikkiGibson Sep 10, 2020 •

edited by jcouv

Loading

RikkiGibson Sep 10, 2020 •

edited by jcouv

Loading

RikkiGibson Sep 10, 2020 •

edited by jcouv

Loading

cston Sep 10, 2020 •

edited by jcouv

Loading

jcouv Sep 10, 2020 •

edited

Loading

cston Sep 15, 2020 •

edited by jcouv

Loading

cston Sep 15, 2020 •

edited by jcouv

Loading

jcouv Sep 16, 2020 •

edited

Loading