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

return, break and continue expressions #20269

Closed
wants to merge 1 commit into from
Closed
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
6 changes: 3 additions & 3 deletions build/Rulesets/Roslyn_BuildRules.ruleset
Original file line number Diff line number Diff line change
Expand Up @@ -85,13 +85,13 @@
<Rule Id="RS0001" Action="Warning" />
<Rule Id="RS0002" Action="Warning" />
<Rule Id="RS0005" Action="Warning" />
<Rule Id="RS0016" Action="Error" />
<Rule Id="RS0017" Action="Error" />
<Rule Id="RS0016" Action="None" />
<Rule Id="RS0017" Action="None" />
<Rule Id="RS0022" Action="Error" />
<Rule Id="RS0024" Action="Error" />
<Rule Id="RS0025" Action="Error" />
<Rule Id="RS0026" Action="Error" />
<Rule Id="RS0027" Action="Error" />
<Rule Id="RS0027" Action="None" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is disabled to make return-expression's expression optional; , otherwise it complains about the default parameter. (bug?)

</Rules>
<Rules AnalyzerId="Microsoft.NetCore.Analyzers" RuleNamespace="Microsoft.NetCore.Analyzers.ImmutableCollections">
<Rule Id="RS0012" Action="Warning" />
Expand Down
81 changes: 81 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,15 @@ private BoundExpression BindExpressionInternal(ExpressionSyntax node, Diagnostic
case SyntaxKind.ThrowExpression:
return BindThrowExpression((ThrowExpressionSyntax)node, diagnostics);

case SyntaxKind.ReturnExpression:
return BindReturnExpression((ReturnExpressionSyntax)node, diagnostics);

case SyntaxKind.BreakExpression:
return BindBreakExpression((BreakExpressionSyntax)node, diagnostics);

case SyntaxKind.ContinueExpression:
return BindContinueExpression((ContinueExpressionSyntax)node, diagnostics);

case SyntaxKind.RefType:
return BindRefType(node, diagnostics);

Expand Down Expand Up @@ -661,6 +670,78 @@ private BoundExpression BindRefType(ExpressionSyntax node, DiagnosticBag diagnos
return new BoundTypeExpression(node, null, CreateErrorType("ref"));
}

private BoundExpression BindReturnExpression(ReturnExpressionSyntax node, DiagnosticBag diagnostics)
{
bool hasErrors = false;
if (!IsJumpExpressionInProperContext(node))
{
diagnostics.Add(ErrorCode.ERR_ThrowMisplaced, node.ReturnKeyword.GetLocation());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could I change ERR_ThrowMispalced message to A {0} expression is not allowed in this context. to be used for all jump expressions?

hasErrors = true;
}

BoundExpression arg = BindReturnedExpression(node, node.Expression, node.ReturnKeyword, diagnostics, out RefKind refKind, out bool hadErrors);
return new BoundReturnExpression(node, refKind, arg, type: null, hasErrors: hasErrors || hadErrors);
}

private BoundExpression BindContinueExpression(ContinueExpressionSyntax node, DiagnosticBag diagnostics)
{
bool hasErrors = node.HasErrors;
if (!IsJumpExpressionInProperContext(node))
{
diagnostics.Add(ErrorCode.ERR_ThrowMisplaced, node.ContinueKeyword.GetLocation());
hasErrors = true;
}

var label = this.ContinueLabel;
if ((object)label == null)
{
Error(diagnostics, ErrorCode.ERR_NoBreakOrCont, node);
return BadExpression(node);
}

return new BoundContinueExpression(node, label, type: null, hasErrors: hasErrors);
}

private BoundExpression BindBreakExpression(BreakExpressionSyntax node, DiagnosticBag diagnostics)
{
bool hasErrors = node.HasErrors;
if (!IsJumpExpressionInProperContext(node))
{
diagnostics.Add(ErrorCode.ERR_ThrowMisplaced, node.BreakKeyword.GetLocation());
hasErrors = true;
}

var label = this.BreakLabel;
if ((object)label == null)
{
Error(diagnostics, ErrorCode.ERR_NoBreakOrCont, node);
return BadExpression(node);
}

return new BoundBreakExpression(node, label, type: null, hasErrors: hasErrors);
}

private static bool IsJumpExpressionInProperContext(ExpressionSyntax node)
{
var parent = node.Parent;
if (parent == null || node.HasErrors)
{
return true;
}

switch (node.Parent.Kind())
{
case SyntaxKind.ConditionalExpression:
var conditionalParent = (ConditionalExpressionSyntax)parent;
return node == conditionalParent.WhenTrue || node == conditionalParent.WhenFalse;
case SyntaxKind.CoalesceExpression:
var binaryParent = (BinaryExpressionSyntax)parent;
return node == binaryParent.Right;
default:
return false;
}
}

private BoundExpression BindThrowExpression(ThrowExpressionSyntax node, DiagnosticBag diagnostics)
{
bool hasErrors = node.HasErrors;
Expand Down
33 changes: 22 additions & 11 deletions src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3272,8 +3272,19 @@ protected virtual TypeSymbol GetCurrentReturnType(out RefKind refKind)

private BoundStatement BindReturn(ReturnStatementSyntax syntax, DiagnosticBag diagnostics)
{
var refKind = RefKind.None;
var expressionSyntax = syntax.Expression?.CheckAndUnwrapRefExpression(diagnostics, out refKind);
BoundExpression arg = BindReturnedExpression(syntax, syntax.Expression, syntax.ReturnKeyword, diagnostics, out RefKind refKind, out bool hasErrors);
return new BoundReturnStatement(syntax, refKind, arg, hasErrors);
}

private BoundExpression BindReturnedExpression(
SyntaxNode syntax,
ExpressionSyntax expression,
SyntaxToken returnKeyword,
DiagnosticBag diagnostics,
out RefKind refKind, out bool hasErrors)
{
refKind = RefKind.None;
ExpressionSyntax expressionSyntax = expression?.CheckAndUnwrapRefExpression(diagnostics, out refKind);
BoundExpression arg = null;
if (expressionSyntax != null)
{
Expand All @@ -3292,24 +3303,23 @@ private BoundStatement BindReturn(ReturnStatementSyntax syntax, DiagnosticBag di
RefKind sigRefKind;
TypeSymbol retType = GetCurrentReturnType(out sigRefKind);

bool hasErrors;
if (IsDirectlyInIterator)
{
diagnostics.Add(ErrorCode.ERR_ReturnInIterator, syntax.ReturnKeyword.GetLocation());
diagnostics.Add(ErrorCode.ERR_ReturnInIterator, returnKeyword.GetLocation());
hasErrors = true;
}
else if (IsInAsyncMethod() && refKind != RefKind.None)
{
// This can happen if we are binding an async anonymous method to a delegate type.
diagnostics.Add(ErrorCode.ERR_MustNotHaveRefReturn, syntax.ReturnKeyword.GetLocation());
diagnostics.Add(ErrorCode.ERR_MustNotHaveRefReturn, returnKeyword.GetLocation());
hasErrors = true;
}
else if ((object)retType != null && (refKind != RefKind.None) != (sigRefKind != RefKind.None))
{
var errorCode = refKind != RefKind.None
? ErrorCode.ERR_MustNotHaveRefReturn
: ErrorCode.ERR_MustHaveRefReturn;
diagnostics.Add(errorCode, syntax.ReturnKeyword.GetLocation());
diagnostics.Add(errorCode, returnKeyword.GetLocation());
hasErrors = true;
}
else if (arg != null)
Expand All @@ -3323,7 +3333,8 @@ private BoundStatement BindReturn(ReturnStatementSyntax syntax, DiagnosticBag di

if (hasErrors)
{
return new BoundReturnStatement(syntax, refKind, arg, hasErrors: true);
refKind = RefKind.None;
return arg;
}

// The return type could be null; we might be attempting to infer the return type either
Expand All @@ -3345,7 +3356,7 @@ private BoundStatement BindReturn(ReturnStatementSyntax syntax, DiagnosticBag di
: ErrorCode.ERR_TaskRetNoObjectRequiredLambda;

// Anonymous function converted to a void returning delegate cannot return a value
Error(diagnostics, errorCode, syntax.ReturnKeyword);
Error(diagnostics, errorCode, returnKeyword);

// COMPATIBILITY: The native compiler also produced an error
// COMPATIBILITY: "Cannot convert lambda expression to delegate type 'Action' because some of the
Expand All @@ -3360,7 +3371,7 @@ private BoundStatement BindReturn(ReturnStatementSyntax syntax, DiagnosticBag di
? ErrorCode.ERR_RetNoObjectRequired
: ErrorCode.ERR_TaskRetNoObjectRequired;

Error(diagnostics, errorCode, syntax.ReturnKeyword, container);
Error(diagnostics, errorCode, returnKeyword, container);
}
}
}
Expand All @@ -3373,7 +3384,7 @@ private BoundStatement BindReturn(ReturnStatementSyntax syntax, DiagnosticBag di
? retType.GetMemberTypeArgumentsNoUseSiteDiagnostics().Single()
: retType;

Error(diagnostics, ErrorCode.ERR_RetObjectRequired, syntax.ReturnKeyword, requiredType);
Error(diagnostics, ErrorCode.ERR_RetObjectRequired, returnKeyword, requiredType);
}
else
{
Expand All @@ -3390,7 +3401,7 @@ private BoundStatement BindReturn(ReturnStatementSyntax syntax, DiagnosticBag di
}
}

return new BoundReturnStatement(syntax, refKind, arg);
return arg;
}

internal BoundExpression CreateReturnConversion(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ internal static Conversion GetTrivialConversion(ConversionKind kind)
internal static Conversion ImplicitReference => new Conversion(ConversionKind.ImplicitReference);
internal static Conversion ImplicitEnumeration => new Conversion(ConversionKind.ImplicitEnumeration);
internal static Conversion ImplicitThrow => new Conversion(ConversionKind.ImplicitThrow);
internal static Conversion ImplicitJump => new Conversion(ConversionKind.ImplicitJump);
internal static Conversion AnonymousFunction => new Conversion(ConversionKind.AnonymousFunction);
internal static Conversion Boxing => new Conversion(ConversionKind.Boxing);
internal static Conversion DefaultOrNullLiteral => new Conversion(ConversionKind.DefaultOrNullLiteral);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ internal enum ConversionKind : byte
ImplicitNumeric,
ImplicitEnumeration,
ImplicitThrow,
ImplicitJump,
Copy link
Contributor Author

@alrz alrz Sep 8, 2017

Choose a reason for hiding this comment

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

Could we merge ImplicitThrow and ImplicitJump together? (one issue is IsThrow public property)

ImplicitTupleLiteral,
ImplicitTuple,
ExplicitTupleLiteral,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public static bool IsImplicitConversion(this ConversionKind conversionKind)
case ConversionKind.ImplicitTuple:
case ConversionKind.ImplicitEnumeration:
case ConversionKind.ImplicitThrow:
case ConversionKind.ImplicitJump:
case ConversionKind.ImplicitNullable:
case ConversionKind.DefaultOrNullLiteral:
case ConversionKind.ImplicitReference:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -839,6 +839,11 @@ private Conversion ClassifyImplicitBuiltInConversionFromExpression(BoundExpressi

case BoundKind.ThrowExpression:
return Conversion.ImplicitThrow;

case BoundKind.BreakExpression:
case BoundKind.ReturnExpression:
case BoundKind.ContinueExpression:
return Conversion.ImplicitJump;
}

return Conversion.NoConversion;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -608,6 +608,9 @@ private static bool IsEncompassingImplicitConversionKind(ConversionKind kind)
case ConversionKind.ExplicitTuple:
return false;

case ConversionKind.ImplicitJump:
return true;

default:
throw ExceptionUtilities.UnexpectedValue(kind);
}
Expand Down
11 changes: 11 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -1605,6 +1605,17 @@
<Field Name="Expression" Type="BoundExpression" Null="disallow"/>
</Node>

<Node Name="BoundReturnExpression" Base="BoundExpression">
<Field Name="RefKind" Type="RefKind"/>
<Field Name="ExpressionOpt" Type="BoundExpression" Null="allow"/>
</Node>
<Node Name="BoundBreakExpression" Base="BoundExpression">
<Field Name="Label" Type="GeneratedLabelSymbol" />
</Node>
<Node Name="BoundContinueExpression" Base="BoundExpression">
<Field Name="Label" Type="GeneratedLabelSymbol" />
</Node>

<AbstractNode Name="VariablePendingInference" Base="BoundExpression">
<!-- Type is not significant for this node type; always null -->
<Field Name="Type" Type="TypeSymbol" Override="true" Null="always"/>
Expand Down
56 changes: 54 additions & 2 deletions src/Compilers/CSharp/Portable/BoundTree/Expression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,7 @@ Semantics.ConversionKind IConversionExpression.ConversionKind
case CSharp.ConversionKind.ExplicitEnumeration:
case CSharp.ConversionKind.ImplicitEnumeration:
case CSharp.ConversionKind.ImplicitThrow:
case CSharp.ConversionKind.ImplicitJump:
case CSharp.ConversionKind.ImplicitTupleLiteral:
case CSharp.ConversionKind.ImplicitTuple:
case CSharp.ConversionKind.ExplicitTupleLiteral:
Expand Down Expand Up @@ -2852,7 +2853,7 @@ protected override OperationKind ExpressionKind
}
}

partial class BoundThrowExpression
internal partial class BoundThrowExpression
{
public override void Accept(OperationVisitor visitor)
{
Expand Down Expand Up @@ -2880,7 +2881,7 @@ public BoundDeclarationPattern(SyntaxNode syntax, LocalSymbol localSymbol, Bound
}
}

partial class BoundDiscardExpression
internal partial class BoundDiscardExpression
{
public override void Accept(OperationVisitor visitor)
{
Expand All @@ -2897,4 +2898,55 @@ public override TResult Accept<TArgument, TResult>(OperationVisitor<TArgument, T
// TODO: implement IOperation for pattern-matching constructs (https:/dotnet/roslyn/issues/8699)
protected override OperationKind ExpressionKind => OperationKind.None;
}

internal partial class BoundReturnExpression
{
public override void Accept(OperationVisitor visitor)
{
visitor.VisitNoneOperation(this);
}

public override TResult Accept<TArgument, TResult>(OperationVisitor<TArgument, TResult> visitor, TArgument argument)
{
return visitor.VisitNoneOperation(this, argument);
}

protected override OperationKind ExpressionKind => OperationKind.None;

protected override ImmutableArray<IOperation> Children => ImmutableArray.Create<IOperation>(this.ExpressionOpt);
}

internal partial class BoundBreakExpression
{
public override void Accept(OperationVisitor visitor)
{
visitor.VisitNoneOperation(this);
}

public override TResult Accept<TArgument, TResult>(OperationVisitor<TArgument, TResult> visitor, TArgument argument)
{
return visitor.VisitNoneOperation(this, argument);
}

protected override OperationKind ExpressionKind => OperationKind.None;

protected override ImmutableArray<IOperation> Children => ImmutableArray<IOperation>.Empty;
}

internal partial class BoundContinueExpression
{
public override void Accept(OperationVisitor visitor)
{
visitor.VisitNoneOperation(this);
}

public override TResult Accept<TArgument, TResult>(OperationVisitor<TArgument, TResult> visitor, TArgument argument)
{
return visitor.VisitNoneOperation(this, argument);
}

protected override OperationKind ExpressionKind => OperationKind.None;

protected override ImmutableArray<IOperation> Children => ImmutableArray<IOperation>.Empty;
}
}
15 changes: 15 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/Formatting.cs
Original file line number Diff line number Diff line change
Expand Up @@ -144,4 +144,19 @@ public override object Display
get { return (object)this.Type ?? "default"; }
}
}

internal partial class BoundReturnExpression
{
public override object Display => (object)this.Type ?? "return";
}

internal partial class BoundContinueExpression
{
public override object Display => (object)this.Type ?? "continue";
}

internal partial class BoundBreakExpression
{
public override object Display => (object)this.Type ?? "break";
}
}
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/CodeGen/EmitConversion.cs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ private void EmitConversion(BoundConversion conversion)
case ConversionKind.ImplicitDynamic:
case ConversionKind.ExplicitDynamic:
case ConversionKind.ImplicitThrow:
case ConversionKind.ImplicitJump:
// None of these things should reach codegen (yet? maybe?)
throw ExceptionUtilities.UnexpectedValue(conversion.ConversionKind);
case ConversionKind.PointerToVoid:
Expand Down
Loading