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

Expression breakpoints for switch expression. #42711

Merged
merged 7 commits into from
Apr 1, 2020
Merged

Expression breakpoints for switch expression. #42711

merged 7 commits into from
Apr 1, 2020

Conversation

gafter
Copy link
Member

@gafter gafter commented Mar 24, 2020 •
edited
Loading

Fixes #22016
Fixes #37805
Fixes #42579

@gafter gafter added this to the Compiler.Net5 milestone Mar 24, 2020
@gafter gafter self-assigned this Mar 24, 2020
@jcouv jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 24, 2020
@gafter gafter force-pushed the patterns3-expr-breakpoints branch from f347d44 to 1d3ef40 Compare March 26, 2020 17:14
@gafter gafter force-pushed the patterns3-expr-breakpoints branch from 1d3ef40 to 2053f1e Compare March 26, 2020 18:20
@gafter gafter removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 26, 2020
@gafter gafter requested review from agocke, tmat and RikkiGibson March 26, 2020 18:21
@gafter gafter marked this pull request as ready for review March 26, 2020 18:22
@gafter gafter requested review from a team as code owners March 26, 2020 18:22
@gafter
Copy link
Member Author

gafter commented Mar 26, 2020 •
edited
Loading

@dotnet/roslyn-compiler @tmat Please have a look at this implementation of support for breakpoints inside switch expressions. #Resolved

@gafter
Copy link
Member Author

gafter commented Mar 26, 2020 •
edited
Loading

@dotnet/roslyn-ide Could I also please have some eyes on this from the IDE perspective? #Resolved

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Mar 26, 2020 •
edited by gafter
Loading

@gafter On it :) #Resolved

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Mar 26, 2020 •
edited by gafter
Loading

IDE LGTM. Minor cleanup nits. #Resolved

jcouv
jcouv reviewed Mar 26, 2020
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 1)

@gafter
Copy link
Member Author

gafter commented Mar 26, 2020 •
edited
Loading

@jcouv I think I've addressed all of your comments. Do you have any others? #Resolved

jcouv
jcouv approved these changes Mar 26, 2020
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Compiler changes LGTM Thanks (iteration 2)

@gafter
Copy link
Member Author

gafter commented Mar 26, 2020

@dotnet/roslyn-compiler May I have a second review please?

@jcouv jcouv self-assigned this Mar 26, 2020
@gafter
Copy link
Member Author

gafter commented Mar 30, 2020

@agocke @RikkiGibson @dotnet/roslyn-compiler May I have a second review please?

@tmat
Copy link
Member

tmat commented Mar 30, 2020 •
edited by gafter
Loading 

    /// 4) The expression is a foreach initializer.

Update comment with switch expression info #Resolved

Refers to: src/Features/CSharp/Portable/EditAndContinue/BreakpointSpans.cs:719 in 200f9c8. [](commit_id = 200f9c8, deletion_comment = False)

@tmat
Copy link
Member

tmat commented Mar 30, 2020

Looks good.

A couple of questions:

  1. should this rather go to master? It doesn't depend on anything in the features/patterns branch, does it?
  2. If we merge this change it will break EnC in cases when an active statement in a switch expression is on the stack because EnC won't know to map it.
    We can either accept the break in features/patterns3 branch and block merge to master until I follow up with my PR that fixes it, or I can update this PR with necessary EnC changes.

@gafter
Copy link
Member Author

gafter commented Mar 31, 2020 •
edited
Loading

should this rather go to master? It doesn't depend on anything in the features/patterns branch, does it?

This work is on a branch that differs substantially from master in the area of the changes.

If we merge this change it will break EnC in cases when an active statement in a switch expression is on the stack because EnC won't know to map it.
We can either accept the break in features/patterns3 branch and block merge to master until I follow up with my PR that fixes it, or I can update this PR with necessary EnC changes.

Up to you. I'm still waiting on a second review from the compiler team. I will aim to get that today so you can target the fixes to the features/patterns3 branch. #WontFix

agocke
agocke approved these changes Apr 1, 2020
View reviewed changes
Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM although some docs would be nice.

jcouv
jcouv reviewed Apr 1, 2020
View reviewed changes
docs/compilers/CSharp/Expression Breakpoints.md
@@ -53,3 +53,5 @@ Both the `BoundRestorePreviousSequencePoint` and `BoundStepThroughSequencePoint`
This can be seen, for example, in test `SwitchExpressionSequencePoints`

The purpose of this instruction sequence is to cause there to be an unreachable IL opcode (the `nop`) having a sequence point, followed by a hidden sequence point to prevent the debugger from stopping at the following location. However, once it is executing the following code, the debugger's idea of the program's "current source location" will appear to be that location mentioned in the sequence point.

A `BoundStepThroughSequencePoint` also modifies the debugger's view of the "enclosing statement", but without creating a location where a breakpoint can be set. While evaluating the state machine of a *switch expression*, this is used to make the "current statement" appear to be the *switch expression*.
Copy link
Member

Choose a reason for hiding this comment

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

@gafter Just to confirm: in var x = i switch { 1 => Method(); ... }, if I have a breakpoint inside Method, I expect the call stack will highlight the branch of the switch expression Method(). Is that the current behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

@gafter gafter merged commit 56b937b into dotnet:features/patterns3 Apr 1, 2020
This was referenced Apr 1, 2020
Closed
Closed
Closed
@CyrusNajmabadi CyrusNajmabadi mentioned this pull request Apr 10, 2020
Closed
29 tasks
@gafter gafter mentioned this pull request Apr 22, 2020
Open
k15tfu
k15tfu reviewed Nov 7, 2020
View reviewed changes
src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs
_builder.EmitConstantValue(ConstantValue.Create(true));
_builder.EmitBranch(ILOpCode.Brtrue, label);
EmitSequencePoint(syntaxTree, span);
_builder.EmitOpCode(ILOpCode.Nop);
Copy link

Choose a reason for hiding this comment

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

@gafter Just curious if sequence points can be empty w/o instructions, why having EmitSequencePoint() followed by EmitHiddenSequencePoint() is not sufficient?

I'm asking because coverage tools show uncovered statements, for example: https://developercommunity.visualstudio.com/content/problem/1140065/coverage-on-switch-expression-dropped-in-167.html , and now they should always ignore the unreachable SP in ldc.i4.1 brtrue.s 1 nop pattern. Is there a guarantee the user's code will never produce such IL?

Copy link
Member Author

Choose a reason for hiding this comment

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

The user's code can indeed produce such IL:
https://sharplab.io/#v2:C4LglgNgNAJiDUAfAAgJgIwFgBQyDMABGgQMIEDeOB1RhYAdsAQLIAUAlBVTTwG4CGAJwJMAvCMEBXAKYBubj2pgAZgVbBO87IprIA7AXRaeAXxwmgA=

@KirillOsenkov
Copy link
Member

So how do we get this shipped?

@jcouv
Copy link
Member

jcouv commented May 5, 2021

@KirillOsenkov This was shipped a year ago. I've observed that new behavior in VS for a while now.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcouv jcouv jcouv approved these changes

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@k15tfu k15tfu k15tfu left review comments

@tmat tmat tmat left review comments

@agocke agocke agocke approved these changes

@RikkiGibson RikkiGibson Awaiting requested review from RikkiGibson

Assignees

@gafter gafter

@jcouv jcouv

Labels
Area-Compilers Feature - Pattern Matching Pattern Matching
Projects
None yet
Milestone
Compiler.Net5
Development

Successfully merging this pull request may close these issues.
7 participants
@gafter @CyrusNajmabadi @tmat @KirillOsenkov @jcouv @agocke @k15tfu