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

Special case NamedTuple.From for arguments derived from Tuple #22449

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aherlihy
Copy link
Contributor

Fixes #22036

Some questions:

  • NamedTuple.From(<TupleN>) works as is, so maybe there's a better way to fully normalize/evaluate the arguments to NamedTuple.From at the start of fieldsOf, instead of special casing types derived from Tuple?
  • is there a better way to extract the expected keys (ConstantType(Constant(_<n>)) than just hard coding "_"?

val fieldTypesOpt = tupleElementTypes(arg)
fieldTypesOpt match
case Some(fieldTypes) =>
val fieldLabels = fieldTypes.zipWithIndex.map((_, i) => ConstantType(Constant(s"_$i")))
Copy link
Member

Choose a reason for hiding this comment

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

The first field of a tuple is _1 and not _0. Also to avoid materializing an intermediate collection with zipWithIndex we could do something like:

Suggested change
val fieldLabels = fieldTypes.zipWithIndex.map((_, i) => ConstantType(Constant(s"_$i")))
val fieldLabels = (for i <- 1 to fieldTypes.length yield ConstantType(Constant(s"_$i")))).toList

@smarter
Copy link
Member

smarter commented Jan 24, 2025

NamedTuple.From() works as is, so maybe there's a better way to fully normalize/evaluate the arguments to NamedTuple.From at the start of fieldsOf, instead of special casing types derived from Tuple?

TupleN works because each TupleN class is a case class. In theory I'd expect summon[Mirror.Of[type]] to be more generic, but in practice it currently only works with tuples of arity <= 22 too:

val reason = s"it reduces to a tuple with arity $arity, expected arity <= $maxArity"
(which surprises me, I thought we got rid of arbitrary limitations like that).
Mirror.Of is also less generic because it doesn't work with case classes with multiple parameter lists, whereas NamedTuple.From works (by only returning the first parameter list).

is there a better way to extract the expected keys (ConstantType(Constant()) than just hard coding ""?

I don't think so since Constant takes a String and not a Name.

@bishabosha
Copy link
Member

bishabosha commented Jan 24, 2025

(which surprises me, I thought we got rid of arbitrary limitations like that).

no one decided how it should work if there isnt an actual accessor to back up the name, for TupleXXL there are no _23 methods

@aherlihy aherlihy requested a review from smarter January 25, 2025 12:47
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.

NamedTuple.From does not reduce with match type argument
3 participants