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.

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

Merged
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
f4fadd6
Share code
alrz Aug 23, 2019
1dc2c3f
Move members
alrz Aug 23, 2019
272d696
Cleanup
alrz Aug 23, 2019
a11806d
Consider next statement
alrz Aug 23, 2019
4387cba
Cleanup
alrz Aug 23, 2019
2f43d10
Fix formatting
alrz Aug 23, 2019
285680b
Indent code
alrz Aug 23, 2019
adf6d32
Suggest to convert to switch expression
alrz Aug 23, 2019
85adc88
Revert and add applicable spans
alrz Aug 23, 2019
1dbee18
Simplify code
alrz Sep 9, 2019
314a11d
Make method abstract
alrz Sep 9, 2019
e4c3100
Move variables
alrz Sep 9, 2019
532d880
All nodes are supposed to be switch labels
alrz Sep 9, 2019
e48cb01
Preserve parentheses
alrz Sep 9, 2019
22dabe4
Move types and make nodes generic
alrz Sep 9, 2019
563efc0
Simplify code
alrz Sep 9, 2019
5c64053
Add comments
alrz Sep 9, 2019
691a8bc
Add comments
alrz Sep 9, 2019
469f841
Merge remote-tracking branch 'origin/master' into unify-if-to-switch
alrz Sep 9, 2019
5a1ef42
PR feedback
alrz Sep 9, 2019
3cebb97
Update resources
alrz Sep 10, 2019
3c37fd8
Add comments
alrz Sep 10, 2019
efa85e7
Add expression not-null helper
alrz Sep 10, 2019
c5a96b0
Simplify code
alrz Sep 10, 2019
10a2e3f
Formatting
alrz Sep 10, 2019
d067254
Simplify code.
alrz Sep 10, 2019
9641019
Unwrap conversions
alrz Sep 11, 2019
c1cd335
Check language version
alrz Sep 29, 2019
c7bba4a
Merge remote-tracking branch 'origin/release/dev16.5-preview1' into u…
alrz Oct 10, 2019
ea8f144
Fix nullables
alrz Oct 14, 2019
cdf4fd3
Use doc comments
alrz Oct 14, 2019
12f9859
Attach the simplifier
alrz Oct 14, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

cute!

Copy link
Member

@jcouv jcouv Sep 10, 2019

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:

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

Copy link
Member Author

@alrz alrz Sep 10, 2019

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.

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.

{
using (var e = source.GetEnumerator())
{
if (!e.MoveNext())
{
return default;
}

var result = e.Current;
while (e.MoveNext())
{
result = func(result, e.Current);
}

return result;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

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

not on you, but may be something for parentheses remover to remove.

Copy link
Member

Choose a reason for hiding this comment

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

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

return;
case int i:
return;
Expand Down Expand Up @@ -380,7 +380,7 @@ void M(object o)
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsConvertIfToSwitch)]
public async Task TestPreserveTrivia()
public async Task TestComplexExpression_01()
Copy link
Member

Choose a reason for hiding this comment

The 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
Expand All @@ -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:
Expand Down Expand Up @@ -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()
{
Expand Down Expand Up @@ -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();
Expand All @@ -828,7 +855,7 @@ int M(int i)
}",
@"class C
{
int M(int i)
int M(int x, int z)
{
#if TRUE
Console.WriteLine();
Expand All @@ -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 */
Expand All @@ -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*/ )
{
Expand Down Expand Up @@ -1168,7 +1195,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;
Expand Down Expand Up @@ -1318,7 +1345,7 @@ void M(int i)
{
switch (i)
{
case 1 when (i == 2) && i == 3:
case 1 when ((i == 2) && i == 3):
jasonmalinowski marked this conversation as resolved.
Show resolved Hide resolved
return;
case 10:
return;
Expand Down Expand Up @@ -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;
}
}
}");
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,9 @@ End Class",
Return 5
Case 20
Return 6
Case Else
Return 7
End Select
Return 7
End Function
End Class")
End Function
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// 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 override bool SupportsSwitchExpression => true;
public override bool SupportsCaseGuard => true;
public override bool SupportsRangePattern => false;
public override bool SupportsTypePattern => true;
public override bool SupportsSourcePattern => true;
public override bool SupportsRelationalPattern => false;

public CSharpAnalyzer(ISyntaxFactsService syntaxFacts)
: base(syntaxFacts)
{
}

public override bool HasUnreachableEndPoint(IOperation operation)
=> !operation.SemanticModel.AnalyzeControlFlow(operation.Syntax).EndPointIsReachable;
Copy link
Member

Choose a reason for hiding this comment

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

interesting. this is different from VB?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 => CodeAnalysis.CSharpExtensions.IsKind((SyntaxNode)n, SyntaxKind.BreakStatement));
Copy link
Member

Choose a reason for hiding this comment

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

CodeAnalysis.CSharpExtensions.IsKind?

Copy link
Member Author

Choose a reason for hiding this comment

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

hah. must have been inserted by the codefix

}
}
}

This file was deleted.

Loading