Support Mirror for generic tuples arity > 22 #23363
Conversation
tests/pos/i15398.scala
Outdated
|
|
||
| summon[Tuple.Size[Tuple23] =:= 23] | ||
| val m = summon[scala.deriving.Mirror.Of[Tuple23]] | ||
| summon[m.MirroredLabel =:= "Tuple23"] |
There was a problem hiding this comment.
I guess this is the main bike-shedding moment - i.e. should it include arity, or just Tuple (or another variant) - i.e. case class can evolve over time to add new fields but its MirroredLabel doesn't change - so I dont think unique label is necessary.
There was a problem hiding this comment.
should also update the docs (or create an issue to) for Mirror generation here https://www.scala-lang.org/api/3.7.1/docs/docs/reference/contextual/derivation.html
There was a problem hiding this comment.
Created #23475 for the docs because it was not immediately clear to me how to resolve it.
case class can evolve over time to add new fields but its MirroredLabel doesn't change - so I dont think unique label is necessary.
For named tuples it always "NamedTuple". So based on that we should not include the arity, but should we then also drop it for arity < 22 to avoid inconsistency?
I guess this is the main bike-shedding moment - i.e. should it include arity, or just Tuple (or another variant) - i.e.
Is there so good way to make progress on this. Feel like someone just need to make a decision.
There was a problem hiding this comment.
Well we should not change existing behaviour so I would just use Tuple as the label for size >=23
There was a problem hiding this comment.
Should I read that as you are suggesting changing the PR to have that behavior instead of including the arity? Or it just a comment that we should not break existing behavior.
There was a problem hiding this comment.
but should we then also drop it for arity < 22 to avoid inconsistency?
no, because that would change the behavior. Anyway my code suggestions are what i meant :)
| else | ||
| val elemLabels = (for i <- 1 to arity yield ConstantType(Constant(s"_$i"))).toList | ||
| val mirrorRef: Type => Tree = _ => newTupleMirror(arity) | ||
| makeProductMirror(tps, elemLabels, s"Tuple$arity".toTermName, mirrorRef) |
There was a problem hiding this comment.
| makeProductMirror(tps, elemLabels, s"Tuple$arity".toTermName, mirrorRef) | |
| makeProductMirror(tps, elemLabels, defn.TupleClass.name, mirrorRef) |
There was a problem hiding this comment.
to use a cached name - another option is define tpnme.Tuple
tests/pos/i15398.scala
Outdated
|
|
||
| summon[Tuple.Size[Tuple23] =:= 23] | ||
| val m = summon[scala.deriving.Mirror.Of[Tuple23]] | ||
| summon[m.MirroredLabel =:= "Tuple23"] |
There was a problem hiding this comment.
but should we then also drop it for arity < 22 to avoid inconsistency?
no, because that would change the behavior. Anyway my code suggestions are what i meant :)
fixes #15398