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

Make named tuples a standard feature #21680

Merged
merged 5 commits into from
Oct 4, 2024

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Oct 1, 2024

  • Deprecate experimental language import
  • Make named tuple features conditional on -source >= 3.6 instead
  • Make the NamedTuple object non-experimental.
  • Move NamedTuple it to src-bootstrapped since it relies on clause interleaving which is only standard in 3.6 as well.
  • Drop the experimental.namedTuple import from tests

 - Deprecate experimental language import
 - Make named tuple features conditional on -source >= 3.6 instead
 - Make the NamedTuple object non-experimental.
 - Move NamedTuple it to src-bootstrapped since it relies on clause interleaving
   which is only standard in 3.6 as well.
 - Drop the experimental.namedTuple import from tests
The previous syntax `before copy (name = ...)` is now interpreted as a binary
operation with a named tuple on the RHS. The correct syntax is `before.copy(name = ...)`.
Copy link
Contributor

@aherlihy aherlihy left a comment

Choose a reason for hiding this comment

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

lgtm, with one question which is if this is a blocker: #21413?

We sometimes face a problem that we inline a reference `x: T` which upon further inlining
is adapted to an expected type `x`. It only seems to occur in complicated scenarios. I could
not completely narrow it down. But in any case it's safe to drop the widening cast in order
to avoid a type error here. We do that in a last-effort adaptation step that's only enabled
in the Inliner: Faced with an expression `x: T` and a singleton expected type `y.type` where
`x.type <: y.type`, rewrite to `x`.
@odersky
Copy link
Contributor Author

odersky commented Oct 3, 2024

#21413 was an Inliner problem. I managed to fix it in the last commit.

@odersky
Copy link
Contributor Author

odersky commented Oct 3, 2024

Fixes #21413

@odersky odersky linked an issue Oct 3, 2024 that may be closed by this pull request
@Gedochao Gedochao added the needs-minor-release This PR cannot be merged until the next minor release label Oct 4, 2024
@odersky
Copy link
Contributor Author

odersky commented Oct 4, 2024

@EugeneFlesselle I suggest to put any changes to Tuple/NamedTuple code in separate PRs. That way it's easier to coordinate.

@odersky odersky merged commit 49d39d9 into scala:main Oct 4, 2024
28 checks passed
@odersky odersky deleted the finalize-namedtuples branch October 4, 2024 17:59
@EugeneFlesselle
Copy link
Contributor

@EugeneFlesselle I suggest to put any changes to Tuple/NamedTuple code in separate PRs. That way it's easier to coordinate.

Ah I was about to finish my review. I'll submit the comments here anyway and we can see afterwards where to apply potential changes.

@odersky
Copy link
Contributor Author

odersky commented Oct 4, 2024

Sorry, I was too hasty. Yes, please add the comments to this PR and I'll open another one that incorporates any changes.

Copy link
Contributor

@EugeneFlesselle EugeneFlesselle left a comment

Choose a reason for hiding this comment

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

As previously mentioned, there are a few cosmetic changes (pushed here) to the NamedTuple sources which I propose while we have the chance:

  • Use the NamedTuple type ops for the result types of the term ops
  • Drop asInstanceOf from the NamedTuple term ops where possible

Some similar minor things, which I think are fine, but could be considered before dropping @experimental:

  • We could add a NamedTuple.Union type alias for the union of the element types, we use it already twice within NamedTuples extension methods.
  • The type Init could be defined in terms of Take, as we do for Tail in terms of Drop.

On the documentation side:

named-tuples.md#L206 mentions:

The operations on named tuples are defined in object scala.NamedTuple.

They are defined within the NamedTupleDecomposition object which we then export. This fine to ignore for the comment there. But more annoyingly however, it seems the exported methods do not show up in the linked documentation page.

NamedTuple.scala#L39-L44 has two commented out method definitions and a comment which I think went along methods which were since moved into NamedTupleDecomposition. Should we move or delete them?

And some final minor documentation notes, which again are probably fine, but to which I am bringing attention in case we wanted to add anything:

  • There are a few NamedTuple members without any doc, namely withNames, apply and unapply.
  • Perhaps we should mention, or reference to, the question of Conformance and Convertibility in the scaladoc of the NamedTuple.NamedTuple opaque type definition.

This is a long comment but overall It's very cool to see named tuples become standard!

@@ -1,5 +1,4 @@
//> using options -source future
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//> using options -source future

Should -source future still be necessary?

@EugeneFlesselle
Copy link
Contributor

Sorry, I was too hasty. Yes, please add the comments to this PR and I'll open another one that incorporates any changes.

No problem! and sorry about the slow review

@odersky
Copy link
Contributor Author

odersky commented Oct 5, 2024

Can we make a PR our of finalize-namedtuples-2 and do the changes proposed here on this one?

@EugeneFlesselle
Copy link
Contributor

EugeneFlesselle commented Oct 5, 2024

Can we make a PR our of finalize-namedtuples-2

I have opened #21710

and do the changes proposed here on this one?

All or any of them specifically?

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.

Cannot select from named tuple literal
5 participants