Parse classfile generic parameters defined in supertype generic signature#26317
Parse classfile generic parameters defined in supertype generic signature#26317SolalPirelli wants to merge 3 commits into
Conversation
| @@ -0,0 +1,10 @@ | |||
| -- [E173] Reference Error: tests/neg/26312/Use_2.scala:3:16 ------------------------------------------------------------ | |||
There was a problem hiding this comment.
without this fix, there's a nopos-error "broken classfile"
| // Warning: sigToType contains nested completers which might be forced in a later run! | ||
| // So local methods need their own ctx parameters. | ||
| def sig2type(tparams: immutable.Map[Name, Symbol], skiptvs: Boolean)(using Context): Type = { | ||
| def sig2type(tparams: immutable.Map[Name, Symbol], skiptvs: Boolean, isParent: Boolean = false)(using Context): Type = { |
There was a problem hiding this comment.
I was just going to complain that the unfortunate thing about moving the definition of var tparams is that now var tparams is slightly more likely to be confused with tparams parameter of the methods sig2type and sig2typeBounds. Just after that I asked Claude for some help with the review and it spotted a different problem:
Consider this Java:
////////
public class Wrapper<A, B> { }
class Outer {
public <Oth> Wrapper<Oth, Oth> make() {
return new Wrapper<Oth, Oth>() { }; // anonymous class Outer$1
}
}
////////
Outer$1's classfile signature would be something like:
LWrapper<TOth;TOth;>;
When the parser hits the 'L' case for Wrapper and enters processTypeArgs, it loops over the two type arguments calling sig2type(tparams, skiptvs, isParent) for each:
// First iteration: index at TOth;
sig2type(...) → tparams.get("Oth") == None → creates symbol S1, adds to classTParams
// Second iteration: index at TOth; again
sig2type(...) → tparams.get("Oth") == None ← still None, tparams unchanged
→ creates symbol S2, enters S2 into owner.asClass again
Both S1 and S2 get entered into the owner class's scope under the same name Oth. Whether this actually causes a problem depends on how owner.asClass.enter handles duplicate names — it may silently overwrite or accumulate both, potentially confusing later type-checking of that class.
Unless I missed something, getting rid of tparams parameter in favour of referencing the tparams variable directly combined with updating tparams as well when classTParams is updated with a new symbol below should solve both problems a the same time.
Also, probably it would be worth to add a test for handling such a duplicate type param reference
There was a problem hiding this comment.
Thanks, I forgot about that.
I tried refactoring the tparams stuff but it's quite confusing and a simple attempt causes a couple tests to fail with clearly generic type parameters related stuff. What exactly the two tparams are supposed to mean isn't obvious.
So I added an orElse(classTParams.get(n)) here instead, which fixes the problem without broader repercussions.
4d42ef9 to
826b0e2
Compare
Fixes #26312
How much have you relied on LLM-based tools in this contribution?
Not at all
How was the solution tested?
New automated tests (including the issue's reproducer, if applicable)