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.

Sign up for GitHub

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

Jump to bottom

Collection expressions: analyze nullability of element assignments #70192

Merged
merged 14 commits into from
Oct 5, 2023
Merged

Collection expressions: analyze nullability of element assignments #70192

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
6517f58
Collection expressions: analyze nullability of element assignments
jcouv Sep 27, 2023
a08a9a2
Add nested test
jcouv Sep 30, 2023
c3b3a74
Test updates
jcouv Oct 4, 2023
9389b5a
Use ConditionalFact
jcouv Oct 5, 2023
55e4bc7
Merge remote-tracking branch 'dotnet/features/CollectionLiterals' int…
jcouv Oct 5, 2023
d320641
Fix conflicts
jcouv Oct 5, 2023
010ddfb
Merge remote-tracking branch 'dotnet/features/CollectionLiterals' int…
jcouv Oct 5, 2023
fad6025
Tweaks
jcouv Oct 5, 2023
a9afc31
Revert tweak
jcouv Oct 5, 2023
f44757d
Merge remote-tracking branch 'dotnet/features/CollectionLiterals' int…
jcouv Oct 5, 2023
9521597
Resolve conflict
jcouv Oct 5, 2023
6dd8cab
Remove unused local function
jcouv Oct 5, 2023
8fac2d0
Don't conditionally skip spread tests
jcouv Oct 5, 2023
33320c4
space
jcouv Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4025,8 +4025,8 @@ private bool HasLocalScope(BoundCollectionExpression expr)
switch (collectionTypeKind)
{
case CollectionExpressionTypeKind.ReadOnlySpan:
Debug.Assert(elementType is { });
return !LocalRewriter.ShouldUseRuntimeHelpersCreateSpan(expr, elementType);
Debug.Assert(elementType.Type is { });
return !LocalRewriter.ShouldUseRuntimeHelpersCreateSpan(expr, elementType.Type);
case CollectionExpressionTypeKind.Span:
return true;
case CollectionExpressionTypeKind.CollectionBuilder:
Expand Down
8 changes: 4 additions & 4 deletions src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -781,19 +781,19 @@ private void GenerateImplicitConversionErrorForCollectionExpression(
TypeSymbol targetType,
BindingDiagnosticBag diagnostics)
{
var collectionTypeKind = ConversionsBase.GetCollectionExpressionTypeKind(Compilation, targetType, out var elementType);
var collectionTypeKind = ConversionsBase.GetCollectionExpressionTypeKind(Compilation, targetType, out TypeWithAnnotations elementTypeWithAnnotations);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
if (collectionTypeKind == CollectionExpressionTypeKind.CollectionBuilder)
{
Debug.Assert(elementType is null); // GetCollectionExpressionTypeKind() does not set elementType for CollectionBuilder cases.
if (!TryGetCollectionIterationType((ExpressionSyntax)node.Syntax, targetType, out TypeWithAnnotations elementTypeWithAnnotations))
Debug.Assert(elementTypeWithAnnotations.Type is null); // GetCollectionExpressionTypeKind() does not set elementType for CollectionBuilder cases.
if (!TryGetCollectionIterationType((ExpressionSyntax)node.Syntax, targetType, out elementTypeWithAnnotations))
{
Error(diagnostics, ErrorCode.ERR_CollectionBuilderNoElementType, node.Syntax, targetType);
return;
}
Debug.Assert(elementTypeWithAnnotations.HasType);
elementType = elementTypeWithAnnotations.Type;
}

var elementType = elementTypeWithAnnotations.Type;
if (collectionTypeKind == CollectionExpressionTypeKind.ImplementsIEnumerableT
&& findSingleIEnumerableTImplementation(targetType, Compilation) is { } implementation)
{
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/Conversions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,16 +154,16 @@ protected override Conversion GetCollectionExpressionConversion(
ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
var syntax = node.Syntax;
var collectionTypeKind = GetCollectionExpressionTypeKind(Compilation, targetType, out var elementType);

var collectionTypeKind = GetCollectionExpressionTypeKind(Compilation, targetType, out TypeWithAnnotations elementTypeWithAnnotations);
var elementType = elementTypeWithAnnotations.Type;
switch (collectionTypeKind)
{
case CollectionExpressionTypeKind.None:
return Conversion.NoConversion;

case CollectionExpressionTypeKind.CollectionBuilder:
{
_binder.TryGetCollectionIterationType((Syntax.ExpressionSyntax)syntax, targetType, out TypeWithAnnotations elementTypeWithAnnotations);
_binder.TryGetCollectionIterationType((Syntax.ExpressionSyntax)syntax, targetType, out elementTypeWithAnnotations);
elementType = elementTypeWithAnnotations.Type;
if (elementType is null)
{
Expand Down
19 changes: 9 additions & 10 deletions src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1624,15 +1624,15 @@ private static bool HasAnonymousFunctionConversion(BoundExpression source, TypeS
return IsAnonymousFunctionCompatibleWithType((UnboundLambda)source, destination, compilation) == LambdaConversionResult.Success;
}

internal static CollectionExpressionTypeKind GetCollectionExpressionTypeKind(CSharpCompilation compilation, TypeSymbol destination, out TypeSymbol? elementType)
internal static CollectionExpressionTypeKind GetCollectionExpressionTypeKind(CSharpCompilation compilation, TypeSymbol destination, out TypeWithAnnotations elementType)
{
Debug.Assert(compilation is { });

if (destination is ArrayTypeSymbol arrayType)
{
if (arrayType.IsSZArray)
{
elementType = arrayType.ElementType;
elementType = arrayType.ElementTypeWithAnnotations;
return CollectionExpressionTypeKind.Array;
}
}
Expand All @@ -1659,7 +1659,7 @@ internal static CollectionExpressionTypeKind GetCollectionExpressionTypeKind(CSh
}
else if (implementsSpecialInterface(compilation, destination, SpecialType.System_Collections_Generic_IEnumerable_T))
{
elementType = null;
elementType = default;
return CollectionExpressionTypeKind.ImplementsIEnumerableT;
}
else if (implementsSpecialInterface(compilation, destination, SpecialType.System_Collections_IEnumerable))
Expand All @@ -1670,27 +1670,26 @@ internal static CollectionExpressionTypeKind GetCollectionExpressionTypeKind(CSh
// would be better. If we use CollectionInitializerTypeImplementsIEnumerable() here, we'd need
// to check for nullable to disallow: Nullable<StructCollection> s = [];
// Instead, we just walk the implemented interfaces.
elementType = null;
elementType = default;
return CollectionExpressionTypeKind.ImplementsIEnumerable;
}
else if (destination.IsArrayInterface(out TypeWithAnnotations typeArg))
else if (destination.IsArrayInterface(out elementType))
{
elementType = typeArg.Type;
return CollectionExpressionTypeKind.ArrayInterface;
}

elementType = null;
elementType = default;
return CollectionExpressionTypeKind.None;

static bool isSpanOrListType(CSharpCompilation compilation, TypeSymbol targetType, WellKnownType spanType, [NotNullWhen(true)] out TypeSymbol? elementType)
static bool isSpanOrListType(CSharpCompilation compilation, TypeSymbol targetType, WellKnownType spanType, [NotNullWhen(true)] out TypeWithAnnotations elementType)
{
if (targetType is NamedTypeSymbol { Arity: 1 } namedType
&& ReferenceEquals(namedType.OriginalDefinition, compilation.GetWellKnownType(spanType)))
{
elementType = namedType.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0].Type;
elementType = namedType.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0];
return true;
}
elementType = null;
elementType = default;
return false;
}

Expand Down
40 changes: 29 additions & 11 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,9 @@ private PooledDictionary<BoundExpression, Func<TypeWithAnnotations, TypeWithStat
private static readonly ImmutableArray<BoundKind> s_skippedExpressions = ImmutableArray.Create(BoundKind.ArrayInitialization,
BoundKind.ObjectInitializerExpression,
BoundKind.CollectionInitializerExpression,
BoundKind.DynamicCollectionElementInitializer);
BoundKind.DynamicCollectionElementInitializer,
BoundKind.CollectionExpressionSpreadElement); // https://github.com/dotnet/roslyn/issues/68786: We should handle spreads

#endif

/// <summary>
Expand Down Expand Up @@ -3476,29 +3478,45 @@ protected override void VisitStatement(BoundStatement statement)
// https://github.com/dotnet/roslyn/issues/68786: Use inferInitialObjectState() to set the initial
// state of the instance: see the call to InheritNullableStateOfTrackableStruct() in particular.
int containerSlot = GetOrCreatePlaceholderSlot(node);
bool delayCompletionForType = false; // https://github.com/dotnet/roslyn/issues/68786: Should be true if the collection expression is target typed.
NullableFlowState resultState = NullableFlowState.NotNull;

var collectionKind = ConversionsBase.GetCollectionExpressionTypeKind(this.compilation, node.Type, out var targetElementType);
if (collectionKind is CollectionExpressionTypeKind.CollectionBuilder)
{
var createMethod = node.CollectionBuilderMethod;
if (createMethod is not null)
{
var readOnlySpan = (NamedTypeSymbol)createMethod.Parameters[0].Type;
targetElementType = readOnlySpan.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0];
var annotations = createMethod.GetFlowAnalysisAnnotations();
resultState = ApplyUnconditionalAnnotations(createMethod.ReturnTypeWithAnnotations, annotations).ToTypeWithState().State;
}
}

// https://github.com/dotnet/roslyn/issues/68786: Set nullability of elements from the inferred target type nullability.
foreach (var element in node.Elements)
{
switch (element)
{
case BoundCollectionElementInitializer initializer:
var collectionType = initializer.AddMethod.ContainingType;
var completion = VisitCollectionElementInitializer(initializer, collectionType, delayCompletionForType);
if (completion is { })
{
// https://github.com/dotnet/roslyn/issues/68786: Complete the analysis later.
completion(containerSlot, collectionType);
}

var completion = VisitCollectionElementInitializer(initializer, collectionType,
delayCompletionForType: false /* All collection expressions are target-typed */);

Debug.Assert(completion is null);
break;
case BoundCollectionExpressionSpreadElement spread:
// https://github.com/dotnet/roslyn/issues/68786: We should check the spread
Visit(spread);
Copy link
Contributor

@RikkiGibson RikkiGibson Oct 4, 2023 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything to do besides visit the spread, and warn if the result may be null? (since we want to enumerate the operand value, it should be non-null.) #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may want VisitRvalue here because if a spread puts us into a split state after visiting (like in an error scenario) that could mess things up later on.

Copy link
Member

@cston cston Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, with #70197, NullableWalker.VisitCollectionExpressionSpreadElement() relies on base.VisitCollectionExpressionSpreadElement(node) which simply calls VisitRvalue(node.Expression).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be comfortable with this assuming we have a test demonstrating that things don't fall over when an expression inside a spread splits the state. e.g. [..(x is not null)]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spreads are not in scope for this PR, so I don't want to go too far. I'd be okay with removing this entirely if you prefer.

As things stand, ..(x is null) hits an assertion: System.InvalidOperationException : Analyzed 11 nodes in NullableWalker, but DebugVerifier expects 10. Example of unverified node: BoundCollectionExpressionSpreadElement ..(x is null)

In release mode, the test produces the expected diagnsotic (binding error). Added a test

Copy link
Member Author

@jcouv jcouv Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: a similar assertion is hit if I remove this case, even on simple cases. I still think it's beneficial to keep it in the interim (before we have fully coverage on spreads)

System.InvalidOperationException : Analyzed 33 nodes in NullableWalker, but DebugVerifier expects 27. Example of unverified node: BoundCollectionExpressionSpreadElement ..a

Copy link
Contributor

@RikkiGibson RikkiGibson Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as the compiler doesn't blow up in the retail build in these cases, I'm happy. Since it looks like we are actually visiting the spread operand, I think we are just missing the top-level nullable warning for when the operand may be null. That's OK to address later. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chuck pointed out the assertion problem can be solved by undoing the s_skippedExpressions change. Done

break;
default:
VisitRvalue(element);
_ = VisitOptionalImplicitConversion(element, targetElementType, useLegacyWarnings: false, trackMembers: false, AssignmentKind.Assignment);

break;
}
}

SetResultType(node, TypeWithState.Create(node.Type, NullableFlowState.NotNull));
SetResultType(node, TypeWithState.Create(node.Type, resultState));
return null;
}

Expand Down
Loading
Loading