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

Remove unnecessary tracking of unmatched parameters in method invocation escape analysis #62847

Merged
merged 2 commits into from
Jul 26, 2022
Merged

Remove unnecessary tracking of unmatched parameters in method invocation escape analysis #62847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a673b59
Remove unnecessary tracking of unmatched parameters in method invocat…
cston Jul 21, 2022
529018b
PR feedback
cston Jul 25, 2022
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
129 changes: 43 additions & 86 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 @@ -1486,6 +1486,10 @@ static bool hasRefStructType(Symbol symbol)
bool isRefEscape
)
{
#if DEBUG
Debug.Assert(AllParametersConsideredInEscapeAnalysisHaveArguments(argsOpt, parameters, argsToParamsOpt));
#endif

// SPEC: (also applies to the CheckInvocationEscape counterpart)
//
// An lvalue resulting from a ref-returning method invocation e1.M(e2, ...) is ref-safe - to - escape the smallest of the following scopes:
Expand All @@ -1507,8 +1511,6 @@ bool isRefEscape
//by default it is safe to escape
uint escapeScope = Binder.ExternalScope;

ArrayBuilder<bool>? inParametersMatchedWithArgs = null;

if (!argsOpt.IsDefault)
{
moreArguments:
Expand All @@ -1531,7 +1533,7 @@ bool isRefEscape
goto moreArguments;
}

RefKind effectiveRefKind = GetEffectiveRefKindAndMarkMatchedInParameter(argIndex, argRefKindsOpt, parameters, argsToParamsOpt, ref inParametersMatchedWithArgs, out DeclarationScope scope);
RefKind effectiveRefKind = GetEffectiveRefKind(argIndex, argRefKindsOpt, parameters, argsToParamsOpt, out DeclarationScope scope);

// ref escape scope is the narrowest of
// - ref escape of all byref arguments
Expand All @@ -1554,25 +1556,12 @@ bool isRefEscape

if (escapeScope >= scopeOfTheContainingExpression)
{
// no longer needed
inParametersMatchedWithArgs?.Free();

// can't get any worse
return escapeScope;
}
}
}

// handle omitted optional "in" parameters if there are any
ParameterSymbol? unmatchedInParameter = TryGetUnmatchedInParameterAndFreeMatchedArgs(parameters, ref inParametersMatchedWithArgs);

// unmatched "in" parameter is the same as a literal, its ref escape is scopeOfTheContainingExpression (can't get any worse)
// its val escape is ExternalScope (does not affect overall result)
if (unmatchedInParameter != null && isRefEscape)
{
return scopeOfTheContainingExpression;
}

// check receiver if ref-like
if (receiver?.Type?.IsRefLikeType == true)
{
Expand Down Expand Up @@ -1605,6 +1594,10 @@ bool isRefEscape
bool isRefEscape
)
{
#if DEBUG
Debug.Assert(AllParametersConsideredInEscapeAnalysisHaveArguments(argsOpt, parameters, argsToParamsOpt));
#endif

// SPEC:
// In a method invocation, the following constraints apply:
//• If there is a ref or out argument to a ref struct type (including the receiver), with safe-to-escape E1, then
Expand All @@ -1617,8 +1610,6 @@ bool isRefEscape
receiver = null;
}

ArrayBuilder<bool>? inParametersMatchedWithArgs = null;

if (!argsOpt.IsDefault)
{
moreArguments:
Expand All @@ -1641,7 +1632,7 @@ bool isRefEscape
goto moreArguments;
}

RefKind effectiveRefKind = GetEffectiveRefKindAndMarkMatchedInParameter(argIndex, argRefKindsOpt, parameters, argsToParamsOpt, ref inParametersMatchedWithArgs, out DeclarationScope scope);
RefKind effectiveRefKind = GetEffectiveRefKind(argIndex, argRefKindsOpt, parameters, argsToParamsOpt, out DeclarationScope scope);

// ref escape scope is the narrowest of
// - ref escape of all byref arguments
Expand All @@ -1662,9 +1653,6 @@ bool isRefEscape

if (!valid)
{
// no longer needed
inParametersMatchedWithArgs?.Free();

ErrorCode errorCode = GetStandardCallEscapeError(checkingReceiver);

string parameterName;
Expand All @@ -1689,22 +1677,6 @@ bool isRefEscape
}
}

// handle omitted optional "in" parameters if there are any
ParameterSymbol? unmatchedInParameter = TryGetUnmatchedInParameterAndFreeMatchedArgs(parameters, ref inParametersMatchedWithArgs);

// unmatched "in" parameter is the same as a literal, its ref escape is scopeOfTheContainingExpression (can't get any worse)
// its val escape is ExternalScope (does not affect overall result)
if (unmatchedInParameter != null && isRefEscape)
{
var parameterName = unmatchedInParameter.Name;
if (string.IsNullOrEmpty(parameterName))
{
parameterName = unmatchedInParameter.Ordinal.ToString();
}
Error(diagnostics, GetStandardCallEscapeError(checkingReceiver), syntax, symbol, parameterName);
return false;
}

// check receiver if ref-like
if (receiver?.Type?.IsRefLikeType == true)
{
Expand Down Expand Up @@ -1828,9 +1800,6 @@ bool isRefEscape
}
}

//NB: we do not care about unmatched "in" parameters here.
// They have "outer" val escape, so cannot be worse than escapeTo.

// check val escape of receiver if ref-like
if (receiverOpt?.Type?.IsRefLikeType == true)
{
Expand Down Expand Up @@ -1881,21 +1850,49 @@ void updateEscapeTo(BoundExpression argument, RefKind refKind, uint scopeOfTheCo
};
}

#if DEBUG
private static bool AllParametersConsideredInEscapeAnalysisHaveArguments(
ImmutableArray<BoundExpression> argsOpt,
ImmutableArray<ParameterSymbol> parameters,
ImmutableArray<int> argsToParamsOpt)
{
if (parameters.IsDefaultOrEmpty) return true;

var paramsMatched = BitVector.Create(parameters.Length);
for (int argIndex = 0; argIndex < argsOpt.Length; argIndex++)
{
int paramIndex = argsToParamsOpt.IsDefault ? argIndex : argsToParamsOpt[argIndex];
paramsMatched[paramIndex] = true;
}
for (int paramIndex = 0; paramIndex < parameters.Length; paramIndex++)
{
if (!paramsMatched[paramIndex])
{
// Default arguments for params arrays are not created during
// binding (see https://github.com/dotnet/roslyn/issues/49602),
// but a params array cannot contain references or ref structs.
if (parameters[paramIndex] is not { IsParams: true, Type.TypeKind: TypeKind.Array })
{
return false;
}
}
}
return true;
}
#endif

/// <summary>
/// Gets "effective" ref kind of an argument.
/// If the ref kind is 'in', marks that that corresponding parameter was matched with a value
/// We need that to detect when there were optional 'in' parameters for which values were not supplied.
/// Gets "effective" ref kind of an argument.
///
/// NOTE: Generally we know if a formal argument is passed as ref/out/in by looking at the call site.
/// However, 'in' may also be passed as an ordinary val argument so we need to take a look at corresponding parameter, if such exists.
/// There are cases like params/vararg, when a corresponding parameter may not exist, then val cannot become 'in'.
/// </summary>
private static RefKind GetEffectiveRefKindAndMarkMatchedInParameter(
private static RefKind GetEffectiveRefKind(
int argIndex,
ImmutableArray<RefKind> argRefKindsOpt,
ImmutableArray<ParameterSymbol> parameters,
ImmutableArray<int> argsToParamsOpt,
ref ArrayBuilder<bool>? inParametersMatchedWithArgs,
out DeclarationScope scope)
{
int paramIndex = argIndex < parameters.Length ?
Expand All @@ -1909,53 +1906,13 @@ void updateEscapeTo(BoundExpression argument, RefKind refKind, uint scopeOfTheCo
if (parameters[paramIndex].RefKind == RefKind.In)
{
effectiveRefKind = RefKind.In;
inParametersMatchedWithArgs = inParametersMatchedWithArgs ?? ArrayBuilder<bool>.GetInstance(parameters.Length, fillWithValue: false);
inParametersMatchedWithArgs[paramIndex] = true;
}
}

scope = paramIndex >= 0 ? parameters[paramIndex].Scope : DeclarationScope.Unscoped;
return effectiveRefKind;
}

/// <summary>
/// Gets a "in" parameter for which there is no argument supplied, if such exists.
/// That indicates an optional "in" parameter. We treat it as an RValue passed by reference via a temporary.
/// The effective scope of such variable is the immediately containing scope.
/// </summary>
private static ParameterSymbol? TryGetUnmatchedInParameterAndFreeMatchedArgs(ImmutableArray<ParameterSymbol> parameters, ref ArrayBuilder<bool>? inParametersMatchedWithArgs)
{
try
{
if (!parameters.IsDefault)
{
for (int i = 0; i < parameters.Length; i++)
{
var parameter = parameters[i];
if (parameter.IsParams)
{
break;
}

if (parameter.RefKind == RefKind.In &&
inParametersMatchedWithArgs?[i] != true &&
parameter.Type.IsRefLikeType == false)
{
return parameter;
}
}
}

return null;
}
finally
{
inParametersMatchedWithArgs?.Free();
// make sure noone uses it after.
inParametersMatchedWithArgs = null;
}
}

#nullable disable

private static ErrorCode GetStandardCallEscapeError(bool checkingReceiver)
Expand Down
43 changes: 43 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2341,6 +2341,49 @@ class Program
Diagnostic(ErrorCode.ERR_EscapeVariable, "x").WithArguments("R").WithLocation(16, 71));
}

[Fact]
public void MethodInvocation_Params()
{
var source =
@"ref struct R
{
public R(ref int i) { }
}
class Program
{
static R M1(R input, params object[] array)
{
return input;
}
static R M2()
{
int i = 42;
var r = new R(ref i);
return M1(r);
}
static R M3()
{
int i = 42;
var r = new R(ref i);
return M1(r, 0, 1, 2);
}
}";
var comp = CreateCompilation(source);
comp.VerifyEmitDiagnostics(
// (15,16): error CS8347: Cannot use a result of 'Program.M1(R, params object[])' in this context because it may expose variables referenced by parameter 'input' outside of their declaration scope
// return M1(r);
Diagnostic(ErrorCode.ERR_EscapeCall, "M1(r)").WithArguments("Program.M1(R, params object[])", "input").WithLocation(15, 16),
// (15,19): error CS8352: Cannot use variable 'r' in this context because it may expose referenced variables outside of their declaration scope
// return M1(r);
Diagnostic(ErrorCode.ERR_EscapeVariable, "r").WithArguments("r").WithLocation(15, 19),
// (21,16): error CS8347: Cannot use a result of 'Program.M1(R, params object[])' in this context because it may expose variables referenced by parameter 'input' outside of their declaration scope
// return M1(r, 0, 1, 2);
Diagnostic(ErrorCode.ERR_EscapeCall, "M1(r, 0, 1, 2)").WithArguments("Program.M1(R, params object[])", "input").WithLocation(21, 16),
// (21,19): error CS8352: Cannot use variable 'r' in this context because it may expose referenced variables outside of their declaration scope
// return M1(r, 0, 1, 2);
Diagnostic(ErrorCode.ERR_EscapeVariable, "r").WithArguments("r").WithLocation(21, 19));
}

[Fact]
public void MethodArgumentsMustMatch_01()
{
Expand Down