Code refactor action: Remove redundant else statement #68529
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
ghost
added
the
Community
kandicst
changed the title
...arp/Analyzers/RemoveRedundantElseStatement/RemoveRedundantElseStatementDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
...arp/Analyzers/RemoveRedundantElseStatement/RemoveRedundantElseStatementDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
...arp/Analyzers/RemoveRedundantElseStatement/RemoveRedundantElseStatementDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
...arp/Analyzers/RemoveRedundantElseStatement/RemoveRedundantElseStatementDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
|
|
||
| return elseClause; | ||
| } | ||
| private static bool IsJumpStatement(StatementSyntax? statement) |
There was a problem hiding this comment.
nit: make local functino above.
...arp/Analyzers/RemoveRedundantElseStatement/RemoveRedundantElseStatementDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
...arp/Analyzers/RemoveRedundantElseStatement/RemoveRedundantElseStatementDiagnosticAnalyzer.cs
Show resolved
Hide resolved
...CSharp/CodeFixes/RemoveRedundantElseStatement/RemoveRedundantElseStatementCodeFixProvider.cs
Outdated
Show resolved
Hide resolved
...CSharp/CodeFixes/RemoveRedundantElseStatement/RemoveRedundantElseStatementCodeFixProvider.cs
Outdated
Show resolved
Hide resolved
...CSharp/CodeFixes/RemoveRedundantElseStatement/RemoveRedundantElseStatementCodeFixProvider.cs
Outdated
Show resolved
Hide resolved
...CSharp/CodeFixes/RemoveRedundantElseStatement/RemoveRedundantElseStatementCodeFixProvider.cs
Outdated
Show resolved
Hide resolved
...CSharp/CodeFixes/RemoveRedundantElseStatement/RemoveRedundantElseStatementCodeFixProvider.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/CSharp/Tests/RemoveRedundantElseStatement/RemoveRedundantElseStatementTests.cs
Outdated
Show resolved
Hide resolved
| @@ -257,6 +257,11 @@ private static Option2<CodeStyleOption2<ExpressionBodyPreference>> CreatePreferE | |||
| "csharp_style_prefer_primary_constructors", | |||
| CSharpIdeCodeStyleOptions.Default.PreferPrimaryConstructors); | |||
|
|
|||
| public static readonly Option2<CodeStyleOption2<bool>> PreferRemoveRedundantElseStatement = CreateOption( | |||
| CSharpCodeStyleOptionGroups.CodeBlockPreferences, | |||
| "csharp_style_prefer_remove_redundant_else_statement", | |||
There was a problem hiding this comment.
this should probably be dotnet_... since it applies to VB as well. csharp_style is only for constructs that are c# specific.
There was a problem hiding this comment.
Making that change only would fail this check since Option language is C# by default
Should this option then go in some other file or just create it here with language as null?
There was a problem hiding this comment.
correct. it goes into another file with teh rest of the dotnet style options.
|
This is a great start. Thanks for the contribution! |
...CSharp/CodeFixes/RemoveRedundantElseStatement/RemoveRedundantElseStatementCodeFixProvider.cs
Outdated
Show resolved
Hide resolved
|
Fyi. I'm currently in vacation for another week. But I am very enthusiastic about this pr. I will try to get some reviews in early, but can't promise because of commitments. |
src/Analyzers/CSharp/Tests/RemoveRedundantElseStatement/RemoveRedundantElseStatementTests.cs
Outdated
Show resolved
Hide resolved
| RemoveRedundantElseStatementCodeFixProvider>; | ||
|
|
||
| [Trait(Traits.Feature, Traits.Features.CodeActionsRemoveRedundantElseStatement)] | ||
| public class RemoveRedundantElseStatementTests |
There was a problem hiding this comment.
can you add some fix-all tests? That should have if-statements within the an outer if's 'else' block. both should have redundant else blocks.
Note: because of fix all, it's possible one fix will change things such that there might be an issue with the inner local variables now being a collision problem when fixing the outer. i can suggest ways to address that if that occurs.
There was a problem hiding this comment.
added the fix-all, but now sure how to reproduce the inner/outer collision case you described
There was a problem hiding this comment.
The inner else should have a variable that can either move up two levels, or only one.
|
@kandicst Ok. Done with cleanup/rewriting. Tehre are still some pieces of feedback i've left in the PR that would be good to take care of. Thanks! |
| /// (return, break, continue or throw) in order to preserve the program's correctness. | ||
| /// </summary> | ||
| [DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
| internal sealed class RemoveRedundantElseStatementDiagnosticAnalyzer : AbstractBuiltInCodeStyleDiagnosticAnalyzer |
There was a problem hiding this comment.
📝 I worry about adding this as an analyzer and not a refactoring. There have been many cases where I included an explicit else for clarity, and I wouldn't want an analyzer undoing that work.
There was a problem hiding this comment.
I think both can work here. It's fine as an analyzer (it's off by default).
When off, it would make sense to have as a refactoring too. I can make such a change
Fixes #60385