-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
Update IfToSwitch refactoring for switch expressions #38238
Changes from all commits
f4fadd6
1dc2c3f
272d696
a11806d
4387cba
2f43d10
285680b
adf6d32
85adc88
1dbee18
314a11d
e4c3100
532d880
e48cb01
22dabe4
563efc0
5c64053
691a8bc
469f841
5a1ef42
3cebb97
3c37fd8
efa85e7
c5a96b0
10a2e3f
d067254
9641019
c1cd335
c7bba4a
ea8f144
cdf4fd3
12f9859
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -380,7 +380,7 @@ void M(object o) | |
} | ||
|
||
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertIfToSwitch)] | ||
public async Task TestPreserveTrivia() | ||
public async Task TestComplexExpression_01() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
{ | ||
await TestInRegularAndScriptAsync( | ||
@"class C | ||
|
@@ -405,8 +405,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: | ||
M(o: 0); | ||
break; | ||
case int i: | ||
|
@@ -562,12 +561,40 @@ int M(int? i) | |
return 5; | ||
case 0: | ||
return 6; | ||
default: | ||
return 7; | ||
} | ||
return 7; | ||
} | ||
}"); | ||
} | ||
|
||
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertIfToSwitch)] | ||
public async Task TestSwitchExpression_01() | ||
{ | ||
await TestInRegularAndScriptAsync( | ||
@"class C | ||
{ | ||
int M(int? i) | ||
{ | ||
[||]if (i == null) return 5; | ||
if (i == 0) return 6; | ||
return 7; | ||
} | ||
}", | ||
@"class C | ||
{ | ||
int M(int? i) | ||
{ | ||
return i switch | ||
{ | ||
null => 5, | ||
0 => 6, | ||
_ => 7 | ||
}; | ||
} | ||
}", index: 1); | ||
} | ||
|
||
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertIfToSwitch)] | ||
public async Task TestSubsequentIfStatements_02() | ||
{ | ||
|
@@ -810,7 +837,7 @@ public async Task TestTrivia1() | |
await TestInRegularAndScriptAsync( | ||
@"class C | ||
{ | ||
int M(int i) | ||
int M(int x, int z) | ||
{ | ||
#if TRUE | ||
Console.WriteLine(); | ||
|
@@ -828,7 +855,7 @@ int M(int i) | |
}", | ||
@"class C | ||
{ | ||
int M(int i) | ||
int M(int x, int z) | ||
{ | ||
#if TRUE | ||
Console.WriteLine(); | ||
|
@@ -854,7 +881,7 @@ public async Task TestTrivia2() | |
await TestInRegularAndScriptAsync( | ||
@"class C | ||
{ | ||
int M(int i) | ||
int M(int i, string[] args) | ||
jasonmalinowski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
[||]if (/* t0 */args.Length /* t1*/ == /* t2 */ 2) | ||
return /* t3 */ 0 /* t4 */; /* t5 */ | ||
|
@@ -864,7 +891,7 @@ int M(int i) | |
}", | ||
@"class C | ||
{ | ||
int M(int i) | ||
int M(int i, string[] args) | ||
{ | ||
switch (/* t0 */args.Length /* t1*/ ) | ||
{ | ||
|
@@ -958,7 +985,7 @@ void M(int i) | |
{ | ||
switch (i) | ||
{ | ||
case 1 when i == 2 && (i == 3): | ||
case 1 when i == 2 && i == 3: | ||
return; | ||
case 10: | ||
return; | ||
|
@@ -988,7 +1015,7 @@ void M(int i) | |
{ | ||
switch (i) | ||
{ | ||
case 1 when (i == 2) && i == 3: | ||
case 1 when i == 2 && i == 3: | ||
return; | ||
case 10: | ||
return; | ||
|
@@ -1018,7 +1045,7 @@ void M(int i) | |
{ | ||
switch (i) | ||
{ | ||
case 1 when (i == 2) && (i == 3): | ||
case 1 when i == 2 && i == 3: | ||
return; | ||
case 10: | ||
return; | ||
|
@@ -1078,7 +1105,7 @@ void M(int i) | |
{ | ||
switch (i) | ||
{ | ||
case 1 when i == 2 && (i == 3): | ||
case 1 when i == 2 && i == 3: | ||
return; | ||
case 10: | ||
return; | ||
|
@@ -1108,7 +1135,7 @@ void M(int i) | |
{ | ||
switch (i) | ||
{ | ||
case 1 when (i == 2) && i == 3: | ||
case 1 when i == 2 && i == 3: | ||
return; | ||
case 10: | ||
return; | ||
|
@@ -1138,7 +1165,7 @@ void M(int i) | |
{ | ||
switch (i) | ||
{ | ||
case 1 when (i == 2) && (i == 3): | ||
case 1 when i == 2 && i == 3: | ||
return; | ||
case 10: | ||
return; | ||
|
@@ -1258,7 +1285,7 @@ void M(int i) | |
{ | ||
switch (i) | ||
{ | ||
case 1 when (i == 2) && i == 3: | ||
case 1 when i == 2 && i == 3: | ||
return; | ||
case 10: | ||
return; | ||
|
@@ -1288,7 +1315,7 @@ void M(int i) | |
{ | ||
switch (i) | ||
{ | ||
case 1 when (i == 2) && (i == 3): | ||
case 1 when i == 2 && i == 3: | ||
return; | ||
case 10: | ||
return; | ||
|
@@ -1318,7 +1345,7 @@ void M(int i) | |
{ | ||
switch (i) | ||
{ | ||
case 1 when (i == 2) && i == 3: | ||
case 1 when i == 2 && i == 3: | ||
return; | ||
case 10: | ||
return; | ||
|
@@ -1348,12 +1375,46 @@ void M(int i) | |
{ | ||
switch (i) | ||
{ | ||
case 1 when i == 2 && (i == 3): | ||
case 1 when i == 2 && i == 3: | ||
return; | ||
case 10: | ||
return; | ||
} | ||
} | ||
}"); | ||
} | ||
|
||
[WorkItem(37035, "https:/dotnet/roslyn/issues/37035")] | ||
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertIfToSwitch)] | ||
public async Task TestComplexExpression_02() | ||
{ | ||
await TestInRegularAndScriptAsync( | ||
@"class C | ||
{ | ||
void M(object o) | ||
{ | ||
[||]if (o is string text && | ||
int.TryParse(text, out var n) && | ||
n < 5 && n > -5) | ||
{ | ||
} | ||
else | ||
{ | ||
} | ||
} | ||
}", | ||
@"class C | ||
{ | ||
void M(object o) | ||
{ | ||
switch (o) | ||
{ | ||
case string text when int.TryParse(text, out var n) && n < 5 && n > -5: | ||
break; | ||
default: | ||
break; | ||
} | ||
} | ||
}"); | ||
} | ||
} | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System.Linq; | ||
using Microsoft.CodeAnalysis.LanguageServices; | ||
using Microsoft.CodeAnalysis.Operations; | ||
|
||
namespace Microsoft.CodeAnalysis.CSharp.ConvertIfToSwitch | ||
{ | ||
internal sealed partial class CSharpConvertIfToSwitchCodeRefactoringProvider | ||
{ | ||
private sealed class CSharpAnalyzer : Analyzer | ||
{ | ||
public CSharpAnalyzer(ISyntaxFactsService syntaxFacts, Feature features) | ||
: base(syntaxFacts, features) | ||
{ | ||
} | ||
|
||
public override bool HasUnreachableEndPoint(IOperation operation) | ||
=> !operation.SemanticModel.AnalyzeControlFlow(operation.Syntax).EndPointIsReachable; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. interesting. this is different from VB? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In VB we need to use first/last overload, in C# it's not necessary but we could use it for both in the base class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does VB need the first/last one? Not sure if this is really an API request to the compiler team in disguise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because in VB there's no such a thing as an embedded statement, we need to analyze a list of statements, so that's the overload being used. |
||
|
||
// We do not offer a fix if the if-statement contains a break-statement, e.g. | ||
// | ||
// while (...) | ||
// { | ||
// if (...) { | ||
// break; | ||
// } | ||
// } | ||
// | ||
// When the 'break' moves into the switch, it will have different flow control impact. | ||
public override bool CanConvert(IConditionalOperation operation) | ||
=> !operation.SemanticModel.AnalyzeControlFlow(operation.Syntax).ExitPoints.Any(n => n.IsKind(SyntaxKind.BreakStatement)); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cute!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be simpler to assume an aggregation function that does nothing on default values, allowing us to write:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want a fold function without a seed (reduce), the initial default there doesn't work well with the use case.
that would only make the definition simpler, it has to be handled by the caller.