change tuple pattern to match any Sequence

[gavinking]

Should we change the semantics of patterns in case so that: case ([Float x, Float y]) matches any Sequence of length 2, not just a Tuple?

Note that, importantly, the syntax is not case (is [Float x, Float y]), so it's not clear that the interpretation should be that this is testing the type.

See discussion in #4127.

This, in principle, would be a breaking change.

[jvasileff]

I believe yes. I also think this would be good for 1.3.1, and is important to be in place for #4127.

For this, I believe the distinction between Tuple and Sequence is only a hindrance (never of value.)

ArraySequences are not something you try to have over Tuples, and Tuples are what you have if you're "lucky" enough to have code structured in a way to statically know that you have one, so that certain features can be unlocked. But the usefulness of destructuring in switch extends well beyond cases where you are likely to be "lucky".

For non-switch destructuring, there is no risk of bugs, because the compiler produces an error when attempting to destructure an incompatible type.

For switch destructuring, risk of bugs is high, since no error or warning is necessarily given. And, the bugs may be tricky to track down, since you are likely to use Tuples for test cases while clients may use ArraySequences.

For #4127, I think these arguments are 10-times as important.

This, in principle, would be a breaking change

While technically breaking, my intuition is that it's likely to fix bugs in existing code, and very unlikely to break existing code.

[gavinking]

ArraySequences are not something you try to have over Tuples, and Tuples are what you have if you're "lucky" enough to have code structured in a way to statically know that you have one, so that certain features can be unlocked. But the usefulness of destructuring in switch extends well beyond cases where you are likely to be "lucky".

I share this intuition. Well, with the possible exception of Range, at least.

While technically breaking, my intuition is that it's likely to fix bugs in existing code, and very unlikely to break existing code.

Obviously I agree, or I would not be contemplating a breaking change to the semantics of the language.

I also think this would be good for 1.3.1

OK, let me reassign it to 1.3.1, but it's up to @tombentley and @chochos to actually implement the change.

[jvasileff]

@gavinking wasn't this done with desugaring?

Also, the error "error: cases are not disjoint: 'Anything[2]' and 'Float[2]'" for the code below is a bit wonky because it's attached to a node w/o location info:

shared void run() {
    Anything[2] tup = [1.0, 2.0];
    switch (tup)
    case ([Float x, Float y]) {}
    case ([Anything x, Anything y]) {}
}

[gavinking]

@jvasileff yes, but there is a boolean getExpressionPattern() on the Destructure node which tells you that this is what is going on. I could easily add an additional flag somewhere else if that's needed.

[jvasileff]

Well, at this point it seems like maybe it should be implemented without desugaring.

I mean, there's a bogus IsCase, incorrect flow typing, and no size check or element type checks, and a Destructure using an instance with a possibly incorrect runtime type. If anything, it seems like the nodes generated by the typechecker just get in the way.

If everyone things that's ok, then fine with me, but also, this is allowed:

    [Anything*] seq = sequence { 1.0, 2.0 };
    switch (seq)
    case ([Float x, Float y]) {
        Float[2] f2 = seq; // oops
    }
    else {}

[gavinking]

Well, at this point it seems like maybe it should be implemented without desugaring.

Yes, sure, I agree, I would love to get rid of the implementation based on desugaring.

incorrect flow typing, and no size check or element type checks, and a Destructure using an instance with a possibly incorrect runtime type.

I have seriously no idea what you're talking about here.

[gavinking]

this is allowed:

    [Anything*] seq = sequence { 1.0, 2.0 };
    switch (seq)
    case ([Float x, Float y]) {
        Float[2] f2 = seq; // oops
    }
    else {}

Ok, hrm, that's not right, given the new proposed semantics.

[jvasileff]

I have seriously no idea what you're talking about here.

Ok, I'll just wait for everyone else to sort it out before I try to tackle the Dart impl :smile:

But FTR, none of this looks right to me for what we actually need:

|  |  |  |  |  |  +  [IsCase]                                                                                                      
|  |  |  |  |  |  |  +  [TupleType] : Float[2]                                                                                     
|  |  |  |  |  |  |  |  +  [BaseType] (4:11-4:15) : Float : class Float(Float float)                                               
|  |  |  |  |  |  |  |  |  + Float [Identifier] (4:11-4:15)                                                                        
|  |  |  |  |  |  |  |  +  [BaseType] (4:20-4:24) : Float : class Float(Float float)                                               
|  |  |  |  |  |  |  |  |  + Float [Identifier] (4:20-4:24)                                                                        
|  |  |  |  |  |  |  +  [Variable] : value seq => Float[2]                                                                         
|  |  |  |  |  |  |  |  + seq [Identifier]                                                                                         
|  |  |  |  |  |  |  |  +  [SyntheticVariable] : Float[2]                                                                          
|  |  |  |  |  |  |  |  +  [SpecifierExpression]                                                                                   
|  |  |  |  |  |  |  |  |  +  [Expression] : Anything[]                                                                            
|  |  |  |  |  |  |  |  |  |  +  [BaseMemberExpression] : Anything[] : seq : value run.seq => Anything[]                           
|  |  |  |  |  |  |  |  |  |  |  + seq [Identifier]                                                                                
|  |  |  |  |  |  |  |  |  |  |  +  [InferredTypeArguments]                                                                        
|  |  |  |  |  |  + {} [Block] (4:30-6:4)                                                                                          
|  |  |  |  |  |  |  +  [Destructure] (4:10-4:27)                                                                                  
|  |  |  |  |  |  |  |  +  [ValueModifier] : unknown                                                                               
|  |  |  |  |  |  |  |  + [] [TuplePattern] (4:10-4:27)                                                                            
|  |  |  |  |  |  |  |  |  +  [VariablePattern] (4:11-4:17)                                                                        
|  |  |  |  |  |  |  |  |  |  +  [Variable] (4:11-4:17) : value x => Float                                                         
|  |  |  |  |  |  |  |  |  |  |  + x [Identifier] (4:17-4:17)                                                                      
|  |  |  |  |  |  |  |  |  |  |  +  [BaseType] (4:11-4:15) : Float : class Float(Float float)                                      
|  |  |  |  |  |  |  |  |  |  |  |  + Float [Identifier] (4:11-4:15)                                                               
|  |  |  |  |  |  |  |  |  +  [VariablePattern] (4:20-4:26)                                                                        
|  |  |  |  |  |  |  |  |  |  +  [Variable] (4:20-4:26) : value y => Float                                                         
|  |  |  |  |  |  |  |  |  |  |  + y [Identifier] (4:26-4:26)                                                                      
|  |  |  |  |  |  |  |  |  |  |  +  [BaseType] (4:20-4:24) : Float : class Float(Float float)                                      
|  |  |  |  |  |  |  |  |  |  |  |  + Float [Identifier] (4:20-4:24)                                                               

[gavinking]

OK, fine, I agree that the implementation based on desugaring isn't appropriate for the semantics proposed here.

So, I'll again remove this from the 1.3.1 milestone.

[tombentley]

I'm not quite sure what I need to do here. The CustomTree.IsCase still has a tuple type. I suppose I need to add a test for it being a sequence, and test the type of each element?

And I assume I don't need to support @jvasileff's oops above?

[tombentley]

@gavinking did you see my comment?

[gavinking]

@tombentley I suggest we prioritize other issues right now.

[jvasileff]

Here's a short motivating example for this change, inspired by an observation @Zambonifofex made on Gitter about the runtime type of arguments for variadics:

shared void run() {
    assert (foo(*["string"]));
    assert (!foo("string"));
}

Boolean foo(Anything* arg)
    =>  switch (arg)
        case ([String s]) true
        else false;