Skip to content

Missing arity check in fromProduct for Tuples #15399

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
nicolasstucki opened this issue Jun 8, 2022 · 6 comments · Fixed by #15404
Closed

Missing arity check in fromProduct for Tuples #15399

nicolasstucki opened this issue Jun 8, 2022 · 6 comments · Fixed by #15404

Comments

@nicolasstucki
Copy link
Contributor

Compiler version

438dfb0

Minimized code

@main def Test =
  val tup2Mirror = summon[scala.deriving.Mirror.Of[(Int, Int)]]
  val tup2a: (Int, Int) = tup2Mirror.fromProduct((1, 2, 3)) // fails silently and creates (1, 2)
  println(tup2a)
  val tup2b: (Int, Int) = tup2Mirror.fromProduct(Tuple(1)) // crashes with index out of bounds
  println(tup2b)

Output

Exception in thread "main" java.lang.IndexOutOfBoundsException: 1 is out of bounds (min 0, max 0)
        at scala.Product1.productElement(Product1.scala:42)
        at scala.Product1.productElement$(Product1.scala:40)
        at scala.Tuple1.productElement(Tuple1.scala:23)
        at Test$package$$anon$1.fromProduct(Test.scala:4)
        at Test$package$$anon$1.fromProduct(Test.scala:4)
        at Test$package$.Test(Test.scala:6)
        at Test.main(Test.scala:3)

Expectation

fromProduct should assert that the arity of the given tuple is correct. It should fail with a meaningful error message.

@bishabosha
Copy link
Member

bishabosha commented Jun 8, 2022

we can patch this for newly compiled code but its still not going to work when depending on libraries compiled by 3.1.x - but fromProduct is supposed to be unsafe anyway - what if the arity is correct but the element types are wrong?

@nicolasstucki
Copy link
Contributor Author

nicolasstucki commented Jun 8, 2022

What I would propose is instead of generating a new anonymous class for each tuple mirror we could define a class in the library.

Right now we do

val tup2Mirror = 
          {
            final class $anon() extends Object(), Serializable {
              type MirroredMonoType = (Int, Int) // not sure why this is added here
            }
            (new $anon():Object & Serializable)
          }.$asInstanceOf[
              scala.deriving.Mirror.Product{
                MirroredMonoType = (Int, Int); MirroredType = (Int, Int); 
                  MirroredLabel = ("Tuple2" : String)
                ; MirroredElemTypes = (Int, Int); 
                  MirroredElemLabels = (("_1" : String), ("_2" : String))
              }
            
          ]

instead we could define in the library runtime

package scala.runtime
class TupleMirror(arity: Int) extends Mirror.Product {
  def fromProduct(p: scala.Product): MirroredMonoType = 
    assert(p.productArity == arity, "...")
    Tuple.fromProduct(p).asInstanceOf[MirroredMonoType]
}

and then we can infer

val tup2Mirror = 
          new runtime.TupleMirror(2).$asInstanceOf[
              scala.deriving.Mirror.Product{
                MirroredMonoType = (Int, Int); MirroredType = (Int, Int); 
                  MirroredLabel = ("Tuple2" : String)
                ; MirroredElemTypes = (Int, Int); 
                  MirroredElemLabels = (("_1" : String), ("_2" : String))
              }
            
          ]

@nicolasstucki
Copy link
Contributor Author

This also made me notice that the tuple mirrors are extremely inefficient as each inferred instance creates a new class with a duplication of the same code. This would be solved by the runtime.TupleMirror with or without an arity.

@nicolasstucki
Copy link
Contributor Author

Furthermore, the runtime.TupleMirror would also work for large tuples (see #15398).

@bishabosha
Copy link
Member

this sounds good to me - I didn't quite get before that this change was just for tuples. - and the runtime.TupleMirror should not leak into public apis. would that need to be experimental still?

@nicolasstucki
Copy link
Contributor Author

We could add the runtime.TupleMirror as experimental in a patch release but we will not be able to use it until the next minor release. Maybe it is cleaner to add the class and change the mirror inference at the same time on the minor release.

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.

3 participants