Collection expressions: method type re-inference#71214
Conversation
648151d to
a19fb43
Compare
7a3a0a2 to
e2d587d
Compare
e2d587d to
169ba5d
Compare
src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs
Outdated
Show resolved
Hide resolved
| } | ||
| void M3(string? maybeNull, string notNull) | ||
| { | ||
| M(ref notNull, [notNull, ""]); |
src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs
Outdated
Show resolved
Hide resolved
|
@333fred @RikkiGibson for second review. Thanks |
|
|
||
| public TypeWithState RValueType => VisitResult.RValueType; | ||
| public TypeWithAnnotations LValueType => VisitResult.LValueType; | ||
| return new VisitResult(RValueType, lvalueType); |
There was a problem hiding this comment.
Can this be simplified to return new VisitResult(RValueType, lvalueType, StateForLambda);? #Resolved
|
@dotnet/roslyn-compiler for another review. Thanks |
RikkiGibson
left a comment
There was a problem hiding this comment.
Done review pass, just had a few clarifying questions etc
| } | ||
| return; | ||
|
|
||
| bool shouldMakeNotNullRvalue(BoundExpression node) => node.IsSuppressed || node.HasAnyErrors || !IsReachable(); |
There was a problem hiding this comment.
Consider inlining this local function and removing preceding return statement.
| """; | ||
|
|
||
| // Missing diagnostics for conversion of element to iteration type in Add scenario. | ||
| // Tracked by https://github.com/dotnet/roslyn/issues/68786 |
There was a problem hiding this comment.
This feels like a very common case. Are we planning to address for 17.10? #Resolved
There was a problem hiding this comment.
That'll be a question for Chuck.
Let me investigate
There was a problem hiding this comment.
Will fix the comment. This scenario is already tracked in code in VisitCollectionExpression
There was a problem hiding this comment.
Btw, the reason this is not handled in this PR is it was not trivial to analyze this conversion given that we don't have the right/corresponding conversion node in the bound tree. I wasn't sure how to do that, maybe I'm missing something.
| S<object?> x = [1]; | ||
| x[0].ToString(); // 1 | ||
| S<object> y = [null]; // 2 | ||
| S<object> y = [null]; |
There was a problem hiding this comment.
Consider matching the format of the previous test, i.e. leaving in the // 2 comments and referencing the tracking issue. #Resolved
There was a problem hiding this comment.
LDM decided that we should not analyze the Add method in collection expressions, so I don't expect we'll add this diagnostic later.
https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-11-15.md#nullability-analysis-of-collection-expressions
| Diagnostic(ErrorCode.WRN_NullAsNonNullable, "null").WithLocation(8, 28), | ||
| // (13,11): warning CS8625: Cannot convert null literal to non-nullable reference type. | ||
| // x3 = [null]; | ||
| Diagnostic(ErrorCode.WRN_NullAsNonNullable, "null").WithLocation(13, 11) |
There was a problem hiding this comment.
Is it expected for this diagnostic to be restored at some point in the future? Consider linking the issue which tracks that. #Resolved
There was a problem hiding this comment.
Indeed. There should be nullability warnings for the Create method. Will add comment
| var comp = CreateCompilation(src).VerifyEmitDiagnostics( | ||
| // (13,14): warning CS8618: Non-nullable field 'Element' must contain a non-null value when exiting constructor. Consider declaring the field as nullable. | ||
| // public T Element; | ||
| Diagnostic(ErrorCode.WRN_UninitializedNonNullableField, "Element").WithArguments("field", "Element").WithLocation(13, 14) |
There was a problem hiding this comment.
warning feels irrelevant to the test, consider adding = null!; to suppress. #Resolved
| model.GetSymbolInfo(invocation0).Symbol.ToTestDisplayString(includeNonNullable: true)); | ||
|
|
||
| var invocation3 = GetSyntax<InvocationExpressionSyntax>(tree, "M([Copy(maybeNull, out var maybeNull2), maybeNull2.ToString()])"); | ||
| Assert.Equal("System.Object! C.M<System.Object!>(System.Object![]! a)", |
There was a problem hiding this comment.
What explains this difference? Perhaps for maybeNull in M2() the flow state is not-null after issuing the warning, resulting in a different inference for the containing M()? #Resolved
There was a problem hiding this comment.
I think in the first scenario the second element (maybeNul2) contributes a maybe-null to method type inference, but in the second scenario the argument (maybeNull2.ToString()) contributes not-null.
There was a problem hiding this comment.
I am curious if this PR resolves #71522 (where we saw that expressions were reported to have oblivious nullability, even though they were in a nullable-enabled context.)
(Now that I think of it, this seems unlikely, as the linked issue is complaining about an array-creation, not a collection-expression.)
Implements part of #68786
Overview of the change:
NestedVisitResultsandStateForLambdatoVisitResult.NestedVisitResultsallows to gather theVisitResultsfor elements, so that they can contribute to method type inference (MethodTypeInferrerhandles unconverted collection expressions by making inferences from the elements directly).GetArgumentsForMethodTypeInferenceuses those nested visit results to prepare suitable arguments for method type inference.StateForLambdaallows tracking the State just prior to visiting a lambda expression. Previously, this was done withVisitArgumentResult, but that only works at the argument level. Now we need this for elements too.IEnumerable<T>interfaces). Code below should help once that element type is determined.Update: there is one IDE test failure, for which I added a corresponding compiler repro. The problem is that the logic in
NullableWalker.VisitArgumentsto get corresponding parameter types doesn't match the logic inBinder.BuildArgumentsForErrorRecovery. When we have a bad call, the binder tries to find corresponding parameter types across the various candidates, but the nullable walker logic only consider the method stored inBoundCall.Method(which is anErrorMethodSymbol).Because of this mismatch, the binder was able to convert the collection expression to a parameter type, but the nullable walker isn't, so it asserts because we detect that a converted target-typed collection wasn't fully processed.
As a workaround, the last commit softens an assertion (which was incorrect in
main) and allows us to recover without triggering assertions. We could file a follow-up bug for this.