Fix duplicate requirements across type class bounds - #1228
Conversation
Two bounds may require the same operation. Bounds are ordered and an earlier one wins, which findRequiredMethod already documented, but member lookup offered every match so each call to a shared operation was rejected as ambiguous. It now stops at the first bound that supplies the name. classNeedsSpecialization memoised an answer computed under the caller's visited set, so a query made while one of the class's own functions was already on the path recorded a negative result that then stood for every later query. Whether a class dispatches on a bound is a property of the class, so it is now computed and cached as one. A dispatch left in a function that has a specialization cannot run, because every reachable call was rewritten to that specialization. This backend keeps generics rather than removing them, so those originals are still translated and the dispatch would reach a backend with no way to express it; it is replaced by a default value. A dispatch in a function that was never specialized would run, and is now reported plainly instead of failing as an unimplemented backend case.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ee147e71a
ℹ️ 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".
| if (found) { | ||
| return; |
There was a problem hiding this comment.
When an earlier bound declares show(Q) and a later bound declares the distinct overload show(int, Q), this name-only early return prevents the later candidate from entering overload resolution, so a valid Q.show(1, x) is rejected as having too many arguments. Earlier bounds should win only when requirements have equivalent signatures; differently shaped overloads from later bounds must remain available, with a regression test covering this multi-bound case.
AGENTS.md reference: AGENTS.md:L62-L64
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and it was a regression from my own fix in this PR — I applied the preference by name, which is too coarse. Fixed in 4124bb8.
Earlier bounds now win only where the requirements have the same shape, which is the case where offering both really would make every call ambiguous. Candidates are gathered from every bound and a later one is dropped only when an earlier bound already supplied the same parameter types, so a differently shaped overload stays available and overload resolution chooses between them as usual.
Regression test overloadFromLaterBoundStaysAvailable covers your case directly: First.show(Q) alongside Second.show(Q) and Second.show(int, Q), asserting that the one argument call resolves to the earlier bound and the two argument call to the later one.
Preferring the earlier bound was applied by name, so a later bound declaring a distinct overload of that name never reached overload resolution and a valid call to it was rejected as having too many arguments. Earlier bounds should only win where the requirements have the same shape, since that is the case where offering both would make every call ambiguous. Candidates are now gathered from every bound and a later one is dropped only when an earlier bound already supplied the same parameter types.
|
@codex review Fixed in 4124bb8, and it was a regression from this PR's own change: preferring the earlier bound was applied by name, which also hid genuinely different overloads. Earlier bounds now win only over requirements of the same shape, which is the case where offering both would make every call ambiguous. Candidates come from every bound, and a later one is dropped only when an earlier bound already supplied the same parameter types.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4124bb8fd3
ℹ️ 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".
| throw new CompileError(dispatch.attrTrace().attrSource(), | ||
| "Type class dispatch of " + dispatch.getTypeClassFunc().getName() | ||
| + " could not be resolved for this target: the concrete type is not available" | ||
| + " where it is used."); |
There was a problem hiding this comment.
When a bounded generic function or class method is declared but never called, its owner has no row in specializedFunctions, so this throws even though the dispatch is unreachable; settleRemainingDispatches() runs before optimizer.removeGarbage(), which would otherwise delete that function. I reproduced this with an unused function unused<Q: Show>(Q x) returns string during Lua compilation: the parent revision accepts it, while this change reports that the concrete type is unavailable. Determine reachability or remove garbage before rejecting unresolved dispatches so unused generic APIs remain valid on Lua as they are on Jass.
AGENTS.md reference: AGENTS.md:L215-L221
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 13eb5f5, another regression from this PR rather than a pre-existing one. Declaring a bounded generic and never calling it is entirely reasonable, and my settle pass rejected it because it could not distinguish unreachable from unresolvable.
Rather than compute reachability there, I stopped deciding it. The pass now only neutralises dispatches it can prove dead, which is those in a function that does have a specialization, since every reachable call was rewritten to it. Everything else is left to the passes that already decide reachability, and garbage removal deletes the unused generic as before.
A dispatch which survives that far and reaches the Lua backend is now reported there rather than failing as an unimplemented case. That is the point where it is known to be both reachable and unresolvable, so the message is worth something.
Tests: unusedBoundedGenericFunctionLua is your repro; I added unusedBoundedGenericClassLua and unusedBoundedGenericFunction alongside it, since an unused generic class and the Jass path are the neighbouring cases and my last two fixes here were both too narrow.
Rejecting a dispatch whose function had no specialization also rejected a bounded generic that is merely declared and never called. Nothing has to supply a concrete type for such a function, and garbage removal deletes it later, so the rejection made an unused generic API fail to compile on Lua while remaining fine on Jass. Only the dispatches this pass can prove dead are neutralised now: those in a function which does have a specialization, since every reachable call was rewritten to it. Everything else is left for the passes that already decide reachability. A dispatch which survives that far and reaches the Lua backend is reported there instead of failing as an unimplemented case. By then it is known to be both reachable and unresolvable, which is exactly when saying so is useful.
|
@codex review Fixed in 13eb5f5. Correct on both counts: it was unreachable rather than unresolvable, and it was a regression introduced by this PR. I removed the judgement rather than improving it. The settle pass now only neutralises dispatches it can prove dead — those in a function that has a specialization, since every reachable call was rewritten to it — and leaves everything else to the passes that already determine reachability, so garbage removal deletes an unused generic as it did before. A dispatch that survives to the Lua backend is reported there, which is the point at which it is known to be both reachable and unresolvable. Tests: your repro plus the two neighbouring cases, an unused bounded generic class and the same declaration on Jass. |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
|
Full suite on 13eb5f5: 1634 tests, 0 failures. |
1 participant
Three fixes to the type class bounds landed in #1226, found by review after that PR had already been merged.
Duplicate requirement across bounds
Two bounds may require the same operation:
TypeClassConstraints.findRequiredMethodalready defined earlier bounds as winning, but member lookup offered every match, so each call to a shared operation was rejected as ambiguous and the combination was unusable. Lookup now stops at the first bound that supplies the name, applying the rule that was already written down.A specialization memo that cached a context-dependent answer
classNeedsSpecializationthreaded the caller's visited set into the computation and then memoised the result. A query made while one of the class's own functions was already on the visited path therefore recorded a negative result, which then stood for every later query and left the class unspecialized.Whether a class dispatches on a bound is a property of the class, not of the path that asked, so it is now computed that way and safely cached.
Leftover dispatches settled rather than reaching the backend
A dispatch left in a function which has a specialization cannot run: every reachable call was rewritten to that specialization. This backend keeps generics rather than removing them wholesale, so such originals are still translated, and the dispatch reached a backend with no way to express it, surfacing as
Error: not implemented. Those are replaced by a default value.A dispatch in a function that was never specialized is a different case, since it would run with no concrete type available. That is now reported as a compiler error naming the requirement, instead of an unimplemented backend case.
Known gaps, unchanged by this PR
Dispatch inside a bounded generic class's constructor still fails on Lua, and dispatch inside a closure nested in a bounded generic fails on both targets. Both now produce the clear error above rather than an unimplemented backend case. The cause is visible in the specialization fixed point: specializing the constructor helper does not make its own call to the constructor visible to the next collection round, so that call is never re-examined with concrete arguments. Fixing it belongs with consolidating the type argument binding, which is currently spread across the interpreter and the specializer and has been the source of most of the review findings on #1226.
Testing
TypeClassTests45, including the duplicate requirement case as a regression test.