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

ISwitchExpressionOperation.IsExhaustive API extension #52703

Merged
merged 33 commits into from
May 28, 2021

Conversation

bernd5
Copy link
Contributor

@bernd5 bernd5 commented Apr 17, 2021

fixes #52577

@bernd5 bernd5 changed the title Draft PR: IOperation switchexpr exhaustive Draft PR: ISwitchExpressionOperation.IsExhaustive API extension Apr 17, 2021
@bernd5 bernd5 marked this pull request as ready for review April 18, 2021 07:42
@bernd5 bernd5 requested a review from a team as a code owner April 18, 2021 07:42
@bernd5 bernd5 marked this pull request as draft April 18, 2021 07:42
@bernd5 bernd5 marked this pull request as ready for review April 18, 2021 10:40
@bernd5
Copy link
Contributor Author

bernd5 commented Apr 18, 2021

@333fred or @CyrusNajmabadi please review

@bernd5 bernd5 changed the title Draft PR: ISwitchExpressionOperation.IsExhaustive API extension ISwitchExpressionOperation.IsExhaustive API extension Apr 18, 2021
@bernd5
Copy link
Contributor Author

bernd5 commented Apr 19, 2021

Something to add or change?

Copy link
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.

Done review pass (commit 10)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 27, 2021

    private ISwitchExpressionOperation CreateBoundSwitchExpressionOperation(BoundSwitchExpression boundSwitchExpression)

It looks like we can safely change this type to BoundConvertedSwitchExpression.


In reply to: 827918882


In reply to: 827918882


Refers to: src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs:2153 in 955cd47. [](commit_id = 955cd47, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 27, 2021

Done with review pass (commit 11), test are not looked at


In reply to: 827922761


In reply to: 827922761

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 27, 2021

It looks like ControlFlowGraphBuilder always adds SwitchExpressionException branch for a switch expression. Should we adjust that to be conditional based on the new property?


In reply to: 827926846

@bernd5
Copy link
Contributor Author

bernd5 commented May 18, 2021

@AlekseyTs : I tried to follow your comment - but I'm not quite sure if it is correct

@bernd5
Copy link
Contributor Author

bernd5 commented May 18, 2021

@AlekseyTs : thanks for your commits.
Could you look at the failing tests?

@AlekseyTs
Copy link
Contributor

Could you look at the failing tests?

The best way to deal with test that are failing due to baseline difference is to update the baselines. Then we can review the changes and confirm that the baseline changes are expected.
Also, it looks like there is a formatting error:

##[error]src\Compilers\Test\Core\Compilation\TestOperationVisitor.cs(1289,1): error IDE0055: (NETCORE_ENGINEERING_TELEMETRY=Build) Fix formatting

@333fred
Copy link
Member

333fred commented May 27, 2021

Alright, I've dug through the test failures here. The problem is that we are introducing a branch for the following (example) tree:

        Block[B4] - Block
            Predecessors: [B2]
            Statements (0)
            Jump if False (Regular) to Block[B6]
                IIsPatternOperation (OperationKind.IsPattern, Type: System.Boolean) (Syntax: '_ => true')
                  Value: 
                    IFlowCaptureReferenceOperation: 2 (OperationKind.FlowCaptureReference, Type: System.Int32, IsImplicit) (Syntax: 'input')
                  Pattern: 
                    IDiscardPatternOperation (OperationKind.DiscardPattern, Type: null) (Syntax: '_') (InputType: System.Int32, NarrowedType: System.Int32)
                Leaving: {R2}

            Next (Regular) Block[B5]
        Block[B5] - Block
            Predecessors: [B4]
            Statements (1)
                IFlowCaptureOperation: 1 (OperationKind.FlowCapture, Type: null, IsImplicit) (Syntax: 'true')
                  Value: 
                    ILiteralOperation (OperationKind.Literal, Type: System.Boolean, Constant: True) (Syntax: 'true')

            Next (Regular) Block[B6]
                Leaving: {R2}
    }

    Block[B6] - Block
        Predecessors: [B3] [B4] [B5]
        Statements (1)
            IExpressionStatementOperation (OperationKind.ExpressionStatement, Type: null) (Syntax: 'result = in ... };')
              Expression: 
                ISimpleAssignmentOperation (OperationKind.SimpleAssignment, Type: System.Boolean) (Syntax: 'result = in ... }')
                  Left: 
                    IFlowCaptureReferenceOperation: 0 (OperationKind.FlowCaptureReference, Type: System.Boolean, IsImplicit) (Syntax: 'result')
                  Right: 
                    IFlowCaptureReferenceOperation: 1 (OperationKind.FlowCaptureReference, Type: System.Boolean, IsImplicit) (Syntax: 'input switc ... }')

        Next (Regular) Block[B7]
            Leaving: {R1}

Note how B6 is using flow capture 1, which is not initialized by B4. It feels like we need to skip branches for pure discard branches in the flow tree.

@AlekseyTs
Copy link
Contributor

@333fred

I've dug through the test failures here. The problem is that we are introducing a branch for the following (example) tree:

What unit-test is that?

@333fred
Copy link
Member

333fred commented May 28, 2021

Alright, looking at the test failures there is quite a bit more that would need to be done to eliminate false jumps here. For example, boolVal switch { true => ..., false => ... } needs to not generate jumps on the false case because it's an unconditional fallthrough. @bernd5, can you please force this back before the changes to the CFG builder and we can file a followup to do this in a separate PR? It's far too complex to try and do right now.

Copy link
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 33)

@333fred 333fred merged commit a86dbfb into dotnet:main May 28, 2021
@ghost ghost added this to the Next milestone May 28, 2021
@333fred
Copy link
Member

333fred commented May 28, 2021

Thanks for the patience and persistence @bernd5!

@bernd5 bernd5 deleted the ioperation_switchexpr_exhaustive branch May 28, 2021 17:17
@RikkiGibson RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing compiler generated switch expression arm in/from ISwitchExpressionOperation
5 participants