Skip to content

Add feature to convert switch statement to switch expression#31812

Merged
mavasani merged 45 commits intodotnet:masterfrom
alrz:switch
May 23, 2019
Merged

Add feature to convert switch statement to switch expression#31812
mavasani merged 45 commits intodotnet:masterfrom
alrz:switch

Conversation

@alrz
Copy link
Member

@alrz alrz commented Dec 15, 2018 •
edited
Loading

No description provided.

@alrz alrz requested a review from a team as a code owner December 15, 2018 03:37
@alrz
Copy link
Member Author

alrz commented Dec 15, 2018 •
edited
Loading

/cc @gafter, @CyrusNajmabadi

@CyrusNajmabadi
Copy link
Contributor

CyrusNajmabadi commented Dec 15, 2018

Definitely intrigued! but needs more explanation as to what it's doing :D

@jinujoseph jinujoseph added this to the 16.0 milestone Dec 15, 2018
@alrz alrz mentioned this pull request Dec 15, 2018
Open
59 tasks
mavasani
mavasani reviewed Feb 19, 2019
View reviewed changes
...res/CSharpTest/ConvertSwitchStatementToExpression/ConvertSwitchStatementToExpressionTests.cs Outdated
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertSwitchStatementToExpression)]
public async Task TestAssignmnet_Array()
Copy link
Contributor

@mavasani mavasani Feb 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Typo Assignmnet

...res/CSharpTest/ConvertSwitchStatementToExpression/ConvertSwitchStatementToExpressionTests.cs Outdated
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertSwitchStatementToExpression)]
public async Task TestMissingOnDefalutBreak_01()
Copy link
Contributor

@mavasani mavasani Feb 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Typo Defalut

Copy link
Contributor

@mavasani mavasani Feb 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same typo in tests below.

...ertSwitchStatementToExpression/ConvertSwitchStatementToExpressionCodeFixProvider.Rewriter.cs

// Only on simple assignments we attempt to remove variable declarators.
var isSimpleAssignment = nodeToGenerate == SyntaxKind.SimpleAssignmentExpression;
var generateDeclaration = isSimpleAssignment && rewriter.TryRemoveVariableDeclarators(switchStatement, semanticModel, editor);
Copy link
Contributor

@mavasani mavasani Feb 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you attempt to switch to using this service? I think that might simplify the code quite a bit.

...e/ConvertSwitchStatementToExpression/ConvertSwitchStatementToExpressionDiagnosticAnalyzer.cs Outdated
=> false;

public override DiagnosticAnalyzerCategory GetAnalyzerCategory()
=> DiagnosticAnalyzerCategory.SyntaxTreeWithoutSemanticsAnalysis;
Copy link
Contributor

@mavasani mavasani Feb 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong, as explained in detail by this comment added recently by @CyrusNajmabadi. You want SemanticSpanAnalysis here.

src/Workspaces/CSharp/Portable/CodeStyle/CSharpCodeStyleOptions.cs
EditorConfigStorageLocation.ForBoolCodeStyleOption("csharp_style_conditional_delegate_call"),
new RoamingProfileStorageLocation("TextEditor.CSharp.Specific.PreferConditionalDelegateCall")});

public static readonly Option<CodeStyleOption<bool>> PreferSwitchExpression = CreateOption(
Copy link
Contributor

@mavasani mavasani Feb 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is failing as this change needs a corresponding baseline update in tests within http://source.roslyn.io/#Microsoft.VisualStudio.LanguageServices.UnitTests/Options/CSharpEditorConfigGeneratorTests.vb,12.

@mavasani
Copy link
Contributor

mavasani commented May 13, 2019

@alrz Would you be able to address the last set of feedback comments? PR seems very close to completion.

alrz
alrz commented May 17, 2019
View reviewed changes
...ertSwitchStatementToExpression/ConvertSwitchStatementToExpressionCodeFixProvider.Rewriter.cs Outdated
var generateDeclaration = isSimpleAssignment && rewriter.TryRemoveVariableDeclarators(switchStatement, semanticModel, editor);

// Generate the final statement to wrap the switch expression, e.g. a "return" or an assignment.
return rewriter.GetFinalStatement(switchExpression,
Copy link
Member Author

@alrz alrz Feb 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a cast if none of arms have a neutral type.

@alrz
Copy link
Member Author

alrz commented May 17, 2019

Did you attempt to switch to using this service? I think that might simplify the code quite a bit.

I did not go that path for now because at the moment we rewrite the document in one pass.

We should eventually use recursive approach so that we can handle nested switches as well.

@alrz
Copy link
Member Author

alrz commented May 17, 2019

I'll resolve conflicts as the last step. ping me if this looks good so far.

mavasani
mavasani approved these changes May 17, 2019
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alrz.

alrz added 5 commits May 22, 2019 13:33
@alrz
Merge remote-tracking branch 'origin/master' into switch-merge
35c5cf7 
# Conflicts:
#	src/Features/CSharp/Portable/CSharpFeaturesResources.resx
#	src/Features/Core/Portable/Diagnostics/Analyzers/IDEDiagnosticIds.cs
#	src/VisualStudio/CSharp/Impl/CSharpVSResources.resx
#	src/VisualStudio/CSharp/Impl/Options/Formatting/StyleViewModel.cs
#	src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.cs.xlf
#	src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.de.xlf
#	src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.es.xlf
#	src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.fr.xlf
#	src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.it.xlf
#	src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.ja.xlf
#	src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.ko.xlf
#	src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.pl.xlf
#	src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.pt-BR.xlf
#	src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.ru.xlf
#	src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.tr.xlf
#	src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.zh-Hans.xlf
#	src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.zh-Hant.xlf
@alrz
Update resources.
4565b19
@alrz
Fix formatting.
04a5297
@alrz
Fix typo.
b63c5f1
@alrz
Fix test.
833c7cd
@alrz alrz closed this May 22, 2019
@alrz alrz reopened this May 22, 2019
@alrz
Copy link
Member Author

alrz commented May 22, 2019

tests are not running for some reason?

@mavasani mavasani closed this May 22, 2019
@mavasani mavasani reopened this May 22, 2019
@mavasani
Copy link
Contributor

mavasani commented May 22, 2019

@dotnet-bot retest this please

@mavasani mavasani closed this May 23, 2019
@mavasani mavasani reopened this May 23, 2019
@mavasani mavasani merged commit 8911c9f into dotnet:master May 23, 2019
@CyrusNajmabadi
Copy link
Contributor

CyrusNajmabadi commented May 23, 2019

I cannot wait for this :)

@mavasani
Copy link
Contributor

mavasani commented May 23, 2019

I cannot wait for this :)

Totally, agree. Thanks @alrz

@jinujoseph jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label May 29, 2019
@jinujoseph jinujoseph modified the milestones: 16.0, 16.2.P2 May 29, 2019
@alrz alrz deleted the switch branch July 19, 2019 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jcouv jcouv jcouv left review comments

+2 more reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@mavasani mavasani mavasani approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mavasani mavasani

Labels

Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. Feature - Pattern Matching Pattern Matching Feature Request

Projects

None yet

Milestone

16.2.P2

Development

Successfully merging this pull request may close these issues.

5 participants

@alrz @CyrusNajmabadi @mavasani @jcouv @jinujoseph