Keep default getters next to the original definition #5965
Conversation
While this was the intention, this never worked for synthetic methods with defaults, like case class factory methods. Also fix the matching logic for constructors.
|
I wonder if there's a useful test? I tweaked something similar once, I'll have to look if there was a test for it. I bet there was no test. We don't need no stinkin' tests. |
|
Good point, @som-snytt. The bug report has a repro, I have to dive into the bytecode test infrastructure, maybe there's something I could use. |
| @@ -3187,7 +3187,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper | |||
| def matches(stat: Tree, synt: Tree) = (stat, synt) match { | |||
| // synt is default arg for stat | |||
| case (DefDef(_, statName, _, _, _, _), DefDef(mods, syntName, _, _, _, _)) => | |||
| mods.hasDefault && syntName.toString.startsWith(statName.toString) | |||
| mods.hasDefault && syntName.decodedName.startsWith(statName) | |||
There was a problem hiding this comment.
interestingly this is the line that fixes scala/bug#10343
There was a problem hiding this comment.
Indeed, I just wanted to have a full stable order even when apply has default values (or copy).. so I went ahead with the sort as well ;-)
|
@dragos I pushed a test case directly to your branch (muhaha) |
| assertEquals(pm.methods.asScala.map(_.name).toList, | ||
| // after typer, `"$lessinit$greater$default$1"` is next to `<init>`, but the constructor phase | ||
| // and code gen change module constructors around. the second `apply` is a bridge, created in erasure. | ||
| List("<clinit>", "$lessinit$greater$default$1", "toString", "apply", "apply$default$1", "unapply", "readResolve", "apply", "<init>")) |
There was a problem hiding this comment.
Side note: when creating the names of lambda impl methods corresponding to lambdas in constructors, we opted for "new$" rather than "$lessinit$greater" (taking a leaf from the book of javac)
There was a problem hiding this comment.
I think this is just using the method name as a prefix for default getters, and constructors should still be called <init> AFAIK. It's probably simpler not to add a different naming convention for default getters for constructor parameters, although I agree the method name looks terrible.
|
@lrytz was lightning fast to come up with a test case, and now I learned about bytecode tests :) Simpler than I thought! Thanks for the quick turnaround! |
While this was the intention, this never worked for synthetic methods
with defaults, like case class factory methods. Also fix the matching
logic for constructors.
Fix scala/bug#10343