Collection expressions: nullable analysis of spread element expression #74686
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
cston
force-pushed
the
74667
branch
4 times, most recently
from
5eb7963 to
9dc9b7f
Compare
|
/azp run roslyn-CI |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@dotnet/roslyn-compiler, please review. |
| return base.VisitCollectionExpression(node); | ||
| // See NullableWalker.VisitCollectionExpression.getCollectionDetails() which | ||
| // does not have an element type for the ImplementsIEnumerable case. | ||
| var hasElementType = node.CollectionTypeKind is not (CollectionExpressionTypeKind.None or CollectionExpressionTypeKind.ImplementsIEnumerable); |
There was a problem hiding this comment.
That would be the non-generic IEnumerable case, right? So that's like converting an object~ element type to the target element type, there's no nullability check that needs to be done there? #Resolved
There was a problem hiding this comment.
CollectionExpressionTypeKind.ImplementsIEnumerable represents any collection initializer type where the type implements IEnumerable and elements are added through Add().
This PR does not handle nullability analysis for those cases. There is a separate item in #68786.
- Checking conversion of elements to iteration type for collection initializer types
| } | ||
| if (hasElementType) | ||
| { | ||
| Visit(((BoundExpressionStatement?)spread.IteratorBody)?.Expression); |
There was a problem hiding this comment.
Do we actually need to dig into the expression here, instead of just visiting the statement, for the verifier to pick it up? #Resolved
There was a problem hiding this comment.
NullableWalker only visits the expression within the BoundExpressionStatement of spread.IteratorBody. (The containing BoundExpressionStatement is only there to allow sharing code in binding between foreach and .. since the foreach infrastructure expects the body to be a statement.)
There was a problem hiding this comment.
OK, sounds like if we visited the statement here, then we would fail verification because NullableWalker did not also visit that statement.
| var itemResult = spread.EnumeratorInfoOpt == null ? default : _visitResult; | ||
| var iteratorBody = ((BoundExpressionStatement)spread.IteratorBody).Expression; | ||
| AddPlaceholderReplacement(spread.ElementPlaceholder, expression: null, itemResult); | ||
| var completion = VisitOptionalImplicitConversion(iteratorBody, elementType, |
There was a problem hiding this comment.
It looks like we are assuming the iteratorBody is something that would be convertible to the elementType? But I thought it could represent a call to a void-returning Add method, for example. #Resolved
There was a problem hiding this comment.
The Add() cases are not handled in this PR. For those cases, elementType.HasType == false so we shouldn't reach this code path.
There was a problem hiding this comment.
I don't find the connection between elementType.HasType and the shape of the spread IteratorBody straightforward to follow. It seems like there are several places where we dig into the IteratorBody and expect to get an expression which meets certain assumptions. Perhaps it would be helpful to add some extension(s) to BoundCollectionExpressionSpreadElement which implies and verifies the assumption the developer is making.
Also, it feels like using a name like targetElementType instead of elementType would make this code a little easier to follow, to be clear we are not talking about the element type of the spread operand itself.
That said, none of the above changes need to happen in this PR.
There was a problem hiding this comment.
Thanks. I've renamed elementType to targetElementType. We can consider adding methods to BoundCollectionExpressionSpreadElement as needed in separate PRs.
| var comp = CreateCompilation(source); | ||
| comp.VerifyEmitDiagnostics(); | ||
| comp.VerifyEmitDiagnostics( | ||
| // (10,44): warning CS8619: Nullability of reference types in value of type 'IEnumerable<string?>' doesn't match target type 'IEnumerable<object>'. |
There was a problem hiding this comment.
What is being analyzed to yield this warning? Are we actually converting the b to IEnumerable<object> before enumerating in the bound tree and warning on that?
There was a problem hiding this comment.
It looks like the comment refers to ab = [..a, ..b] in TypeInference_Spread_08.
In that test, b has type IEnumerable<string?>[] and the target collection has type IEnumerable<object!>[].
NullableWalker.VisitCollectionExpression is reporting the warning from the implicit conversion of the element of the spread element expression, with type IEnumerable<string?>, to the element type IEnumerable<object!> of the target collection.
| CreateCompilation(src).VerifyEmitDiagnostics( | ||
| // (5,19): warning CS8602: Dereference of a possibly null reference. | ||
| // string[] y1 = [.. x1]; | ||
| Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x1").WithLocation(5, 19)); |
There was a problem hiding this comment.
Parking comment here, but it relates to a test below. I think we can remove the comment in SpreadNullability_SplitExpression. #Closed
| static void Main() | ||
| { | ||
| IEnumerable<string?> x = [null]; | ||
| IEnumerable<object> y = [..x]; |
There was a problem hiding this comment.
Consider adding the converse scenario, where the elements are not-null and the target collection allows for null elements:
IEnumerable<string> x = [""];
IEnumerable<object?> y = [..x]; // no warning
``` #Closed
| """; | ||
| var comp = CreateCompilation(source); | ||
| comp.VerifyEmitDiagnostics(); | ||
| } |
There was a problem hiding this comment.
Do we have a test where the spread operand is more than a simple local? For instance M(x), which could produce nullability warnings of its own. #Closed
There was a problem hiding this comment.
It looks like the operand in test Spread_Nullable_09 is not producing warnings of its own. It sounded like Julien was asking for an F() which warns about a possible null argument, for example.
| @@ -3767,6 +3779,34 @@ static NullableFlowState getResultState(BoundCollectionExpression node, Collecti | |||
| } | |||
| } | |||
|
|
|||
| public override BoundNode? VisitCollectionExpressionSpreadElement(BoundCollectionExpressionSpreadElement node) | |||
| { | |||
| VisitRvalue(node.Expression); | |||
There was a problem hiding this comment.
It looks like a case like int[] x = [..(int[]?)null] is handled by the VisitForEachExpression itself? Since we would be visiting a loop like foreach (var elem in (int[]?)null) { ... }? #Resolved
There was a problem hiding this comment.
Yes, the top-level nullability of the expression being spread is handled in VisitForEachExpression(). For an example test, see CollectionExpressionTests.SpreadNullability.
From #68786:
Fixes #74667