-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Adjust scope of declarations within a Labeled and Local Function Statements according to the latest design. #13273
Conversation
…to the latest design. Related to dotnet#12597.
…ing to the latest design. Related to dotnet#12597.
…o the latest design. Related to dotnet#12597.
… the latest design. Related to dotnet#12597.
Since no feedback has been provided for this small change, I added two new commits to handle Return/Throw statements. |
Looking at this now. In reply to: 241485281 [](ancestors = 241485281) |
} | ||
} | ||
break; | ||
|
||
case SyntaxKind.ExpressionStatement: | ||
case SyntaxKind.IfStatement: | ||
case SyntaxKind.YieldReturnStatement: | ||
case SyntaxKind.ReturnStatement: | ||
case SyntaxKind.ThrowStatement: | ||
ExpressionVariableFinder.FindExpressionVariables(this, locals, innerStatement, enclosingBinder.GetBinder(innerStatement) ?? enclosingBinder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enclosingBinder.GetBinder(innerStatement) [](start = 103, length = 41)
Under what conditions would this return a non-null result, and under which conditions a null result? I'm guessing it has to do with whether or not the SyntaxList<StatementSyntax> statements
passed in comes from the real tree or not, but some comment around here would be helpful, please.
return new BoundBlock(statement.Syntax, locals, localFunctions, | ||
ImmutableArray.Create(statement)) | ||
{ WasCompilerGenerated = true }; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid the problem of documenting and knowing whether to call WrapWithVariablesIfAny
or the new method, maybe the new logic could be added to the existing method instead.
The caller would always use WrapWithVariablesIfAny
and that existing method would be smart to check if the scope could declare local functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check to know "if the scope could declare local functions" wouldn't be simple. Besides, the code and APIs in Binder are intentionally structured in such a way so that requests for locals and local functions were intentional and only in right contexts.
LGTM Thanks |
Related to #12597.
Plus addressing feedback from previous PR.
@dotnet/roslyn-compiler, @gafter Please review.