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.

Sign up for GitHub

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

Jump to bottom

Add feature to convert switch statement to switch expression #31812

Merged
merged 45 commits into from May 23, 2019
Merged

Add feature to convert switch statement to switch expression #31812

merged 45 commits into from May 23, 2019

Conversation

alrz
Copy link
Member

@alrz alrz commented Dec 15, 2018 •
edited

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

/cc @gafter, @CyrusNajmabadi

@CyrusNajmabadi
Copy link
Member

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
Member

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
Member

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
Member

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
Member

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
Member

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
@@ -45,14 +45,23 @@ private static Option<T> CreateOption<T>(OptionGroup group, string name, T defau
EditorConfigStorageLocation.ForBoolCodeStyleOption("csharp_style_conditional_delegate_call"),
new RoamingProfileStorageLocation("TextEditor.CSharp.Specific.PreferConditionalDelegateCall")});

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

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
Member

@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

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
Member

@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
Member

@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
Member

I cannot wait for this :)

@mavasani
Copy link
Member

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

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@jcouv jcouv jcouv left review comments

@mavasani mavasani mavasani approved these changes

Assignees

@mavasani mavasani

Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. Feature Request New Language Feature - Pattern Matching Pattern Matching
Projects
None yet
Milestone
16.2.P2
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@alrz @CyrusNajmabadi @mavasani @jcouv @jinujoseph