Update IfToSwitch refactoring for switch expressions #38238

alrz · 2019-08-23T14:26:19Z

Also the feature is rewritten to share most logic for VB and C# as they get close to feature parity.

CyrusNajmabadi · 2019-08-23T16:10:19Z

Overall looking good! Can review on full later today. One thing I noticed is that there's a file with a great overview on comments about the supported forms. However, I think the rest of the file could use some comments as well as each form is handled

CyrusNajmabadi · 2019-08-23T18:03:44Z

src/Compilers/Core/Portable/InternalUtilities/EnumerableExtensions.cs

@@ -546,5 +546,24 @@ public static bool SequenceEqual<T>(this IEnumerable<T> first, IEnumerable<T> se

 return true;
 }
+
+ public static T AggregateOrDefault<T>(this IEnumerable<T> source, Func<T, T, T> func)


would it be simpler to assume an aggregation function that does nothing on default values, allowing us to write:

T result = default; foreach (var x in source) { result = agg(result, x); } return result;

we want a fold function without a seed (reduce), the initial default there doesn't work well with the use case.

would it be simpler to assume an aggregation function that does nothing on default

that would only make the definition simpler, it has to be handled by the caller.

src/EditorFeatures/CSharpTest/CodeActions/ConvertIfToSwitch/ConvertIfToSwitchTests.cs

CyrusNajmabadi · 2019-08-23T18:04:50Z

...Features/Core/Portable/ConvertIfToSwitch/AbstractConvertIfToSwitchCodeRefactoringProvider.cs

- await CreateAnalyzer(syntaxFacts, semanticModel)
- .ComputeRefactoringsAsync(context).ConfigureAwait(false);
+ var analyzer = CreateAnalyzer(context.Document.GetLanguageService<ISyntaxFactsService>());
+ return analyzer.ComputeRefactoringsAsync(context);


nit: consider inlining the temp and making a => function.

I think I'm not quite done with code structure here, the Analyzer class should probably be splitted to a few other. would appreciate any idea on that

CyrusNajmabadi · 2019-08-23T18:05:22Z

...Features/Core/Portable/ConvertIfToSwitch/AbstractConvertIfToSwitchCodeRefactoringProvider.cs

 {
 Task ComputeRefactoringsAsync(CodeRefactoringContext context);
 }

- protected abstract IAnalyzer CreateAnalyzer(ISyntaxFactsService syntaxFacts, SemanticModel semanticModel);
+ public abstract class Pattern


don't love hte name here (yet). without a comment, i have no idea what this conveys.

CyrusNajmabadi · 2019-08-23T18:06:02Z

...Features/Core/Portable/ConvertIfToSwitch/AbstractConvertIfToSwitchCodeRefactoringProvider.cs

- where TExpressionSyntax : SyntaxNode
- where TIfStatementSyntax : SyntaxNode
- where TSwitchLabelSyntax : SyntaxNode
+ public sealed class SwitchLabel


consider comments. i can't tell if this is supposed to represent the code that was anlayzed, the code that is being generated, (both), etc.

...Features/Core/Portable/ConvertIfToSwitch/AbstractConvertIfToSwitchCodeRefactoringProvider.cs

...Features/CSharp/Portable/ConvertIfToSwitch/CSharpConvertIfToSwitchCodeRefactoringProvider.cs

CyrusNajmabadi · 2019-08-23T18:12:42Z

...Features/CSharp/Portable/ConvertIfToSwitch/CSharpConvertIfToSwitchCodeRefactoringProvider.cs

+ expression: (ExpressionSyntax)expression,
+ closeParenToken: ifStatement.CloseParenToken.WithPrependedLeadingTrivia(ElasticMarker),
+ openBraceToken: block?.OpenBraceToken ?? Token(SyntaxKind.OpenBraceToken),
+ sections: List(sectionList.OfType<SwitchSectionSyntax>()),


when do we have a section list not built out of SwitchSectionSyntax?

if we have other types of nodes in that list, it looks like we're just droppin it on the floor. is that ok?

this is not changed in this PR. it should just use Cast (or generic if possible)

i somewhat see thsi change as near a total rewrite. So not a bad time to jsut clean these things up as well :)

...Features/CSharp/Portable/ConvertIfToSwitch/CSharpConvertIfToSwitchCodeRefactoringProvider.cs

CyrusNajmabadi · 2019-08-23T18:13:27Z

...Features/CSharp/Portable/ConvertIfToSwitch/CSharpConvertIfToSwitchCodeRefactoringProvider.cs

- var requiresBlock = !_semanticModel.AnalyzeDataFlow(node).VariablesDeclared.IsDefaultOrEmpty;
+ var node = operation.Syntax;
+ var requiresBreak = operation.SemanticModel.AnalyzeControlFlow(node).EndPointIsReachable;
+ var requiresBlock = !operation.SemanticModel.AnalyzeDataFlow(node).VariablesDeclared.IsDefaultOrEmpty;


can this method not somehow be shared between VB and C#?

CyrusNajmabadi · 2019-08-23T18:13:51Z

...Features/CSharp/Portable/ConvertIfToSwitch/CSharpConvertIfToSwitchCodeRefactoringProvider.cs

- case CastExpressionSyntax castExpression:
- return castExpression.Expression;
+ return expression is null ? null : WhenClause(expression);
+ }


CyrusNajmabadi · 2019-08-23T18:14:12Z

...Features/CSharp/Portable/ConvertIfToSwitch/CSharpConvertIfToSwitchCodeRefactoringProvider.cs

+ return CasePatternSwitchLabel(
+ AsPatternSyntax(label.Pattern),
+ AsWhenClause(label),
+ Token(SyntaxKind.ColonToken));
 }


nit: =>

can we make this return a TSwitchLabelSyntax?

CyrusNajmabadi · 2019-08-23T18:14:37Z

...Features/CSharp/Portable/ConvertIfToSwitch/CSharpConvertIfToSwitchCodeRefactoringProvider.cs

 {
- var block = ifStatement.Statement as BlockSyntax;
+ return AsWhenClause(label.Guards
+ .Cast<ExpressionSyntax>()


if you know all the guards are expressions, can we make it TExpressionSyntax?

CyrusNajmabadi · 2019-08-23T18:15:04Z

...Features/CSharp/Portable/ConvertIfToSwitch/CSharpConvertIfToSwitchCodeRefactoringProvider.cs

- var block = ifStatement.Statement as BlockSyntax;
+ return AsWhenClause(label.Guards
+ .Cast<ExpressionSyntax>()
+ .AggregateOrDefault((prev, current) => BinaryExpression(SyntaxKind.LogicalAndExpression, current.WalkDownParentheses(), prev)));


if you walk down parens, could you not screw up precedence in some way?

src/Workspaces/Core/Portable/Utilities/Contract.cs

alrz · 2019-10-03T19:32:17Z

ping @jasonmalinowski
@ryzngard could you drive this PR as well? thanks

jasonmalinowski

Only real questions here are:

A suspicious ! to suppress a nullable warning that wasn't obvious why it was the case and could either use a comment as to why it's safe or just a Contract.ThrowIfNull() if it's not obvious.
Why a test that was asserting whitespace isn't doing that anymore. I can't say I really care about the whitespace scenario but do worry if we're dropping other kinds of trivia somehow.

Otherwise most of my comments are just requests to convert regular comments to doc comments since it's nice when navigating around to be able to hover and see explanations for things. @alrz I'm happy to fix those up if you don't have the time as penance for not reviewing this soon enough.

jasonmalinowski · 2019-10-08T19:51:23Z

src/EditorFeatures/CSharpTest/CodeActions/ConvertIfToSwitch/ConvertIfToSwitchTests.cs

@@ -341,7 +341,7 @@ void M(object o)
 {
 switch (o)
 {
- case string s when s.Length > 5 && s.Length < 10:
+ case string s when (s.Length > 5 && s.Length < 10):


Why did it get worse though? I would have assumed the parenthesis remover was previously running?

jasonmalinowski · 2019-10-08T19:52:17Z

src/EditorFeatures/CSharpTest/CodeActions/ConvertIfToSwitch/ConvertIfToSwitchTests.cs

@@ -380,7 +380,7 @@ void M(object o)
 }

 [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertIfToSwitch)]
- public async Task TestPreserveTrivia()
+ public async Task TestComplexExpression_01()


The original test was trying to test for maintaining whitespace, is that something we're considering not necessary any more? I see there's plenty of tests dealing with non-whitespace trivia so if you're not worried then I'm not worried.

src/EditorFeatures/CSharpTest/CodeActions/ConvertIfToSwitch/ConvertIfToSwitchTests.cs

jasonmalinowski · 2019-10-08T20:11:40Z

...Core/Portable/ConvertIfToSwitch/AbstractConvertIfToSwitchCodeRefactoringProvider.Analyzer.cs

+ //
+ internal abstract class Analyzer
+ {
+ private SyntaxNode? _switchTargetExpression;


(this might have been a good use for a doc comment so it's friendly to quick info)

jasonmalinowski · 2019-10-08T20:12:19Z

...Core/Portable/ConvertIfToSwitch/AbstractConvertIfToSwitchCodeRefactoringProvider.Analyzer.cs

+ sections.Add(new AnalyzedSwitchSection(labels: default, defaultBodyOpt, defaultBodyOpt.Syntax));
+ }
+
+ return (sections.ToImmutableAndFree(), _switchTargetExpression!);


Why the ! here on _switchTargetExpression?

_switchTargetExpression is initially unset be we don't want to allow assigning null values, plus, at the end it is expected to be set.

I think SyntaxNode _switchTargetExpression = null! fits the above requirements?

But we need to assert anyways though I think it should be a DEBUG assertion - ~~we don't have access to RoslynDebug~~

You can either RoslynDebug.Assert() if you want or you can use Contract.ThrowIfNull(). I have no problem having a release assert in that I'm not entirely confident this would not crash if this was null...

@sharwell also has a nice way you can annotate this as "it might be null to start but can't be assigned null" but I forget what it is off hand...

...Portable/ConvertIfToSwitch/AbstractConvertIfToSwitchCodeRefactoringProvider.AnalyzedNodes.cs

jasonmalinowski · 2019-10-08T20:17:58Z

...Core/Portable/ConvertIfToSwitch/AbstractConvertIfToSwitchCodeRefactoringProvider.Analyzer.cs

+
+ if (!ParseIfStatementSequence(operations.Slice(1), sections, out defaultBodyOpt))
+ {
+ var nextStatement = operations[1];


The second bit "if there was no if/else then it would have returned false" might warrant a comment in the code here. (The question about Slice we can just call a learning opportunity for @CyrusNajmabadi and I.)

jasonmalinowski · 2019-10-08T20:25:31Z

...Core/Portable/ConvertIfToSwitch/AbstractConvertIfToSwitchCodeRefactoringProvider.Analyzer.cs

+ {
+ return operatorKind switch
+ {
+ LessThan => GreaterThan,


Honestly, at first I went "uh..." until I saw @CyrusNajmabadi's that indicated this was probably intentional. A doc comment of "changes the direction the operator is pointing" or something is probably for the best.

jasonmalinowski · 2019-10-08T20:57:13Z

...CSharp/Portable/ConvertIfToSwitch/CSharpConvertIfToSwitchCodeRefactoringProvider.Analyzer.cs

+ }
+
+ public override bool HasUnreachableEndPoint(IOperation operation)
+ => !operation.SemanticModel.AnalyzeControlFlow(operation.Syntax).EndPointIsReachable;


Why does VB need the first/last one? Not sure if this is really an API request to the compiler team in disguise.

jasonmalinowski · 2019-10-08T22:03:34Z

Also @alrz given this is more or less a rewrite of the feature and has enough churn, at this point per our internal bar we'd be targeting this for 16.5 instead of 16.4. The code looks great but that's a bar we'd be applying to any newer feature work like this at this point. I'll update the targeted branch but shouldn't need any other work from you at this point. Let me know if that causes any problems or concerns for you.

As usual, great code! 😄

alrz · 2019-10-09T17:39:47Z

It's weird I can't reply to some of review comments - there's no comment box. Is it possible that you have reviewed an older commit?

ah nevermind, you were replying to the previous reviews but github shows another diff here.

jasonmalinowski · 2019-10-09T21:29:10Z

@alrz Yeah the UI is strange -- it'll show just my comments leaving you guessing what I was responding to.

…nify-if-to-switch

alrz · 2019-10-14T17:57:44Z

I think the only major remaining issue is the trivia handling, we could file a bug to track further work.

jasonmalinowski

Changes looking good but @alrz did you want to add an assert for that null suppression? That one worries me since it's definitely not "obvious"...

jasonmalinowski · 2019-10-16T00:11:08Z

...Core/Portable/ConvertIfToSwitch/AbstractConvertIfToSwitchCodeRefactoringProvider.Analyzer.cs

+ sections.Add(new AnalyzedSwitchSection(labels: default, defaultBodyOpt, defaultBodyOpt.Syntax));
+ }
+
+ return (sections.ToImmutableAndFree(), _switchTargetExpression!);


You can either RoslynDebug.Assert() if you want or you can use Contract.ThrowIfNull(). I have no problem having a release assert in that I'm not entirely confident this would not crash if this was null...

jasonmalinowski · 2019-10-16T00:13:34Z

...Core/Portable/ConvertIfToSwitch/AbstractConvertIfToSwitchCodeRefactoringProvider.Analyzer.cs

+ sections.Add(new AnalyzedSwitchSection(labels: default, defaultBodyOpt, defaultBodyOpt.Syntax));
+ }
+
+ return (sections.ToImmutableAndFree(), _switchTargetExpression!);


@sharwell also has a nice way you can annotate this as "it might be null to start but can't be assigned null" but I forget what it is off hand...

jasonmalinowski · 2019-10-16T00:17:38Z

And oh yeah regarding trivia: I don't see you breaking any tests, so do we think we really made anything worse or it's as "bad" as it ever was?

alrz · 2019-10-16T06:35:52Z

You can either RoslynDebug.Assert() if you want

This is the one being used in the latest iteration.

regarding trivia: I don't see you breaking any tests, so do we think we really made anything worse or it's as "bad" as it ever was?

It's the trivia for the guard expression that is lost (see #38238 (comment)).

alrz · 2019-10-16T06:45:14Z

@sharwell also has a nice way you can annotate this as "it might be null to start but can't be assigned null" but I forget what it is off hand...

I barely remember but I think that was for generics? .. anyways I used a non-nullable reference type initialized with null! for that.

jasonmalinowski · 2019-10-16T16:29:48Z

Ah sorry, for some reason I still saw the old version. I think this PR at this point is pushing GitHub's limits in terms of UI...

In any case, awesome work for all of this, happy to merge!

alrz · 2019-10-17T09:02:03Z

I found an issue with this when subsequent blocks are not handled properly (they are not removed) but it's unlikely to happen in code and it won't crash anyways. I will add more tests and fix possible issues whenever I could.

jasonmalinowski · 2019-10-17T16:19:19Z

@alrz Thanks for the heads up, it happens! Even if you can't get the code fix, file a bug quickly just so we can track it and have somebody else take a look if necessary.

alrz added 9 commits August 23, 2019 18:43

Share code

f4fadd6

Move members

1dc2c3f

Cleanup

272d696

Consider next statement

a11806d

Cleanup

4387cba

Fix formatting

2f43d10

Indent code

285680b

Suggest to convert to switch expression

adf6d32

Revert and add applicable spans

85adc88

alrz requested review from a team as code owners August 23, 2019 14:26