Skip to content

Add ReturnOnly scope #64090

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
RikkiGibson merged 9 commits into from
Sep 20, 2022
Merged

Add ReturnOnly scope #64090

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ec524e6
Add ReturnOnly scope
RikkiGibson Sep 16, 2022
29839c6
enable haskell mode
RikkiGibson Sep 16, 2022
dca30f5
fix RefLocalsAndReturnsTests.RefReadonlyCall
RikkiGibson Sep 17, 2022
e582e59
Add suggested tests
RikkiGibson Sep 19, 2022
d8a761e
simplify
RikkiGibson Sep 19, 2022
d0749e5
add a bit of test coverage
RikkiGibson Sep 19, 2022
95e8f68
address some test feedback
RikkiGibson Sep 19, 2022
efb9aa8
more
RikkiGibson Sep 19, 2022
33438c0
more
RikkiGibson Sep 19, 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
123 changes: 44 additions & 79 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 @@ -22,17 +22,20 @@ internal partial class Binder
/// <summary>
/// For the purpose of escape verification we operate with the depth of local scopes.
/// The depth is a uint, with smaller number representing shallower/wider scopes.
/// The 0 and 1 are special scopes -
/// 0 is the "external" or "return" scope that is outside of the containing method/lambda.
/// If something can escape to scope 0, it can escape to any scope in a given method or can be returned.
/// 1 is the "parameter" or "top" scope that is just inside the containing method/lambda.
/// 0, 1 and 2 are special scopes -
/// 0 is the "calling method" scope that is outside of the containing method/lambda.
/// If something can escape to scope 0, it can escape to any scope in a given method through a ref parameter or return.
/// 1 is the "return-only" scope that is outside of the containing method/lambda.
/// If something can escape to scope 1, it can escape to any scope in a given method or can be returned, but it can't escape through a ref parameter.
/// 2 is the "current method" scope that is just inside the containing method/lambda.
/// If something can escape to scope 1, it can escape to any scope in a given method, but cannot be returned.
/// n + 1 corresponds to scopes immediately inside a scope of depth n.
/// Since sibling scopes do not intersect and a value cannot escape from one to another without
/// escaping to a wider scope, we can use simple depth numbering without ambiguity.
/// </summary>
internal const uint ExternalScope = 0;
internal const uint TopLevelScope = 1;
internal const uint ReturnOnlyScope = 1;
internal const uint TopLevelScope = 2;

// Some value kinds are semantically the same and the only distinction is how errors are reported
// for those purposes we reserve lowest 2 bits
Expand Down Expand Up @@ -719,7 +722,7 @@ private static bool CheckLocalRefEscape(SyntaxNode node, BoundLocal local, uint
return true;
}

if (escapeTo == Binder.ExternalScope)
if (escapeTo is Binder.ExternalScope or Binder.ReturnOnlyScope)
{
if (localSymbol.RefKind == RefKind.None)
{
Expand Down Expand Up @@ -794,21 +797,17 @@ private uint GetParameterValEscape(ParameterSymbol parameter)

private uint GetParameterRefEscape(ParameterSymbol parameter)
{
if (UseUpdatedEscapeRules)

@RikkiGibson RikkiGibson Sep 16, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I suspect that more places where this check is used can be simplified. If the ParameterSymbol APIs behave appropriately depending on which escape rules are in use, then doing these checks separately could be redundant.

{
return parameter.RefKind is RefKind.None || parameter.EffectiveScope != DeclarationScope.Unscoped ? Binder.TopLevelScope : Binder.ExternalScope;
}
else
return parameter switch
{
// byval parameters can escape to method's top level. Others can escape further.
// NOTE: "method" here means nearest containing method, lambda or local function.
return parameter.RefKind == RefKind.None ? Binder.TopLevelScope : Binder.ExternalScope;
}
{ RefKind: RefKind.None } or { EffectiveScope: not DeclarationScope.Unscoped } => Binder.TopLevelScope,
{ EffectiveScope: DeclarationScope.Unscoped, Type.IsRefLikeType: true } => Binder.ReturnOnlyScope,

@cston cston Sep 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

EffectiveScope: DeclarationScope.Unscoped

Is this check unnecessary given the or above?

@RikkiGibson RikkiGibson Sep 19, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes. The state machine won't test the property again in this path, though.

It feels like it might be good to either:

  • add RefKind: not RefKind.None to make the case fully self-describing, or
  • remove EffectiveScope to try and reduce redundancy.

@RikkiGibson RikkiGibson Sep 19, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I went with removing.

_ => Binder.ExternalScope
};
}

private bool CheckParameterValEscape(SyntaxNode node, BoundParameter parameter, uint escapeTo, BindingDiagnosticBag diagnostics)
{
Debug.Assert(escapeTo == Binder.ExternalScope);
Debug.Assert(escapeTo is Binder.ExternalScope or Binder.ReturnOnlyScope);
if (UseUpdatedEscapeRules)
{
var parameterSymbol = parameter.ParameterSymbol;
Expand All @@ -826,22 +825,32 @@ private bool CheckParameterValEscape(SyntaxNode node, BoundParameter parameter,
}
}

private bool CheckParameterRefEscape(SyntaxNode node, BoundParameter parameter, uint escapeTo, bool checkingReceiver, BindingDiagnosticBag diagnostics)
private bool CheckParameterRefEscape(SyntaxNode node, BoundExpression parameter, ParameterSymbol parameterSymbol, uint escapeTo, bool checkingReceiver, BindingDiagnosticBag diagnostics)
{
var parameterSymbol = parameter.ParameterSymbol;

if (GetParameterRefEscape(parameterSymbol) > escapeTo)
var refSafeToEscape = GetParameterRefEscape(parameterSymbol);
if (refSafeToEscape > escapeTo)
{
var isRefScoped = parameterSymbol.EffectiveScope == DeclarationScope.RefScoped;
Debug.Assert(parameterSymbol.RefKind == RefKind.None || isRefScoped);
if (checkingReceiver)
Debug.Assert(parameterSymbol.RefKind == RefKind.None || isRefScoped || refSafeToEscape == Binder.ReturnOnlyScope);

if (parameter is BoundThisReference)
{
Error(diagnostics, isRefScoped ? ErrorCode.ERR_RefReturnScopedParameter2 : ErrorCode.ERR_RefReturnParameter2, parameter.Syntax, parameterSymbol.Name);
Error(diagnostics, ErrorCode.ERR_RefReturnStructThis, node);
return false;
}
else

#pragma warning disable format
var (errorCode, syntax) = (checkingReceiver, isRefScoped, refSafeToEscape) switch
{
Error(diagnostics, isRefScoped ? ErrorCode.ERR_RefReturnScopedParameter : ErrorCode.ERR_RefReturnParameter, node, parameterSymbol.Name);
}
(checkingReceiver: true, isRefScoped: true, _) => (ErrorCode.ERR_RefReturnScopedParameter2, parameter.Syntax),
(checkingReceiver: true, isRefScoped: false, Binder.ReturnOnlyScope) => (ErrorCode.ERR_RefReturnOnlyParameter, parameter.Syntax),
(checkingReceiver: true, isRefScoped: false, _) => (ErrorCode.ERR_RefReturnParameter2, parameter.Syntax),
(checkingReceiver: false, isRefScoped: true, _) => (ErrorCode.ERR_RefReturnScopedParameter, node),
(checkingReceiver: false, isRefScoped: false, Binder.ReturnOnlyScope) => (ErrorCode.ERR_RefReturnOnlyParameter, node),
(checkingReceiver: false, isRefScoped: false, _) => (ErrorCode.ERR_RefReturnParameter, node)
};
#pragma warning restore format
Error(diagnostics, errorCode, syntax, parameterSymbol.Name);
return false;
}

Expand Down Expand Up @@ -2362,7 +2371,7 @@ private static ErrorCode GetStandardLvalueError(BindValueKind kind)

private static ErrorCode GetStandardRValueRefEscapeError(uint escapeTo)
{
if (escapeTo == Binder.ExternalScope)
if (escapeTo is Binder.ExternalScope or Binder.ReturnOnlyScope)
{
return ErrorCode.ERR_RefReturnLvalueExpected;
}
Expand Down Expand Up @@ -2524,28 +2533,9 @@ internal uint GetRefEscape(BoundExpression expr, uint scopeOfTheContainingExpres
return ((BoundLocal)expr).LocalSymbol.RefEscapeScope;

case BoundKind.ThisReference:
Debug.Assert(this.ContainingMember() is MethodSymbol { ThisParameter: not null });

var thisref = (BoundThisReference)expr;

// "this" is an RValue, unless in a struct.
if (!thisref.Type.IsValueType)
{
break;
}

if (UseUpdatedEscapeRules)
{
if (this.ContainingMember() is MethodSymbol { ThisParameter: var thisParameter } &&
thisParameter.EffectiveScope == DeclarationScope.Unscoped)
{
return Binder.ExternalScope;
}
}

//"this" is not returnable by reference in a struct.
// can ref escape to any other level
return Binder.TopLevelScope;
var thisParam = ((MethodSymbol)this.ContainingMember()).ThisParameter;
Debug.Assert(thisParam.Type.Equals(((BoundThisReference)expr).Type, TypeCompareKind.ConsiderEverything));
return GetParameterRefEscape(thisParam);

case BoundKind.ConditionalOperator:
var conditional = (BoundConditionalOperator)expr;
Expand Down Expand Up @@ -2761,7 +2751,7 @@ internal bool CheckRefEscape(SyntaxNode node, BoundExpression expr, uint escapeF
case BoundKind.RefValueOperator:
// The undocumented __refvalue(tr, T) expression results in an lvalue of type T.
// for compat reasons it is not ref-returnable (since TypedReference is not val-returnable)
if (escapeTo == Binder.ExternalScope)
if (escapeTo is Binder.ExternalScope or Binder.ReturnOnlyScope)
{
break;
}
Expand All @@ -2782,41 +2772,16 @@ internal bool CheckRefEscape(SyntaxNode node, BoundExpression expr, uint escapeF

case BoundKind.Parameter:
var parameter = (BoundParameter)expr;
return CheckParameterRefEscape(node, parameter, escapeTo, checkingReceiver, diagnostics);
return CheckParameterRefEscape(node, parameter, parameter.ParameterSymbol, escapeTo, checkingReceiver, diagnostics);

case BoundKind.Local:
var local = (BoundLocal)expr;
return CheckLocalRefEscape(node, local, escapeTo, checkingReceiver, diagnostics);

case BoundKind.ThisReference:
Debug.Assert(this.ContainingMember() is MethodSymbol { ThisParameter: not null });

var thisref = (BoundThisReference)expr;

// "this" is an RValue, unless in a struct.
if (!thisref.Type.IsValueType)
{
break;
}

//"this" is not returnable by reference in a struct.
if (escapeTo == Binder.ExternalScope)
{
if (UseUpdatedEscapeRules)
{
if (this.ContainingMember() is MethodSymbol { ThisParameter: var thisParameter } &&
thisParameter.EffectiveScope == DeclarationScope.Unscoped)
{
// can ref escape to any other level
return true;
}
}
Error(diagnostics, ErrorCode.ERR_RefReturnStructThis, node);
return false;
}

// can ref escape to any other level
return true;
var thisParam = ((MethodSymbol)this.ContainingMember()).ThisParameter;
Debug.Assert(thisParam.Type.Equals(((BoundThisReference)expr).Type, TypeCompareKind.ConsiderEverything));
return CheckParameterRefEscape(node, expr, thisParam, escapeTo, checkingReceiver, diagnostics);

case BoundKind.ConditionalOperator:
var conditional = (BoundConditionalOperator)expr;
Expand Down
10 changes: 5 additions & 5 deletions src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ private BoundStatement BindYieldReturnStatement(YieldStatementSyntax node, Bindi
BoundExpression argument = (node.Expression == null)
? BadExpression(node).MakeCompilerGenerated()
: BindValue(node.Expression, diagnostics, BindValueKind.RValue);
argument = ValidateEscape(argument, ExternalScope, isByRef: false, diagnostics: diagnostics);
argument = ValidateEscape(argument, ReturnOnlyScope, isByRef: false, diagnostics: diagnostics);

if (!argument.HasAnyErrors)
{
Expand Down Expand Up @@ -1546,7 +1546,7 @@ private BoundAssignmentOperator BindAssignment(
var rightEscape = GetRefEscape(op2, LocalScopeDepth);
if (leftEscape < rightEscape)
{
Error(diagnostics, ErrorCode.ERR_RefAssignNarrower, node, getName(op1), op2.Syntax);
Error(diagnostics, rightEscape == Binder.ReturnOnlyScope ? ErrorCode.ERR_RefAssignReturnOnly : ErrorCode.ERR_RefAssignNarrower, node, getName(op1), op2.Syntax);
op2 = ToBadExpression(op2);
}
}
Expand Down Expand Up @@ -2979,7 +2979,7 @@ private BoundStatement BindReturn(ReturnStatementSyntax syntax, BindingDiagnosti
else
{
arg = CreateReturnConversion(syntax, diagnostics, arg, sigRefKind, retType);
arg = ValidateEscape(arg, Binder.ExternalScope, refKind != RefKind.None, diagnostics);
arg = ValidateEscape(arg, Binder.ReturnOnlyScope, refKind != RefKind.None, diagnostics);
}
}
}
Expand Down Expand Up @@ -3421,7 +3421,7 @@ static BoundBlock bindExpressionBodyAsBlockInternal(ArrowExpressionClauseSyntax
ExpressionSyntax expressionSyntax = expressionBody.Expression.CheckAndUnwrapRefExpression(diagnostics, out refKind);
BindValueKind requiredValueKind = bodyBinder.GetRequiredReturnValueKind(refKind);
BoundExpression expression = bodyBinder.BindValue(expressionSyntax, diagnostics, requiredValueKind);
expression = bodyBinder.ValidateEscape(expression, Binder.ExternalScope, refKind != RefKind.None, diagnostics);
expression = bodyBinder.ValidateEscape(expression, Binder.ReturnOnlyScope, refKind != RefKind.None, diagnostics);

return bodyBinder.CreateBlockFromExpression(expressionBody, bodyBinder.GetDeclaredLocalsForScope(expressionBody), refKind, expression, expressionSyntax, diagnostics);
}
Expand All @@ -3439,7 +3439,7 @@ public BoundBlock BindLambdaExpressionAsBlock(ExpressionSyntax body, BindingDiag
var expressionSyntax = body.CheckAndUnwrapRefExpression(diagnostics, out refKind);
BindValueKind requiredValueKind = GetRequiredReturnValueKind(refKind);
BoundExpression expression = bodyBinder.BindValue(expressionSyntax, diagnostics, requiredValueKind);
expression = ValidateEscape(expression, Binder.ExternalScope, refKind != RefKind.None, diagnostics);
expression = ValidateEscape(expression, Binder.ReturnOnlyScope, refKind != RefKind.None, diagnostics);

return bodyBinder.CreateBlockFromExpression(body, bodyBinder.GetDeclaredLocalsForScope(body), refKind, expression, expressionSyntax, diagnostics);
}
Expand Down
9 changes: 9 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5080,6 +5080,12 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_RefReturnScopedParameter2" xml:space="preserve">
<value>Cannot return by reference a member of parameter '{0}' because it is scoped to the current method</value>
</data>
<data name="ERR_RefReturnOnlyParameter" xml:space="preserve">
<value>Cannot return a parameter by reference '{0}' through a ref parameter; it can only be returned in a return statement</value>
</data>
<data name="ERR_RefReturnOnlyParameter2" xml:space="preserve">
<value>Cannot return by reference a member of parameter '{0}' through a ref parameter; it can only be returned in a return statement</value>
</data>
<data name="ERR_RefReturnLocal" xml:space="preserve">
<value>Cannot return local '{0}' by reference because it is not a ref local</value>
</data>
Expand Down Expand Up @@ -5794,6 +5800,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_RefAssignNarrower" xml:space="preserve">
<value>Cannot ref-assign '{1}' to '{0}' because '{1}' has a narrower escape scope than '{0}'.</value>
</data>
<data name="ERR_RefAssignReturnOnly" xml:space="preserve">
<value>Cannot ref-assign '{1}' to '{0}' because '{1}' can only escape the current method through a return statement.</value>
</data>
<data name="IDS_FeatureEnumGenericTypeConstraint" xml:space="preserve">
<value>enum generic type constraints</value>
</data>
Expand Down
5 changes: 4 additions & 1 deletion src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2124,7 +2124,10 @@ internal enum ErrorCode
WRN_ScopedMismatchInParameterOfTarget = 9073,
WRN_ScopedMismatchInParameterOfOverrideOrImplementation = 9074,
ERR_RefReturnScopedParameter = 9075,
ERR_RefReturnScopedParameter2 = 9076
ERR_RefReturnScopedParameter2 = 9076,
ERR_RefReturnOnlyParameter = 9077,
ERR_RefReturnOnlyParameter2 = 9078,
ERR_RefAssignReturnOnly = 9079,

#endregion

Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2228,6 +2228,9 @@ internal static bool IsBuildOnlyDiagnostic(ErrorCode code)
case ErrorCode.WRN_ScopedMismatchInParameterOfOverrideOrImplementation:
case ErrorCode.ERR_RefReturnScopedParameter:
case ErrorCode.ERR_RefReturnScopedParameter2:
case ErrorCode.ERR_RefReturnOnlyParameter:
case ErrorCode.ERR_RefReturnOnlyParameter2:
case ErrorCode.ERR_RefAssignReturnOnly:
return false;
default:
// NOTE: All error codes must be explicitly handled in this switch statement
Expand Down
15 changes: 15 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading