Skip to content

Deprecate the helpers in scala.deriving. #10436

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

Merged
merged 1 commit into from
Nov 23, 2020

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented Nov 21, 2020

  • ArrayProduct
  • EmptyProduct
  • productElement

These helpers are not mandated by the spec, nor used by the
compiler. They were used in a few tests for typeclass derivation,
but it does not seem that they are fundamental. We now use
replacements in the relevant tests.

EmptyProduct can be replaced by EmptyTuple.

ArrayProduct seems to be used by "unpickling" kinds of
derivations (an impression confirmed by the community build). This
is too specific a use case to be defined generally in
scala.deriving. Moreover, it is not clear why Arrays receive
special treatment, but not other kinds of sequences or even
immutable arrays. ArrayProduct can be replaced by a custom
new Product {...} wrapper, which in the case of our tests ends up
being simpler than the "feature-rich" ArrayProduct.

productElement seems to be more generally useful, but at the same
time is very unsafe, and is not hard to re-implement. Removing it
forces the call sites to perform the asInstanceOfs themselves,
which is not a bad thing as it allows to better reason about the
code. In particular, our tests benefitted from pushing the cast to
Product ahead in the call chain, at a place where it is
relatively easier to convince oneself that the value is indeed a
Product.


Since it looks like we're going to release an M2, it makes sense to deprecate in M2, and later remove in RC1. Compared to #10405, I have done deeper changes in the tests, that illustrate that the replacements are actually better, IMO, than using the helpers anyway. Especially, the decoupling of the places where the two casts of the former productElement seems really easier to reason about, IMO.

@sjrd sjrd added this to the 3.0.0-M2 milestone Nov 22, 2020
* `ArrayProduct`
* `EmptyProduct`
* `productElement`

These helpers are not mandated by the spec, nor used by the
compiler. They were used in a few tests for typeclass derivation,
but it does not seem that they are fundamental. We now use
replacements in the relevant tests.

`EmptyProduct` can be replaced by `EmptyTuple`.

`ArrayProduct` seems to be used by "unpickling" kinds of
derivations (an impression confirmed by the community build). This
is too specific a use case to be defined generally in
`scala.deriving`. Moreover, it is not clear why Arrays receive
special treatment, but not other kinds of sequences or even
immutable arrays. `ArrayProduct` can be replaced by a custom
`new Product {...}` wrapper, which in the case of our tests ends up
being simpler than the "feature-rich" `ArrayProduct`.

`productElement` seems to be more generally useful, but at the same
time is very unsafe, and is not hard to re-implement. Removing it
forces the call sites to perform the `asInstanceOf`s themselves,
which is not a bad thing as it allows to better reason about the
code. In particular, our tests benefitted from pushing the cast to
`Product` ahead in the call chain, at a place where it is
relatively easier to convince oneself that the value is indeed a
`Product`.
@sjrd sjrd force-pushed the deprecate-scala-deriving-helpers branch from 55988b3 to 0f24d27 Compare November 22, 2020 15:05
@sjrd
Copy link
Member Author

sjrd commented Nov 23, 2020

Approved during the meeting.

@sjrd sjrd merged commit e0dae95 into scala:master Nov 23, 2020
@sjrd sjrd deleted the deprecate-scala-deriving-helpers branch November 23, 2020 16:02
@Kordyjan Kordyjan modified the milestones: 3.0.0-M2, 3.0.0 Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants