Bugfixes for ConvertSwitchStatementToExpression #38007

alrz · 2019-08-15T15:11:27Z

Fixes #37949 (leave open for offering the fix instead of bail out)
~~Fixes #37950~~ (will be resolved on the compiler side)
Fixes #36876
Fixes #37873
Fixes #37872
Fixes #37907
Fixes #37947
Closes #36877 (duplicate of #37873)

...res/CSharpTest/ConvertSwitchStatementToExpression/ConvertSwitchStatementToExpressionTests.cs

...ertSwitchStatementToExpression/ConvertSwitchStatementToExpressionCodeFixProvider.Rewriter.cs

ryzngard · 2019-08-16T21:53:31Z

...res/CSharpTest/ConvertSwitchStatementToExpression/ConvertSwitchStatementToExpressionTests.cs

+ case 0: name = ""1""; break;
+ case 1: name = ""2""; break;
+ }
+ throw new Exception(name);


This looks like it will result in non-intuitive behavior. I'd prefer we still support this and refactor it correctly.

This fixes the incorrect behavior by not suggesting the fix. However, for this particular case we can use the initializer value for the last arm expression as suggested in #37949. we can keep it open to track that case.

ryzngard · 2019-08-16T21:56:25Z

...SwitchStatementToExpression/ConvertSwitchStatementToExpressionDiagnosticAnalyzer.Analyzer.cs

+ if (shouldRemoveNextStatement &&
+ semanticModel.AnalyzeDataFlow(node.GetNextStatement()).DataFlowsIn.Contains(symbol))
+ {
+ // Bail out if data flows into the next statement that we want to move


Instead of bailing, can we just not remove the next statement and leave it outside the switch?

Just looking through the AnalyzeSwitchStatement implementation, we're setting shouldRemoveNextStatement always to true if there is no default. That's potentially wrong, especially in the case noted in #37949

Maybe we can check if there is a default that makes sense before assuming we need to remove next?

Instead of bailing, can we just not remove the next statement and leave it outside the switch?

At that point, we must have a viable statement after switch, otherwise this is not an exhaustive switch and we can't offer the fix

ryzngard

#37949 fix still is lacking. I'd rather we fix that or leave open and take another stab in a separate PR.

Everything else looks good to me ✔

...ertSwitchStatementToExpression/ConvertSwitchStatementToExpressionCodeFixProvider.Rewriter.cs

alrz · 2019-08-21T15:38:05Z

Should we rebase this bugfix PR to 16.4-preview1 as well?

jasonmalinowski · 2019-08-22T01:17:33Z

@alrz Hmm, didn't realize this was out or I would have reviewed it. Don't rebase (the branch is switching around) but I might actually want to get this to go into 16.3...

...SwitchStatementToExpression/ConvertSwitchStatementToExpressionDiagnosticAnalyzer.Analyzer.cs

CyrusNajmabadi · 2019-08-22T05:27:33Z

...SwitchStatementToExpression/ConvertSwitchStatementToExpressionDiagnosticAnalyzer.Analyzer.cs

+ var declaration = declarator.GetAncestor<StatementSyntax>();
+ if (declaration.Parent == node.Parent && declarator.Initializer is null)
+ {
+ var beforeSwitch = node.GetPreviousStatement() is {} previousStatement


i'd honestly just prefer something like is StatementSyntax previousStatement

is StatementSyntax doesn't scream null check? this is a usage that's been anticipated for recursive patterns afterall.

is StatementSyntax doesn't scream null check?

It does to me. Because it just matches is Whatever which has been in the language forever and does a null check.

If we get a not null pattern in the future i'd also be ok with that. I think that {} works, but just doesn't read well. It makes me feel as if it's missing something and trying to say "i need some props...i just forgot what they were :)"

It does to me. Because it just matches is Whatever which has been in the language forever

So does is object , perhaps I should quote #37539 (comment) from Cyrus

I agree. I'm actually not a huge fan of is object primarily because, for me, it heavily evokes a type check, when in reality all i'm really trying to convey is the null check in a safe way

I'd agree too.

If we get a not null pattern in the future i'd also be ok with that

Sadly, not null pattern only works when you don't need a variable, you'd need to say
not null and var x to also introduce a variable of a null-checked expression that would err if it's not a reference type - {} doesn't.

I think that {} works, but just doesn't read well. It makes me feel as if it's missing something and trying to say "i need some props...i just forgot what they were :)

There were many requests to have a null-checked var-pattern before C# 8.0 and the answer was to use {}, so I'd say that's pretty idiomatic.

I myself proposed var x? similar to swift's "optional patterns" - but it got superseded by {} which is a completely accidental feature of recursive patterns.

basically, i'd rather wait until we have a good pattern here tbh.

I'm with @CyrusNajmabadi and also detest this pattern: it feels like somebody realized you could use a screwdriver to hammer in a nail, and because we've all decided hammers aren't the new hotness we're all trying to use screwdrivers. :-)

Note: i'm also very amenable to the idea that i'm being curmudgeonly here :) I'm not opposed to us migrating to new patterns and practices (lord knows you can see how often i massively do that to some part of the IDE).

That said, it's sometimes good to have a period were we dip our toes in and decide how we feel as a whole and come to some general consensus on a good balance.

i.e. i like patterns. i like them a lot. BUt i also think they can become unreadable. So finding a good balance on how they can be used is great. But we shouldn't necessarily try to make 100% of all code into a pattern.

This is similar to if we get sequence-expression, and things like return-expressoins. We'd likely be able to make tons and tons of statements into pure expressions. But should we? Probably not. It will really depend on the code and what the team as a whole feels is a good balance between brevity and readability.

So, in this case, i would use another pattern. but know that maybe after a few months to a year, the team may start liking this more and more and it may eventually move back :)

alrz · 2019-08-22T20:59:25Z

#37950 will be resolved on the compiler side, should revert the change here.

alrz · 2019-08-29T22:38:21Z

@CyrusNajmabadi @ryzngard do you have any more comments?

ryzngard

LGTM with plan to resolve #37949 with a different change

ryzngard · 2019-10-03T00:15:18Z

@CyrusNajmabadi can you take another pass?

CyrusNajmabadi · 2019-10-03T00:49:21Z

@CyrusNajmabadi can you take another pass?

Sure. Will try to do tonight or tomorrow!

CyrusNajmabadi · 2019-10-03T18:06:24Z

LGTM. Thanks for taking care of all of these cases!

CyrusNajmabadi · 2019-10-03T18:07:11Z

...res/CSharpTest/ConvertSwitchStatementToExpression/ConvertSwitchStatementToExpressionTests.cs

+ {
+ return input switch
+ {
+ 1 => 42,// this little piggy went to market


nit. probably want space after the comma.

(super super minor nit)

that's just me not knowing how trivia works. there is no resource and I should just infer from code. not an easy task for something this subtle.

CyrusNajmabadi · 2019-10-03T18:07:34Z

...res/CSharpTest/ConvertSwitchStatementToExpression/ConvertSwitchStatementToExpressionTests.cs

+ return dayOfWeek switch
+ {
+ DayOfWeek.Monday => ""Monday"",
+ _ => ""Other"",


CyrusNajmabadi · 2019-10-03T18:08:29Z

@ryzngard I'm good here!

ryzngard · 2019-10-03T19:13:52Z

Thanks for contributing @alrz !

alrz added 2 commits August 15, 2019 19:33

Fix reported issues with ConvertSwitchStatementToExpression

ac13660

Add tests

6078380

alrz requested a review from a team as a code owner August 15, 2019 15:11

alrz added 2 commits August 15, 2019 20:23

Fix condition

1cd3268

Consider next statement when analyzing nested switch statements

c69a5d9

alrz force-pushed the fix-0066 branch from b000989 to c69a5d9 Compare August 15, 2019 16:05

jinujoseph assigned ryzngard Aug 15, 2019

jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Aug 15, 2019

mavasani reviewed Aug 16, 2019

View reviewed changes

...res/CSharpTest/ConvertSwitchStatementToExpression/ConvertSwitchStatementToExpressionTests.cs Show resolved Hide resolved

mavasani reviewed Aug 16, 2019

View reviewed changes

...ertSwitchStatementToExpression/ConvertSwitchStatementToExpressionCodeFixProvider.Rewriter.cs Outdated Show resolved Hide resolved

jinujoseph assigned mavasani Aug 16, 2019

ryzngard reviewed Aug 16, 2019

View reviewed changes

ryzngard requested changes Aug 16, 2019

View reviewed changes

alrz commented Aug 17, 2019

View reviewed changes

...ertSwitchStatementToExpression/ConvertSwitchStatementToExpressionCodeFixProvider.Rewriter.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Aug 18, 2019

View reviewed changes

...ertSwitchStatementToExpression/ConvertSwitchStatementToExpressionCodeFixProvider.Rewriter.cs Outdated Show resolved Hide resolved

alrz added 4 commits August 21, 2019 19:39

Add comments

1a1187d

Rename tuple elements

4ea941e

Fix test code

5cf07d5

Use out argument instead of ref

cc32e40

jasonmalinowski self-assigned this Aug 22, 2019

CyrusNajmabadi reviewed Aug 22, 2019

View reviewed changes

...SwitchStatementToExpression/ConvertSwitchStatementToExpressionDiagnosticAnalyzer.Analyzer.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Aug 22, 2019

View reviewed changes

alrz mentioned this pull request Aug 22, 2019

IDE0066 incorrectly handles nullable value types #37950

Closed

alrz added 3 commits August 29, 2019 14:35

No need to cast null literals

4d2588d

Address feedback from Cyrus

93c7cf1

Remove redundant code

8544486

alrz mentioned this pull request Sep 10, 2019

Update IfToSwitch refactoring for switch expressions #38238

Merged

ryzngard approved these changes Oct 3, 2019

View reviewed changes

CyrusNajmabadi approved these changes Oct 3, 2019

View reviewed changes

CyrusNajmabadi reviewed Oct 3, 2019

View reviewed changes

ryzngard merged commit 77db8b1 into dotnet:master Oct 3, 2019

jinujoseph added this to the 16.4.P3 milestone Oct 3, 2019

alrz deleted the fix-0066 branch February 8, 2021 06:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfixes for ConvertSwitchStatementToExpression #38007

Bugfixes for ConvertSwitchStatementToExpression #38007

alrz commented Aug 15, 2019 •

edited

Loading

ryzngard Aug 16, 2019

alrz Aug 21, 2019

ryzngard Aug 16, 2019

ryzngard Aug 16, 2019

alrz Aug 21, 2019 •

edited

Loading

ryzngard left a comment

alrz commented Aug 21, 2019

jasonmalinowski commented Aug 22, 2019

CyrusNajmabadi Aug 22, 2019

alrz Aug 22, 2019 •

edited

Loading

CyrusNajmabadi Aug 22, 2019

alrz Aug 22, 2019

CyrusNajmabadi Aug 22, 2019

jasonmalinowski Aug 23, 2019

CyrusNajmabadi Aug 23, 2019

alrz commented Aug 22, 2019

alrz commented Aug 29, 2019

ryzngard left a comment

ryzngard commented Oct 3, 2019

CyrusNajmabadi commented Oct 3, 2019

CyrusNajmabadi commented Oct 3, 2019

CyrusNajmabadi Oct 3, 2019

CyrusNajmabadi Oct 3, 2019

alrz Oct 3, 2019

CyrusNajmabadi Oct 3, 2019

CyrusNajmabadi commented Oct 3, 2019

ryzngard commented Oct 3, 2019

Bugfixes for ConvertSwitchStatementToExpression #38007

Bugfixes for ConvertSwitchStatementToExpression #38007

Conversation

alrz commented Aug 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrz Aug 21, 2019 • edited Loading

Choose a reason for hiding this comment

ryzngard left a comment

Choose a reason for hiding this comment

alrz commented Aug 21, 2019

jasonmalinowski commented Aug 22, 2019

Choose a reason for hiding this comment

alrz Aug 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrz commented Aug 22, 2019

alrz commented Aug 29, 2019

ryzngard left a comment

Choose a reason for hiding this comment

ryzngard commented Oct 3, 2019

CyrusNajmabadi commented Oct 3, 2019

CyrusNajmabadi commented Oct 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Oct 3, 2019

ryzngard commented Oct 3, 2019

alrz commented Aug 15, 2019 •

edited

Loading

alrz Aug 21, 2019 •

edited

Loading

alrz Aug 22, 2019 •

edited

Loading