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

NamedTuple.From reducing on abstract types and singleton types lead to soundness holes #20517

Closed
smarter opened this issue Jun 3, 2024 · 2 comments · Fixed by #20947
Closed
Assignees
Labels
area:experimental:named-tuples Issues tied to the named tuples feature. itype:bug itype:soundness Soundness bug (it lets us compile code that crashes at runtime with a ClassCastException)

Comments

@smarter
Copy link
Member

smarter commented Jun 3, 2024

Compiler version

3.5.1-RC1-bin-20240602-c6fbe6f-NIGHTLY

Minimized code

import scala.language.experimental.namedTuples
import NamedTuple.From

case class Foo[+T](elem: T)

trait Base[T]:
  def dep(foo: T): From[T]

class SubAny[T <: Foo[Any]] extends Base[T]:
  def dep(foo: T): From[T] = (elem = "")

object Test:
  @main def run =
    val f: Foo[Int] = Foo(elem = 1)
    val b: Base[Foo[Int]] = SubAny[Foo[Int]]()
    val nt: (elem: Int) = b.dep(f)
    val x: Int = nt.elem // ClassCastException

Output

Exception in thread "main" java.lang.ClassCastException: class java.lang.String cannot be cast to class java.lang.Integer (java.lang.String and java.lang.Integer are in module java.base of loader 'bootstrap')
        at scala.runtime.BoxesRunTime.unboxToInt(BoxesRunTime.java:99)
        at Test$.run(nt-from.scala:20)
        at run.main(nt-from.scala:16)

Expectation

If From behaved like a match type then:

  def dep(foo: T): From[T] = (elem = "")

would be a type error as From[T] would not reduce.

And just like with match types, we can also run into troubles because of singleton types (at least those coming from dependent methods parameters as in #19746, though I had to make the test case more complex to delay reduction because NamedTuple.From doesn't compose with asSeenFrom like match types do):

import scala.language.experimental.namedTuples
import NamedTuple.From

case class Foo[+T](elem: T)

trait Base[M[_]]:
  def dep(foo: Foo[Any]): M[foo.type]

class SubAny extends Base[From]:
  def dep(foo: Foo[Any]): From[foo.type] = (elem = "")

object Test:
  @main def run =
    val f: Foo[Int] = Foo(elem = 1)
    val b: Base[From] = SubAny()
    val nt: (elem: Int) = b.dep(f)
    val x: Int = nt.elem // ClassCastException

So it seems that in general, NamedTuple.From and match types should share a lot more logic to prevent unsoundness.

@smarter smarter added itype:bug area:experimental:named-tuples Issues tied to the named tuples feature. labels Jun 3, 2024
@smarter smarter added the itype:soundness Soundness bug (it lets us compile code that crashes at runtime with a ClassCastException) label Jun 3, 2024
@smarter
Copy link
Member Author

smarter commented Jun 3, 2024

@sjrd do you see a way of adapting the match type machinery to NamedTuple.From ? It seems we should at least have its argument checked via îsConcrete before proceeding with reduction:

/* Concreteness checking
*
* When following a baseType and reaching a non-wildcard, in-variant-pos type capture,
* we have to make sure that the scrutinee is concrete enough to uniquely determine
* the values of the captures. This comes down to checking that we do not follow any
* upper bound of an abstract type.
*
* See notably neg/wildcard-match.scala for examples of this.
*
* See neg/i13780.scala, neg/i13780-1.scala and neg/i19746.scala for
* ClassCastException reproducers if we disable this check.
*/
def isConcrete(tp: Type): Boolean =

@sjrd
Copy link
Member

sjrd commented Jun 3, 2024

Yes, we could/should extract isConcrete and reuse it here. It makes sense to share the logic.

odersky added a commit to dotty-staging/dotty that referenced this issue Jul 2, 2024
 - Move isConcrete to a new object `MatchTypes`. We should also move other MatchType-related
   stuff from Types and TypeComparer here. Type and TypeComparer are already unconfortably big,
   and MatchTypes are a coherent topic where everything should work together.
 - Streamline isConcrete a bit.
 - Re-use isConcrete for a similar test in CheckRealizable.
 - Re-use isConcrete for evaluating NamedTuple.From

Fixes scala#20517
@smarter smarter closed this as completed in 532a9da Jul 3, 2024
smarter added a commit that referenced this issue Jul 3, 2024
- Move isConcrete to a new object `MatchTypes`. We should also move
other MatchType-related stuff from Types and TypeComparer here. Type and
TypeComparer are already unconfortably big, and MatchTypes are a
coherent topic where everything should work together.
 - Streamline isConcrete a bit.
 - Re-use isConcrete for a similar test in CheckRealizable.
 - Re-use isConcrete for evaluating NamedTuple.From

Fixes #20517
WojciechMazur pushed a commit that referenced this issue Aug 27, 2024
 - Move isConcrete to a new object `MatchTypes`. We should also move other MatchType-related
   stuff from Types and TypeComparer here. Type and TypeComparer are already unconfortably big,
   and MatchTypes are a coherent topic where everything should work together.
 - Streamline isConcrete a bit.
 - Re-use isConcrete for a similar test in CheckRealizable.
 - Re-use isConcrete for evaluating NamedTuple.From

Fixes #20517

[Cherry-picked 532a9da]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:experimental:named-tuples Issues tied to the named tuples feature. itype:bug itype:soundness Soundness bug (it lets us compile code that crashes at runtime with a ClassCastException)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants