Fix crash in is operator used with tuples #19413

gafter · 2017-05-10T22:53:03Z

Customer scenario

Certain patterns of erroneous pattern-matching were not previously diagnosed. Specifically, when you match a nullable of one integral type with a declaration pattern of another integral type, there was no error. We introduce this error. This is a breaking change in a scenario related to the recently-released pattern-matching feature, so we would like to take the break as soon as possible so that customers do not rely on being able to write the erroneous code. We do not believe the pattern of code is very likely to arise in practice.

This PR also fixes a crash in the existing is operator when used with tuple types.

Bugs this fixes:

Fixes crash in is operator used with tuples
Fixes Crash in is operator used with tuple #19310
Matching an expression of type byte? with a pattern of type int should not be allowed
Fixes Matching an expression of type byte? with a pattern of type int should not be allowed #19151
Unify subsumption implementations
Fixes Merge subsumption implementations #19273
Greatly improved code gen for value type pattern match
Fixes pattern matching with structs causes unnecessary boxing, nullable conversions #16129
Fixes Struct unboxing - very bad performance of pattern matching #18944
Fixes Code generation for type pattern: optimize value type cases #14252

Workarounds, if any

N/A

Risk

The purpose of the change is to reduce future risk. Also, because we now combine what were previously three separate implementations, we have one third fewer places there could be bugs.

Performance impact

None expected, as there are only small changes to the code paths taken during compilation.

Is this a regression from a previous update?

No.

Root cause analysis:

For the crash, we did not test scenarios involving mixing tuples with the existing is operator.

How was the bug found?

I found the crash reviewing the existing code while preparing the fix for the other two issues.

* Fixes crash in `is` operator used with tuples Fixes dotnet#19310 * Unify subsumption implementations Fixes dotnet#19273 * Matching an expression of type `byte?` with a pattern of type `int` should not be allowed Fixes dotnet#19151 * Add documentation for the breaking change matching `int?` against the pattern `long x`

Fixes dotnet#16129

gafter · 2017-05-11T04:32:31Z

Also fixes #16129.

gafter · 2017-05-11T05:01:02Z

That ubuntu_14_debug_prtest failure is a big WTF.

21:41:56 
   Microsoft.CodeAnalysis.CSharp.UnitTests.Declarations.SourcePlusMetadataTests.InterfaceName [FAIL]
21:41:56       Assert.Equal() Failure
21:41:56       Expected: System.IComparable
21:41:56       Actual:   System.IComparable
21:41:56       Stack Trace:
21:41:56             at Microsoft.CodeAnalysis.CSharp.UnitTests.Declarations.SourcePlusMetadataTests.InterfaceName()

It passed on ubuntu_16.

gafter · 2017-05-11T05:04:45Z

@dotnet-bot please test ubuntu_14_debug_prtest

jcouv · 2017-05-11T20:57:17Z

docs/compilers/CSharp/Compiler Breaking Changes - post VS2017.md

@@ -14,3 +14,8 @@ For instance, `var t = (a, b.c, this.d);` will produce a tuple with element name
 Consider the case where the type of `a` is `System.Func<bool>` and you write `var local = t.a();`. This will now find the first element of the tuple and invoke it, whereas previously it could only mean "invoke an extension method named 'a'".

 - https://github.com/dotnet/roslyn/issues/16870 In C# 7.0 and before C# 7.1, the compiler accepted self-assignments in deconstruction-assignment. The compiler now produces a warning for that. For instance, in `(x, y) = (x, 2);`.
+
+- The compiler is now more precise in detecting erroneous pattern-matching operations because the expression could not possibly match the pattern. The following situations now cause an error:


Consider adding a link to one representative issue, so that it is easier to pull for more information/details. #Closed

There is only one issue, and it doesn't have more information, but I'll add a link. #Closed

alrz · 2017-05-11T22:39:58Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Patterns.cs

                return _factory.Sequence(
                    ImmutableArray.Create(s, i),
                    ImmutableArray.Create<BoundExpression>(
-                        _factory.AssignmentExpression(_factory.Local(i), _factory.Convert(tmpType, loweredInput)),
+                        _factory.AssignmentExpression(_factory.Local(i), _factory.Convert(objectType, loweredInput)),
                        _factory.AssignmentExpression(loweredTarget, _factory.Conditional(


What would it take to remove the need for the boolean local s? I mean, ideally o is S ? (S)o : e and o is S s ? s : e should produce the same code as long as CanChangeValueBetweenReads for o is false.

jcouv · 2017-05-15T21:29:59Z

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

@@ -1468,6 +1468,8 @@ internal enum ErrorCode
        ERR_VoidInTuple = 8210,
        #endregion more stragglers for C# 7

+        #region diagnostics introduced for C# 7.1


Although I did some compaction on bottom of the list, I just noticed the gap from 8210 to 8300.
Opened #19523 to compact more. #WontFix

Fixes dotnet#19122

gafter · 2017-05-15T23:05:20Z

This appears to fix #19122 as well.

gafter · 2017-05-16T00:00:15Z

@AlekseyTs @dotnet/roslyn-compiler May I please have a couple of reviews for this fix for 7.1?

gafter · 2017-05-16T23:56:38Z

@AlekseyTs @dotnet/roslyn-compiler May I please have a couple of reviews of this fix for 7.1?

AlekseyTs · 2017-05-17T17:46:16Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_PatternSwitchStatement.cs

@@ -392,7 +392,8 @@ private void LowerConstantValueDecision(DecisionTree.ByValue byValue)
                    {
                        var loweredRight = kv.Key;
                        var loweredLeft = kv.Value;
-                        loweredRight = _factory.Convert(loweredLeft.Type, loweredRight);
+                        //loweredRight = _factory.Convert(loweredLeft.Type, loweredRight);
+                        Debug.Assert(loweredLeft.Type.Equals(loweredRight.Type, TypeCompareKind.IgnoreDynamicAndTupleNames));


IgnoreDynamicAndTupleNames [](start = 96, length = 26)

I think we should use AllIgnoreOptions here. #Closed

AlekseyTs · 2017-05-17T17:50:02Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_PatternSwitchStatement.cs

@@ -392,7 +392,8 @@ private void LowerConstantValueDecision(DecisionTree.ByValue byValue)
                    {
                        var loweredRight = kv.Key;
                        var loweredLeft = kv.Value;
-                        loweredRight = _factory.Convert(loweredLeft.Type, loweredRight);
+                        //loweredRight = _factory.Convert(loweredLeft.Type, loweredRight);


//loweredRight = _factory.Convert(loweredLeft.Type, loweredRight); [](start = 24, length = 66)

Please remove obsolete code. #Closed

AlekseyTs · 2017-05-17T17:54:45Z

src/Compilers/CSharp/Test/Emit/CodeGen/PatternTests.cs

@@ -31,16 +31,7 @@ static class C {
            compilation.GetDiagnostics().Verify();
            compilation.GetEmitDiagnostics().Verify(
                // warning CS8021: No value for RuntimeMetadataVersion found. No assembly containing System.Object was found nor was a value for RuntimeMetadataVersion specified through options.
-                Diagnostic(ErrorCode.WRN_NoRuntimeMetadataVersion).WithLocation(1, 1),
-                // (9,48): error CS0518: Predefined type 'System.Nullable`1' is not defined or imported


// (9,48): error CS0518: Predefined type 'System.Nullable`1' is not defined or imported [](start = 16, length = 87)

It looks like we lost tests for error scenarios like this one, none of them are hitting the interesting code paths. We should come up with new scenarios that do. #Closed

We aren't hitting them because those code paths don't exist any longer. We use a different lowering strategy for this code now.

AlekseyTs · 2017-05-17T17:55:05Z

src/Compilers/CSharp/Test/Emit/CodeGen/PatternTests.cs

-                // (9,48): error CS0518: Predefined type 'System.Nullable`1' is not defined or imported
-                //     public static bool M() => ((object)123) is int i;
-                Diagnostic(ErrorCode.ERR_PredefinedTypeNotFound, "int i").WithArguments("System.Nullable`1").WithLocation(9, 48),
-                // (9,48): error CS0656: Missing compiler required member 'System.Nullable`1.GetValueOrDefault'


// (9,48): error CS0656: Missing compiler required member 'System.Nullable`1.GetValueOrDefault' [](start = 16, length = 95)

It looks like we lost tests for error scenarios like this one, none of them are hitting the interesting code paths. We should come up with new scenarios that do. #Closed

We aren't hitting them because those code paths don't exist any longer. We use a different lowering strategy for this code now.

AlekseyTs · 2017-05-17T17:55:16Z

src/Compilers/CSharp/Test/Emit/CodeGen/PatternTests.cs

-                // (9,48): error CS0656: Missing compiler required member 'System.Nullable`1.GetValueOrDefault'
-                //     public static bool M() => ((object)123) is int i;
-                Diagnostic(ErrorCode.ERR_MissingPredefinedMember, "int i").WithArguments("System.Nullable`1", "GetValueOrDefault").WithLocation(9, 48),
-                // (9,48): error CS0656: Missing compiler required member 'System.Nullable`1.get_HasValue'


// (9,48): error CS0656: Missing compiler required member 'System.Nullable`1.get_HasValue' [](start = 16, length = 90)

It looks like we lost tests for error scenarios like this one, none of them are hitting the interesting code paths. We should come up with new scenarios that do. #Closed

We aren't hitting them because those code paths don't exist any longer. We use a different lowering strategy for this code now.

gafter · 2017-05-17T17:57:46Z

Regarding the risk, how confident that we feel? Reading from the template, reducing future risk does not imply risk of fix is low. thx.

@MeiChin-Tsai This fixes a few reported crashes. I expect that the compiler after this fix will be much more stable in this area than before the fix. The most significant part of this change replaces recently written code with slightly modified code that had been in production use for a long time.

AlekseyTs · 2017-05-17T17:57:56Z

src/Compilers/CSharp/Test/Emit/CodeGen/SwitchTests.cs

 }"
            );
        }

-        [Fact, WorkItem(19280, "https://github.com/dotnet/roslyn/issues/19280")]
-        public void ShareLikeKindedTemps_01()


ShareLikeKindedTemps_01 [](start = 20, length = 23)

I am not sure what happened with this test, it looks like it was deleted. Intentional? #Closed

I think this was a merge error. Adding it again (and the next test).

AlekseyTs · 2017-05-17T18:05:28Z

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests.cs

@@ -4780,10 +4800,13 @@ public static void Main(string[] args)
            CreateCompilationWithMscorlib45(program).VerifyDiagnostics(
                // (10,18): error CS0266: Cannot implicitly convert type 'double' to 'int?'. An explicit conversion exists (are you missing a cast?)
                //             case double.NaN:
-                Diagnostic(ErrorCode.ERR_NoImplicitConvCast, "double.NaN").WithArguments("double", "int?").WithLocation(10, 18),
-                // (13,18): error CS8121: An expression of type int? cannot be handled by a pattern of type string.
+                Diagnostic(ErrorCode.ERR_NoImplicitConvCast, "double.NaN").WithArguments("double", "int?"),


Diagnostic(ErrorCode.ERR_NoImplicitConvCast, "double.NaN").WithArguments("double", "int?"), [](start = 16, length = 91)

Location is lost, please restore. #Closed

AlekseyTs · 2017-05-17T18:05:36Z

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests.cs

                //             case string _:
-                Diagnostic(ErrorCode.ERR_PatternWrongType, "string").WithArguments("int?", "string").WithLocation(13, 18)
+                Diagnostic(ErrorCode.ERR_PatternWrongType, "string").WithArguments("int?", "string"),


Diagnostic(ErrorCode.ERR_PatternWrongType, "string").WithArguments("int?", "string"), [](start = 16, length = 85)

Location is lost, please restore. #Closed

AlekseyTs · 2017-05-17T18:07:48Z

src/Compilers/CSharp/Test/Semantic/Semantics/SwitchTests.cs

@@ -332,7 +350,7 @@ public void foo2(char i)
                Diagnostic(ErrorCode.ERR_NoImplicitConvCast, "97.0f").WithArguments("float", "char").WithLocation(34, 18),
                // (38,18): error CS0266: Cannot implicitly convert type 'int' to 'char'. An explicit conversion exists (are you missing a cast?)
                //             case 97:
-                Diagnostic(ErrorCode.ERR_NoImplicitConvCast, "97").WithArguments("int", "char").WithLocation(38, 18)
+                Diagnostic(ErrorCode.ERR_NoImplicitConvCast, "97").WithArguments("int", "char")


Diagnostic(ErrorCode.ERR_NoImplicitConvCast, "97").WithArguments("int", "char") [](start = 16, length = 79)

Location is lost, please restore. #Closed

AlekseyTs · 2017-05-17T18:16:05Z

Done with review pass. #Closed

MeiChin-Tsai · 2017-05-17T20:38:00Z

approved.

gafter · 2017-05-17T22:23:10Z

@AlekseyTs The PR has been modified in response to your review comments. Do you have anything else?

@dotnet/roslyn-compiler May I please have a second reviewer?

gafter · 2017-05-17T23:01:34Z

retest windows_debug_vs-integration_prtest please

AlekseyTs · 2017-05-18T05:35:33Z

The PR has been modified in response to your review comments. Do you have anything else?

In order to answer this question or to sign-off, I need to do another review pass. It is on my list of things to do. #Closed

AlekseyTs · 2017-05-18T21:53:03Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Patterns.cs

+                requiresNullTest = requiresNullTest && MatchConstantValue(loweredInput, type, true) != true;
+                if (loweredInput.Type.IsNullableType())
+                {
+                    var getValueOrDefault = _factory.SpecialMethod(SpecialMember.System_Nullable_T_GetValueOrDefault).AsMember((NamedTypeSymbol)loweredInput.Type);


var getValueOrDefault = _factory.SpecialMethod(SpecialMember.System_Nullable_T_GetValueOrDefault).AsMember((NamedTypeSymbol)loweredInput.Type); [](start = 20, length = 143)

Do we have a test that hits this code path with missing SpecialMember.System_Nullable_T_GetValueOrDefault?

Yes (see MissingNullable_03).

AlekseyTs · 2017-05-18T21:53:34Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Patterns.cs

+                        //}
+
+                        var input = _factory.SynthesizedLocal(loweredInput.Type, syntax); // we copy the input to avoid double evaluation
+                        var getHasValue = _factory.SpecialMethod(SpecialMember.System_Nullable_T_get_HasValue).AsMember((NamedTypeSymbol)loweredInput.Type);


var getHasValue = _factory.SpecialMethod(SpecialMember.System_Nullable_T_get_HasValue).AsMember((NamedTypeSymbol)loweredInput.Type); [](start = 24, length = 132)

Do we have a test that hits this code path with missing SpecialMember.System_Nullable_T_get_HasValue?

_factory.SpecialMethod handles this one the same way it handles the one you asked about a few lines earlier. Every call to that helper method is handled the same way in LocalRewriter (and causes the rest of the calling method to be skipped).

Also see 9e05b04 which adds a test that triggers the requested condition.

AlekseyTs · 2017-05-18T21:58:35Z

Please answer a new question #19413 (comment), the thread appears as "outdated".

AlekseyTs · 2017-05-18T22:10:02Z

Done with review pass. Just have three questions around test coverage above.

gafter · 2017-05-18T23:13:36Z

The question was

@gafter I see that you deleted the code. Could you please confirm that you added a test that would fail without this fix?

That is not possible. There are two call sites for this method, one of which passes parameters such that the extra conditions later do not make a difference in the returned value. The other call site treats true and null return values the same, so it would not care.

AlekseyTs

Thanks. LGTM

gafter · 2017-05-19T00:01:34Z

@dotnet/roslyn-compiler May I please have a second review?

gafter · 2017-05-19T19:23:40Z

@dotnet/roslyn-compiler May I please have a second review?

jcouv

LGTM
Thanks for the code walkthrough.

gafter added 4 - In Review A fix for the issue is submitted for review. Area-Compilers Bug Feature - Pattern Matching Pattern Matching Feature - Tuples Tuples labels May 10, 2017

gafter added this to the 15.3 milestone May 10, 2017

gafter self-assigned this May 10, 2017

dnfclas added the cla-already-signed label May 10, 2017

gafter added 2 commits May 10, 2017 21:01

Update tests per changed code gen.

6710b79

Add a test verifying improved code gen for irrefutable pattern match.

8fb06c3

Fixes dotnet#16129

Adjust more tests for changed code gen.

a0b217e

gafter changed the title ~~Refactor subsumption implementation~~ Fix crash in is operator used with tuples May 11, 2017

jcouv reviewed May 11, 2017

View reviewed changes

alrz reviewed May 11, 2017

View reviewed changes

gafter added 2 commits May 15, 2017 13:11

Merge from master

b512f50

Add a link for the breaking change.

bb6c11b

jcouv reviewed May 15, 2017

View reviewed changes

gafter closed this May 15, 2017

gafter reopened this May 15, 2017

dnfclas added the cla-already-signed label May 15, 2017

Add test confirming that dotnet#19122 is fixed.

dc1ac8a

Fixes dotnet#19122

gafter requested a review from AlekseyTs May 15, 2017 23:59

AlekseyTs reviewed May 17, 2017

View reviewed changes

Adjust PR per code review.

2dfa508

Merge with master

69aaeb9

AlekseyTs reviewed May 18, 2017

View reviewed changes

Add a test

9e05b04

AlekseyTs approved these changes May 18, 2017

View reviewed changes

jcouv approved these changes May 19, 2017

View reviewed changes

gafter merged commit b057348 into dotnet:master May 19, 2017

gafter removed the 4 - In Review A fix for the issue is submitted for review. label May 19, 2017

Fix crash in is operator used with tuples #19413

Fix crash in is operator used with tuples #19413

Conversation

gafter commented May 10, 2017 • edited Loading

gafter commented May 11, 2017

gafter commented May 11, 2017 • edited Loading

gafter commented May 11, 2017

jcouv May 11, 2017 • edited Loading

Choose a reason for hiding this comment

gafter May 15, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

alrz May 11, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv May 15, 2017 • edited Loading

Choose a reason for hiding this comment

gafter commented May 15, 2017

gafter commented May 16, 2017

gafter commented May 16, 2017

AlekseyTs May 17, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 17, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented May 17, 2017

AlekseyTs May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 17, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 17, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 17, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented May 17, 2017 • edited Loading

MeiChin-Tsai commented May 17, 2017

gafter commented May 17, 2017

gafter commented May 17, 2017

AlekseyTs commented May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 18, 2017

AlekseyTs commented May 18, 2017

gafter commented May 18, 2017 • edited Loading

AlekseyTs left a comment

Choose a reason for hiding this comment

gafter commented May 19, 2017

gafter commented May 19, 2017

jcouv left a comment

Choose a reason for hiding this comment

gafter commented May 10, 2017 •

edited

Loading

gafter commented May 11, 2017 •

edited

Loading

jcouv May 11, 2017 •

edited

Loading

gafter May 15, 2017 •

edited by jcouv

Loading

alrz May 11, 2017 •

edited

Loading

jcouv May 15, 2017 •

edited

Loading

AlekseyTs May 17, 2017 •

edited

Loading

AlekseyTs May 17, 2017 •

edited

Loading

AlekseyTs May 17, 2017 •

edited

Loading

AlekseyTs May 17, 2017 •

edited

Loading

AlekseyTs May 17, 2017 •

edited

Loading

AlekseyTs May 17, 2017 •

edited

Loading

AlekseyTs May 17, 2017 •

edited

Loading

AlekseyTs May 17, 2017 •

edited

Loading

AlekseyTs May 17, 2017 •

edited

Loading

AlekseyTs commented May 17, 2017 •

edited

Loading

AlekseyTs commented May 18, 2017 •

edited

Loading

gafter commented May 18, 2017 •

edited

Loading