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

Implement more validation for inline array element access and conversion scenarios #68146

Merged
merged 3 commits into from
May 12, 2023
Merged

Implement more validation for inline array element access and conversion scenarios #68146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e49802d
Implement more validation for inline array element access and convers…
AlekseyTs May 9, 2023
abd7c23
Add tests
AlekseyTs May 11, 2023
f792e12
PR feedback
AlekseyTs May 12, 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
2 changes: 1 addition & 1 deletion 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 @@ -743,7 +743,7 @@ internal bool CheckValueKind(SyntaxNode node, BoundExpression expr, BindValueKin
return true;
}

getItemOrSliceHelper = getItemOrSliceHelper.AsMember(getItemOrSliceHelper.ContainingType.Construct(ImmutableArray.Create(elementAccess.Expression.Type.TryGetInlineArrayElementType())));
getItemOrSliceHelper = getItemOrSliceHelper.AsMember(getItemOrSliceHelper.ContainingType.Construct(ImmutableArray.Create(elementAccess.Expression.Type.TryGetInlineArrayElementField().TypeWithAnnotations)));

return CheckMethodReturnValueKind(getItemOrSliceHelper, elementAccess.Syntax, node, valueKind, checkingReceiver, diagnostics);
}
Expand Down
3 changes: 1 addition & 2 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 @@ -412,8 +412,7 @@ void checkConstraintLanguageVersionAndRuntimeSupportForConversion(SyntaxNode syn
{
// PROTOTYPE(InlineArrays): Check runtime support
CheckFeatureAvailability(syntax, MessageID.IDS_FeatureInlineArrays, diagnostics);

// PROTOTYPE(InlineArrays): Report use site errors for the field, it might have some custom modifiers that we do not like.
diagnostics.ReportUseSite(source.Type!.TryGetInlineArrayElementField(), syntax);

if (destination.OriginalDefinition.Equals(Compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T), TypeCompareKind.AllIgnoreOptions))
{
Expand Down
21 changes: 12 additions & 9 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7985,10 +7985,10 @@ private BoundExpression BindElementOrIndexerAccess(ExpressionSyntax node, BoundE

if (allowInlineArrayElementAccess &&
!InAttributeArgument && !InParameterDefaultValue && // These checks prevent cycles caused by attribute binding when HasInlineArrayAttribute check triggers that.
expr.Type.HasInlineArrayAttribute(out _) && analyzedArguments.Arguments.Count == 1 && expr.Type.TryGetInlineArrayElementType() is { HasType: true } elementType &&
expr.Type.HasInlineArrayAttribute(out _) && analyzedArguments.Arguments.Count == 1 && expr.Type.TryGetInlineArrayElementField() is FieldSymbol elementField &&
tryImplicitConversionToInlineArrayIndex(node, analyzedArguments.Arguments[0], diagnostics, out WellKnownType indexOrRangeWellknownType) is { } convertedIndex)
{
return bindInlineArrayElementAccess(node, expr, analyzedArguments, convertedIndex, indexOrRangeWellknownType, elementType, diagnostics);
return bindInlineArrayElementAccess(node, expr, analyzedArguments, convertedIndex, indexOrRangeWellknownType, elementField, diagnostics);
}

return BindElementAccessCore(node, expr, analyzedArguments, diagnostics);
Expand Down Expand Up @@ -8019,7 +8019,7 @@ BoundExpression tryImplicitConversionToInlineArrayIndex(ExpressionSyntax node, B
return convertedIndex;
}

BoundExpression bindInlineArrayElementAccess(ExpressionSyntax node, BoundExpression expr, AnalyzedArguments analyzedArguments, BoundExpression convertedIndex, WellKnownType indexOrRangeWellknownType, TypeWithAnnotations elementType, BindingDiagnosticBag diagnostics)
BoundExpression bindInlineArrayElementAccess(ExpressionSyntax node, BoundExpression expr, AnalyzedArguments analyzedArguments, BoundExpression convertedIndex, WellKnownType indexOrRangeWellknownType, FieldSymbol elementField, BindingDiagnosticBag diagnostics)
{
// Check required well-known members. They may not be needed
// during lowering, but it's simpler to always require them to prevent
Expand Down Expand Up @@ -8087,13 +8087,16 @@ BoundExpression bindInlineArrayElementAccess(ExpressionSyntax node, BoundExpress

_ = GetWellKnownTypeMember(WellKnownMember.System_Runtime_CompilerServices_Unsafe__As_T, diagnostics, syntax: node);
_ = GetWellKnownTypeMember(createSpanHelper, diagnostics, syntax: node);
_ = GetWellKnownTypeMember(getItemOrSliceHelper, diagnostics, syntax: node);
var spanMethod = GetWellKnownTypeMember(getItemOrSliceHelper, diagnostics, syntax: node);

if (spanMethod is { ContainingType: { Kind: SymbolKind.NamedType } spanType })
{
spanType.Construct(ImmutableArray.Create(elementField.TypeWithAnnotations)).CheckConstraints(new ConstraintsHelper.CheckConstraintsArgs(this.Compilation, this.Conversions, node.GetLocation(), diagnostics));
}

// PROTOTYPE(InlineArrays): Check runtime support
CheckFeatureAvailability(node, MessageID.IDS_FeatureInlineArrays, diagnostics);

// PROTOTYPE(InlineArrays): Verify constraints for the elementType (can be used as a type argument)
// PROTOTYPE(InlineArrays): Report use site errors for the field, it might have some custom modifiers that we do not like.
diagnostics.ReportUseSite(elementField, node);

TypeSymbol resultType;

Expand All @@ -8102,11 +8105,11 @@ BoundExpression bindInlineArrayElementAccess(ExpressionSyntax node, BoundExpress
// The symbols will be verified as return types of 'createSpanHelper', no need to check them again
resultType = Compilation.GetWellKnownType(
getItemOrSliceHelper is WellKnownMember.System_ReadOnlySpan_T__Slice_Int_Int ? WellKnownType.System_ReadOnlySpan_T : WellKnownType.System_Span_T).
Construct(ImmutableArray.Create(elementType));
Construct(ImmutableArray.Create(elementField.TypeWithAnnotations));
}
else
{
resultType = elementType.Type;
resultType = elementField.Type;
}

return new BoundInlineArrayAccess(node, expr, convertedIndex, isValue, getItemOrSliceHelper, resultType);
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -1104,7 +1104,7 @@ private Conversion ClassifyImplicitBuiltInConversionFromExpression(BoundExpressi
// Neither Span<T>, nor ReadOnlySpan<T> can be wrapped into a Nullable<T>, therefore, there is no point to check for an attempt to convert to Nullable types here.
if (!IsAttributeArgumentBinding && !IsParameterDefaultValueBinding && // These checks prevent cycles caused by attribute binding when HasInlineArrayAttribute check triggers that.
source?.HasInlineArrayAttribute(out _) == true &&
source.TryGetInlineArrayElementType() is { HasType: true } elementType &&
source.TryGetInlineArrayElementField() is { TypeWithAnnotations: var elementType } &&
(destination.OriginalDefinition.Equals(Compilation.GetWellKnownType(WellKnownType.System_Span_T), TypeCompareKind.AllIgnoreOptions) ||
destination.OriginalDefinition.Equals(Compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T), TypeCompareKind.AllIgnoreOptions)) &&
HasIdentityConversionInternal(((NamedTypeSymbol)destination.OriginalDefinition).Construct(ImmutableArray.Create(elementType)), destination))
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/BoundTree/BoundInlineArrayAccess.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,15 @@ WellKnownMember.System_ReadOnlySpan_T__Slice_Int_Int or
);
#pragma warning restore format

Debug.Assert(((NamedTypeSymbol)Type).TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0].Equals(Expression.Type!.TryGetInlineArrayElementType(), TypeCompareKind.ConsiderEverything));
Debug.Assert(((NamedTypeSymbol)Type).TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0].Equals(Expression.Type?.TryGetInlineArrayElementField()?.TypeWithAnnotations ?? default, TypeCompareKind.ConsiderEverything));
}
else
{
Debug.Assert(GetItemOrSliceHelper is
WellKnownMember.System_ReadOnlySpan_T__get_Item or
WellKnownMember.System_Span_T__get_Item);

Debug.Assert(Type.Equals(Expression.Type!.TryGetInlineArrayElementType().Type, TypeCompareKind.ConsiderEverything));
Debug.Assert(Type.Equals(Expression.Type?.TryGetInlineArrayElementField()?.Type, TypeCompareKind.ConsiderEverything));
}
#endif
}
Expand Down
4 changes: 2 additions & 2 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 @@ -4436,7 +4436,7 @@ private static void GetArrayElements(BoundArrayInitialization node, ArrayBuilder

VisitRvalue(node.Argument);

TypeWithAnnotations type = expressionType.TryGetInlineArrayElementType();
TypeWithAnnotations type = expressionType.TryGetInlineArrayElementField()!.TypeWithAnnotations;

if (node.GetItemOrSliceHelper is WellKnownMember.System_ReadOnlySpan_T__Slice_Int_Int or WellKnownMember.System_Span_T__Slice_Int_Int)
{
Expand Down Expand Up @@ -7405,7 +7405,7 @@ private bool UseExpressionForConversion([NotNullWhen(true)] BoundExpression? val
default:
if (!_binder.InAttributeArgument && !_binder.InParameterDefaultValue && // These checks prevent cycles caused by attribute binding when HasInlineArrayAttribute check triggers that.
value.Type.HasInlineArrayAttribute(out _) == true &&
value.Type.TryGetInlineArrayElementType() is { HasType: true })
value.Type.TryGetInlineArrayElementField() is not null)
{
return true;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ private BoundExpression MakeIndexerAccess(
createSpan = _factory.ModuleBuilderOpt.EnsureInlineArrayAsSpanExists(node.Syntax, spanType, (NamedTypeSymbol)getItemOrSliceHelper.Parameters[0].Type, _diagnostics.DiagnosticBag);
}

createSpan = createSpan.Construct(node.Expression.Type, node.Expression.Type.TryGetInlineArrayElementType().Type);
createSpan = createSpan.Construct(node.Expression.Type, node.Expression.Type.TryGetInlineArrayElementField()!.Type);

getItemOrSliceHelper = getItemOrSliceHelper.AsMember((NamedTypeSymbol)createSpan.ReturnType);

Expand Down
22 changes: 14 additions & 8 deletions src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2469,31 +2469,37 @@ ITypeSymbol ITypeSymbolInternal.GetITypeSymbol()

internal abstract bool HasInlineArrayAttribute(out int length);

internal TypeWithAnnotations TryGetInlineArrayElementType()
#nullable enable
internal FieldSymbol? TryGetInlineArrayElementField()
{
Debug.Assert(HasInlineArrayAttribute(out var length) && length > 0);

TypeWithAnnotations elementType = default;
FieldSymbol? elementField = null;

if (this.TypeKind == TypeKind.Struct)
{
foreach (Symbol member in GetMembers())
foreach (FieldSymbol field in ((NamedTypeSymbol)this).OriginalDefinition.GetFieldsToEmit())
{
if (member is FieldSymbol { IsStatic: false } field)
if (!field.IsStatic)
{
if (field.RefKind != RefKind.None || elementType.HasType)
if (field.RefKind != RefKind.None || elementField is not null)
{
return default;
return null;
}
else
{
elementType = field.TypeWithAnnotations;
elementField = field;
}
}
}
}

return elementType;
if (elementField is not null && elementField.ContainingType.IsGenericType)
Copy link
Member

@cston cston May 11, 2023 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

elementField.ContainingType.IsGenericType

Are we testing nested types, with and without generic type parameters? #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs May 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Are we testing nested types, with and without generic type parameters?

We are not at the moment, but IsGenericType check covers the scenario. I will add a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests.

{
elementField = elementField.AsMember((NamedTypeSymbol)this);
}

return elementField;
}
}
}
Loading