Relax conversion requirements for pattern-matching involving type parameters. #18784

gafter · 2017-04-18T19:05:48Z

Customer scenario

This is a tiny language change for 7.1. See dotnet/csharplang#154.

See #16195 for customer scenario. In short, pattern-matching can give a compile-time error unexpectedly when type parameters are involved. The compiler is correct to give these errors according to the spec, but we wish to relax the language to make it legal.

Bugs this fixes:

Fixes #16195

Workarounds, if any

Cast the expression being matched to object.

Risk

Low. This has little impact on existing code.

Performance impact

None expected.

Is this a regression from a previous update?

No.

…ameters. Fixes dotnet#16195 This is a language change for 7.1. See dotnet/csharplang#154. Some tests may fail until dotnet#18756 is integrated.

…-16195

gafter · 2017-04-21T00:27:10Z

@jaredpar Please look at this and determine if it is suitable for 15.3. A number of customers have complained, and the fix is small.

CyrusNajmabadi · 2017-04-22T22:42:17Z

@gafter Is #16993 another manifestation of this issue?

gafter · 2017-04-25T20:16:47Z

This PR is pending a compiler team feature review.

VSadov · 2017-05-01T21:09:57Z

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

                // It is possible that the input value is already of the correct type, in which case the pattern
-                // is irrefutable, and we can just do the assignment and return true.
+                // is irrefutable (there is no way for a non-nullable value type to be null), and we can just do
+                // the assignment and return true.
                if (loweredInput.Type == type)


This type comparison should ignore dynamic/tuple/modopt differences

Essentially if there is an identity conversion, then match is irrefutable.

That may be true (the suggested change would be a code improvement), but it is not relevant to the change in the PR.

VSadov · 2017-05-01T21:32:50Z

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

+                    ImmutableArray.Create(s, i),
+                    ImmutableArray.Create<BoundExpression>(
+                        _factory.AssignmentExpression(_factory.Local(i), _factory.Convert(tmpType, loweredInput)),
+                        _factory.AssignmentExpression(_factory.Local(s), _factory.Is(_factory.Local(i), type)),


we should have some tests that validate IL, just to be sure how this is emitted.

gafter · 2017-05-02T20:28:09Z

@dotnet/roslyn-compiler May I please have a second review? @VSadov I believe the tests you suggested have been added.

@jaredpar May I have permission to merge when reviews are complete?

cston · 2017-05-02T21:47:54Z

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

+            compilation = CreateCompilationWithMscorlib45(source, references: new MetadataReference[] { CSharpRef, SystemCoreRef }, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular7_1);
+            compilation.VerifyDiagnostics();
+            CompileAndVerify(compilation, expectedOutput: "True1False0");
+        }


Consider testing types that are not type parameters but where .ContainsTypeParameter() is true: T[], A<T>.B, etc.

Consider testing types that are not type parameters but where .ContainsTypeParameter() is true: T[], A.B, etc.

Yes, please add several tests for scenarios like that.

… type.

AlekseyTs · 2017-05-03T22:30:56Z

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

+                    ImmutableArray.Create<BoundExpression>(
+                        _factory.AssignmentExpression(_factory.Local(i), _factory.Convert(tmpType, loweredInput)),
+                        _factory.AssignmentExpression(_factory.Local(s), _factory.Is(_factory.Local(i), type)),
+                        _factory.AssignmentExpression(loweredTarget, _factory.Conditional(_factory.Local(s), _factory.Convert(type, _factory.Local(i)), _factory.Default(type), type))


_factory.Local(s) [](start = 90, length = 17)

Can we inline the assignment to s here?

Done. But it made no difference in any generated code.

AlekseyTs · 2017-05-03T22:33:25Z

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

+}
+class X : B { }
+";
+            CreateStandardCompilation(source).VerifyDiagnostics(


CreateStandardCompilation(source).VerifyDiagnostics( [](start = 12, length = 52)

Consider explicitly using C# 7 version, otherwise this test will break once we bump the default version.

AlekseyTs · 2017-05-03T22:34:06Z

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

+    }
+}
+";
+            CreateStandardCompilation(source).VerifyDiagnostics(


CreateStandardCompilation(source).VerifyDiagnostics( [](start = 12, length = 52)

Consider explicitly using C# 7 version, otherwise this test will break once we bump the default version.

AlekseyTs · 2017-05-03T22:34:19Z

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

+        }
+    }
+}";
+            CreateStandardCompilation(source).VerifyDiagnostics(


CreateStandardCompilation(source).VerifyDiagnostics( [](start = 12, length = 52)

Consider explicitly using C# 7 version, otherwise this test will break once we bump the default version.

AlekseyTs · 2017-05-03T22:35:23Z

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

+    }
+}
+";
+            var compilation = CreateCompilationWithMscorlib45(source, references: new MetadataReference[] { CSharpRef, SystemCoreRef }, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular);


var compilation = CreateCompilationWithMscorlib45(source, references: new MetadataReference[] { CSharpRef, SystemCoreRef }, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular); [](start = 12, length = 192)

Consider explicitly using C# 7 version, otherwise this test will break once we bump the default version.

AlekseyTs · 2017-05-03T22:36:15Z

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

+    }
+}
+";
+            var compilation = CreateCompilationWithMscorlib45(source, references: new MetadataReference[] { CSharpRef, SystemCoreRef }, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular);


var compilation = CreateCompilationWithMscorlib45(source, references: new MetadataReference[] { CSharpRef, SystemCoreRef }, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular); [](start = 12, length = 192)

Consider explicitly using C# 7 version, otherwise this test will break once we bump the default version.

AlekseyTs · 2017-05-03T22:38:47Z

Done with review pass.

Previously the code assumed the type would be the same. Also use CanContainNull() rather than !IsNonNullableValueType() to improve code for type parameters

gafter · 2017-05-04T00:47:19Z

src/Compilers/CSharp/Portable/BoundTree/DecisionTreeBuilder.cs

+                            }
+                            else
+                            {
+                                byValue.Default = new DecisionTree.ByType(byValue.Expression, byValue.Type, null);


This is a bug fix to insert a needed conversion.

gafter · 2017-05-04T00:48:01Z

src/Compilers/CSharp/Portable/BoundTree/DecisionTreeBuilder.cs

                        }
                        else
                        {
-                            Debug.Assert(byValue.Default.Type == type);
-                            return Add(byValue.Default, makeDecision);
+                            result = AddByType(byValue.Default, type, makeDecision);


This is a bug fix to insert a needed conversion.

gafter · 2017-05-04T00:49:18Z

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

@@ -93,10 +94,10 @@ private BoundExpression MakeIsDeclarationPattern(BoundDeclarationPattern lowered

                return _factory.Sequence(ImmutableArray.Create(temp),
                         sideEffects: ImmutableArray<BoundExpression>.Empty,
-                         result: MakeIsDeclarationPattern(loweredPattern.Syntax, loweredInput, discard, requiresNullTest: true));
+                         result: MakeIsDeclarationPattern(loweredPattern.Syntax, loweredInput, discard, requiresNullTest: loweredInput.Type.CanContainNull()));


This is a bug fix to remove an unneeded null check.

gafter · 2017-05-04T00:49:30Z

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

            }

-            return MakeIsDeclarationPattern(loweredPattern.Syntax, loweredInput, loweredPattern.VariableAccess, requiresNullTest: true);
+            return MakeIsDeclarationPattern(loweredPattern.Syntax, loweredInput, loweredPattern.VariableAccess, requiresNullTest: loweredInput.Type.CanContainNull());


This is a bug fix to remove an unneeded null check.

gafter · 2017-05-04T00:49:53Z

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

-            Debug.Assert(!type.IsNullableType());
+            // It is possible that the input value is already of the correct type, in which case the pattern
+            // is irrefutable, and we can just do the assignment and return true (or perform the null test).
+            if (MatchIsIrrefutable(loweredInput.Type, loweredTarget.Type, requiresNullTest))


This is a bug fix to remove an unneeded type test.

…-16195

cston · 2017-05-04T15:42:44Z

src/Compilers/CSharp/Portable/BoundTree/DecisionTreeBuilder.cs

@@ -221,7 +221,7 @@ private DecisionTree AddByValue(DecisionTree.ByType byType, BoundConstantPattern
                var kvp = byType.TypeAndDecision[i];
                var matchedType = kvp.Key;
                var decision = kvp.Value;
-                if (matchedType.TupleUnderlyingTypeOrSelf() == value.Value.Type.TupleUnderlyingTypeOrSelf())
+                if (matchedType.Equals(value.Value.Type, TypeCompareKind.IgnoreDynamicAndTupleNames))


Please add tests that ignore dynamic.

I can't think of a way to make that happen. The is no constant pattern whose type is generic. This could probably be simplified to (matchedType == value.Value.Type).

Oh, I know a way to make it happen. An enum type nested in a generic class.

…are ignored in patterns.

cston · 2017-05-04T23:10:16Z

LGTM

gafter · 2017-05-05T00:26:46Z

@agocke or @AlekseyTs Please re-review for my second review. There have been changes since @agocke looked at this.

gafter · 2017-05-05T00:42:50Z

@MeiChin-Tsai @jaredpar for ask-mode approval (once reviews are complete).

MeiChin-Tsai · 2017-05-05T16:47:22Z

Approved once you get code review and tests done. Thanks.

agocke

LGTM, except for test case (which may already be there and I missed it)

agocke · 2017-05-07T23:25:18Z

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

+        Console.Write(x is Container<T>.Derived[] b0);
+        switch (x)
+        {
+            case Container<T>.Derived[] b1:


Is it legal to use an unbound type here? i.e., case Container<>.Derived d:?

Even if not, consider adding a test case for it.

Unbound types are an error in type binding everywhere except in a typeof(). I don't think we have tests for each place a type can occur.

Relax conversion requirements for pattern-matching involving type par…

9d0ab48

…ameters. Fixes dotnet#16195 This is a language change for 7.1. See dotnet/csharplang#154. Some tests may fail until dotnet#18756 is integrated.

gafter added Area-Compilers Feature Request PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Apr 18, 2017

gafter added this to the 15.3 milestone Apr 18, 2017

dnfclas added the cla-already-signed label Apr 18, 2017

gafter self-assigned this Apr 18, 2017

gafter mentioned this pull request Apr 18, 2017

Champion "pattern-matching with generics" (C# 7.1) dotnet/csharplang#154

Open

5 tasks

gafter added the 4 - In Review A fix for the issue is submitted for review. label Apr 18, 2017

Merge branch 'master' of https://github.com/dotnet/roslyn into master…

47a9d7f

…-16195

gafter removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 21, 2017

gafter added the Blocked label Apr 25, 2017

VSadov reviewed May 1, 2017

View reviewed changes

gafter added 3 commits May 1, 2017 15:24

Merge from master

8e19970

Add tests per test plan

0570548

Additional switch tests.

98287a8

gafter removed the Blocked label May 2, 2017

agocke approved these changes May 2, 2017

View reviewed changes

cston reviewed May 2, 2017

View reviewed changes

gafter added 2 commits May 3, 2017 13:35

Add some tests, and some decision tree fixes for switching on a value…

1ad87bf

… type.

Merge from master

8615a76

AlekseyTs reviewed May 3, 2017

View reviewed changes

gafter added 2 commits May 3, 2017 15:59

Reduce redundant null check, and handle irrefutable pattern better.

3257f88

Insert a ByType following the Default of a ByValue if needed.

e2a1eb1

Previously the code assumed the type would be the same. Also use CanContainNull() rather than !IsNonNullableValueType() to improve code for type parameters

gafter commented May 4, 2017

View reviewed changes

gafter added 3 commits May 3, 2017 18:59

Correct inverted condition.

089227f

Merge branch 'master' of https://github.com/dotnet/roslyn into master…

1eb6954

…-16195

Adjust and add tests recommended in code review.

fd2109e

cston reviewed May 4, 2017

View reviewed changes

Add tests demonstrating that dynamic/object and tuple names in types …

d9bff8f

…are ignored in patterns.

agocke approved these changes May 7, 2017

View reviewed changes

gafter merged commit 02a0942 into dotnet:master May 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relax conversion requirements for pattern-matching involving type parameters. #18784

Relax conversion requirements for pattern-matching involving type parameters. #18784

gafter commented Apr 18, 2017 •

edited

Loading

gafter commented Apr 21, 2017

CyrusNajmabadi commented Apr 22, 2017

gafter commented Apr 25, 2017

VSadov May 1, 2017

VSadov May 1, 2017

gafter May 1, 2017

VSadov May 1, 2017

gafter commented May 2, 2017

cston May 2, 2017

AlekseyTs May 3, 2017

AlekseyTs May 3, 2017

gafter May 4, 2017

AlekseyTs May 3, 2017

AlekseyTs May 3, 2017

AlekseyTs May 3, 2017

AlekseyTs May 3, 2017

AlekseyTs May 3, 2017

AlekseyTs commented May 3, 2017

gafter May 4, 2017

gafter May 4, 2017

gafter May 4, 2017

gafter May 4, 2017

gafter May 4, 2017

cston May 4, 2017

gafter May 4, 2017

gafter May 4, 2017

cston commented May 4, 2017

gafter commented May 5, 2017

gafter commented May 5, 2017

MeiChin-Tsai commented May 5, 2017

agocke left a comment

agocke May 7, 2017

gafter May 8, 2017

Relax conversion requirements for pattern-matching involving type parameters. #18784

Relax conversion requirements for pattern-matching involving type parameters. #18784

Conversation

gafter commented Apr 18, 2017 • edited Loading

gafter commented Apr 21, 2017

CyrusNajmabadi commented Apr 22, 2017

gafter commented Apr 25, 2017

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

gafter commented May 2, 2017

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

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 3, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented May 4, 2017

gafter commented May 5, 2017

gafter commented May 5, 2017

MeiChin-Tsai commented May 5, 2017

agocke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented Apr 18, 2017 •

edited

Loading