Fix nested nullability of collection expressions #74490

RikkiGibson · 2024-07-22T23:55:45Z

Adjust NullableWalker to stop passing target-typed expressions to BestTypeInferrer. This was causing the converted collection-expr's type from initial binding to sneak into the nullable best type analysis, causing some NotAnnotated annotations to get replaced with Oblivious in the array element case.

Also adjust BestTypeInferrer to expect that target-typed expressions are not passed to it. It doesn't make sense for a target-typed expression to participate in best-type. Target-typed expressions should only be converted to the resulting best-type, not have their type used as input to best-type analysis.

RikkiGibson · 2024-07-30T17:31:26Z

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

+                // TODO2: what does it mean for these types to differ? Why is it correct to copy over the top-level nullability when the types have differences beyond nullability?
+                if (!visitResult.LValueType.Type.Equals(conversionOpt.Type, TypeCompareKind.AllNullableIgnoreOptions))
+                {
+                    visitResult = withType(visitResult, conversionOpt.Type);


I wasn't able to figure out exactly what to do here. I found that sometimes the visitResult had a nullable-enriched type prior to this call and that calling withType was stomping on that and using the type from initial binding. That was causing the enriched type info from this change to get dropped.

But, other times, the type of conversionOpt.Type was different than visitResult beyond just nullability. In this case, I opted to just drop the type from the visitResult as the original code was doing. It seems most likely that there is no "nullable-enriched" version of the type resulting from the visit, which directly corresponds to the conversionOpt.Type. #Resolved

From offline discussion, TrackAnalyzedNullabilityThroughConversionGroup is incorrect if we think of the general nullability tracking through conversions. But that's not its job (VisitConversion does that full analysis). So only a subset of cases must come through here (error conversions? trivial conversions?). An assertion narrowing down the conversions we expect to see here would help.
Is it okay that it returns types with nullabilities from initial binding in some/many cases?

I've investigated this more.

I noticed that SetAnalyzedNullability is already dropping the visitResult's type if it differs from conversionOpt.Type (with all ignore options). So, I think that 'withType' never actually affects anything. It was essentially repeating a step that SetAnalyzedNullability was already doing.

I have removed the function, and added a comment about the "inaccuracy" of what we are doing here--which is just that we're using the conversion's type from initial binding, instead of using the operand's visit result to reinfer the conversion's type and store the reinferred type instead.

My change has made it so that when the conversion operand's nested nullability is meaningfully reinferred, that info is not dropped, and that none of the existing inaccuracies are made "worse".

There is more room to improve here, but would like to leave it at that for this PR. This method is used for various conversion kinds including user-defined conversions So I didn't find that adding an assertion that, say, only standard conversions are passed into this method, etc., was valid.

RikkiGibson · 2024-07-30T22:45:14Z

@dotnet/roslyn-compiler for review

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

jcouv · 2024-08-05T17:20:37Z

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

@@ -4613,7 +4613,11 @@ private static bool IsTargetTypedExpression(BoundExpression node)
            {
                var (returnExpr, resultType, _) = returns[i];
                resultTypes.Add(resultType);
-                placeholdersBuilder.Add(CreatePlaceholderIfNecessary(returnExpr, resultType));
+
+                if (!IsTargetTypedExpression(returnExpr))


The tests seem to focus on collection expressions as the "target-typed expression". Consider adding tests for the other kinds of target-typed expressions (null, default, switches, conditionals, lambdas, tuples, ...).

the scenario is about when:

a target-typed expression is being converted to the result type of a best-type analysis (e.g. b ? ["1"] : new[] { "2" }),

and, there is a nested nullability reinference occurring in the other best-type candidate expressions, which we want to "win" over any initial-bound type which the target-typed expression may have.

So, there might be an interesting scenario to test like in b ? ("1", "2") : GenericMethod("1"), or something like that.

Have added a test based on this suggestion.

I didn't follow. The test you added doesn't have lambdas.

There are three places where an IsTargetTypedExpression check was added: conditionals, lambdas and switches. The added test seems to cover conditionals.
Consider adding a similar test showing the effects of the fix on other target-typed expressions besides collection expressions in lambdas and switches.

Switches are covered in TargetTypedExpressions_NestedNullability.
Lambdas are covered in CollectionExpression_NestedNullability_06. Note that one of the cases involves collection-exprs, and the other case doesn't involve collection-exprs, but still shows an incorrect behavior, tracked in #74609

src/Compilers/CSharp/Test/Symbol/Symbols/Source/NullablePublicAPITests.cs

jcouv · 2024-08-05T17:26:44Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/NullablePublicAPITests.cs

+            var model = comp.GetSemanticModel(tree);
+
+            var root = tree.GetRoot();
+            var collectionExpr = root.DescendantNodes().OfType<CollectionExpressionSyntax>().Single();


nit: just FYI in case you find it convenient, I added a test helper "GetSyntax" which allows GetSyntax<CollectionExpressionSyntax>(tree, """["1"]""") to do the trick in this case.

jcouv

Done with review pass (iteration 3). Only minor questions on tests

jcouv

LGTM Thanks (iteration 10)

cston · 2024-08-14T20:00:40Z

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

@@ -4613,7 +4613,11 @@ private static bool IsTargetTypedExpression(BoundExpression node)
            {
                var (returnExpr, resultType, _) = returns[i];
                resultTypes.Add(resultType);
-                placeholdersBuilder.Add(CreatePlaceholderIfNecessary(returnExpr, resultType));
+
+                if (!IsTargetTypedExpression(returnExpr))


if (!IsTargetTypedExpression(returnExpr))

Is this change covered by a test? It didn't seem like the new tests were affected by this check.

That's correct, but, I would prefer to leave this check in order to satisfy new assertions in BestTypeInferrer.

I didn't investigate closely but I am guessing something is causing the return type we wanted to infer here to be replaced unexpectedly with a type from initial binding.

Actually since this is only used in initial binding we might be able to remove the check and just assert that a target typed expr is not being used here.

urgh, this code probably would have behaved incorrectly if one of the exprs actually was target-typed, it expects that 'resultTypes ' and 'placeholdersBuilder' have the same count after the loop.

cston · 2024-08-14T20:11:04Z

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

+            else if (IsTargetTypedExpression(alternative))
+            {
+                resultType = consequenceRValue.Type;
+            }


Are we testing this case?

cston · 2024-08-14T20:11:37Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/NullablePublicAPITests.cs

+                #nullable enable
+
+                var b = false;
+                var arr = b ? ["1"] : new[] { "2" };


var arr = b ? ["1"] : new[] { "2" };

Consider also testing:

arr = b ? new[] { "1" } : ["2"];

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 22, 2024

RikkiGibson force-pushed the issue71522 branch from 16c2c0d to 9fc06bb Compare July 29, 2024 23:33

Do not pass target typed expressions to BestTypeInferrer

757cf84

RikkiGibson force-pushed the issue71522 branch from 9fc06bb to 757cf84 Compare July 29, 2024 23:35

RikkiGibson commented Jul 30, 2024

View reviewed changes

jcouv self-assigned this Jul 30, 2024

RikkiGibson changed the title ~~Fix nested nullability of collection expressions in public APIs~~ Fix nested nullability of collection expressions Jul 30, 2024

RikkiGibson mentioned this pull request Jul 30, 2024

Implicitly typed lambdas returning arrays can have unexpected oblivious element nullability #74609

Open

RikkiGibson added 2 commits July 30, 2024 15:44

Add test for best type lambda returns

408eb28

Remove TODO2

eef8986

RikkiGibson marked this pull request as ready for review July 30, 2024 22:45

RikkiGibson requested a review from a team as a code owner July 30, 2024 22:45

jcouv reviewed Aug 5, 2024

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Show resolved Hide resolved

jcouv reviewed Aug 5, 2024

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/Source/NullablePublicAPITests.cs Show resolved Hide resolved

jcouv reviewed Aug 5, 2024

View reviewed changes

which tests fail when I do this

00790e2

RikkiGibson mentioned this pull request Aug 9, 2024

Default literals should not contribute their type from initial binding to best-type in nullable analysis #74693

Open

RikkiGibson added 6 commits August 8, 2024 21:47

Cleanup and test more target typed expressions

65d6719

Merge branch 'issue71522' into issue71522-wip

158894c

commenting out withType fails no tests. What about an assertion?

bdbc3a3

fix

ca49867

fix

5f55798

Simplify and add test baseline

73cd3ec

RikkiGibson requested a review from jcouv August 9, 2024 19:02

build-analysis bot mentioned this pull request Aug 9, 2024

Our services aren't available right now dotnet/dnceng#3069

Open

3 tasks

jcouv approved these changes Aug 9, 2024

View reviewed changes

RikkiGibson requested a review from a team August 9, 2024 21:44

cston reviewed Aug 14, 2024

View reviewed changes

Address feedback

452c53c

cston approved these changes Aug 14, 2024

View reviewed changes

RikkiGibson merged commit e041433 into dotnet:main Aug 15, 2024
24 checks passed

RikkiGibson deleted the issue71522 branch August 15, 2024 00:41

dotnet-policy-service bot added this to the Next milestone Aug 15, 2024

This was referenced Aug 21, 2024

[Automated] PRs inserted in VS build main-35220.189 #74834

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35223.06 #74874

Closed

dibarbet modified the milestones: Next, 17.12 P2 Aug 26, 2024

dotnet-bot mentioned this pull request Aug 28, 2024

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35227.190 #74926

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix nested nullability of collection expressions #74490

Fix nested nullability of collection expressions #74490

RikkiGibson commented Jul 22, 2024 •

edited

Loading

RikkiGibson Jul 30, 2024 •

edited by jcouv

Loading

jcouv Aug 5, 2024

RikkiGibson Aug 9, 2024 •

edited

Loading

RikkiGibson commented Jul 30, 2024

jcouv Aug 5, 2024

RikkiGibson Aug 5, 2024 •

edited

Loading

RikkiGibson Aug 9, 2024

jcouv Aug 9, 2024

jcouv Aug 9, 2024

RikkiGibson Aug 9, 2024

jcouv Aug 5, 2024

jcouv left a comment

jcouv left a comment

cston Aug 14, 2024

RikkiGibson Aug 14, 2024

RikkiGibson Aug 14, 2024

RikkiGibson Aug 14, 2024

cston Aug 14, 2024

cston Aug 14, 2024

Fix nested nullability of collection expressions #74490

Fix nested nullability of collection expressions #74490

Conversation

RikkiGibson commented Jul 22, 2024 • edited Loading

RikkiGibson Jul 30, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

RikkiGibson commented Jul 30, 2024

Choose a reason for hiding this comment

RikkiGibson Aug 5, 2024 • edited Loading

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 left a comment

Choose a reason for hiding this comment

jcouv left a 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

Choose a reason for hiding this comment

RikkiGibson commented Jul 22, 2024 •

edited

Loading

RikkiGibson Jul 30, 2024 •

edited by jcouv

Loading

RikkiGibson Aug 9, 2024 •

edited

Loading

RikkiGibson Aug 5, 2024 •

edited

Loading