Skip to content

Extensions: ref safety analysis #77967

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

Merged
merged 8 commits into from
Apr 8, 2025
Merged

Extensions: ref safety analysis #77967

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2b0180d
Extensions: ref safety analysis
jcouv Apr 1, 2025
4e6ab4a
Revert code change
jcouv Apr 3, 2025
eb391b9
Alternate approach
jcouv Apr 3, 2025
7cdc808
Comment
jcouv Apr 4, 2025
87979cc
Address feedback
jcouv Apr 5, 2025
a49e052
Skip if static. Use new parameters for clarity
jcouv Apr 7, 2025
56934ed
Re-use parameters
jcouv Apr 7, 2025
e40d3f8
Address feedback
jcouv Apr 7, 2025
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
124 changes: 107 additions & 17 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 @@ -85,6 +85,35 @@ internal static MethodInfo Create(PropertySymbol property, AccessorKind accessor
internal static MethodInfo Create(BoundIndexerAccess expr) =>
Create(expr.Indexer, expr.AccessorKind);

internal MethodInfo ReplaceWithExtensionImplementation(out bool wasError)
{
var method = replace(Method);
var setMethod = replace(SetMethod);
Symbol symbol = ReferenceEquals(Symbol, Method) && method is not null ? method : Symbol;

wasError = (Method is not null && method is null) || (SetMethod is not null && setMethod is null);

// Tracked by https://github.com/dotnet/roslyn/issues/76130 : Test with indexers (ie. "method") and in compound assignment (ie. "setMethod")
return new MethodInfo(symbol, method, setMethod);

static MethodSymbol? replace(MethodSymbol? method)
{
if (method is null)
{
return null;
}

if (method.OriginalDefinition.TryGetCorrespondingExtensionImplementationMethod() is MethodSymbol implementationMethod)
{
return implementationMethod.AsMember(method.ContainingSymbol.ContainingType).
ConstructIfGeneric(method.ContainingType.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics.Concat(method.TypeArgumentsWithAnnotations));
}

// Tracked by https://github.com/dotnet/roslyn/issues/76130 : Test this code path
return null;
}
}

public override string? ToString() => Method?.ToString();
}

Expand Down Expand Up @@ -1971,9 +2000,12 @@ bool isRefEscape
Debug.Assert(AllParametersConsideredInEscapeAnalysisHaveArguments(argsOpt, parameters, argsToParamsOpt));
#endif

MethodInfo localMethodInfo = methodInfo;
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 7, 2025 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

methodInfo

Is there a specific reason why we keep the original value accessible in the rest of the method through this parameter? This might be bug prone in the long run. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved offline

ReplaceWithExtensionImplementationIfNeeded(ref localMethodInfo, ref parameters, ref receiver, ref argsOpt, ref argRefKindsOpt, ref argsToParamsOpt);

if (methodInfo.UseUpdatedEscapeRules)
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

if (methodInfo.UseUpdatedEscapeRules)

Should this check be moved into the local function?

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to leave this structure/pattern (if (newRules) handleNewRules() else handleOldRules())

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to leave this structure/pattern (if (newRules) handleNewRules() else handleOldRules())

I wasn't suggesting to change that, and I do not think moving the if into the local function would change that.

{
return GetInvocationEscapeWithUpdatedRules(methodInfo, receiver, receiverIsSubjectToCloning, parameters, argsOpt, argRefKindsOpt, argsToParamsOpt, localScopeDepth, isRefEscape);
return GetInvocationEscapeWithUpdatedRules(localMethodInfo, receiver, receiverIsSubjectToCloning, parameters, argsOpt, argRefKindsOpt, argsToParamsOpt, localScopeDepth, isRefEscape);
}

// SPEC: (also applies to the CheckInvocationEscape counterpart)
Expand All @@ -1991,7 +2023,7 @@ bool isRefEscape
SafeContext escapeScope = SafeContext.CallingMethod;
var escapeValues = ArrayBuilder<EscapeValue>.GetInstance();
GetEscapeValuesForOldRules(
in methodInfo,
in localMethodInfo,
// Receiver handled explicitly below
receiver: null,
receiverIsSubjectToCloning: ThreeState.Unknown,
Expand Down Expand Up @@ -2036,7 +2068,7 @@ bool isRefEscape
}

// check receiver if ref-like
if (methodInfo.Method?.RequiresInstanceReceiver == true && receiver?.Type?.IsRefLikeOrAllowsRefLikeType() == true)
if (localMethodInfo.Method?.RequiresInstanceReceiver == true && receiver?.Type?.IsRefLikeOrAllowsRefLikeType() == true)
{
escapeScope = escapeScope.Intersect(GetValEscape(receiver, localScopeDepth));
}
Expand Down Expand Up @@ -2100,6 +2132,57 @@ private SafeContext GetInvocationEscapeWithUpdatedRules(
return escapeScope;
}

private void ReplaceWithExtensionImplementationIfNeeded(ref MethodInfo methodInfo, ref ImmutableArray<ParameterSymbol> parameters,
ref BoundExpression? receiver, ref ImmutableArray<BoundExpression> argsOpt, ref ImmutableArray<RefKind> argRefKindsOpt,
ref ImmutableArray<int> argsToParamsOpt)
{
Symbol? symbol = methodInfo.Symbol;
if (symbol?.GetIsNewExtensionMember() != true)
{
return;
}

var extensionParameter = symbol.ContainingType.ExtensionParameter;
Debug.Assert(extensionParameter is not null);
Debug.Assert(receiver is not null);
methodInfo = methodInfo.ReplaceWithExtensionImplementation(out bool wasError);
if (wasError)
{
return;
}

parameters = parameters.IsDefault ? [extensionParameter] : [extensionParameter, .. parameters];
argsOpt = argsOpt.IsDefault ? [receiver] : [receiver, .. argsOpt];
receiver = null;

if (argRefKindsOpt.IsDefault)
{
if (extensionParameter.RefKind == RefKind.Ref)
{
var argRefKindsBuilder = ArrayBuilder<RefKind>.GetInstance(argsOpt.Length + 1, fillWithValue: RefKind.None);
argRefKindsBuilder[0] = RefKind.Ref;
argRefKindsOpt = argRefKindsBuilder.ToImmutableAndFree();
}
}
else
{
var receiverRefKind = extensionParameter.RefKind == RefKind.Ref ? RefKind.Ref : RefKind.None;
argRefKindsOpt = [receiverRefKind, .. argRefKindsOpt];
}

if (!argsToParamsOpt.IsDefault)
{
var argsToParamsBuilder = ArrayBuilder<int>.GetInstance(argsToParamsOpt.Length + 1);
argsToParamsBuilder.Add(0);
for (int i = 0; i < argsToParamsOpt.Length; i++)
{
argsToParamsBuilder.Add(argsToParamsOpt[i] + 1);
}

argsToParamsOpt = argsToParamsBuilder.ToImmutableAndFree();
}
}

/// <summary>
/// Validates whether given invocation can allow its results to escape from <paramref name="escapeFrom"/> level to <paramref name="escapeTo"/> level.
/// The result indicates whether the escape is possible.
Expand Down Expand Up @@ -2128,9 +2211,12 @@ bool isRefEscape
Debug.Assert(AllParametersConsideredInEscapeAnalysisHaveArguments(argsOpt, parameters, argsToParamsOpt));
#endif

MethodInfo localMethodInfo = methodInfo;
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 7, 2025 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

methodInfo

Similar comment as in GetInvocationEscapeScope #Resolved

ReplaceWithExtensionImplementationIfNeeded(ref localMethodInfo, ref parameters, ref receiver, ref argsOpt, ref argRefKindsOpt, ref argsToParamsOpt);

if (methodInfo.UseUpdatedEscapeRules)
{
return CheckInvocationEscapeWithUpdatedRules(syntax, methodInfo, receiver, receiverIsSubjectToCloning, parameters, argsOpt, argRefKindsOpt, argsToParamsOpt, checkingReceiver, escapeFrom, escapeTo, diagnostics, isRefEscape);
return CheckInvocationEscapeWithUpdatedRules(syntax, localMethodInfo, receiver, receiverIsSubjectToCloning, parameters, argsOpt, argRefKindsOpt, argsToParamsOpt, checkingReceiver, escapeFrom, escapeTo, diagnostics, isRefEscape, methodInfo.Symbol);
}

// SPEC:
Expand All @@ -2139,7 +2225,7 @@ bool isRefEscape
// o no ref or out argument(excluding the receiver and arguments of ref-like types) may have a narrower ref-safe-to-escape than E1; and
// o no argument(including the receiver) may have a narrower safe-to-escape than E1.

var symbol = methodInfo.Symbol;
var symbol = localMethodInfo.Symbol;
if (!symbol.RequiresInstanceReceiver())
{
// ignore receiver when symbol is static
Expand All @@ -2148,7 +2234,7 @@ bool isRefEscape

var escapeArguments = ArrayBuilder<EscapeArgument>.GetInstance();
GetInvocationArgumentsForEscape(
methodInfo,
localMethodInfo,
receiver: null, // receiver handled explicitly below
receiverIsSubjectToCloning: ThreeState.Unknown,
parameters,
Expand Down Expand Up @@ -2180,7 +2266,7 @@ bool isRefEscape
{
if (symbol is not SignatureOnlyMethodSymbol)
{
ReportInvocationEscapeError(syntax, symbol, parameter, checkingReceiver, diagnostics);
ReportInvocationEscapeError(syntax, methodInfo.Symbol, parameter, checkingReceiver, diagnostics);
}

return false;
Expand Down Expand Up @@ -2214,7 +2300,8 @@ private bool CheckInvocationEscapeWithUpdatedRules(
SafeContext escapeFrom,
SafeContext escapeTo,
BindingDiagnosticBag diagnostics,
bool isRefEscape)
bool isRefEscape,
Symbol symbolForReporting)
{
bool result = true;

Expand All @@ -2231,7 +2318,6 @@ private bool CheckInvocationEscapeWithUpdatedRules(
ignoreArglistRefKinds: true, // https://github.com/dotnet/roslyn/issues/63325: for compatibility with C#10 implementation.
argsAndParamsAll);

var symbol = methodInfo.Symbol;
var returnsRefToRefStruct = methodInfo.ReturnsRefToRefStruct;
foreach (var (param, argument, _, isArgumentRefEscape) in argsAndParamsAll)
{
Expand All @@ -2253,9 +2339,9 @@ private bool CheckInvocationEscapeWithUpdatedRules(
// For consistency with C#10 implementation, we don't report an additional error
// for the receiver. (In both implementations, the call to Check*Escape() above
// will have reported a specific escape error for the receiver though.)
if ((object)((argument as BoundCapturedReceiverPlaceholder)?.Receiver ?? argument) != receiver && symbol is not SignatureOnlyMethodSymbol)
if ((object)((argument as BoundCapturedReceiverPlaceholder)?.Receiver ?? argument) != receiver && (Symbol?)methodInfo.Symbol is not SignatureOnlyMethodSymbol)
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

(Symbol?)

Is this cast necessary? #Closed

{
ReportInvocationEscapeError(syntax, symbol, param, checkingReceiver, diagnostics);
ReportInvocationEscapeError(syntax, symbolForReporting, param, checkingReceiver, diagnostics);
}
result = false;
break;
Expand Down Expand Up @@ -2778,17 +2864,20 @@ private bool CheckInvocationArgMixing(
SafeContext localScopeDepth,
BindingDiagnosticBag diagnostics)
{
MethodInfo localMethodInfo = methodInfo;
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 7, 2025 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

methodInfo

Similar comment as in GetInvocationEscapeScope #Resolved

ReplaceWithExtensionImplementationIfNeeded(ref localMethodInfo, ref parameters, ref receiverOpt, ref argsOpt, ref argRefKindsOpt, ref argsToParamsOpt);

if (methodInfo.UseUpdatedEscapeRules)
{
return CheckInvocationArgMixingWithUpdatedRules(syntax, methodInfo, receiverOpt, receiverIsSubjectToCloning, parameters, argsOpt, argRefKindsOpt, argsToParamsOpt, localScopeDepth, diagnostics);
return CheckInvocationArgMixingWithUpdatedRules(syntax, localMethodInfo, receiverOpt, receiverIsSubjectToCloning, parameters, argsOpt, argRefKindsOpt, argsToParamsOpt, localScopeDepth, diagnostics, methodInfo.Symbol);
}

// SPEC:
// In a method invocation, the following constraints apply:
// - If there is a ref or out argument of a ref struct type (including the receiver), with safe-to-escape E1, then
// - no argument (including the receiver) may have a narrower safe-to-escape than E1.

var symbol = methodInfo.Symbol;
var symbol = localMethodInfo.Symbol;
if (!symbol.RequiresInstanceReceiver())
{
// ignore receiver when symbol is static
Expand All @@ -2801,7 +2890,7 @@ private bool CheckInvocationArgMixing(
// collect all writeable ref-like arguments, including receiver
var escapeArguments = ArrayBuilder<EscapeArgument>.GetInstance();
GetInvocationArgumentsForEscape(
methodInfo,
localMethodInfo,
receiverOpt,
receiverIsSubjectToCloning,
parameters,
Expand Down Expand Up @@ -2845,7 +2934,7 @@ private bool CheckInvocationArgMixing(
if (!hasMixingError && !CheckValEscape(argument.Syntax, argument, localScopeDepth, escapeTo, false, diagnostics))
{
string parameterName = GetInvocationParameterName(parameter);
Error(diagnostics, ErrorCode.ERR_CallArgMixing, syntax, symbol, parameterName);
Error(diagnostics, ErrorCode.ERR_CallArgMixing, syntax, methodInfo.Symbol, parameterName);
hasMixingError = true;
}
}
Expand Down Expand Up @@ -2876,7 +2965,8 @@ private bool CheckInvocationArgMixingWithUpdatedRules(
ImmutableArray<RefKind> argRefKindsOpt,
ImmutableArray<int> argsToParamsOpt,
SafeContext localScopeDepth,
BindingDiagnosticBag diagnostics)
BindingDiagnosticBag diagnostics,
Symbol symbolForReporting)
{
var mixableArguments = ArrayBuilder<MixableDestination>.GetInstance();
var escapeValues = ArrayBuilder<EscapeValue>.GetInstance();
Expand Down Expand Up @@ -2913,7 +3003,7 @@ private bool CheckInvocationArgMixingWithUpdatedRules(
if (!valid)
{
string parameterName = GetInvocationParameterName(fromParameter);
Error(diagnostics, ErrorCode.ERR_CallArgMixing, syntax, methodInfo.Symbol, parameterName);
Error(diagnostics, ErrorCode.ERR_CallArgMixing, syntax, symbolForReporting, parameterName);
break;
}
}
Expand Down
Loading