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

Named tuples second implementation #19174

Merged
merged 68 commits into from May 7, 2024
Merged

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Dec 3, 2023

This implementation follows the alternative representation scheme, where a named tuple type is represented as a
pair of two tuples: one for the names, the other for the values.

Compare with #19075, where named tupes were regular types, with special element types that combine name and value.

In both cases, we use an opaque type alias so that named tuples are represented at runtime by just their values - the names are forgotten.

The new implementation has some advantages

  • we can control in the type that named and unnamed elements are not mixed,
  • no element types are leaked to user code,
  • non-sensical operations such as concatenating a named and an unnamed tuple are statically excluded,
  • it's generally easier to enforce well-formedness constraints on the type level.

The main disadvantage compared to #19075 is that there is a certain amount of duplication in types and methods between Tuple and NamedTuple. On the other hand, we can make sure by this that no non-sensical tuple operations are accidentally applied to named tuples.

@odersky odersky added the needs-minor-release This PR cannot be merged until the next minor release label Dec 3, 2023
@odersky odersky marked this pull request as draft December 3, 2023 14:24
@lrytz
Copy link
Member

lrytz commented Dec 5, 2023

Is it possible to change toString?

scala> println((a = 1, b = 2))
(1,2)

also the REPL doesn't seem to display tuple literals for some reason.

scala> (1, 2)
val res8: (Int, Int) = (1,2)

scala> (a = 1, b = 2)

scala>

@odersky
Copy link
Contributor Author

odersky commented Dec 5, 2023

@lrytz We can't change toString since named tuples are to tuples. We should seriously start with implementing a general Show typeclass to address these issues.

I don't know how to run the repl with a language import in the latest release. Who else can look into this?

@lrytz
Copy link
Member

lrytz commented Dec 5, 2023

I don't know how to run the repl with a language import in the latest release. Who else can look into this?

I used

scala> import language.experimental.namedTuples

Another observation:

scala> def f: NamedTuple.NamedTuple[(Int, Any), (Int, String)] = ???
[error] scala.MatchError: TypeRef(TermRef(ThisType(TypeRef(NoPrefix,module class <root>)),object scala),class Int) (of class dotty.tools.dotc.core.Types$CachedTypeRef)
[error] 	at dotty.tools.dotc.core.TypeUtils.$anonfun$2(TypeUtils.scala:84)
[error] 	at scala.collection.immutable.List.map(List.scala:246)
[error] 	at dotty.tools.dotc.core.TypeUtils.namedTupleElementTypesUpTo(TypeUtils.scala:84)
[error] 	at dotty.tools.dotc.printing.RefinedPrinter.appliedText$1(RefinedPrinter.scala:241)

val _: (String, Int) = (name = "", age = 0) // error
val _: NameOnly = person // error
val _: Person = nameOnly // error

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

val _: Person = (name = "") ++ nameOnly

tests/run/named-tuples.scala Show resolved Hide resolved
@experimental
object NamedTuple:

opaque type AnyNamedTuple = Any
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this separate definition is needed because NamedTuple[?, ?] is illegal, but this definition is very limiting compared to what can be done with a regular Tuple:

def elems(x: Tuple): Int =
  x.size // ok
def wrap(x: Tuple): Tuple.Map[x.type, List] =
  x.map([t] => (x: t) => List(x)) // ok

None of these work on x: AnyNamedTuple because no extension methods are defined on it.

I can't think of a better way to enable this than writing something like:

opaque type AnyNamedTuple = Tuple
extension (x: AnyNamedTuple)
  inline def toTuple: Tuple = x
  inline def size: Tuple.Size[Tuple] = toTuple.size
  // ...
  inline def map[F[_]](f: [t] => t => F[t]): AnyNamedTuple =
    toTuple.map(f).asInstanceOf
  // ...

and hope that the more precise extension is picked when it is usable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If NamedTuple was covariant in both of its arguments we could maybe write:

opaque type NamedTuple[+N <: Tuple, +V <: Tuple] >: V = V
type AnyNamedTuple = NamedTuple[Tuple, Tuple]

But this breaks the reduction of all match types defined in NamedTuple.scala and upper-bounded by AnyNamedTuple for some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try AnyNamedTuple = Tuple? I think I tried that before and it did not work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't seem to help, somehow all the variants I tried break reduction:

type Person = (name: String, age: Int)
val xx: NamedTuple.Names[Person] = ("name", "age")
  // error: Found (String, String)  Expected: NamedTuple.Names[Person]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. It works for me.

@odersky
Copy link
Contributor Author

odersky commented Dec 14, 2023

Some new comments on the older thread on subtyping: #19075 (comment)

My recommendation is that we want to be very flexible with tuple convertibility, mirroring what other languages do. So we could have subtyping in one direction and an implicit conversion in the other. Which directions? it comes down to these two questions:

val x: (A, B)
val y1: (a1: A, b1:  B)
val y2: (a2: A, b2: B)
val z1 = if ??? then x else y1 // -- what is the type of z1?
val z2 = if ??? then y1 else y2 // -- what is the type of z2

Subtyping unnamed <:< named with implicit conversion in the other direction answers as follows:

val z1: (a1: A, b1: B)
val z2 // error, untypable [EDIT: Actually, it's AnyNamedTuple]

That is, name info is kept when available, type error when it conflicts.

When swapping the directions, we get:

val z1: (A, B)
val z2: (A, B)

That is, name info is erased unless all parts agree on a name.

Typescript seems to follow the first approach. I also think it makes more sense. I think it's ergonomically better since it detects errors earlier.

@soronpo
Copy link
Contributor

soronpo commented Dec 14, 2023

val z2 = if ??? then y1 else y2 // -- what is the type of z2

Shouldn't it be a union of both types (a1: A, b1: B) | (a2: A, b2: B) or widened to NonEmptyTuple?

@odersky
Copy link
Contributor Author

odersky commented Dec 14, 2023

Shouldn't it be a union of both types (a1: A, b1: B) | (a2: A, b2: B) or widened to NonEmptyTuple?

Actually, widened to AnyNamedTuple, with my last commit. Before it was a type error but for a weird reason. (we got unreducible inferred type).

@odersky
Copy link
Contributor Author

odersky commented Jan 12, 2024

@sjrd After rebasing to the new match type implementation, I get this:

[error] -- [E191] Type Error: /Users/odersky/workspace/dotty/library/src/scala/NamedTuple.scala:107:36 
[error] 107 |  type Names[X <: AnyNamedTuple] <: Tuple = X match
[error]     |                                    ^
[error]     |                The match type contains an illegal case:
[error]     |                    case NamedTuple.NamedTuple[n, _] => n
[error]     |                (this error can be ignored for now with `-source:3.3`)
[error] 108 |    case NamedTuple[n, _] => n
[error] -- [E191] Type Error: /Users/odersky/workspace/dotty/library/src/scala/NamedTuple.scala:111:41 
[error] 111 |  type DropNames[NT <: AnyNamedTuple] <: Tuple = NT match
[error]     |                                         ^
[error]     |                The match type contains an illegal case:
[error]     |                    case NamedTuple.NamedTuple[_, x] => x
[error]     |                (this error can be ignored for now with `-source:3.3`)
[error] 112 |    case NamedTuple[_, x] => x
[error] two errors found

The error message is not exactly helpful. Can you advise what the problem is and how to fix it?

@sjrd
Copy link
Member

sjrd commented Jan 12, 2024

The definition of the two match types:

  /** The names of a named tuple, represented as a tuple of literal string values. */
  type Names[X <: AnyNamedTuple] <: Tuple = X match
    case NamedTuple[n, _] => n

  /** The value types of a named tuple represented as a regular tuple. */
  type DropNames[NT <: AnyNamedTuple] <: Tuple = NT match
    case NamedTuple[_, x] => x

are in a scope that sees the transparent alias NamedTuple[N, V] = V, i.e., NamedTuple = [N, V] =>> V. Therefore, when it sees

    case NamedTuple[n, _] => n

Since NamedTuple is neither a class type, nor an abstract type constructor, and there is at least one type capture in its type arguments, it is an illegal match type case.

This makes sense because it is equivalent by dealiasing to

    case ([N, V] =>> V)[n, _] => n

and that means it collapses to

    case _ => n // oops! n is not defined here!

The "standard workaround" is to move the definition of these match types to a scope that does not see the transparent alias. This exploits the fact that NamedTuple will be an abstract type constructor in such a scope.

@odersky
Copy link
Contributor Author

odersky commented Jan 12, 2024

Thanks for the explanation! I'll try that. It would be good to work on the error diagnostics so that one could suggest fixes like this automatically.

@odersky odersky added this to the 3.5.0 milestone Jan 15, 2024
@odersky odersky requested a review from dwijnand January 17, 2024 18:05
@odersky odersky self-assigned this Jan 17, 2024
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly minor questions and comments. I think the only real issue I have is the subtype relationship. The test cases seems satisfied if we only support using tuple syntax to define a named tuple, looking to get a green PR of that in #19532.

compiler/src/dotty/tools/dotc/ast/Desugar.scala Outdated Show resolved Hide resolved
library/src/scala/NamedTuple.scala Outdated Show resolved Hide resolved
library/src/scala/NamedTuple.scala Show resolved Hide resolved
case tp: SingletonType =>
if tp.termSymbol == defn.EmptyTupleModule then Some(Nil) else None
if tp.termSymbol == defn.EmptyTupleModule then Some(Nil)
else if normalize then recur(tp.widen, bound)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this and the SkolemType changes needed?

library/src/scala/Tuple.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/ast/Desugar.scala Outdated Show resolved Hide resolved
library/src/scala/NamedTuple.scala Outdated Show resolved Hide resolved
library/src/scala/NamedTuple.scala Outdated Show resolved Hide resolved
@odersky odersky force-pushed the named-tuples-2 branch 2 times, most recently from 13c0b17 to fea6fff Compare February 7, 2024 17:46
odersky and others added 22 commits May 6, 2024 17:11
as is the case for `Concat`
to only require being defined on the element types.
Similar to what we have for `type FlatMap`
This is for the same reason as we changed
`type Head[X <: NonEmptyTuple] = ...` to
`type Head[X <: Tuple] = ...`

Also, this is no more unsafe than the other operations already defined for all tuples.
`drop(1)` for example was always defined, even though `tail` wasn't.
The tuple term level definitions were not being tested before
odersky and others added 2 commits May 7, 2024 09:47
Keep only the changes we need for making NamedTuple work properly.
We still keep operations Contains and Disjoint in Tuple and make Reverse standard.

We remove or amend tests that relied on the changes to Tuple. No
tests about NamedTuple's are affected.
If we add it as is now, we will *not* be able to add the bound in
a future release. It is best to leave it completely undefined. The
typer is happy to ignore the case where it does not exist at all.
@sjrd sjrd mentioned this pull request May 7, 2024
Copy link
Member

@bishabosha bishabosha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amazing

@odersky odersky merged commit 7e27c4b into scala:main May 7, 2024
19 checks passed
@odersky odersky deleted the named-tuples-2 branch May 7, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants