Skip to content

Bind switch expression with checked-aware binder#61890

Merged
jcouv merged 4 commits intodotnet:mainfrom
jcouv:checked-switch
Jun 17, 2022
Merged

Bind switch expression with checked-aware binder#61890
jcouv merged 4 commits intodotnet:mainfrom
jcouv:checked-switch

Conversation

@jcouv
Copy link
Copy Markdown
Member

@jcouv jcouv commented Jun 14, 2022 •
edited
Loading

Fixes #61843
Fixes #60419

The issue is that Binder.BindSwitchExpression is dropping the binder Flags, so we're losing the OverflowChecks.Enabled flag set by Binder.BindCheckedExpression.

        private BoundExpression BindSwitchExpression(SwitchExpressionSyntax node, BindingDiagnosticBag diagnostics)
        {
            RoslynDebug.Assert(node is { });
            Binder? switchBinder = this.GetBinder(node);
            RoslynDebug.Assert(switchBinder is { });
            return switchBinder.BindSwitchExpressionCore(node, switchBinder, diagnostics);
        }

        private BoundExpression BindCheckedExpression(CheckedExpressionSyntax node, BindingDiagnosticBag diagnostics)
        {
            // the binder is not cached since we only cache statement level binders
            return this.WithCheckedOrUncheckedRegion(node.Kind() == SyntaxKind.CheckedExpression).
                BindParenthesizedExpression(node.Expression, diagnostics);
        }

@jcouv jcouv self-assigned this Jun 14, 2022
@ghost ghost added the Area-Compilers label Jun 14, 2022
@jcouv jcouv marked this pull request as ready for review June 14, 2022 22:02
@jcouv jcouv requested a review from a team as a code owner June 14, 2022 22:02
@333fred
Copy link
Copy Markdown
Member

333fred commented Jun 14, 2022 •
edited
Loading

It would probably be worth checking other binder flags as well: is unsafe handled correctly, for example?

@jcouv jcouv requested a review from AlekseyTs June 15, 2022 05:04
@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Jun 15, 2022

It would probably be worth checking other binder flags as well: is unsafe handled correctly, for example?

That's beyond the scope of this fix, but nothing jumps out from cursory glance at other flags (including unsafe, see LocalBinderFactory.VisitUnsafeStatement).

AlekseyTs
AlekseyTs reviewed Jun 15, 2022
View reviewed changes
src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs

public override void VisitCheckedExpression(CheckedExpressionSyntax node)
{
Binder binder = _enclosing.WithCheckedOrUncheckedRegion(@checked: node.Kind() == SyntaxKind.CheckedExpression);
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jun 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

binder

I would expect Binder.BindCheckedExpression to take advantage of this binder. I.e. it probably should be added to the map. Also, it probably should affect how SemanticModel figures out enclosing binder. #Closed

Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jun 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

For example, speculating inside a checked expression should use the context, which should be observable through operators used. That should fix #60419, I think.

Copy link
Copy Markdown
Member Author

@jcouv jcouv Jun 15, 2022

Choose a reason for hiding this comment

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

Thanks for the pointer. I'll look at fixing #60419 together with this.

Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jun 15, 2022

Choose a reason for hiding this comment

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

I think the tests for #60419 are already checked in.

@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Jun 15, 2022 •
edited
Loading

Done with review pass (commit 1)

In reply to: 1156511984

@jcouv jcouv marked this pull request as draft June 15, 2022 17:18
@jcouv jcouv requested a review from AlekseyTs June 15, 2022 18:19
@jcouv jcouv marked this pull request as ready for review June 15, 2022 18:19
AlekseyTs
AlekseyTs reviewed Jun 16, 2022
View reviewed changes
src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs Outdated
{
binder = rootBinder.GetBinder(current);
}
else if (current is CheckedExpressionSyntax)
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

current is CheckedExpressionSyntax

We probably should check that we are inside parens. We do similar check for argument list and switch section, for example. if we are on the checked/unchecked keyword, we haven't entered the context yet, that should be possible to observe through a test. #Closed

@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Jun 16, 2022

Done with review pass (commit 3)

AlekseyTs
AlekseyTs approved these changes Jun 16, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 4), assuming CI is passing

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Jun 16, 2022

@dotnet/roslyn-compiler for second review. Thanks

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Jun 16, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Jun 16, 2022

Azure Pipelines successfully started running 4 pipeline(s).

333fred
333fred approved these changes Jun 16, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 4), assuming tests are passing.

@jcouv jcouv enabled auto-merge (squash) June 16, 2022 17:41
@jcouv jcouv merged commit 0730214 into dotnet:main Jun 17, 2022
@ghost ghost added this to the Next milestone Jun 17, 2022
chsienki added a commit to chsienki/roslyn that referenced this pull request Jun 23, 2022
@chsienki
Revert "Bind switch expression with checked-aware binder (dotnet#61890)"
37cee08 
This reverts commit 0730214.
@RikkiGibson RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022
@ryzngard ryzngard mentioned this pull request Jul 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@333fred 333fred 333fred approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees

@jcouv jcouv

Labels

Area-Compilers

Projects

None yet

Milestone

17.3 P3

Development

Successfully merging this pull request may close these issues.

checked expression containing switch expression has no effect SemanticModel ignores evaluation context introduced by checked/unchecked expression

4 participants

@jcouv @333fred @AlekseyTs @RikkiGibson