Skip to content

Empty Tuple is not instance of Product #9033

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
letalvoj opened this issue May 24, 2020 · 27 comments · Fixed by #9049
Closed

Empty Tuple is not instance of Product #9033

letalvoj opened this issue May 24, 2020 · 27 comments · Fixed by #9049

Comments

@letalvoj
Copy link

Minimized code w Output

scala> ().asInstanceOf[Tuple].isInstanceOf[Product]
val res5: Boolean = false

scala> (1,2,3).asInstanceOf[Tuple].isInstanceOf[Product]
val res6: Boolean = true

Expectation

I should be able to call ($m: Mirror.Product).fromProduct(()). Currently it's not possible due to the bug above.

@sjrd
Copy link
Member

sjrd commented May 24, 2020

Unfortunately, as long as the empty tuple is undistinguishable from unit, this is unfixable. The instance of Unit cannot be an instance of Product, or any type of than AnyRef for that matter, as that would break the foundations of Scala.js.

@letalvoj
Copy link
Author

@sjrd Well, I would be much more satisfiet if the behaviour was the following

().asInstanceOf[Tuple].isInstanceOf[Product] // true
().isInstanceOf[Product] // false

Because currently we can get a runtime error in a case like the following:

scala> val t:Tuple = ()
val t: Tuple = ()

scala> summon[Mirror.Of[...]].fromProduct(t.asInstanceOf)                   
java.lang.ClassCastException: class scala.runtime.BoxedUnit cannot be cast to class scala.Product (scala.runtime.BoxedUnit and scala.Product are in unnamed module of loader sbt.internal.classpath.ClassLoaderCache$Key$CachedClassLoader @37c469b4)

@sjrd
Copy link
Member

sjrd commented May 24, 2020

@sjrd Well, I would be much more satisfiet if the behaviour was the following

().asInstanceOf[Tuple].isInstanceOf[Product] // true
().isInstanceOf[Product] // false

That would run contrary to a core expectation that asInstanceOf does not perform any conversion, and hence cannot change the nature of its argument, nor the result of subsequent type tests.

Because currently we can get a runtime error in a case like the following:

scala> val t:Tuple = ()
val t: Tuple = ()

scala> summon[Mirror.Of[...]].fromProduct(t.asInstanceOf)                   
java.lang.ClassCastException: class scala.runtime.BoxedUnit cannot be cast to class scala.Product (scala.runtime.BoxedUnit and scala.Product are in unnamed module of loader sbt.internal.classpath.ClassLoaderCache$Key$CachedClassLoader @37c469b4)

There's an asInstanceOf there, which happens not to be valid. It's expected that it would result in a CCE.

@letalvoj
Copy link
Author

@sjrd Well, I would be much more satisfiet if the behaviour was the following

().asInstanceOf[Tuple].isInstanceOf[Product] // true
().isInstanceOf[Product] // false

That would run contrary to a core expectation that asInstanceOf does not perform any conversion, and hence cannot change the nature of its argument, nor the result of subsequent type tests.

I was aiming more to () being an instance of a different trait in different contexts. Sorry, should have been something more like

(():Tuple).isInstanceOf[Product] // true
(():Unit).isInstanceOf[Product] // false

Does it make more sense?

@sjrd
Copy link
Member

sjrd commented May 25, 2020

I was aiming more to () being an instance of a different trait in different contexts. Sorry, should have been something more like

(():Tuple).isInstanceOf[Product] // true
(():Unit).isInstanceOf[Product] // false

Does it make more sense?

Ah sorry, I had completely misunderstood. Yes, that makes much more sense, and in principle I would like to see that happen. I am not sure what the ramifications of that would be, though. It might prove impractical, or prone to more dangerous warts.

@letalvoj
Copy link
Author

letalvoj commented May 25, 2020

Well, I checked how is it possible that shapeless-3 is not having issues with this feature and there is a custom ArrayProduct.

I suppose that this is a known feature which was widely discussed internally. There seem to be other related functionality in place.

scala> ((1,2):Tuple).isInstanceOf[Product]                                                                                                            
val res7: Boolean = true

scala> ((1,2):Tuple):Product
1 |((1,2):Tuple):Product
  | ^^^^^^^^^^^
  | Found:    Tuple
  | Required: Product

Anyway, I'd want to raise a concern about having inconsistencies such as ^^ in the type system. I'd expect to be able to execute the following statement ((1,2):Tuple):Product. Maybe it might be worth decoupling Unit from Tuple, either explicitly, or implicitly using an approach such as #9033 (comment)

@sjrd
Copy link
Member

sjrd commented May 25, 2020

That's not an inconsistency. This happens all the time in all object-oriented type systems.

scala> trait A; trait B; class C extends A with B
trait A
trait B
class C

scala> (new C(): A).isInstanceOf[B]
val res0: Boolean = true

scala> (new C(): A): B
               ^
       error: type mismatch;
        found   : A
        required: B

@letalvoj
Copy link
Author

@sjrd oh my... I did not know that Tuple itsef is not Product. Thanks for clarification.
https://github.com/lampepfl/dotty/blob/f807eee1a5c0a9c4b980926cc46bf490eb0740bc/library/src/scala/Tuple.scala#L7

That explains the behaviour but it is rather surprising to me. Since the trait is sealed, i.e. std lib has full control over it's instances, I can see only reason for it being the case and that's supporting the "Unit is not Product" edge case. Am I correct?

@sjrd
Copy link
Member

sjrd commented May 25, 2020

That's correct.

@nicolasstucki
Copy link
Contributor

Currently we define these types, which do not exist at runtime.

sealed trait Tuple extends Any { ... }
sealed trait NonEmptyTuple extends Tuple { ... }
sealed abstract class *:[+H, +T <: Tuple] extends NonEmptyTuple { ... }

All tuple types are made a subtype of *: and Unit is made a subtype of Tuple.

At runtime we use Tuple1 to Tuple22 if the size of the tuple fits, if it is larger we use TupleXXL, otherwise, if the tuple is empty we use ().

final class TupleXXL private (es: IArray[Object]) extends Product

The issue source of the issue is that Unit does not extend Product and therefore Tuple cannot extend Product. One would be tempted to extend Unit with Product but this would break fundamental assumptions on the Unit.

The solution is to simply define a proper Tuple0 that is a Product

object Tuple0 extends Product

The only reason we have not done this is to be able to use the syntax () in combination with *:.

val tup = 1 *: 2 *: () // equivalent to: (1, 2)
tup match
  case x *: y *: () => // equivalent to: case (x, y) =>
  case () => // no equivalent (or maybe using a training comma `(,)`)

We usually never create or match using *: if we know the size of the tuple as it is always longer and harder to read. Therefore the real reason to have () as an empty tuple right now is to be able to create and match against the empty tuple.

@nicolasstucki
Copy link
Contributor

I wonder if we should use trailing commas to write tuples of size 0 and 1 as (,) and (5,). This seems to give a non-ambiguous syntax for them. This syntax is used in other languages such as Python.

@letalvoj
Copy link
Author

letalvoj commented May 25, 2020

@nicolasstucki thanks for exhaustive explanation. I am not much of a fan of (,) On second thought (,) might be good. That's how it's done in Python as well.

On the other hand, first thing which comes to my mind is that curly braces usually define a body of code and round braces data, such as tuples.

Hence one way out of this might be to reserve

  • () for Tuple
  • {} for Unit

IMHO that should work since it seems to be a valid statement even now.

scala> def shrug(params: Any*):Unit = {}                                                                                                              
def shrug(params: Any*): Unit

scala> (i:Int) => {i*i;{}}
val res0: Int => Unit = Lambda$15678/0x0000000806430040@403504fe

Actually, personally I always write {} to get empty body since that's what I would do in Java. 🤔

@letalvoj
Copy link
Author

letalvoj commented May 25, 2020

Quick grep of dotty codebase yields that there is ~ 1.1k of () statements
out of which only 3 are in bootstrapped codebase. Hence there still might be time for such proposal.

Moreover there is ~1k of {} statements as well. But these include empty class / trait body definitions.

$ grep --include='*.scala' -nri '[^a-zA-Z0-9]()$' . | grep -v '\]()' | wc -l
1154

@smarter
Copy link
Member

smarter commented May 25, 2020

() for Tuple0

I think that's only doable if we also have an automatic conversion from Tuple0 to Unit, otherwise a lot of code would break (and even with an automatic conversion, some breakage is likely)

@letalvoj
Copy link
Author

letalvoj commented May 25, 2020

automatic conversion from Tuple0 to Unit

You mean like in the language? That sounds like yet another corner-case-ish feature.

Is there going to be a semi-automatic migration script from 2 to 3? If yes than that's the place where such functionality should be, since any usage of () as Unit in Scala 2 codebases should be easily identifiable.

@nicolasstucki
Copy link
Contributor

@smarter Not sure how we would do the pattern matching with a conversion. Could you elaborate?

@smarter
Copy link
Member

smarter commented May 25, 2020

Is there going to be a semi-automatic migration script from 2 to 3?

Yes, but that's not good enough: we have to support cross-compiling code between Scala 2 and 3, so we can't introduce breaking changes like that.

@smarter Not sure how we would do the pattern matching with a conversion. Could you elaborate?

I have no idea.

@letalvoj
Copy link
Author

letalvoj commented May 25, 2020

can't introduce breaking changes like that

Maybe part of Scala2Compat ?

@nicolasstucki
Copy link
Contributor

Every time we look at this problem we end up realizing the only good way is to have new syntax. We always stop there because () looks nice and concise even though we do not use it much and it is ambiguous with the unit value.

@letalvoj
Copy link
Author

And now the new syntax change could be keeping only {}:Unit and reserving ():Tuple 🙂

The "only" issue is backwards compatibility for Scala 2, but it would clean up the Product vs Tuple vs Unit discrepancy.

@ritschwumm
Copy link

@letalvoj since Unit logically is nothing else but an empty tuple, the would not exactly help anyone...

@nicolasstucki
Copy link
Contributor

There is another big semantic difference between tuples and unit. Tuples have (or should have/mostly have) object semantics while Unit is a value.

This shows another issue with the current state as Tuple must be a subtype of Any instead of AnyRef. This implies that eq does not work on Tuple, but works on tuples of known size.

@sjrd
Copy link
Member

sjrd commented May 25, 2020

@letalvoj since Unit logically is nothing else but an empty tuple, the would not exactly help anyone...

Null is also logically nothing else but an empty tuple, yet it is useful to distinguish them, precisely because they have different subtyping relationships with the rest of the type system. So yes, it would actually help.

Other types that are "logically nothing else but an empty tuple" according to the same criteria: all objects, all singleton literal types.

@letalvoj
Copy link
Author

letalvoj commented May 25, 2020

@letalvoj since Unit logically is nothing else but an empty tuple, the would not exactly help anyone...

We can argue that if that was the case then the current issues should not be happening in the first place. There are two options with the 0-tuple interpretation of Unit

  • it's useful (sound) and therefore Unit should likely behave consistently with the rest of instances of Tuple
    • @sjrd said that such change would break Scala.js
  • it's not useful (unsound) and hence could be re-assessed

@nicolasstucki
Copy link
Contributor

After some offline discussion, we found that we should add the following methods

object Tuple {
  def apply(): Unit = ...
  def unapply(x: Any): Boolean = ...
}

then we could write

val tup: Tuple = 1 *: 2 *: Tuple() 
tup match
  case head *: tail =>
  case Tuple() =>

Then we can replace Unit by some EmptyTuple trait/object that implements Product.

After this, we can also explore the possibility of adding an apply that takes arguments and an unapply that matches any size of tuple. Ideally this one should be typed precisely.

This was referenced May 25, 2020
@letalvoj
Copy link
Author

@nicolasstucki I understand why, yet I still think that not using round braces for tuples is a missed opportunity. Pity that preserving backwards compatibility with round braces () being Unit is such a blocker. Hopefully, at least the trailing comma variant (, ) will become a syntactic sugar for Tuple().

@letalvoj
Copy link
Author

Related issue on 0.24-RC1 in an non-inlined function

[error] 80 |        case o:Unit => 
[error]    |             ^
[error]    |this case is unreachable since type Tuple and class BoxedUnit are unrelated

yet o.getClass is scala.runtime.BoxedUnit :) Great that this is getting fixed.

@nicolasstucki nicolasstucki linked a pull request May 28, 2020 that will close this issue
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Jun 8, 2020
We change to the following hierarchy where all generic tuple types erase to `Product`.

```
Product -- Tuple -+- EmptyTuple
                  |
                  +- NonEmptyTuple -- *:[Head, Tail <: Tuple]
```

These are encoded using the following compile-time only classes

```scalla
sealed trait Tuple extends Product { ... }
object EmptyTuple extends Tuple { ... }
type EmptyTuple = EmptyTuple.type
sealed trait NonEmptyTuple extends Tuple { ... }
sealed abstract class *:[+H, +T <: Tuple] extends NonEmptyTuple { ... }
```
`TupleN` classes for `1 < N <= 22` are made subtypes of `*:` with precise type (as done before). But `Unit` is not anymore related to `Tuple`.

Construction of empty tuples can be done with `Tuple()`, tuples with one element with `Tuple(e)` while any other tuple are created with `(e1, ..., en)`.
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Jun 8, 2020
We change to the following hierarchy where all generic tuple types erase to `Product`.

```
Product -- Tuple -+- EmptyTuple
                  |
                  +- NonEmptyTuple -- *:[Head, Tail <: Tuple]
```

These are encoded using the following compile-time only classes

```scalla
sealed trait Tuple extends Product { ... }
object EmptyTuple extends Tuple { ... }
type EmptyTuple = EmptyTuple.type
sealed trait NonEmptyTuple extends Tuple { ... }
sealed abstract class *:[+H, +T <: Tuple] extends NonEmptyTuple { ... }
```
`TupleN` classes for `1 < N <= 22` are made subtypes of `*:` with precise type (as done before). But `Unit` is not anymore related to `Tuple`.

Construction of empty tuples can be done with `Tuple()`, tuples with one element with `Tuple(e)` while any other tuple are created with `(e1, ..., en)`.
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Jun 8, 2020
We change to the following hierarchy where all generic tuple types erase to `Product`.

```
Product -- Tuple -+- EmptyTuple
                  |
                  +- NonEmptyTuple -- *:[Head, Tail <: Tuple]
```

These are encoded using the following compile-time only classes

```scalla
sealed trait Tuple extends Product { ... }
object EmptyTuple extends Tuple { ... }
type EmptyTuple = EmptyTuple.type
sealed trait NonEmptyTuple extends Tuple { ... }
sealed abstract class *:[+H, +T <: Tuple] extends NonEmptyTuple { ... }
```
`TupleN` classes for `1 < N <= 22` are made subtypes of `*:` with precise type (as done before). But `Unit` is not anymore related to `Tuple`.

Construction of empty tuples can be done with `Tuple()`, tuples with one element with `Tuple(e)` while any other tuple are created with `(e1, ..., en)`.
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Jun 8, 2020
We change to the following hierarchy where all generic tuple types erase to `Product`.

```
Product -- Tuple -+- EmptyTuple
                  |
                  +- NonEmptyTuple -- *:[Head, Tail <: Tuple]
```

These are encoded using the following compile-time only classes

```scalla
sealed trait Tuple extends Product { ... }
object EmptyTuple extends Tuple { ... }
type EmptyTuple = EmptyTuple.type
sealed trait NonEmptyTuple extends Tuple { ... }
sealed abstract class *:[+H, +T <: Tuple] extends NonEmptyTuple { ... }
```
`TupleN` classes for `1 < N <= 22` are made subtypes of `*:` with precise type (as done before). But `Unit` is not anymore related to `Tuple`.

Construction of empty tuples can be done with `Tuple()`, tuples with one element with `Tuple(e)` while any other tuple are created with `(e1, ..., en)`.
odersky added a commit that referenced this issue Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants