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

Move NonEmptyTuple members into Tuple #21291

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

EugeneFlesselle
Copy link
Contributor

@EugeneFlesselle EugeneFlesselle commented Jul 29, 2024

The motivation for this has already been established, among other things:

  • the corresponding type level operations already use Tuple as upper bound;
  • the corresponding NamedTuple operations also do not make a distinction;
  • these operations are no more unsafe than other operations already available on Tuple, such as drop.

Note this should not be a problem for binary compatibility, as both Tuple and NonEmptyTuple are erased to Products (see defn.specialErasure).

See #19175 (comment) for related changes.
Based on #21308

@EugeneFlesselle EugeneFlesselle marked this pull request as ready for review July 29, 2024 16:03
@EugeneFlesselle
Copy link
Contributor Author

@bishabosha By the way, sorry I forgot this PR also had the tuple change for some reason, which I did not mean to assign to you.

I'll split the PR into two, with an example of why the second commit is needed.

EugeneFlesselle added a commit to dotty-staging/dotty that referenced this pull request Jul 31, 2024
This is in particular necessary for scala#21291,
to avoid problems encountered after inlining from scopes defining opaque types
(such as in the example below),
as was already done for the other NamedTuple operations in scala#20504.

```scala
-- Error: tests/pos/named-tuple-combinators.scala:46:17 ------------------------
46 |    val res1 = x.head
   |               ^^^^^^
   |(Int, String) does not conform to bound >:
   |  (x$proxy55 : (x : Test.NT) &
   |    $proxy19.NamedTuple[
   |      Tuple.Concat[
   |        NamedTupleDecomposition.Names[
   |          $proxy19.NamedTuple[Tuple1[("hi" : String)], Tuple1[Int]]],
   |        NamedTupleDecomposition.Names[
   |          $proxy19.NamedTuple[Tuple1[("bla" : String)], Tuple1[String]]]
   |      ],
   |      Tuple.Concat[
   |        NamedTupleDecomposition.DropNames[
   |          $proxy19.NamedTuple[Tuple1[("hi" : String)], Tuple1[Int]]],
   |        NamedTupleDecomposition.DropNames[
   |          $proxy19.NamedTuple[Tuple1[("bla" : String)], Tuple1[String]]]
   |      ]
   |    ]
   |  )
   | <: Tuple
   |----------------------------------------------------------------------------
   |Inline stack trace
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
   |This location contains code that was inlined from NamedTuple.scala:47
47 |    inline def head: Tuple.Elem[V, 0] = x.apply(0)
   |                                        ^^^^^^^
    ----------------------------------------------------------------------------
```
bishabosha added a commit that referenced this pull request Jul 31, 2024
This is in particular necessary for #21291,
to avoid problems encountered after inlining from scopes defining opaque
types (such as in the example below),
as was already done for the other NamedTuple operations in #20504.
@EugeneFlesselle EugeneFlesselle enabled auto-merge (rebase) November 6, 2024 15:53
Addressing scala#19175

The motivation for this has already been established, among other things:
- the corresponding type level operations already use `Tuple` as upper bound;
- the corresponding `NamedTuple` operations also do not make a distinction;
- these operations are no more unsafe than other operations already available
  on `Tuple`, such as `drop`

Note this should _not_ be a problem for binary compatibility,
as both `Tuple` and `NonEmptyTuple` are erased to `Product`s
(see `defn.specialErasure`).
@EugeneFlesselle EugeneFlesselle merged commit 16becd7 into scala:main Nov 11, 2024
29 checks passed
@EugeneFlesselle EugeneFlesselle deleted the tuples/nonempty-ops branch November 11, 2024 08:51
@WojciechMazur WojciechMazur added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Nov 11, 2024
@WojciechMazur WojciechMazur added this to the 3.6.2 milestone Nov 11, 2024
@SethTisue
Copy link
Member

SethTisue commented Nov 12, 2024

👍 This is also a win for discoverability — as seen on #21928 , it did not occur to either me or the user on Discord to look in the Scaladoc for NonEmptyTuple to find some of the operations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants