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

Compiler shouldn't emit hidden sequence points on display class creation instruction in expression context #37237

Closed
Tracked by #43099
tmat opened this issue Jul 15, 2019 · 1 comment

Comments

@tmat
Copy link
Member

tmat commented Jul 15, 2019

using System;
public class C
{
    static int M() 
    {
        return F() switch 
        {
            1 => F() switch
                 {
                     C { P: int p, Q: C { P: int q } } => G(() => p + q),
                     _ => 10
                 },
            C { Q: int s } => G(() => s),
            _ => 0
        };
    }

    object P { get; set; }
    object Q { get; set; }
    static object F() => null;
    static int G(Func<int> f) => 0;
}

PDB contains sequence points

      <sequencePoints>
        <entry offset="0x0" startLine="5" startColumn="5" endLine="5" endColumn="6" document="1" />
        <entry offset="0x1" hidden="true" document="1" />
        <entry offset="0x5b" hidden="true" document="1" />
        <entry offset="0x118" startLine="16" startColumn="5" endLine="16" endColumn="6" document="1" />
      </sequencePoints>

The one at offset 0x5b shouldn't be emitted since it's in the middle of expression evaluation:

  IL_005b:  newobj     instance void C/'<>c__DisplayClass0_1'::.ctor()

If we decide to merge closure scopes of the nested switch expressions to fix #37232, this issue will be resolved as well. If we decide to keep both closures then we need to avoid emitting the hidden sequence point on the inner one.

See test PDBTests.Patterns_SwitchExpression_Closures.

As it turns out, the outer hidden sequence point is also problematic: see #37261

@tmat tmat added this to the Compiler.Next milestone Jul 15, 2019
tmat added a commit to tmat/roslyn that referenced this issue Jul 15, 2019
Update calculation of syntax offset to account for a new case when a node (a switch expression) that is associated with a variable, closure or lambda may share start offset with other node of the same kind (`expr switch { … } switch { … }`). Use the offset of the `switch` keyword instead of the starting offset of the expression to disambiguate.

Assign ordinals to variables synthesized for storing pattern values across cases. This is required to support complex patterns since we can no longer rely on the type of these variables to be distinct. This will require follow up in the IDE to disallow updating/adding/reordering the case clauses of switch expression which there an active statement is present within the switch statement. If the cases are unmodified the compiler guarantees that the order in which the synthesized variables are generated remains the same, so we can map the variables using their ordinal.

Mark all variables synthesized during lowering of switch expression as short-lived. Their lifespan is limited to the switch expression, which does not include a sequence point.

Disallow editing methods that contain switch expression. This is necessary until bugs dotnet#37232, dotnet#37237 are fixed.
@tmat
Copy link
Member Author

tmat commented Jul 16, 2019

Related issue: #37261

tmat added a commit that referenced this issue Jul 16, 2019
* Fix EnC debug information emitted for patterns

Update calculation of syntax offset to account for a new case when a node (a switch expression) that is associated with a variable, closure or lambda may share start offset with other node of the same kind (`expr switch { … } switch { … }`). Use the offset of the `switch` keyword instead of the starting offset of the expression to disambiguate.

Assign ordinals to variables synthesized for storing pattern values across cases. This is required to support complex patterns since we can no longer rely on the type of these variables to be distinct. This will require follow up in the IDE to disallow updating/adding/reordering the case clauses of switch expression which there an active statement is present within the switch statement. If the cases are unmodified the compiler guarantees that the order in which the synthesized variables are generated remains the same, so we can map the variables using their ordinal.

Mark all variables synthesized during lowering of switch expression as short-lived. Their lifespan is limited to the switch expression, which does not include a sequence point.

Disallow editing methods that contain switch expression. This is necessary until bugs #37232, #37237 are fixed.

* Feedback

* Update tests
gafter added a commit to gafter/roslyn that referenced this issue Aug 21, 2019
…ion instruction in expression context

Fixes dotnet#37237

Compiler emits incorrect EnC closure data to PDB for nested switch expressions
Fixes dotnet#37232

Expression bodied method whose expression is a switch expression is missing debug info
Fixes dotnet#37261
@gafter gafter modified the milestones: Compiler.Next, 16.4 Aug 21, 2019
@gafter gafter added the Bug label Sep 19, 2019
@gafter gafter modified the milestones: 16.4, 16.5 Oct 29, 2019
@gafter gafter modified the milestones: 16.5, 16.6 Jan 13, 2020
@gafter gafter modified the milestones: 16.6, Compiler.Net5 Mar 14, 2020
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment