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 emits incorrect EnC closure data to PDB for nested switch expressions #37232

Closed
Tracked by #43099
tmat opened this issue Jul 15, 2019 · 0 comments
Closed
Tracked by #43099

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;
}

A display class is emitted for each switch expression:
image

pdb2xml output:

...
<encLambdaMap>
  <methodOrdinal>0</methodOrdinal>
  <closure offset="59" />
  <closure offset="59" />
  <lambda offset="255" closure="0" />
  <lambda offset="157" closure="1" />
</encLambdaMap>
...

The offset attribute of closure identifies the syntax node that's associated with the closure. This offset must be unique, but in this case the compiler emits the same value - the offset of the outer switch expression node.

The lowering creates a spill sequence with locals defined in the switch expression. The spill sequence node is correctly associated with the switch expression syntax at this point.
Later on SpillSequenceSpiller.VisitSpillSequence takes these locals and puts them in BoundSpillSequenceBuilder. Finally, SpillSequenceSpiller.UpdateStatement creates a new BoundBlock with these locals. The BoundBlock is however no longer associated with the right switch expression syntax node.

See test See test PDBTests.Patterns_SwitchExpression_Closures.

@gafter gafter 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 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
Projects
None yet
2 participants