Skip to content

Skip bypassing unapply for scala 2 case classes to allow for single-element named tuple in unapply #23603

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 5 commits into from
Jul 30, 2025

Conversation

aherlihy
Copy link
Contributor

Fixes #23131

From my understanding, for Scala 2 case classes, the compiler will generate an unapply that returns an Option[(...)] which contains the fields of the class as a tuple and this first case:

if (isSyntheticScala2Unapply(unapp.symbol) && caseAccessors.length == args.length)
def tupleSel(sym: Symbol) =
// If scrutinee is a named tuple, cast to underlying tuple, so that we can
// continue to select with _1, _2, ...
ref(scrutinee).ensureConforms(scrutinee.info.stripNamedTuple).select(sym)
val isGenericTuple = defn.isTupleClass(caseClass) &&
!defn.isTupleNType(tree.tpe match { case tp: OrType => tp.join case tp => tp }) // widen even hard unions, to see if it's a union of tuples
val components =
if isGenericTuple then caseAccessors.indices.toList.map(tupleApp(_, ref(scrutinee)))
else caseAccessors.map(tupleSel)
matchArgsPlan(components, args, onSuccess)
is to bypass this unapply and directly select the case class parameters for performance. However that causes the fixes added by this PR: #22953 to be skipped since the check comes first. I think the simplest fix is to skip the Scala 2 special case for a single-element selector, but not 100% sure.

@aherlihy aherlihy requested a review from smarter July 25, 2025 18:48
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

I think this is fine as a workaround to avoid the runtime crash now, but it really needs a FIXME comment explaining the context and calling for a refactor of the code.

// enable selecting the field. See i23131.scala for test cases.
val wasSingleNamedArgForNamedTuple =
args.length == 1 && args.head.removeAttachment(FirstTransform.WasNamedArg).isDefined &&
isGetMatch(unappType) && unapp.select(nme.get, _.info.isParameterless).tpe.widenDealias.isNamedTupleType
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smarter I added a more precise check but I'm not really sure this is the best way to do it

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine, since .select(nme.get, _.info.isParameterless) appears both here and below in val get = ..., it could be factored out in a method getOfGetMatch or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can add a method for that.

@WojciechMazur WojciechMazur added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jul 29, 2025
@WojciechMazur WojciechMazur added this to the 3.7.3 milestone Jul 29, 2025
@aherlihy aherlihy enabled auto-merge July 30, 2025 08:47
@aherlihy aherlihy merged commit e0aca2d into scala:main Jul 30, 2025
44 of 45 checks passed
@aherlihy aherlihy deleted the i23131 branch July 30, 2025 14:09
WojciechMazur added a commit that referenced this pull request Aug 1, 2025
…r single-element named tuple in unapply" to 3.7.3 (#23645)

Backports #23603 to the 3.7.3-RC1.

PR submitted by the release tooling.
[skip ci]
@WojciechMazur WojciechMazur added backport:done This PR was successfully backported. and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Aug 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:done This PR was successfully backported.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClassCastException when using named pattern one single-component named tuple
4 participants