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.

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

Use correct get / set inside ref safety analysis #73842

Merged
merged 26 commits into from
Jul 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
654 changes: 459 additions & 195 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5762,6 +5762,7 @@ private BoundExpression BindObjectInitializerMember(
ImmutableArray<RefKind> argumentRefKindsOpt = default;
BitVector defaultArguments = default;
bool expanded = false;
AccessorKind accessorKind = AccessorKind.Unknown;

switch (boundMemberKind)
{
Expand Down Expand Up @@ -5800,6 +5801,7 @@ private BoundExpression BindObjectInitializerMember(
argumentRefKindsOpt = indexer.ArgumentRefKindsOpt;
defaultArguments = indexer.DefaultArguments;
expanded = indexer.Expanded;
accessorKind = indexer.AccessorKind;

// If any of the arguments is an interpolated string handler that takes the receiver as an argument for creation,
// we disallow this. During lowering, indexer arguments are evaluated before the receiver for this scenario, and
Expand Down Expand Up @@ -5876,6 +5878,7 @@ private BoundExpression BindObjectInitializerMember(
argsToParamsOpt,
defaultArguments,
resultKind,
accessorKind,
implicitReceiver.Type,
type: boundMember.Type,
hasErrors: hasErrors);
Expand Down Expand Up @@ -9766,6 +9769,7 @@ private BoundExpression BindIndexerOrIndexedPropertyAccessContinued(
argumentNames,
argumentRefKinds,
expanded: resolutionResult.Result.Kind == MemberResolutionKind.ApplicableInExpandedForm,
AccessorKind.Unknown,
argsToParams,
defaultArguments: default,
property.Type,
Expand Down
66 changes: 60 additions & 6 deletions src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs
Original file line number Diff line number Diff line change
Expand Up @@ -667,7 +667,7 @@ protected override void VisitArguments(BoundCall node)
var method = node.Method;
CheckInvocationArgMixing(
node.Syntax,
method,
MethodInfo.Create(method),
node.ReceiverOpt,
node.InitialBindingReceiverIsSubjectToCloning,
method.Parameters,
Expand Down Expand Up @@ -764,9 +764,10 @@ private void VisitObjectCreationExpressionBase(BoundObjectCreationExpressionBase
var constructor = node.Constructor;
if (constructor is { })
{
var methodInfo = MethodInfo.Create(constructor);
CheckInvocationArgMixing(
node.Syntax,
constructor,
in methodInfo,
receiverOpt: null,
receiverIsSubjectToCloning: ThreeState.Unknown,
constructor.Parameters,
Expand All @@ -775,6 +776,58 @@ private void VisitObjectCreationExpressionBase(BoundObjectCreationExpressionBase
node.ArgsToParamsOpt,
_localScopeDepth,
_diagnostics);

if (node.InitializerExpressionOpt is { })
{
// Object initializers are different than a normal constructor in that the
// scope of the receiver is determined by evaluating the inputs to the constructor
// *and* all of the initializers. Another way of thinking about this is that
// every argument in an initializer that can escape to the receiver is
// effectively an argument to the constructor. That means we need to do
// a second mixing pass here where we consider the receiver escaping
// back into the ref parameters of the constructor.
//
// At the moment this is only a hypothetical problem. Because the language
// doesn't support ref field of ref struct mixing like this could not actually
// happen in practice. At the same time we want to error on this now so that
// in a future when we do have ref field to ref struct this is not a breaking
// change. Customers can respond to failures like this by putting scoped on
// such parameters in the constructor.
var escapeValues = ArrayBuilder<EscapeValue>.GetInstance();
var escapeFrom = GetValEscape(node.InitializerExpressionOpt, _localScopeDepth);
GetEscapeValues(
in methodInfo,
receiver: null,
receiverIsSubjectToCloning: ThreeState.Unknown,
constructor.Parameters,
node.Arguments,
node.ArgumentRefKindsOpt,
node.ArgsToParamsOpt,
ignoreArglistRefKinds: false,
mixableArguments: null,
escapeValues);

foreach (var (parameter, argument, _, isRefEscape) in escapeValues)
{
if (!isRefEscape)
{
continue;
}

if (parameter?.Type?.IsRefLikeOrAllowsRefLikeType() != true ||
jaredpar marked this conversation as resolved.
Show resolved Hide resolved
!parameter.RefKind.IsWritableReference())
jaredpar marked this conversation as resolved.
Show resolved Hide resolved
{
continue;
}

if (escapeFrom > GetValEscape(argument, _localScopeDepth))
{
Error(_diagnostics, ErrorCode.ERR_CallArgMixing, argument.Syntax, constructor, parameter.Name);
jaredpar marked this conversation as resolved.
Show resolved Hide resolved
}
}

escapeValues.Free();
}
}
}
}
Expand All @@ -796,7 +849,7 @@ private void VisitObjectCreationExpressionBase(BoundObjectCreationExpressionBase
var indexer = node.Indexer;
CheckInvocationArgMixing(
node.Syntax,
indexer,
MethodInfo.Create(node),
node.ReceiverOpt,
node.InitialBindingReceiverIsSubjectToCloning,
indexer.Parameters,
Expand All @@ -819,7 +872,7 @@ private void VisitObjectCreationExpressionBase(BoundObjectCreationExpressionBase
var method = node.FunctionPointer.Signature;
CheckInvocationArgMixing(
node.Syntax,
method,
MethodInfo.Create(method),
receiverOpt: null,
receiverIsSubjectToCloning: ThreeState.Unknown,
method.Parameters,
Expand Down Expand Up @@ -920,7 +973,7 @@ private void VisitDeconstructionArguments(ArrayBuilder<DeconstructionVariable> v

CheckInvocationArgMixing(
syntax,
deconstructMethod,
MethodInfo.Create(deconstructMethod),
invocation.ReceiverOpt,
invocation.InitialBindingReceiverIsSubjectToCloning,
parameters,
Expand Down Expand Up @@ -1011,7 +1064,7 @@ private static ImmutableArray<BoundExpression> GetDeconstructionRightParts(Bound
out arguments, out refKinds);

collectionEscape = GetInvocationEscapeScope(
equivalentSignatureMethod,
MethodInfo.Create(equivalentSignatureMethod),
receiver: null,
receiverIsSubjectToCloning: ThreeState.Unknown,
equivalentSignatureMethod.Parameters,
Expand All @@ -1038,6 +1091,7 @@ private static ImmutableArray<BoundExpression> GetDeconstructionRightParts(Bound
{
placeholders.Add((targetPlaceholder, collectionEscape));
}

if (node.AwaitOpt is { } awaitableInfo)
{
GetAwaitableInstancePlaceholders(placeholders, awaitableInfo, collectionEscape);
Expand Down
24 changes: 24 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,14 @@ public override Symbol? ExpressionSymbol
}
}

internal enum AccessorKind : byte
{
Unknown,
Get,
Set,
Both
}

internal partial class BoundIndexerAccess
{
public override Symbol? ExpressionSymbol
Expand All @@ -341,6 +349,22 @@ public override LookupResultKind ResultKind
return !this.OriginalIndexersOpt.IsDefault ? LookupResultKind.OverloadResolutionFailure : base.ResultKind;
}
}

public BoundIndexerAccess Update(AccessorKind accessorKind)
{
return this.Update(
ReceiverOpt,
InitialBindingReceiverIsSubjectToCloning,
Indexer,
Arguments,
ArgumentNamesOpt,
ArgumentRefKindsOpt,
Expanded,
accessorKind,
ArgsToParamsOpt,
DefaultArguments,
Type);
}
}

internal partial class BoundDynamicIndexerAccess
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
<ValueType Name="InterpolatedStringHandlerData"/>
<ValueType Name="WellKnownMember"/>
<ValueType Name="CollectionExpressionTypeKind"/>
<ValueType Name="AccessorKind" />

<AbstractNode Name="BoundInitializer" Base="BoundNode"/>

Expand Down Expand Up @@ -2020,6 +2021,7 @@
<Field Name="ArgsToParamsOpt" Type="ImmutableArray&lt;int&gt;" Null="allow"/>
<Field Name="DefaultArguments" Type="BitVector" />
<Field Name="ResultKind" PropertyOverrides="true" Type="LookupResultKind"/>
<Field Name="AccessorKind" Type="AccessorKind" />

<!-- Used by IOperation to reconstruct the receiver for this expression. -->
<Field Name="ReceiverType" Type="TypeSymbol" Null="disallow"/>
Expand Down Expand Up @@ -2208,6 +2210,7 @@
<Field Name="ArgumentNamesOpt" Type="ImmutableArray&lt;string?&gt;" Null="allow"/>
<Field Name="ArgumentRefKindsOpt" Type="ImmutableArray&lt;RefKind&gt;" Null="allow"/>
<Field Name="Expanded" Type="bool"/>
<Field Name="AccessorKind" Type="AccessorKind" />
<Field Name="ArgsToParamsOpt" Type="ImmutableArray&lt;int&gt;" Null="allow"/>
<Field Name="DefaultArguments" Type="BitVector" />
<!--The set of indexer symbols from which this call's indexer was chosen.
Expand Down
9 changes: 6 additions & 3 deletions src/Compilers/CSharp/Portable/BoundTree/Constructors.cs
Original file line number Diff line number Diff line change
Expand Up @@ -280,9 +280,10 @@ public static BoundIndexerAccess ErrorAccess(
namedArguments,
refKinds,
expanded: false,
accessorKind: AccessorKind.Unknown,
argsToParamsOpt: default(ImmutableArray<int>),
defaultArguments: default(BitVector),
originalIndexers,
originalIndexersOpt: originalIndexers,
type: indexer.Type,
hasErrors: true);
}
Expand All @@ -295,11 +296,12 @@ public BoundIndexerAccess(
ImmutableArray<string?> argumentNamesOpt,
ImmutableArray<RefKind> argumentRefKindsOpt,
bool expanded,
AccessorKind accessorKind,
ImmutableArray<int> argsToParamsOpt,
BitVector defaultArguments,
TypeSymbol type,
bool hasErrors = false) :
this(syntax, receiverOpt, initialBindingReceiverIsSubjectToCloning, indexer, arguments, argumentNamesOpt, argumentRefKindsOpt, expanded, argsToParamsOpt, defaultArguments, originalIndexersOpt: default, type, hasErrors)
this(syntax, receiverOpt, initialBindingReceiverIsSubjectToCloning, indexer, arguments, argumentNamesOpt, argumentRefKindsOpt, expanded, accessorKind, argsToParamsOpt, defaultArguments, originalIndexersOpt: default, type, hasErrors)
{ }

public BoundIndexerAccess Update(BoundExpression? receiverOpt,
Expand All @@ -309,10 +311,11 @@ public BoundIndexerAccess Update(BoundExpression? receiverOpt,
ImmutableArray<string?> argumentNamesOpt,
ImmutableArray<RefKind> argumentRefKindsOpt,
bool expanded,
AccessorKind accessorKind,
ImmutableArray<int> argsToParamsOpt,
BitVector defaultArguments,
TypeSymbol type)
=> Update(receiverOpt, initialBindingReceiverIsSubjectToCloning, indexer, arguments, argumentNamesOpt, argumentRefKindsOpt, expanded, argsToParamsOpt, defaultArguments, this.OriginalIndexersOpt, type);
=> Update(receiverOpt, initialBindingReceiverIsSubjectToCloning, indexer, arguments, argumentNamesOpt, argumentRefKindsOpt, expanded, accessorKind, argsToParamsOpt, defaultArguments, this.OriginalIndexersOpt, type);
}

internal sealed partial class BoundConversion
Expand Down
Loading
Loading