Implement tuple equality by jcouv · Pull Request #23356 · dotnet/roslyn

jcouv · 2017-11-22T06:11:15Z

CyrusNajmabadi · 2017-11-24T22:18:45Z

Should we maybe 'lift' all operators onto tuples, just like we can with nullable? You could then have:

var x = GetVector() var y = GetOtherVector(); var z = x + y;

While it expands the original proposal, it seems like it would be fine. And since you're here anyways... :)

AFAICT, it would be fine. An operator is 'lifted' up to two tuples, if they have the same length, and if the operator is defined for all pairwise matching element types of the tuples.

What do you think? (Honestly, not trying to feature creep. It just seems like it would make sense and would fit in well here).

--

Also, there are operators like < and >. If we did those, i imagine they would have to be 'all' operators. i.e. a < b if all of a's elements are less than the constiuent element in b. That would mean (and it would make sense) that you would have tuples that were neither <, == or > than another tuple. That makes sense to me and feels fine.
#Resolved

CyrusNajmabadi · 2017-11-24T22:21:21Z

Are there any IOperation concerns here? #Resolved

jcouv · 2017-11-27T17:22:15Z

📝 I'll address or remove comment before merging. #Closed

jcouv · 2017-11-27T17:22:38Z

📝 I'll address before merging. #Closed

jcouv · 2017-11-27T17:22:55Z

📝 I'll address before merging. #Closed

jcouv · 2017-11-27T17:24:27Z

@dotnet/roslyn-compiler for review. Thanks

@CyrusNajmabadi Yes, that's on my radar. I'll add minimal IOperation support in this PR (IOperation.None) and propose how to represent properly for next PR. #Resolved

CyrusNajmabadi · 2017-11-27T19:42:43Z

Yes, that's on my radar.

What's that in reference to? Lifting all operators to tuples? Or doing IOperation support? #Resolved

jcouv · 2017-11-27T20:43:36Z

Yes, that's on my radar.

I meant IOperation support. GetOperation should work on t == (1, 2). My thinking is that it should expose two IOperation nodes (for left and right) and then also the operator kind.

I'd probably not expose the details of the comparison (which element-wise binary operators are involved). That would follow the example of IOperation support for deconstructions (which doesn't currently expose which Deconstruct methods or conversions are involved).

Lifting all operators to tuples

LDM preferred to limit support to just == and != for now. #Resolved

CyrusNajmabadi · 2017-11-27T22:29:43Z

LDM preferred to limit support to just == and != for now.

Thanks! But also: :'(

in practice probably not that useful. And, if really necessary, could always be added later. #Resolved

gafter · 2017-11-28T18:25:04Z

namespace [](start = 0, length = 9)

I don't see any tests comparing nullable tuples of different types, e.g. (int, int)? t1 and (long, long)? t2, where conversions are needed to do the tests. Can you please add such tests? Also, it would be nice to have at least one test involving a user-defined conversion in the mix with nullable too. #Closed

Good point.
Some other test ideas:

nullable tuples used with != (to make sure the binary logic around HasValue is correct).

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

Also, please add a test with tuple cardinality 1 (using a long tuple's Rest field).

In reply to: 153588129 [](ancestors = 153588129,153580386)

Are there tests for tuples passed as in? #Resolved

gafter · 2017-11-28T18:29:47Z

This does not appear to implement the specification-required overload resolution rules to determine which built-in operator to use. For example, if the left-hand operand is a tuple, and the right-hand operand is not a tuple but has a conversion to a tuple, we should be using tuple equality. Please add such a test. #Closed

This may require LDM review/design of the intended rules if that isn't simple, which I suspect may be the case.

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

See TestEqualityOfTypeConvertingFromTuple and TestEqualityOfTypeConvertingToTuple
The conversion to tuple doesn't contribute (so the binding errors out).
The conversion from tuple works as before.

I believe that matches what you and I had discussed with Mads. #Resolved

gafter · 2017-11-28T18:30:31Z

        var best = this.BinaryOperatorOverloadResolution(kind, left, right, node, diagnostics, out resultKind, out originalUserDefinedOperators);

I believe this is where we should be determining that we should be using a tuple equality or inequality operator. #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:507 in 2bbd8e4. [](commit_id = 2bbd8e4, deletion_comment = False)

gafter · 2017-11-28T18:34:39Z

Do we have a draft spec for the intended semantics? #Closed

jcouv · 2017-11-28T18:49:04Z

Here's the proposed spec: dotnet/csharplang#967 #Resolved

jcouv · 2017-11-28T18:52:54Z

📝 I'll add those before merging. #Closed

jcouv · 2017-11-28T18:54:23Z

Dead comment. I'll remove #Closed

gafter · 2017-11-28T19:54:54Z

TupleBinaryOperatorInfo [](start = 28, length = 23)

TupleBinaryOperatorInfo [](start = 28, length = 23)

TupleBinaryOperatorInfo [](start = 28, length = 23)

Can these types be defined in BoundNodes.xml? #Closed

They could, but since they don't correspond to any specific syntax, I'm not sure it would be beneficial. I'm following the model used for foreach loops, awaits and deconstructions, which all use "info" objects.

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

gafter · 2017-11-28T20:04:57Z

_factory [](start = 19, length = 8)

_factory [](start = 19, length = 8)

I suspect that there may be some lowering not occurring here. For example, what happens if the operands are of type decimal? #Closed

Lowering would turn the operator into a method invocation (e.g. when single.MethodSymbolOpt is non-null)

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

Previously, I had a VisitExpression at the top-level (before returning from VisitTupleBinaryOperator). It turns out that is lowering some nodes twice. I've now fixed the lowering strategy to avoid double-lowering.

In reply to: 153608156 [](ancestors = 153608156,153607936)

gafter

Please add some tests as discussed.

AlekseyTs · 2018-03-05T05:29:50Z

+            //     leftValue = left.GetValueOrDefault(); (or left if !leftNullable)
+            //     rightValue = right.GetValueOrDefault(); (or right if !rightNullable)
+            //     ... logical expression using leftValue and rightValue ...
+            BoundExpression innerSequence = _factory.Sequence(locals: ImmutableArray<LocalSymbol>.Empty, innerEffects.ToImmutableAndFree(), logicalExpression);


innerEffects.ToImmutableAndFree() [](start = 105, length = 33)

Should probably avoid creating a sequence if there are no side-effects #Closed

AlekseyTs · 2018-03-05T05:31:55Z

+            //          : false/true
+            bool boolValue = operatorKind == BinaryOperatorKind.Equal; // true/false
+            BoundExpression outerSequence =
+                _factory.Sequence(ImmutableArray<LocalSymbol>.Empty, outerEffects.ToImmutableAndFree(),


outerEffects.ToImmutableAndFree() [](start = 69, length = 33)

Should probably avoid creating a sequence if there are no side-effects #Closed

AlekseyTs · 2018-03-05T05:37:05Z

+            if (isRightNullable)
+            {
+                rightHasValue = MakeNullableHasValue(right.Syntax, right); // no need for local for right.HasValue since used once
+                rightValue = MakeValueOrDefaultTemp(right, initialEffectsAndTemps.temps, innerEffects);


rightValue = MakeValueOrDefaultTemp(right, initialEffectsAndTemps.temps, innerEffects); [](start = 16, length = 87)

It looks like rightValue is used only once, why do we store it in a temp? #Closed

AlekseyTs · 2018-03-05T05:37:55Z

+            if (isLeftNullable)
+            {
+                leftHasValue = MakeHasValueTemp(left, initialEffectsAndTemps.temps, outerEffects);
+                leftValue = MakeValueOrDefaultTemp(left, initialEffectsAndTemps.temps, innerEffects);


leftValue = MakeValueOrDefaultTemp(left, initialEffectsAndTemps.temps, innerEffects); [](start = 16, length = 85)

It looks like leftValue is used only once, why do we store it in a temp? #Closed

AlekseyTs · 2018-03-05T05:49:41Z

+        /// For tuple literals, we just return the element.
+        /// For expressions with tuple type, we access `Item{i}`.
+        /// </summary>
+        private BoundExpression GetTuplePart(BoundExpression tuple, int i, ArrayBuilder<LocalSymbol> temps, ArrayBuilder<BoundExpression> initEffects)


, ArrayBuilder temps, ArrayBuilder initEffects [](start = 73, length = 76)

It looks like these two parameters are not used. #Closed

AlekseyTs · 2018-03-05T05:57:36Z

+                    result = _factory.Not(result);
+                }
+            }
+            else if (boolConversion != Conversion.Identity)


boolConversion != Conversion.Identity [](start = 21, length = 37)

!boolConversion.IsIdentity? #Closed

AlekseyTs · 2018-03-05T06:04:23Z

+            return result;
+        }
+
+        private class TupleOperatorSideEffectsAndTemps


TupleOperatorSideEffectsAndTemps [](start = 22, length = 32)

With the current approach, it feels like this type does not give us any advantage. Side-effects are only collected locally in VisitTupleBinaryOperator and, I think, the code would be much cleaner if other helpers would keep track of their temps and create sequences accordingly. Even, if we don't change temp management, it is still enough to pass a the same builder for side-effects to ReplaceTerminalElementsWithTemps, and simply pass a builder for temps to other helpers. #Closed

AlekseyTs · 2018-03-05T06:08:37Z

Done with review pass (iteration 17) #Closed

AlekseyTs · 2018-03-05T17:47:20Z

+            // (1, 2) == (1, 2);
+            if (IsTupleExpression(tuple.Kind))
+            {
+                return ((BoundTupleExpression)tuple).Arguments[i];


return ((BoundTupleExpression)tuple).Arguments[i]; [](start = 16, length = 50)

Can this be anything, but a tuple literal? #Closed

The implementation of IsTupleExpression is return kind == BoundKind.TupleLiteral || kind == BoundKind.ConvertedTupleLiteral; and the two sub-types of BoundTupleExpression are BoundTupleLiteral and BoundConvertedTupleLiteral.

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

The implementation of IsTupleExpression is return kind == BoundKind.TupleLiteral || kind == BoundKind.ConvertedTupleLiteral; and the two sub-types of BoundTupleExpression are BoundTupleLiteral and BoundConvertedTupleLiteral.

Given the behavior of ReplaceTerminalElementsWithTemps, it feels like we cannot get here for anything, but a tuple literal

In reply to: 172275316 [](ancestors = 172275316,172270998)

AlekseyTs · 2018-03-05T20:29:52Z

+        {
+            var boolType = node.Type; // we can re-use the bool type
+            var leftInit = ArrayBuilder<BoundExpression>.GetInstance();
+            var rightInit = ArrayBuilder<BoundExpression>.GetInstance();


var rightInit = ArrayBuilder.GetInstance(); [](start = 12, length = 60)

It doesn't look like there is a need to have two builders #Closed

AlekseyTs · 2018-03-05T20:35:52Z

+        {
+            // Example:
+            // (1, 2) == (1, 2);
+            if (IsTupleExpression(tuple.Kind))


IsTupleExpression(tuple.Kind) [](start = 16, length = 29)

I would change this condition to cover only tuple literals too. #Closed

AlekseyTs · 2018-03-05T20:39:24Z

+
+            // PROTOTYPE(tuple-equality) checked
+            // We leave the null literal in nullable-null conversions unconverted because MakeBinaryOperator has special rules for it
+            bool isNullableNullConversion = ((operatorKind & BinaryOperatorKind.NullableNull) == 0);


bool isNullableNullConversion = ((operatorKind & BinaryOperatorKind.NullableNull) == 0); [](start = 12, length = 88)

Is this logic correct? Should we be checking for == 0 here? Consider to use OperandTypes helper here as well. #Closed

AlekseyTs · 2018-03-05T20:45:10Z

Done with review pass (iteration 18) #Closed

gafter · 2018-03-05T21:33:58Z

        // SPEC: For an operation of one of the forms x == null, null == x, x != null, null != x,

This comment is misplaced. I should be after the tuple business. #Resolved

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

AlekseyTs · 2018-03-05T21:47:23Z

+
+            // PROTOTYPE(tuple-equality) checked
+            // We leave the null literal in nullable-null conversions unconverted because MakeBinaryOperator has special rules for it
+            bool isNullableNullConversion = operatorKind.OperandTypes() != BinaryOperatorKind.NullableNull;


isNullableNullConversion [](start = 17, length = 24)

There is still something strange about this statement. The local is named "isNullableNullConversion", but it looks like the initializer checks for an opposite situation. Perhaps the local name is confusing, but the comment above also talks about nullable-null conversions. #Closed

Sorry about that. There is definitely something wrong. Investigating.

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

Thanks a lot for catching that, Aleksey.
The element-wise binary operator for nullable-null conversion has a kind, but no operator and no converted types. So no conversion is needed for either side (the null or the other one) during lowering.
There was a similar bug for nullable-null during binding, where I'd used an error type, when in fact leaving the types as null is perfectly fine.

AlekseyTs · 2018-03-05T21:48:23Z

Done with review pass (iteration 19) #Closed

gafter · 2018-03-05T21:54:26Z

+        /// - nested expressions that aren't tuple literals, like `GetTuple()` in `(..., GetTuple()) == (..., (..., ...))`
+        /// On the other hand, `Item1` and `Item2` of `GetTuple()` are not saved as part of the initialization phase of `GetTuple() == (..., ...)`
+        ///
+        /// Element-wise conversions occur late, together with the element-wise comparisons. They may not be evaluated.


may [](start = 98, length = 3)

"may" should be "might". "may not" is an idiom forbidding something. #Resolved

gafter · 2018-03-05T22:03:52Z

+            // Examples:
+            // in `expr == (..., ...)` we need to save `expr` because it's not a tuple literal
+            // in `(..., expr) == (..., (..., ...))` we need to save `expr` because it is used in a simple comparison
+            return EvaluateSideEffectingArgumentToTemp(VisitExpression(expr), initEffects, ref temps);


EvaluateSideEffectingArgumentToTemp [](start = 19, length = 35)

In looking at the (existing) implementation of the method EvaluateSideEffectingArgumentToTemp, I don't believe it will spill this in a value type. I don't know if that was right or wrong before, but that would appear to be incorrect for this use. #Resolved

Good catch. Added test TestThisStruct with PROTOTYPE marker

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

gafter · 2018-03-05T22:07:08Z

+                BoundExpression dynamicResult = _dynamicFactory.MakeDynamicBinaryOperator(single.Kind, left, right, isCompoundAssignment: false, _compilation.DynamicType).ToExpression();
+                if (operatorKind == BinaryOperatorKind.Equal)
+                {
+                    return _factory.Not(MakeUnaryOperator(UnaryOperatorKind.DynamicFalse, left.Syntax, method: null, dynamicResult, boolType));


_factory.Not(MakeUnaryOperator(UnaryOperatorKind.DynamicFalse [](start = 27, length = 61)

This looks like a double negative. #Resolved

I see, the different operator adds a third negation to make them as different as they are supposed to be.

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

gafter

AlekseyTs

LGTM (Iteration 21)

jcouv · 2018-03-06T21:37:28Z

Thanks a bunch 🎉
I'll merge this and get started on restoring semantic model behavior and basic IOperation (as two separate PRs).

jcouv added the Area-Compilers label Nov 22, 2017

jcouv added this to the 15.7 milestone Nov 22, 2017

jcouv self-assigned this Nov 22, 2017

jcouv changed the title ~~Allow comparing tuple types~~ Allow comparing tuples Nov 22, 2017

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Nov 22, 2017

jcouv force-pushed the tuple-equality branch from c951257 to 9c79e1e Compare November 22, 2017 19:58

CyrusNajmabadi reviewed Nov 24, 2017

View reviewed changes

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Nov 27, 2017

jcouv commented Nov 27, 2017

View reviewed changes

gafter reviewed Nov 28, 2017

View reviewed changes

jcouv commented Nov 28, 2017

View reviewed changes

gafter reviewed Nov 28, 2017

View reviewed changes

gafter previously requested changes Nov 28, 2017

View reviewed changes

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Nov 30, 2017

jcouv requested a review from a team as a code owner January 23, 2018 00:09

jcouv force-pushed the tuple-equality branch 2 times, most recently from c426be8 to b0cf72a Compare January 30, 2018 19:11

AlekseyTs reviewed Mar 5, 2018

View reviewed changes

jcouv mentioned this pull request Mar 5, 2018

Incorrect check on BinaryOperatorKind.NullableNull #25231

Closed

Address PR feedback from Aleksey

032159d

AlekseyTs reviewed Mar 5, 2018

View reviewed changes

Use single builder for effects. Strengthen op kind logic

d9b3a95

AlekseyTs reviewed Mar 5, 2018

View reviewed changes

gafter reviewed Mar 5, 2018

View reviewed changes

Address PR feedback from Neal

d96dcfc

gafter approved these changes Mar 6, 2018

View reviewed changes

Fix issue with nullable-null lowering

364bbc9

jcouv force-pushed the tuple-equality branch from 46915b5 to 364bbc9 Compare March 6, 2018 04:57

AlekseyTs approved these changes Mar 6, 2018

View reviewed changes

jcouv merged commit 6c6faaf into dotnet:features/tuple-equality Mar 6, 2018

jcouv mentioned this pull request Mar 21, 2018

Tuple equality: Optimize cases where nullable always has value #25644

Merged

Conversation

jcouv commented Nov 22, 2017

Uh oh!

CyrusNajmabadi Nov 24, 2017 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Nov 24, 2017 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Nov 27, 2017 • 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 Nov 27, 2017 • 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 Nov 27, 2017 • 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 commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Nov 27, 2017 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Nov 27, 2017 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter Nov 28, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Feb 13, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Nov 28, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 28, 2017 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Nov 28, 2017 • 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 Nov 28, 2017 • 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!

gafter Nov 28, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

gafter Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

CyrusNajmabadi Nov 24, 2017 •

edited by jcouv

Loading

CyrusNajmabadi commented Nov 24, 2017 •

edited by jcouv

Loading

jcouv Nov 27, 2017 •

edited

Loading

jcouv Nov 27, 2017 •

edited

Loading

jcouv Nov 27, 2017 •

edited

Loading

jcouv commented Nov 27, 2017 •

edited

Loading

CyrusNajmabadi commented Nov 27, 2017 •

edited by jcouv

Loading

jcouv commented Nov 27, 2017 •

edited

Loading

CyrusNajmabadi commented Nov 27, 2017 •

edited by jcouv

Loading

gafter Nov 28, 2017 •

edited

Loading

cston Feb 13, 2018 •

edited by jcouv

Loading

gafter Nov 28, 2017 •

edited

Loading

jcouv Nov 28, 2017 •

edited by gafter

Loading

gafter commented Nov 28, 2017 •

edited

Loading

gafter commented Nov 28, 2017 •

edited

Loading

jcouv commented Nov 28, 2017 •

edited

Loading

jcouv Nov 28, 2017 •

edited

Loading

jcouv Nov 28, 2017 •

edited

Loading

gafter Nov 28, 2017 •

edited

Loading

gafter Nov 28, 2017 •

edited

Loading

AlekseyTs Mar 5, 2018 •

edited

Loading

AlekseyTs Mar 5, 2018 •

edited

Loading

AlekseyTs Mar 5, 2018 •

edited

Loading

AlekseyTs Mar 5, 2018 •

edited

Loading

AlekseyTs Mar 5, 2018 •

edited

Loading

AlekseyTs Mar 5, 2018 •

edited

Loading

AlekseyTs Mar 5, 2018 •

edited

Loading

AlekseyTs commented Mar 5, 2018 •

edited

Loading

AlekseyTs Mar 5, 2018 •

edited

Loading

AlekseyTs Mar 5, 2018 •

edited

Loading

AlekseyTs Mar 5, 2018 •

edited

Loading