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

Fix crash in pattern matching #35249

Merged
merged 2 commits into from
Apr 25, 2019
Merged

Fix crash in pattern matching #35249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b668661
Fix crash in pattern matching
agocke Apr 23, 2019
a0bf84b
Respond to PR comments
agocke Apr 25, 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
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,8 @@ internal BoundExpression ConvertPatternExpression(
if (inputType.ContainsTypeParameter())
{
convertedExpression = expression;
if (!hasErrors)
// If the expression does not have a constant value, an error will be reported in the caller
Copy link
Contributor

Choose a reason for hiding this comment

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

// If the expression does not have a constant value, an error will be reported in the caller [](start = 16, length = 92)

@agocke It doesn't look like private void BuildSwitchLabels(SyntaxList<SwitchLabelSyntax> labelsSyntax, Binder sectionBinder, ArrayBuilder<LabelSymbol> labels, DiagnosticBag tempDiagnosticBag) checks for that condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

It feels strange to me that we would be checking for something that is never passed explicitly to ClassifyBuiltInConversion that was crashing. If type is null, probably we should be checking for that and probably not here.

In reply to: 278752470 [](ancestors = 278752470)

Copy link
Member Author

Choose a reason for hiding this comment

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

BuildSwitchLabels doesn't check directly, but when we build the label we bind the constant pattern, which checks then. This is tested in PatternMatchGenericParameterToMethodGroup

if (!hasErrors && expression.ConstantValue is object)
{
HashSet<DiagnosticInfo> useSiteDiagnostics = null;
if (expression.ConstantValue == ConstantValue.Null)
Expand Down
128 changes: 128 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,134 @@ namespace Microsoft.CodeAnalysis.CSharp.UnitTests.Semantics
[CompilerTrait(CompilerFeature.Patterns)]
public class PatternMatchingTests4 : PatternMatchingTestBase
{
[Fact]
[WorkItem(34980, "https://github.com/dotnet/roslyn/issues/34980")]
public void PatternMatchOpenTypeCaseDefault()
{
var comp = CreateCompilation(@"
class C
{
public void M<T>(T t)
{
switch (t)
{
case default:
break;
}
}
}");
comp.VerifyDiagnostics(
// (8,18): error CS0150: A constant value is expected
// case default:
Diagnostic(ErrorCode.ERR_ConstantExpected, "default").WithLocation(8, 18),
// (8,18): error CS8313: A default literal 'default' is not valid as a case constant. Use another literal (e.g. '0' or 'null') as appropriate. If you intended to write the default label, use 'default:' without 'case'.
// case default:
Diagnostic(ErrorCode.ERR_DefaultInSwitch, "default").WithLocation(8, 18));
}

[Fact]
[WorkItem(34980, "https://github.com/dotnet/roslyn/issues/34980")]
public void PatternMatchOpenTypeCaseDefaultT()
{
var comp = CreateCompilation(@"
class C
{
public void M<T>(T t)
{
switch (t)
{
case default(T):
break;
}
}
}");
comp.VerifyDiagnostics(
// (8,18): error CS0150: A constant value is expected
// case default(T):
Diagnostic(ErrorCode.ERR_ConstantExpected, "default(T)").WithLocation(8, 18));
}

[Fact]
[WorkItem(34980, "https://github.com/dotnet/roslyn/issues/34980")]
public void PatternMatchGenericParameterToMethodGroup()
{
var comp = CreateCompilation(@"
class C
{
public void M1(object o)
{
_ = o is M1;
switch (o)
{
case M1:
break;
}
}
public void M2<T>(T t)
{
_ = t is M2;
switch (t)
{
case M2:
break;
}
}
}");
comp.VerifyDiagnostics(
// (6,18): error CS0428: Cannot convert method group 'M1' to non-delegate type 'object'. Did you intend to invoke the method?
// _ = o is M1;
Diagnostic(ErrorCode.ERR_MethGrpToNonDel, "M1").WithArguments("M1", "object").WithLocation(6, 18),
// (9,18): error CS0428: Cannot convert method group 'M1' to non-delegate type 'object'. Did you intend to invoke the method?
// case M1:
Diagnostic(ErrorCode.ERR_MethGrpToNonDel, "M1").WithArguments("M1", "object").WithLocation(9, 18),
// (15,18): error CS0150: A constant value is expected
// _ = t is M2;
Diagnostic(ErrorCode.ERR_ConstantExpected, "M2").WithLocation(15, 18),
// (18,18): error CS0150: A constant value is expected
// case M2:
Diagnostic(ErrorCode.ERR_ConstantExpected, "M2").WithLocation(18, 18)
);
}

[Fact]
[WorkItem(34980, "https://github.com/dotnet/roslyn/issues/34980")]
public void PatternMatchGenericParameterToNonConstantExprs()
{
var comp = CreateCompilation(@"
class C
{
public void M<T>(T t)
{
switch (t)
{
case (() => 0):
break;
case stackalloc int[1] { 0 }:
break;
case new { X = 0 }:
break;
}
}
}");
comp.VerifyDiagnostics(
// (8,18): error CS8129: No suitable 'Deconstruct' instance or extension method was found for type 'T', with 2 out parameters and a void return type.
// case (() => 0):
Diagnostic(ErrorCode.ERR_MissingDeconstruct, "(() => 0)").WithArguments("T", "2").WithLocation(8, 18),
// (8,22): error CS1003: Syntax error, ',' expected
// case (() => 0):
Diagnostic(ErrorCode.ERR_SyntaxError, "=>").WithArguments(",", "=>").WithLocation(8, 22),
// (8,25): error CS1003: Syntax error, ',' expected
// case (() => 0):
Diagnostic(ErrorCode.ERR_SyntaxError, "0").WithArguments(",", "").WithLocation(8, 25),
// (10,18): error CS1525: Invalid expression term 'stackalloc'
// case stackalloc int[1] { 0 }:
Diagnostic(ErrorCode.ERR_InvalidExprTerm, "stackalloc").WithArguments("stackalloc").WithLocation(10, 18),
// (12,18): error CS0150: A constant value is expected
// case new { X = 0 }:
Diagnostic(ErrorCode.ERR_ConstantExpected, "new { X = 0 }").WithLocation(12, 18)
);
}

[Fact]
public void TestPresenceOfITuple()
{
Expand Down