Skip to content
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

Unsound generic tuple type test #9056

Closed
nicolasstucki opened this issue May 27, 2020 · 4 comments · Fixed by #9049
Closed

Unsound generic tuple type test #9056

nicolasstucki opened this issue May 27, 2020 · 4 comments · Fixed by #9049

Comments

@nicolasstucki
Copy link
Contributor

The following two matches succeed when they should not. Tuple, NonEmptyTuple, *: erase to Object or Product and therefore the test is performed on those classes.

("a": Any) match
    case _: Tuple => assert(false)
    case _ => // ok
  case class Foo(a: Int)

  (Foo(1): Any) match
    case a: (_ *: _) => a.tail
    case _ => // ok

As these classes do not exist at runtime we should handle them as abstract types which would yield the unchecked warning. This approach could be combined with TypeTest to provide a type test that does the correct checks at runtime.

@nicolasstucki
Copy link
Contributor Author

nicolasstucki commented May 27, 2020

Minimized to

  assert(((): Any).isInstanceOf[Tuple])
  assert((1, 2).isInstanceOf[Tuple])
  assert((1, 2, 3).isInstanceOf[Tuple])
  assert(!"".isInstanceOf[Tuple]) // fail
  assert(!Some("").isInstanceOf[Tuple]) // fail

  assert(!((): Any).isInstanceOf[NonEmptyTuple])
  assert((1, 2).isInstanceOf[NonEmptyTuple])
  assert((1, 2, 3).isInstanceOf[NonEmptyTuple])
  assert(!("": Any).isInstanceOf[NonEmptyTuple])
  assert(!(Some(""): Any).isInstanceOf[NonEmptyTuple]) // fail

  assert(!((): Any).isInstanceOf[_ *: _])
  assert((1, 2).isInstanceOf[_ *: _])
  assert((1, 2, 3).isInstanceOf[_ *: _])
  assert(!("": Any).isInstanceOf[_ *: _])
  assert(!(Some(""): Any).isInstanceOf[_ *: _]) // fail

All these assertions should pass

@nicolasstucki
Copy link
Contributor Author

Maybe we could erase instance tests on tuples in the following way

x.isInstanceOf[NonEmptyTuple]
isInstanceOfNonEmptyTuple(x)
// In scala.runtime.Tuple
def isInstanceOfNonEmptyTuple(x: Any): Boolean = 
  x.isInstanceOf[Tuple1[_]] 
  || x.isInstanceOf[Tuple2[_, _]] 
  || ... 
  || x.isInstanceOf[Tuple22[_, ..., _]] 
  ||  x.isInstanceOf[TupleXXL] || 

@nicolasstucki
Copy link
Contributor Author

We also have a similar issue with asInstanceOf

@nicolasstucki
Copy link
Contributor Author

This is a blocker for #9049

nicolasstucki added a commit to dotty-staging/dotty that referenced this issue May 28, 2020
@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 May 29, 2020
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Jun 7, 2020
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.

1 participant