Uh oh!
There was an error while loading. Please reload this page.
Find a type variable by what it was copied from, not by its name - #1244
Conversation
Three places copy a function's or a class's type variables and then empty the copy's list, which leaves every reference inside the copy pointing at a node with no owner at all. Comparing names was not compensating for the occasional duplicate the note here described: it was the only thing holding those references together, and removing it broke most of the generics tests. Recording each copy against what it was made from, before the list is emptied, replaces it. Name matching is gone from indexOfTypeVar and from inheritTypeClassBinding through it, so two parameters which merely share a name can no longer be taken for one where dispatch is resolved. The interpreter's lookup still matches names. The record lives on the ImTranslator and the interpreter is handed a program rather than the translation which produced it; the comment there says so rather than leaving it looking like an oversight.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:0192629f91
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * It passes without the canonical type variable change as well as with it, so it does not pin | ||
| * that change — the lookups which used to compare names are not reached with both parameters in | ||
| * one list here, and a program which does reach them that way was not found. It is kept for the | ||
| * language case rather than for the fix. |
There was a problem hiding this comment.
As this comment acknowledges, the added test passes before the fix: convert declares an unbounded T and never dispatches on it, so indexOfTypeVar never has to distinguish the method parameter from the class parameter. Consequently, omitting or breaking any of the new origin registrations would leave the suite green. Make the method parameter bounded, dispatch through it using a type-class implementation distinct from the class parameter's implementation, and assert the selected result so this bug fix has the required failing repro.
AGENTS.md reference: AGENTS.md:L62-L64
Useful? React with 👍 / 👎.
The earlier test gave a class and a method a same-named parameter and left the method's unbounded, so nothing ever dispatched on it and the lookup was never asked to tell the two apart. It passed either way, which I said in the pull request rather than fixing. Bounding the method's parameter and giving it a different instance is what makes both reach the same lookup. The class is built over int, whose instance doubles; the method is called with string, whose instance answers 7. Without the change the string is dispatched through the int instance and the interpreter dies casting it to a number, so the bug was a wrong dispatch rather than the tidiness the backlog described.
Frotty
commented
Aug 17, 2026
You were right, and the recipe was the missing piece. What my version was missing: the method's parameter was unbounded, so nothing ever dispatched on it and With the change, The string argument is dispatched through the int instance, which does I have also corrected the item's note, which claimed a same-named parameter test passes either way. That was true of the weak version and misleading as a general statement; it now records that the bound and the second instance are what bring the two parameters into one lookup. Full suite green, 7m48s. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:0dfd6b91a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @Test | ||
| public void aBoundedMethodParameterMayShareTheClassParameterName() { | ||
| testAssertOkLines(true, |
There was a problem hiding this comment.
testAssertOkLines(true, ...) exercises the pre-transform interpreter and Jass pipeline, but not transformGenericNewOnly, where Lua consumes the changed origin tracking through a different specialization path. Please run this same bounded class/method-name collision with test().testLua(true).executeProg() so a Lua-only wrong-instance regression cannot pass unnoticed; the repository explicitly requires compiler features to be validated on both backends.
AGENTS.md reference: AGENTS.md:L215-L221
Useful? React with 👍 / 👎.
Asked for Lua coverage of the corrected lookup, and it cannot be given: the only shape which brings two same-named parameters into one lookup is a class parameter beside a method parameter, and Lua rejects that with "Generics should match class method type variables" - on master as well, so it is a gap rather than a regression. A version Lua does compile, with the second parameter on a free function, was written and then deleted: it passes with the change and without it, because the two parameters never reach one lookup that way, so it would have claimed coverage it does not provide. The rejection is pinned instead, and the gap behind it is recorded as item 25 with where to start - the arity check counts a method's own parameter against a list that transformGenericNewOnly never lifted the class's variables onto.
Frotty
commented
Aug 17, 2026
Tried, and Lua coverage of this fix cannot be given. What I found instead is a gap worth recording. The shape does not compile on Lua. The only program which brings two same-named parameters into one lookup is a class parameter beside a method parameter, and that is rejected on Lua with A Lua-compatible variant covers nothing. I wrote one with the second parameter on a free function rather than a method. It compiles and runs on Lua, and it passes with the change and without it: the two parameters never reach one lookup that way. I checked it against master before believing it, and deleted it rather than ship a test whose name claims more than it checks — the same mistake as the version you caught. So the Lua behaviour is pinned as a rejection, The gap is backlog item 25, with where to start: the message comes from the arity check between a call's generics and the callee's type variables. On the Jass path a method of a generic class has the class's variables lifted onto it; The interpreter repro from the previous round is unchanged and still fails without the fix, with the Full suite green, 8m. @codex review |
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Uh oh!
There was an error while loading. Please reload this page.
EliminateGenericsfound a type variable's position by comparing names when identity failed. The backlog called this a name-tolerant lookup compensating for several nodes standing for one source parameter, and called removing it mechanical. Neither was quite right.What it actually was
Removing the name comparison broke most of the generics tests, not an edge case — so identity never held on the common path. Tracing which nodes failed to match, the target was always detached: a type variable belonging to nothing.
Three places copy a function's or a class's type variables and then empty the copy's list:
specializeFunctionspecializeClasscopyWithRefscopies the type variables along with the body, so the references inside the copy point at the new nodes; emptying the list then leaves those nodes with no owner. The name comparison was not compensating for the occasional duplicate — it was the only thing holding those references together.The change
Each copy is recorded against what it was made from, before the list is emptied, and the lookup compares those. Same shape as the field origin map in #1239, and for the same reason: the relationship is known where the copy is made and nowhere afterwards.
After pairing all three sites, no lookup falls back to a name, so the comparison is gone from
indexOfTypeVarand frominheritTypeClassBindingthrough it. Two parameters which merely share a name can no longer be taken for one where dispatch is resolved.What is left
ProgramState.getCurrentTypeArgumentstill matches on names. The record lives on theImTranslator, and the interpreter is handed a program rather than the translation which produced it, so reaching it means threading the translator through the interpreter — four construction sites, three of them tests, but an API change all the same. The comment there now says that, rather than leaving it looking like an oversight, and backlog item 10 carries what remains.On the test
aMethodTypeParameterMayShareTheClassParameterNamecovers a class and a method declaring a parameter of the same name. It passes without this change as well as with it, so it does not pin the fix — the lookups are not reached with both parameters in one list by that shape, and I did not find a program which reaches them that way. It is kept for the language case, and its comment says as much. The change rests on removing the name comparison and on the suite showing nothing depended on it.Full suite green, 7m41s.