Fix crash with typeless tuple in "as" operator #18416

jcouv · 2017-04-04T05:15:04Z

There are two crashes with typeless tuples in nullable conversions:
var x = (1, null) as (int, string)? // this will now produce an error
(int, string)? y = (1, null); // this is fixed to work

Fixes #17962
@VSadov @dotnet/roslyn-compiler for review

VSadov

LGTM

VSadov · 2017-04-05T02:26:23Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

@@ -93,7 +93,7 @@ internal partial class Binder
            }

            if (conversion.IsTupleLiteralConversion ||
-                (conversion.Kind == ConversionKind.ImplicitNullable && conversion.UnderlyingConversions[0].IsTupleLiteralConversion))
+                (conversion.IsNullable && conversion.UnderlyingConversions[0].IsTupleLiteralConversion))


Hmm, just missed the check for explicit cast.. #Resolved

jcouv · 2017-04-05T06:22:43Z

@VSadov Some tests were broken by this change. I updated them. I'll stop by to discuss whether they are correct. #Resolved

cston · 2017-04-05T15:19:32Z

src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenTuples.vb

+]]></file>
+</compilation>, additionalRefs:=s_valueTupleRefs)
+
+            comp.AssertNoDiagnostics()


The following results in an assert failure in TypeSymbolExtensions.IsSameType:

Class C(Of T, U) End Class Class C Shared Sub M(Of T)() Dim x = TryCast((0, Nothing), C(Of Integer, T)) End Sub End Class ``` #Resolved

@cston I didn't manage to hit this assertion. Just getting an error:

BC30311: Value of type '(Integer, Object)' cannot be converted to 'C(Of Integer, T)'. Dim x = TryCast((0, Nothing), C(Of Integer, T)) ~~~~~~~~~~~~

I'll try a few things to see if I can hit this again.

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

AlekseyTs · 2017-04-05T17:55:07Z

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

+";
+
+            var comp = CreateCompilationWithMscorlib(source, references: new[] { ValueTupleRef, SystemRuntimeFacadeRef });
+            comp.VerifyEmitDiagnostics();


comp.VerifyEmitDiagnostics(); [](start = 12, length = 29)

Please run the executable and verify the behavior. #Closed

AlekseyTs · 2017-04-05T17:56:36Z

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

+";
+
+            var comp = CreateCompilationWithMscorlib(source, references: new[] { ValueTupleRef, SystemRuntimeFacadeRef });
+            comp.VerifyEmitDiagnostics();


comp.VerifyEmitDiagnostics(); [](start = 11, length = 30)

Please run the executable and verify the behavior. #Closed

AlekseyTs · 2017-04-05T18:06:39Z

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

+
+            var comp = CreateCompilationWithMscorlib(source, references: new[] { ValueTupleRef, SystemRuntimeFacadeRef });
+            comp.VerifyDiagnostics(
+                // (6,17): error CS8304: The first operand of an 'as' operator may not be a tuple literal without a natural type.


// (6,17): error CS8304: The first operand of an 'as' operator may not be a tuple literal without a natural type. [](start = 16, length = 113)

I am not sure why we are not using target typing in this scenario. #Closed

We don't do target-typing for lambdas in this situation either.
I'll forward you the thread with compat council (basically, it is ok to block because doesn't seem useful, could be enabled later). #Resolved

I am not sure why we are not using target typing in this scenario.

The set of conversions permitted are those that don't change the representation of the runtime type of the object on the left-hand-side. See https://github.com/dotnet/csharplang/blob/master/spec/expressions.md#the-as-operator . We do target type, but only the null value. Permitting target-typing of typeless tuples would (like e.g. the target typing of lambdas, method groups, numeric literals, formattable string literals, etc) contradict that intended design.
#Resolved

AlekseyTs · 2017-04-05T18:09:37Z

src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenTuples.vb

+]]></file>
+</compilation>, additionalRefs:=s_valueTupleRefs)
+
+            comp.AssertNoDiagnostics()


comp.AssertNoDiagnostics() [](start = 12, length = 26)

Please run the executable and verify the behavior. Also, please test SematicModel as in C# tests. #Closed

AlekseyTs · 2017-04-05T18:10:00Z

src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenTuples.vb

+]]></file>
+</compilation>, additionalRefs:=s_valueTupleRefs)
+
+            comp.AssertNoDiagnostics()


comp.AssertNoDiagnostics() [](start = 12, length = 26)

Please run the executable and verify the behavior. Also, please test SematicModel as in C# tests. #Closed

AlekseyTs · 2017-04-05T18:10:30Z

src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenTuples.vb

+]]></file>
+</compilation>, additionalRefs:=s_valueTupleRefs)
+
+            comp.AssertNoDiagnostics()


comp.AssertNoDiagnostics() [](start = 12, length = 26)

Please run the executable and verify the behavior. Also, please test SematicModel as in C# tests. #Closed

AlekseyTs · 2017-04-05T18:11:16Z

src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenTuples.vb

@@ -7459,6 +7459,62 @@ BC30512: Option Strict On disallows implicit conversions from 'Double' to 'Strin
        End Sub

        <Fact>
+        Public Sub TupleCTypeNullableConversionWithTypelessTuple()
+            Dim comp = CreateCompilationWithMscorlibAndVBRuntime(


CreateCompilationWithMscorlibAndVBRuntime [](start = 23, length = 41)

It looks like we are missing a test for TryCast conversion. #Closed

AlekseyTs · 2017-04-05T18:15:07Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

@@ -93,7 +93,7 @@ internal partial class Binder
            }

            if (conversion.IsTupleLiteralConversion ||
-                (conversion.Kind == ConversionKind.ImplicitNullable && conversion.UnderlyingConversions[0].IsTupleLiteralConversion))
+                (conversion.IsNullable && conversion.UnderlyingConversions[0].IsTupleLiteralConversion))


(conversion.IsNullable && conversion.UnderlyingConversions[0].IsTupleLiteralConversion)) [](start = 16, length = 88)

Do we have tests for nullable conversions that involve user-defined conversions? #Closed

Added.

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

AlekseyTs · 2017-04-05T18:17:37Z

src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs

+                case BoundKind.TupleLiteral:
+                    if ((object)operand.Type == null)
+                    {
+                        Error(diagnostics, ErrorCode.ERR_TypelessTupleInAs, node);


Error(diagnostics, ErrorCode.ERR_TypelessTupleInAs, node); [](start = 24, length = 58)

I think we should simply target type the tuple as we do for other conversions. #Closed

We do not target type the left operand of as, though I believe there is a special case for null. #Resolved

AlekseyTs · 2017-04-05T18:18:05Z

Done with review pass. #Closed

jcouv · 2017-04-11T22:46:44Z

@AlekseyTs @cston @VSadov for review. I addressed the feedback. #Resolved

cston · 2017-04-11T22:53:28Z

LGTM

AlekseyTs · 2017-04-12T04:03:15Z

src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenTuples.vb

+</compilation>, additionalRefs:=s_valueTupleRefs, options:=TestOptions.DebugExe)
+
+            comp.AssertTheseDiagnostics(<errors>
+BC30311: Value of type '(Integer, Object)' cannot be converted to 'C?'.


This error feels unexpected. #Resolved

Investigated this. The tuple has type (Integer, Object) and the conversion method has input type (Integer, String). So the input would have to go through a narrowing tuple conversion, which gets rejected by DetermineMostSpecificWideningConversion because it is not widening.

On the other side, DetermineMostSpecificNarrowingConversion rejects this method because the out conversion would be widening (widening nullable). See comment below.

From the comment, I think the logic needs to account for tuples in some new way. I'm not sure how. I'll stop by tomorrow to discuss.

' Note that {Narrowing in, Widening (non-identity) out} operator is not considered as an applicable candidate. ' In fact, an operator like this cannot exist unless nullable in/out conversions are involved. ' Basically we would be dealing with an operator that converts from type derived from source to type derived from destination, ' it would have to be defined in one of those types. When we collect operators we only visit source, destination and their ' bases. So, in order for such an operator to be found, there must be an inheritance relationship between source and ' destination and the operator must be defined in a type that is in between of them in the inheritance hierarchy. Thus, ' there would be an inheritance relationship between parameter type ant return type of the operator, which makes the operator ' inapplicable - there would be a predefined conversion between the types. ' Ignoring an operator like this even when nullable conversions are involved, allows us to consider it as a candidate for a ' lifting, otherwise it would be treated as a not-lifted narrowing. ``` #Resolved

Filed #19269 for broader issue.

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

AlekseyTs · 2017-04-12T04:09:16Z

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

+{
+    static void Main()
+    {
+        C? x = (1, null);


C? x = (1, null); [](start = 8, length = 17)

Consider testing explicit casts, including as cast. #Resolved

AlekseyTs · 2017-04-12T04:10:14Z

src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenTuples.vb

+Structure C
+    Shared Sub Main()
+        Dim x As C = (1, Nothing)
+        Dim y As C? = (2, Nothing)


Dim y As C? = (2, Nothing) [](start = 8, length = 26)

Consider also testing all three flavors of explicit casts. #Resolved

cston · 2017-05-03T23:32:56Z

Can we remove this case? It looks like this is handled at line 3004. #Resolved

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:3078 in bd51b3c. [](commit_id = bd51b3c5a66c68417b72eb86000d1325c40d2a88, deletion_comment = False)

jcouv · 2017-05-04T05:22:26Z

src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenTuples.vb

+            comp.AssertTheseDiagnostics(<errors>
+BC30311: Value of type '(Integer, Object)' cannot be converted to 'C?'.
+        Dim y = CType((2, Nothing), C?)
+                      ~~~~~~~~~~~~


This seems strange. I'll investigate as well. #Resolved

This behavior is same as discussed above (test ImplicitConversionOnTypelessTupleWithUserConversion).

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

…ss tuple

jcouv · 2017-05-08T16:02:41Z

@AlekseyTs Could you take another look? Thanks

AlekseyTs · 2017-05-08T18:22:34Z

src/EditorFeatures/VisualBasicTest/CodeActions/InlineTemporary/InlineTemporaryTests.vb

@@ -4401,5 +4401,28 @@ End Class
            Await TestInRegularAndScriptAsync(code, expected, ignoreTrivia:=False)
        End Function

+        <Fact(Skip:="InvalidCastException"), Trait(Traits.Feature, Traits.Features.CodeActionsInlineTemporary)>


InvalidCastException [](start = 21, length = 20)

This should probably be a link to an issue.

It is the link below: #16697

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

As precaution, I also added a note in #16697 to find all tests to un-skip, including this one.

AlekseyTs · 2017-05-08T18:23:28Z

LGTM

jcouv · 2017-05-08T19:13:54Z

Customer scenario
There are two crashes with typeless tuples in nullable conversions:
var x = (1, null) as (int, string)? // this will now produce an error
(int, string)? y = (1, null); // this is fixed to work

Bugs this fixes:
Fixes #17962

Workarounds, if any
Don't use typeless tuples in "as".

Risk
Performance impact
Low. A check in conversions was previously checking for implicit nullable conversion, but now checks for nullable conversion.

How was the bug found?
Reshetnikov ;-)

@MeiChin-Tsai @jaredpar for ask-mode approval.

MeiChin-Tsai · 2017-05-08T20:41:04Z

approved. thx.

jcouv self-assigned this Apr 4, 2017

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

jcouv added 3 - Working Area-Compilers Feature - Tuples Tuples and removed 3 - Working labels Apr 4, 2017

jcouv added this to the 15.3 milestone Apr 4, 2017

jcouv force-pushed the tuple-as branch from 55125e0 to 577f289 Compare April 5, 2017 01:55

VSadov approved these changes Apr 5, 2017

View reviewed changes

jcouv force-pushed the tuple-as branch from ccb67b5 to de30ec5 Compare April 5, 2017 04:31

cston reviewed Apr 5, 2017

View reviewed changes

AlekseyTs reviewed Apr 5, 2017

View reviewed changes

gafter removed the Feature - Tuples Tuples label Apr 11, 2017

gafter added the Feature - Tuples Tuples label Apr 11, 2017

AlekseyTs reviewed Apr 12, 2017

View reviewed changes

jcouv commented May 4, 2017

View reviewed changes

jcouv mentioned this pull request May 4, 2017

VB: User-defined conversion rejected when its "in" conversion is narrowing and its "out" is widening #19269

Open

jcouv force-pushed the tuple-as branch from 82f73b9 to bd51b3c Compare May 4, 2017 22:33

jcouv and others added 10 commits May 5, 2017 16:21

Add test for as operator on typeless tuple

77a75e7

Adding fix for as operator and test for explicit conversion on typele…

d30f65a

…ss tuple

Add VB tests

bb00096

Update impacted tests

66a90c5

PR feedback

0c75a09

Tests with user-defined conversion

16fff07

Add tests for GetSymbolInfo on VB tuple element name

7a41c0e

Add suggested tests

50ba22f

Remove redundant check

abd42fd

Fix CreateStandardCompilation

66d956b

jcouv force-pushed the tuple-as branch from bd51b3c to 66d956b Compare May 6, 2017 00:13

AlekseyTs reviewed May 8, 2017

View reviewed changes

jcouv merged commit 79b2bc6 into dotnet:master May 8, 2017

jcouv deleted the tuple-as branch May 8, 2017 20:41

jcouv mentioned this pull request May 8, 2017

Visual Studio crashes with NullReferenceException on certain applications of the as operator to tuples #17962

Closed

Fix crash with typeless tuple in "as" operator #18416

Fix crash with typeless tuple in "as" operator #18416

Conversation

jcouv commented Apr 4, 2017 • edited Loading

VSadov left a comment

Choose a reason for hiding this comment

VSadov Apr 5, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv commented Apr 5, 2017 • edited Loading

cston Apr 5, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Apr 11, 2017

Choose a reason for hiding this comment

AlekseyTs Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

gafter Apr 6, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv Apr 11, 2017

Choose a reason for hiding this comment

AlekseyTs Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

gafter Apr 6, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs commented Apr 5, 2017 • edited Loading

jcouv commented Apr 11, 2017 • edited Loading

cston commented Apr 11, 2017

AlekseyTs Apr 12, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv May 4, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv May 4, 2017

Choose a reason for hiding this comment

AlekseyTs Apr 12, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs Apr 12, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

cston commented May 3, 2017 • edited by jcouv Loading

jcouv May 4, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv May 4, 2017

Choose a reason for hiding this comment

jcouv commented May 8, 2017

AlekseyTs May 8, 2017

Choose a reason for hiding this comment

jcouv May 8, 2017

Choose a reason for hiding this comment

jcouv May 8, 2017

Choose a reason for hiding this comment

AlekseyTs commented May 8, 2017

jcouv commented May 8, 2017

MeiChin-Tsai commented May 8, 2017

jcouv commented Apr 4, 2017 •

edited

Loading

VSadov Apr 5, 2017 •

edited by jcouv

Loading

jcouv commented Apr 5, 2017 •

edited

Loading

cston Apr 5, 2017 •

edited by jcouv

Loading

AlekseyTs Apr 5, 2017 •

edited

Loading

AlekseyTs Apr 5, 2017 •

edited

Loading

AlekseyTs Apr 5, 2017 •

edited

Loading

jcouv Apr 5, 2017 •

edited

Loading

gafter Apr 6, 2017 •

edited by jcouv

Loading

AlekseyTs Apr 5, 2017 •

edited

Loading

AlekseyTs Apr 5, 2017 •

edited

Loading

AlekseyTs Apr 5, 2017 •

edited

Loading

AlekseyTs Apr 5, 2017 •

edited

Loading

AlekseyTs Apr 5, 2017 •

edited

Loading

AlekseyTs Apr 5, 2017 •

edited

Loading

gafter Apr 6, 2017 •

edited by jcouv

Loading

AlekseyTs commented Apr 5, 2017 •

edited

Loading

jcouv commented Apr 11, 2017 •

edited

Loading

AlekseyTs Apr 12, 2017 •

edited by jcouv

Loading

jcouv May 4, 2017 •

edited

Loading

AlekseyTs Apr 12, 2017 •

edited by jcouv

Loading

AlekseyTs Apr 12, 2017 •

edited by jcouv

Loading

cston commented May 3, 2017 •

edited by jcouv

Loading

jcouv May 4, 2017 •

edited

Loading