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 selection on the result of NamedTuple.Concat doesn't work #20427

Closed
smarter opened this issue May 17, 2024 · 7 comments · Fixed by #20504 · May be fixed by #20457 or #20903
Closed

NamedTuple selection on the result of NamedTuple.Concat doesn't work #20427

smarter opened this issue May 17, 2024 · 7 comments · Fixed by #20504 · May be fixed by #20457 or #20903
Assignees
Labels
area:named-tuples Issues tied to the named tuples feature. itype:bug
Milestone

Comments

@smarter
Copy link
Member

smarter commented May 17, 2024

Compiler version

3.5.0-RC1-bin-20240516-c608177-NIGHTLY

Minimized code

import scala.language.experimental.namedTuples

object Test:
  type NT = NamedTuple.Concat[(hi: Int), (bla: String)]
  def foo(x: NT) =
    x.hi // error
    val y: (hi: Int, bla: String) = x
    y.hi // ok

Output

[error] ./try/nt.scala:6:5
[error] Found:    (x$proxy1 : (x : Test.NT) &
[error]   $proxy1.NamedTuple[
[error]     Tuple.Concat[
[error]       NamedTupleDecomposition.Names[
[error]         $proxy1.NamedTuple[Tuple1[("hi" : String)], Tuple1[Int]]],
[error]       NamedTupleDecomposition.Names[
[error]         $proxy1.NamedTuple[Tuple1[("bla" : String)], Tuple1[String]]]
[error]     ],
[error]     Tuple.Concat[
[error]       NamedTupleDecomposition.DropNames[
[error]         $proxy1.NamedTuple[Tuple1[("hi" : String)], Tuple1[Int]]],
[error]       NamedTupleDecomposition.DropNames[
[error]         $proxy1.NamedTuple[Tuple1[("bla" : String)], Tuple1[String]]]
[error]     ]
[error]   ]
[error] )
[error] Required: (Int, String)
[error]     x.hi // error
[error]     ^

Expectation

A valid selection.

More details

The selection desugars into something equivalent to:

NamedTuple.apply[("hi", "bla"), (Int, String)](x)(0)

With -Xprint:inlining I get:

        val $proxy1:

            NamedTuple.type{
              type AnyNamedTuple = Any;
                type NamedTuple[N <: Tuple,V <: Tuple] = V
            }

         =
          NamedTuple.$asInstanceOf[

              NamedTuple.type{
                type AnyNamedTuple = Any;
                  type NamedTuple[N <: Tuple,V <: Tuple] = V
              }

          ]
        val NamedTuple$_this:

            ($proxy1 :
              NamedTuple.type{
                type AnyNamedTuple = Any;
                  type NamedTuple[N <: Tuple,V <: Tuple] = V
              }
            )

         = $proxy1
        {
          val $scrutinee1: <error unspecified error> =
            NamedTuple$_this.toTuple[(("hi" : String), ("bla" : String)),
              (Int, String)](x$proxy1)
          val tup: <error unspecified error> = $scrutinee1
          tup.apply[(Int, String) & NonEmptyTuple](0).asInstanceOf[
            Tuple.Elem[(Int, String), 0.type]]
  • I'm not sure why we end up creating proxies for the top-level object NamedTuple here
  • The call to toTuple refers to an argument x$proxy1 but the pretty-printed tree doesn't contain any definition with that name.
  • Manually calling toTuple on my concatenated named tuple works.
@smarter smarter added itype:bug area:named-tuples Issues tied to the named tuples feature. labels May 17, 2024
@smarter
Copy link
Member Author

smarter commented May 17, 2024

I'm not sure why we end up creating proxies for the top-level object NamedTuple here

Ah I guess it's to deal with the combination of inline and opaque types

@smarter
Copy link
Member Author

smarter commented May 17, 2024

So it seems the inliner tries to deal with opaqueness by essentially taking the intersection of the original type and the type you get after replacing all the opaque types involved by type aliases. I think this sort of works when the opaque type is defined with an upper-bound but falls apart when it's defined with a lower-bound: (hi: Int, bla: String) & (Int, String) is equal to (Int, String) and not to (hi: Int, bla: String).

cf https://github.com/scala/scala3/blob/main/compiler/src/dotty/tools/dotc/inlines/Inliner.scala#L420-L430 introduced in 1c2a8de

@smarter
Copy link
Member Author

smarter commented May 22, 2024

Actually this isn't specific to lower-bounds, we can minimize this to:

object Foo:
  opaque type Wrapper[T] = T
  inline def part[T](w: Wrapper[T]): T = w
  inline def part2[T](w: Wrapper[T]): T = part(w)
  type Rewrap[W] = Wrapper[Extra.Unwrap[W]]

object Extra:
  type Unwrap[W] = W match
    case Foo.Wrapper[t] => t
  type Rewrap[W] = Foo.Wrapper[Unwrap[W]]

object Test:
  type X = Extra.Rewrap[Foo.Wrapper[Int]]
  def foo(x: X) =
    Foo.part(x) // ok
    Foo.part2(x) // error
-- [E007] Type Mismatch Error: try/opaque-nt.scala:18:14 -----------------------
18 |    Foo.part2(x) // error
   |              ^
   |Found:    (w$proxy2 : (x : Test.X) & $proxy2.Wrapper[Extra.Unwrap[$proxy2.Wrapper[Int]]])
   |Required: Foo$_this.Wrapper[Int]
   |----------------------------------------------------------------------------
   | Explanation (enabled by `-explain`)
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
   |
   | Tree: w$proxy2
   | I tried to show that
   |   (w$proxy2 : (x : Test.X) & $proxy2.Wrapper[Extra.Unwrap[$proxy2.Wrapper[Int]]])
   | conforms to
   |   Foo$_this.Wrapper[Int]
   | but none of the attempts shown below succeeded:
   |
   |   ==> (w$proxy2 : (x : Test.X) & $proxy2.Wrapper[Extra.Unwrap[$proxy2.Wrapper[Int]]])  <:  Foo$_this.Wrapper[Int]
   |     ==> (w$proxy2 : (x : Test.X) & $proxy2.Wrapper[Extra.Unwrap[$proxy2.Wrapper[Int]]])  <:  Int
   |       ==> (x : Test.X) & $proxy2.Wrapper[Extra.Unwrap[$proxy2.Wrapper[Int]]]  <:  Int
   |         ==> (x : Test.X)  <:  Int
   |           ==> Test.X  <:  Int
   |             ==> Extra.Rewrap[Foo.Wrapper[Int]]  <:  Int
   |               ==> Foo.Wrapper[Extra.Unwrap[Foo.Wrapper[Int]]]  <:  Int
   |                 ==> Any  <:  Int  = false
   |         ==> $proxy2.Wrapper[Extra.Unwrap[$proxy2.Wrapper[Int]]]  <:  Int
   |           ==> Extra.Unwrap[$proxy2.Wrapper[Int]]  <:  Int
   |             ==> $proxy2.Wrapper[Int] match {   case Foo.Wrapper[t] => t } <: t  <:  Int
   |               ==> t  <:  Int

Match type reduction for $proxy2.Wrapper[Int] match { case Foo.Wrapper[t] => t } gets stuck in

case MatchTypeCasePattern.AbstractTypeConstructor(tycon, argPatterns) =>
scrut.dealias match
case scrutDealias @ AppliedType(scrutTycon, args) if scrutTycon =:= tycon =>
matchArgs(argPatterns, args, tycon.typeParams, scrutIsWidenedAbstract)
case _ =>
false

This is somewhat similar to #20453 but trying to change match type reduction to get this to work seems a lot dicier /cc @sjrd .

@smarter
Copy link
Member Author

smarter commented May 22, 2024

For the minimized example, we can work around the issue by adding a seemingly useless cast:

  inline def part[T](w: Wrapper[T]): T = w
  inline def part2[T](w: Wrapper[T]): T = part(w.asInstanceOf[Wrapper[T]])

This seems to work for the original NamedTuple issue too (slightly more verbose since toTuple is an extension method):

diff --git library/src/scala/NamedTuple.scala library/src/scala/NamedTuple.scala
index dc6e6c3144..e258ce5cbb 100644
--- library/src/scala/NamedTuple.scala
+++ library/src/scala/NamedTuple.scala
@@ -38,7 +38,7 @@ object NamedTuple:

     /** The value (without the name) at index `n` of this tuple */
     inline def apply(n: Int): Tuple.Elem[V, n.type] =
-      inline toTuple match
+      inline NamedTuple.toTuple(x.asInstanceOf[NamedTuple[N, V]]) match
         case tup: NonEmptyTuple => tup(n).asInstanceOf[Tuple.Elem[V, n.type]]
         case tup => tup.productElement(n).asInstanceOf[Tuple.Elem[V, n.type]]

smarter added a commit to dotty-staging/dotty that referenced this issue May 22, 2024
The inliner tries to handle opaque types by replacing prefixes containing them
by proxy objects with type aliases. When the type we're mapping is a match type
application, this can end up breaking its reduction.

Reducing match type applications instead of performing this mapping seems to
avoid the issue in practice, but I don't know if it completely solves the
problem.

Fixes scala#20427.
smarter added a commit to dotty-staging/dotty that referenced this issue May 22, 2024
The inliner tries to handle opaque types by replacing prefixes containing them
by proxy objects with type aliases. When the type we're mapping is a match type
application, this can end up breaking its reduction.

Reducing match type applications instead of performing this mapping seems to
avoid the issue in practice, but I don't know if it completely solves the
problem.

Fixes scala#20427.
@smarter
Copy link
Member Author

smarter commented May 22, 2024

Tentative fix: #20457

@bishabosha
Copy link
Member

bishabosha commented May 31, 2024

Another solution seems to be moving the apply method into NamedTupleDecomposition, and reexporting it from NamedTuple

@odersky
Copy link
Contributor

odersky commented Jun 21, 2024

I wonder whether we can come up with a general principle here:

In the scope of an opaque type (i.e. where the alias is known) we are not allowed to refer to a match type that contains the opaque type in one of its patterns. Can we maybe make this a warning or even an error? In that case, Jamie's fix which moved inline methods mentioning match types defined in NamedTuple outside of named tuple would address this issue. And we'd likely not have made the mistake in the first place.

EugeneFlesselle added a commit to dotty-staging/dotty that referenced this issue Jul 1, 2024
Change the rhs of the added refinements from `TypeAlias`es to `RealTypeBounds`s.
This allows the opaque types to remain abstract type constructors,
which is necessary for the `MatchTypeCasePattern.AbstractTypeConstructor` logic.

In particular, an AppliedType where the tycon was a reference to the refinement,
used to dealias before proceeding with the comparison of the tycons,
which is a requirement of the aforementioned `AbstractTypeConstructor`
case of the MatchReducer.

Fixes scala#20427
Alternative to scala#20457
@Kordyjan Kordyjan added this to the 3.5.1 milestone Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment