Skip to content

Commit

Permalink
Use correct get / set inside ref safety analysis (#73842)
Browse files Browse the repository at this point in the history 
This fixes a few cases where our code looked at the wrong accessor when doing ref safety analysis. 

closes #35606
closes  #73550
related #73872
related #8253
  • Loading branch information
@jaredpar
jaredpar authored Jul 12, 2024
1 parent e231014 commit 5c77803
Show file tree
Hide file tree
Showing 22 changed files with 1,901 additions and 299 deletions.
654 changes: 459 additions & 195 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
View file

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
View file
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
View file
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 ||
!parameter.RefKind.IsWritableReference())
{
continue;
}

if (escapeFrom > GetValEscape(argument, _localScopeDepth))
{
Error(_diagnostics, ErrorCode.ERR_CallArgMixing, argument.Syntax, constructor, parameter.Name);
}
}

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
View file
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
View file
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
View file
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

0 comments on commit 5c77803

Please sign in to comment.