Restore semantic model functionality in tuple comparisons by jcouv · Pull Request #25297 · dotnet/roslyn

jcouv · 2018-03-07T05:15:00Z

Two parts of this change:

adding ConvertedLeft and ConvertedRight to the bound node for tuple comparison. Those are only used by the semantic model, but skipped by other visitors. One subtlety: They cannot be made by applying a conversion on the top-level tuple, but rather we try to give each element an converted type if we can (even if the containing tuple remains typeless).
adding a third kind of TupleBinaryOperatorKind to represent null == null element-wise comparison while keeping such nulls typeless.

jcouv · 2018-03-07T18:11:08Z

@AlekseyTs @dotnet/roslyn-compiler for review. Thanks
I'm working on a follow-up PR which adds support for default in tuple equality. #Resolved

gafter · 2018-03-08T00:54:53Z

I will aim to look at this tomorrow. #Resolved

jcouv · 2018-03-08T17:17:16Z

@AlekseyTs Would you have time to look at two tuple equality PRs? I'm currently looking at your CFG PR. Thanks #Resolved

AlekseyTs · 2018-03-08T18:47:51Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml


    <Field Name="Left" Type="BoundExpression"/>
    <Field Name="Right" Type="BoundExpression"/>
+    <Field Name="ConvertedLeft" Type="BoundExpression" SkipInVisitor="true"/>


[](start = 4, length = 73)

It would be good to add a comment saying that these properties are meant for SemanticModel use only #Closed

AlekseyTs · 2018-03-08T18:49:50Z

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

+#endif
+        }
+
+        internal enum KindEnum


KindEnum [](start = 22, length = 8)

TupleBinaryOperatorInfoKind? Also, consider moving this declaration to the beginning of the file #Closed

AlekseyTs · 2018-03-08T18:51:24Z

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

+        {
+            internal readonly BinaryOperatorKind Kind;
+
+            internal NullNull(BinaryOperatorKind kind, TypeSymbol leftConvertedTypeOpt, TypeSymbol rightConvertedTypeOpt)


TypeSymbol leftConvertedTypeOpt, TypeSymbol rightConvertedTypeOpt [](start = 55, length = 65)

Are these always null? If so, consider removing these parameters and simply passing nulls to the base #Closed

AlekseyTs · 2018-03-08T18:54:30Z

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

+            return new BoundTupleBinaryOperator(node, left, right, convertedLeft, convertedRight, kind, operators, resultType);
+        }
+
+        BoundExpression ApplyConvertedTypes(BoundExpression expr, TupleBinaryOperatorInfo @operator, bool isRight, DiagnosticBag diagnostics)


BoundExpression ApplyConvertedTypes(BoundExpression expr, TupleBinaryOperatorInfo @operator, bool isRight, DiagnosticBag diagnostics) [](start = 8, length = 133)

private? #Closed

AlekseyTs · 2018-03-08T18:59:15Z

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

+                ImmutableArray<BoundExpression> arguments = tuple.Arguments;
+                int length = arguments.Length;
+
+                Debug.Assert(length == multiple.Operators.Length);


Debug.Assert(length == multiple.Operators.Length); [](start = 16, length = 50)

What about error cases when the multiple.Operators is empty? How do we handle cases like that? #Closed

Yes. That is handled in the next PR (with test hitting it).
Let me know if I should move the fix into this PR instead. #Closed

Let me know if I should move the fix into this PR instead.

I think that would be the right thing to do, especially if this wouldn't be the only follow-up change in this PR.

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

AlekseyTs · 2018-03-08T19:13:25Z

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

+                var builder = ArrayBuilder<BoundExpression>.GetInstance(length);
+                for (int i = 0; i < length; i++)
+                {
+                    builder.Add(ApplyConvertedTypes(arguments[i], multiple.Operators[i], isRight, diagnostics));


builder.Add(ApplyConvertedTypes(arguments[i], multiple.Operators[i], isRight, diagnostics)); [](start = 20, length = 92)

It feels like, instead of applying element-wise conversions explicitly, we should simply apply conversion to the entire literal when the convertedType is not null. Otherwise, we are actually changing the element types but leaving tuple's tuple's type unchanged, then we are trying to convert that hacked literal that contains possibly inconsistent information. That might or might not give us result that we want. Why take chances? I think that explicitly applying element-wise conversions makes sense only for cases when we don't have convertedType. #Closed

AlekseyTs · 2018-03-08T19:31:11Z

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


-            // PROTOTYPE(tuple-equality)
-            return;
+            var lastY = tupleY.Arguments[1].Expression;


var lastY = tupleY.Arguments[1].Expression; [](start = 11, length = 44)

Consider testing the "x" side as well. #Closed

AlekseyTs · 2018-03-08T19:35:03Z

    return t == (null, 1) && t == (""hello"", 1);

Unless other tests cover this, consider testing semantic model information on "t", especially when there are come conversions on its side too. #Closed

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleEqualityTests.cs:589 in 851ae16. [](commit_id = 851ae16, deletion_comment = False)

AlekseyTs · 2018-03-08T19:38:31Z

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

-            //Assert.Null(tupleType1.Type);
-            //Assert.Equal("(System.String, System.String)", tupleType1.ConvertedType.ToTestDisplayString());
+            var tuple1 = tree.GetCompilationUnitRoot().DescendantNodes().OfType<TupleExpressionSyntax>().ElementAt(0);
+            Assert.Equal("(s, null)", tuple1.ToString());


null [](start = 30, length = 4)

It would be good to test TypeInfo on "null", unless other tests cover this. #Closed

AlekseyTs · 2018-03-08T19:39:43Z

    var t1 = (1, 2L);

Consider testing TypeInfo on t1 #Closed

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleEqualityTests.cs:697 in 851ae16. [](commit_id = 851ae16, deletion_comment = False)

AlekseyTs · 2018-03-08T19:40:58Z

    System.Console.Write(t1 == (1L, t2));

Consider testing SymbolInfo on t1 and t2 #Closed

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleEqualityTests.cs:725 in 851ae16. [](commit_id = 851ae16, deletion_comment = False)

AlekseyTs · 2018-03-08T19:43:07Z

    System.Console.Write((1, t) == (1, (null, null)));

It would be good to test TypeInfo on a literal, including when a conversion is involved. #Closed

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleEqualityTests.cs:753 in 851ae16. [](commit_id = 851ae16, deletion_comment = False)

AlekseyTs · 2018-03-08T19:47:17Z

    System.Console.Write((t, (null, null)) == ((null, null), t));

It would be good to test TypeInfo on constituent parts, the ts, nulls, (null, null) #Closed

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleEqualityTests.cs:891 in 851ae16. [](commit_id = 851ae16, deletion_comment = False)

AlekseyTs · 2018-03-08T19:50:55Z

Tests should probably also cover GetConversion API #Closed

AlekseyTs · 2018-03-08T19:54:15Z

    System.Console.Write((null, null, null) == (null, () => { }, Main));

It would be good to test behavior on lambda and the method group. Also, on something within the lambda, references to locals, lambda parameters, etc. should still be recognized. #Closed

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleEqualityTests.cs:1018 in 851ae16. [](commit_id = 851ae16, deletion_comment = False)

AlekseyTs · 2018-03-08T19:59:00Z

And some tests for GetSymbolInfo would be good

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

AlekseyTs · 2018-03-08T20:03:04Z

Done with review pass (iteration 4) #Closed

AlekseyTs · 2018-03-08T20:48:33Z

Not primarily related to this PR, but should we produce any warnings for name mismatch between elements of compared tuples? #Closed

jcouv · 2018-03-08T20:52:02Z

should we produce any warnings for name mismatch between elements of compared tuples?

I don't think so, but I've added a work item to confirm.

Also, I have a PROTOTYPE marker to do tests with tuple names. #Resolved

AlekseyTs · 2018-03-09T00:23:43Z

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

+
+                var newTuple = tuple.Update(argumentNamesOpt: default, inferredNamesOpt: default, convertedArguments, tuple.Type);
+
+                return convertedType is null ? newTuple : GenerateConversionForAssignment(convertedType, newTuple, diagnostics);


convertedType is null [](start = 23, length = 21)

It looks like this condition is always true. #Closed

AlekseyTs · 2018-03-09T00:29:29Z

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

+                return convertedType is null ? newTuple : GenerateConversionForAssignment(convertedType, newTuple, diagnostics);
+            }
+
+            if (convertedType is null)


Consider merging this with the previous if to avoid checking this multiple times. #Closed

AlekseyTs · 2018-03-09T00:37:01Z

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

+            var symbol1 = model.GetTypeInfo(tuple1);
+            Assert.Null(symbol1.Type);
+            Assert.Equal("(System.String, System.Int64)", symbol1.ConvertedType.ToTestDisplayString());
+            Assert.Equal("(System.String, System.Int64)", model.GetDeclaredSymbol(tuple1).ToTestDisplayString());


model.GetDeclaredSymbol(tuple1) [](start = 58, length = 31)

Interesting, does this API work this way on tuple literals in other context? #Closed

I think so, and that is what I expected. A tuple literal is where a tuple and its elements are declared. When in the IDE, tuple.Alice highlights the element.
I'll add a test to confirm.

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

AlekseyTs

LGTM (iteration 5)

jcouv · 2018-03-09T01:02:11Z

Thanks!
Tagging @cston for a second review.

AlekseyTs

LGTM (iteration 6)

cston · 2018-03-09T01:11:51Z

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

+            if (leftNull && rightNull)
+            {
+                return new TupleBinaryOperatorInfo.NullNull(kind);
+            }


Are we planning on using leftNull and rightNull later in the method? If not, consider inlining so we only check right.IsLiteralNull() if necessary.

jcouv added 3 commits March 6, 2018 21:10

Restore semantic model functionality

42e7d04

Fix null==null case

23bda31

Fix more tests

2bd2d83

jcouv added the Area-Compilers label Mar 7, 2018

jcouv added this to the 15.7 milestone Mar 7, 2018

jcouv self-assigned this Mar 7, 2018

jcouv requested a review from a team as a code owner March 7, 2018 05:15

jcouv added PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. Feature - Tuples Tuples and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Mar 7, 2018

gafter self-requested a review March 7, 2018 20:38

Fix conversion in typeless tuple case

851ae16

jcouv force-pushed the tuple-semantic branch from 1670443 to 851ae16 Compare March 7, 2018 21:58

jcouv mentioned this pull request Mar 7, 2018

Give default literals a type in tuple comparisons #25327

Merged

AlekseyTs reviewed Mar 8, 2018

View reviewed changes

jcouv mentioned this pull request Mar 8, 2018

Work items and test plan for "tuple equality" feature #22937

Closed

21 tasks

Address PR feedback from Aleksey

140ed7b

AlekseyTs reviewed Mar 9, 2018

View reviewed changes

AlekseyTs approved these changes Mar 9, 2018

View reviewed changes

Address PR feedback from Aleksey

e353b29

AlekseyTs approved these changes Mar 9, 2018

View reviewed changes

cston reviewed Mar 9, 2018

View reviewed changes

cston approved these changes Mar 9, 2018

View reviewed changes

Address PR feedback from Chuck

9350d47

jcouv merged commit 83f6b06 into dotnet:features/tuple-equality Mar 9, 2018

jcouv deleted the tuple-semantic branch March 9, 2018 02:16


		var newTuple = tuple.Update(argumentNamesOpt: default, inferredNamesOpt: default, convertedArguments, tuple.Type);

		return convertedType is null ? newTuple : GenerateConversionForAssignment(convertedType, newTuple, diagnostics);

Conversation

jcouv commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter commented Mar 8, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 8, 2018 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Mar 8, 2018

Uh oh!

AlekseyTs commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jcouv commented Mar 7, 2018 •

edited

Loading

jcouv commented Mar 7, 2018 •

edited

Loading

gafter commented Mar 8, 2018 •

edited by jcouv

Loading

jcouv commented Mar 8, 2018 •

edited

Loading

AlekseyTs Mar 8, 2018 •

edited

Loading

AlekseyTs Mar 8, 2018 •

edited

Loading

AlekseyTs Mar 8, 2018 •

edited

Loading

AlekseyTs Mar 8, 2018 •

edited

Loading

AlekseyTs Mar 8, 2018 •

edited

Loading

jcouv Mar 8, 2018 •

edited by AlekseyTs

Loading

AlekseyTs Mar 8, 2018 •

edited

Loading

AlekseyTs Mar 8, 2018 •

edited

Loading

AlekseyTs commented Mar 8, 2018 •

edited

Loading

AlekseyTs Mar 8, 2018 •

edited

Loading

AlekseyTs commented Mar 8, 2018 •

edited

Loading

AlekseyTs commented Mar 8, 2018 •

edited

Loading

AlekseyTs commented Mar 8, 2018 •

edited

Loading

AlekseyTs commented Mar 8, 2018 •

edited

Loading

AlekseyTs commented Mar 8, 2018 •

edited

Loading

AlekseyTs commented Mar 8, 2018 •

edited

Loading

AlekseyTs commented Mar 8, 2018 •

edited

Loading

AlekseyTs commented Mar 8, 2018 •

edited

Loading

jcouv commented Mar 8, 2018 •

edited

Loading

AlekseyTs Mar 9, 2018 •

edited

Loading

AlekseyTs Mar 9, 2018 •

edited

Loading

AlekseyTs Mar 9, 2018 •

edited

Loading