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

Handle patterns in populate switch expressions #50984

Merged
merged 8 commits into from
Mar 5, 2021
Merged

Handle patterns in populate switch expressions #50984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
79fc635
Handle patterns in populate switch expressions
Youssef1313 Feb 3, 2021
708274a
Add another test
Youssef1313 Feb 3, 2021
989d4e0
Add more tests involving mixing and with or patterns
Youssef1313 Feb 3, 2021
bc4847a
Update src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpr…
Youssef1313 Feb 3, 2021
c39956f
Update PopulateSwitchExpressionHelpers.cs
Youssef1313 Feb 3, 2021
8fe4efd
Address feedback
Youssef1313 Feb 3, 2021
ec7d69f
Fix formatting
Youssef1313 Feb 3, 2021
39a4ad5
Refactor and fix tests
Youssef1313 Feb 4, 2021
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
212 changes: 210 additions & 2 deletions src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchExpressionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -962,7 +962,7 @@ void Method()
(MyEnum)0 => 1,
(MyEnum)1 => 2,
""Mismatching constant"" => 3,
_ => throw new System.NotImplementedException(),
MyEnum.FizzBuzz => throw new System.NotImplementedException(),
Copy link
Member Author

Choose a reason for hiding this comment

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

When "Mismatching constant" was encountered, the analyzer considered the switch as "complete". Hence, it failed to 'add missing cases'.

The "mismatching constant" is a compile-error anyway. So it probably doesn't matter a lot.

}
}
}");
Expand Down Expand Up @@ -1040,7 +1040,7 @@ void Main()
Bar.Option1 => 1,
Bar.Option2 => 2,
null => null,
_ => throw new System.NotImplementedException(),
Bar.Option3 => throw new System.NotImplementedException(),
Comment on lines 1041 to +1043
Copy link
Member Author

Choose a reason for hiding this comment

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

This test is misleading. It's named TestAddMissingCasesForNullableEnum, but the enum in it isn't nullable. Do you want me to make the enum nullable in it?

Copy link
Member

Choose a reason for hiding this comment

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

dont' change existing tests. you can rename them and/or add new tests. thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

@CyrusNajmabadi Well, I'll leave the test case and test name as-is for now. Only the expected result is updated.

Copy link
Member

Choose a reason for hiding this comment

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

wfm. feel free to update the name as well if you want. but genrally don't make the test fit the name, make the name fit the test. and we feel there is a test hole, where no test checks the original name's case, then we can add that :)

};
}

Expand All @@ -1053,5 +1053,213 @@ public enum Bar
}
");
}

[Fact]
[WorkItem(50982, "https://github.com/dotnet/roslyn/issues/50982")]
public async Task TestOrPatternIsHandled()
{
await TestInRegularAndScript1Async(
@"public static class C
{
static bool IsValidValue(E e)
{
return e [||]switch
{
E.A or E.B or E.C => true,
_ = false
};
}

public enum E
{
A,
B,
C,
D,
E,
F,
G,
}
}
",
@"public static class C
{
static bool IsValidValue(E e)
{
return e [||]switch
{
E.A or E.B or E.C => true,
E.D => throw new System.NotImplementedException(),
E.E => throw new System.NotImplementedException(),
E.F => throw new System.NotImplementedException(),
E.G => throw new System.NotImplementedException(),
_ = false
};
}

public enum E
{
A,
B,
C,
D,
E,
F,
G,
}
}
");
}

[Fact]
[WorkItem(50982, "https://github.com/dotnet/roslyn/issues/50982")]
public async Task TestOrPatternIsHandled_AllEnumValuesAreHandled_NoDiagnostic()
{
await TestMissingInRegularAndScriptAsync(
@"public static class C
{
static bool IsValidValue(E e)
{
return e [||]switch
{
(E.A or E.B) or (E.C or E.D) => true,
(E.E or E.F) or (E.G) => true,
_ = false
};
}

public enum E
{
A,
B,
C,
D,
E,
F,
G,
}
}
");
}

[Fact]
[WorkItem(50982, "https://github.com/dotnet/roslyn/issues/50982")]
public async Task TestMixingOrWithAndPatterns()
{
await TestInRegularAndScript1Async(
@"public static class C
{
static bool M(E e)
{
return e [||]switch
{
(E.A or E.B) and (E.C or E.D) => true,
_ = false
};
}

public enum E
{
A,
B,
C,
D,
E,
F,
G,
}
}
",
@"public static class C
{
static bool M(E e)
{
return e [||]switch
{
(E.A or E.B) and (E.C or E.D) => true,
E.A => throw new System.NotImplementedException(),
E.B => throw new System.NotImplementedException(),
E.C => throw new System.NotImplementedException(),
E.D => throw new System.NotImplementedException(),
E.E => throw new System.NotImplementedException(),
E.F => throw new System.NotImplementedException(),
E.G => throw new System.NotImplementedException(),
_ = false
};
}

public enum E
{
A,
B,
C,
D,
E,
F,
G,
}
}
"
);
}

[Fact]
[WorkItem(50982, "https://github.com/dotnet/roslyn/issues/50982")]
public async Task TestMixingOrWithAndPatterns2()
{
await TestInRegularAndScript1Async(
@"public static class C
{
static bool M(E e)
{
return e [||]switch
{
(E.A or E.B) or (E.C and E.D) => true,
_ = false
};
}

public enum E
{
A,
B,
C,
D,
E,
F,
G,
}
}
",
@"public static class C
{
static bool M(E e)
{
return e [||]switch
{
(E.A or E.B) or (E.C and E.D) => true,
E.C => throw new System.NotImplementedException(),
E.D => throw new System.NotImplementedException(),
E.E => throw new System.NotImplementedException(),
E.F => throw new System.NotImplementedException(),
E.G => throw new System.NotImplementedException(),
_ = false
};
}

public enum E
{
A,
B,
C,
D,
E,
F,
G,
}
}
"
);
}
}
}
44 changes: 26 additions & 18 deletions src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ public static ICollection<ISymbol> GetMissingEnumMembers(ISwitchExpressionOperat
var switchExpression = operation.Value;
var switchExpressionType = switchExpression?.Type;

var enumMembers = new Dictionary<long, ISymbol>();

// Check if the type of the expression is a nullable INamedTypeSymbol
// if the type is both nullable and an INamedTypeSymbol extract the type argument from the nullable
// and check if it is of enum type
Expand All @@ -29,36 +27,46 @@ public static ICollection<ISymbol> GetMissingEnumMembers(ISwitchExpressionOperat

if (switchExpressionType?.TypeKind == TypeKind.Enum)
{
if (!PopulateSwitchStatementHelpers.TryGetAllEnumMembers(switchExpressionType, enumMembers) ||
!TryRemoveExistingEnumMembers(operation, enumMembers))
var enumMembers = new Dictionary<long, ISymbol>();
if (PopulateSwitchStatementHelpers.TryGetAllEnumMembers(switchExpressionType, enumMembers))
{
return SpecializedCollections.EmptyCollection<ISymbol>();
RemoveExistingEnumMembers(operation, enumMembers);
return enumMembers.Values;
}
}

return enumMembers.Values;
return SpecializedCollections.EmptyCollection<ISymbol>();
}

private static bool TryRemoveExistingEnumMembers(
private static void RemoveExistingEnumMembers(
ISwitchExpressionOperation operation, Dictionary<long, ISymbol> enumMembers)
{
foreach (var arm in operation.Arms)
{
if (arm.Pattern is IConstantPatternOperation constantPattern)
RemoveIfConstantPatternHasValue(arm.Pattern, enumMembers);
if (arm.Pattern is IBinaryPatternOperation binaryPattern)
{
var constantValue = constantPattern.Value.ConstantValue;
if (!constantValue.HasValue)
{
// We had a case which didn't resolve properly.
// Assume the switch is complete.
return false;
}

enumMembers.Remove(IntegerUtilities.ToInt64(constantValue.Value));
HandleBinaryPattern(binaryPattern, enumMembers);
}
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
}
}

private static void HandleBinaryPattern(IBinaryPatternOperation? binaryPattern, Dictionary<long, ISymbol> enumMembers)
{
if (binaryPattern?.OperatorKind == BinaryOperatorKind.Or)
{
RemoveIfConstantPatternHasValue(binaryPattern.LeftPattern, enumMembers);
RemoveIfConstantPatternHasValue(binaryPattern.RightPattern, enumMembers);

HandleBinaryPattern(binaryPattern.LeftPattern as IBinaryPatternOperation, enumMembers);
HandleBinaryPattern(binaryPattern.RightPattern as IBinaryPatternOperation, enumMembers);
}
}

return true;
private static void RemoveIfConstantPatternHasValue(IOperation operation, Dictionary<long, ISymbol> enumMembers)
{
if (operation is IConstantPatternOperation { Value: { ConstantValue: { HasValue: true, Value: var value } } })
enumMembers.Remove(IntegerUtilities.ToInt64(value));
}

public static bool HasDefaultCase(ISwitchExpressionOperation operation)
Expand Down