Take a closure's instantiation from its construction on Lua - #1233
Conversation
Exercises type class bounds through the container they were added for. Six of the seven cases pass on both backends with no compiler change: int keys, tuple keys on Jass, a user class key, two specialisations coexisting, and two instances of one specialisation. tupleKeyLua fails and is a real, pre-existing backend bug. Method names become Lua table keys, but luaMethod.initFor passes the name through raw while every sibling (luaVar, luaFunc, luaClassVar) sanitises via uniqueName. Names are valid identifiers in ordinary code, so nothing hit it until a class method was specialised for Lua with more than one type argument: specializeMethod builds name + "_specialized_" + generics.makeName(), and makeName joins arguments with ", ". Two simple arguments give "get_specialized_integer, integer", which emits "Class.get_specialized_integer, integer = impl" -- valid Lua that assigns to two targets and quietly writes a junk global. A tuple argument gives "⦅integer, integer⦆" and fails the syntax check outright.
Method names become Lua table keys, so they must be identifiers. A method specialised with two type arguments was named after them, commas included, and emitted `Class.get_specialized_integer, integer = impl` - valid Lua that quietly assigns to two targets; a tuple argument produced characters luac rejects outright. normalizeMethodNames is the pass that gives one name to a whole dispatch group, so it sanitises before uniquing: two names that differed only in characters Lua has no place for still get a slot each. The backend maps every slot key and every LuaMethod name through the same function, so call sites and class tables keep agreeing. Lua's identifier rule now has one home. The luac check never caught this, because the broken output parses. Assert instead on the names themselves: every emitted function, method, variable, field and call-by-name must be an identifier, checked for every testLua compile.
The execution path read the spawned process's stderr to EOF before touching stdout, and never bounded the wait. A program that fills the stdout pipe blocks writing while the harness blocks reading stderr, and the suite stops with no output, no timeout and no failing test - a stray worker JVM was still sitting on the build directory twenty minutes later. checkLuaSyntax, in the same file, already drained both pipes on their own threads and waited with a timeout. The execution path now uses the same helper, so a program that does not terminate fails its test instead of the run. Also records what building a repro turned up: a bounded generic class cannot be subclassed on either backend, and the div/mod result type asymmetry is reachable through integer literals rather than harmless.
A method claims the slots of every method it might be an override of, and sharesSemanticName accepted a match on either the declared name or the segment after the last underscore of the mangled one. For a specialised method that segment is a fragment of the type argument: get and slotFor of FastHashMap<int, int> both read as 'integer', and since specialisation gave them the same signature too, get claimed slotFor's slot and the class table bound FastHashMap_slotFor_specialized_integer__integer to FastHashMap_get_specialized. Declared names now settle it whenever both methods have one. The segment stays as the fallback for closures and bridges, which have no declaration to ask - that is the case it was there for. The emitted Lua is the only record of which implementation a slot should hold, so the test asserts it there: every slot named after a method must bind an implementation named after the same method.
remove leaves a tombstone rather than an empty slot, because a probe that stopped at one would miss keys put down beyond it. slotFor passes tombstones over when searching and returns the first one when putting, so removing and re-putting a key reuses its slot instead of lengthening the run. The probe is bounded by capacity, so a table full of tombstones cannot spin. The cost claim is asserted on the least optimised configuration, because it has to hold by construction rather than because the inliner removed it: storage is four plain Jass arrays, no hashtable native is reached for, slotFor takes nothing beyond the receiver and the key, and both requirements are direct calls rather than ExecuteFunc or a dispatch wrapper. With optimisation on, hash(key) becomes key and equals(a, key) becomes a != key. The dispatch_ functions in the output are the nullpointer check every class method gets, not type class dispatch, so the test looks at the real function underneath rather than asserting they are absent. Building the fixture also turned up a silent wrong result in the interpreter, recorded as item 16: a never-written array of a type parameter compares unequal to the type argument's default.
Lua specialises what it can reach from a concrete type, and it finds that type on calls that carry type arguments. A closure has no such call: it is reached through the interface it implements, which is not generic, so nothing at the call site says what the body dispatches on. Only the construction knows. Three pieces were missing, each of which already exists for the Jass path: the instantiation is now collected from the allocation, the member access is collected so the write that captures the environment lands on the specialised field rather than being dropped as a dead store, and the specialised methods are bound to the roots their originals were submethods of. Registering the original implementation as specialised lets the dispatch left in it settle rather than reaching a backend with no way to express it. All three are gated on the class being closure-generated, which is the case where no call names the instantiation. Ungated, the object comes from the specialised class while its methods were bound to the erased one, and every FastHashMap Lua test fails - the disagreement AGENTS.md section 9 describes. dispatchInsideClosureIsRejectedForLua pinned the old diagnostic and becomes dispatchInsideClosureLua. The language doc and changelog said Jass only.
A bound dispatched from a constructor works on Jass and is rejected on Lua.
dispatchInsideConstructor covers the working target, and
dispatchInsideConstructorIsRejectedForLua pins that the rejection is a clear
diagnostic rather than a mistranslation.
Not the same gap as the closure case, and the fix from it does not reach. A
constructor belongs to its class rather than to a generic function of its own,
so the call that runs it carries no type arguments at all: the intermediate
language has b = new_Box(21) with b typed Box<integer{show}> and new_Box still
generic. The calls inside it do carry the class's type variable, but nothing
gives the outermost one a concrete argument, and collection requires non-empty
type arguments to start. The backlog records where the instantiation could come
from instead.
![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: 00f89a609d
ℹ️ 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".
| ImClassType clazz = alloc.getClazz(); | ||
| if (clazz.getTypeArguments().isEmpty() | ||
| || typeArgumentsContainTypeVariable(clazz.getTypeArguments()) | ||
| || !isConstructionOnlyInstantiation(clazz.getClassDef())) { |
There was a problem hiding this comment.
When a bounded generic closure dispatches indirectly—such as () -> helper<Q>(x), where helper contains the ImTypeVarDispatch—isConstructionOnlyInstantiation returns false because classNeedsSpecialization only scans the closure's functions for dispatch nodes and does not follow their ImFunctionCall or ImMethodCall targets. Consequently the allocation is skipped here, the enclosing generic call is not recognized as needing specialization, and reachable Lua code retains an unresolved dispatch even though the newly documented closure behavior should work. Use the existing call-aware specialization traversal for this gate rather than requiring a dispatch directly in the closure body.
AGENTS.md reference: AGENTS.md:L324-L326
Useful? React with 👍 / 👎.
Wurst and Lua reserve different words, so a method can be declared repeat or goto and reach the backend under that name. Method names survive it, because the pass that assigns them uniques against the reserved set. A closure does not: it adds the name it implements as a dispatch alias directly, so the alias arrives as a bare keyword and is emitted as a table key. luac rejects that, so it was loud rather than wrong, but the check added alongside it accepted the name - and catching this before the syntax check is the whole point of having it. isValid now rejects keywords and toIdentifier maps them out of the way. Underscores rather than a counter, so a keyword maps to the same name wherever it is derived: call sites and class tables have to agree without consulting each other.
Two tests for the runner itself, both Lua only: run through the Jass configurations the non-terminating one would hang the interpreter instead, which is the same problem somewhere this fix does not reach. - a program that loops forever fails its own test rather than the run - a program that prints twenty thousand lines still finishes The second is the deadlock this change was for: with the streams read one after the other, the program blocks writing stdout while the runner blocks reading stderr. The timeout is overridable so these take seconds rather than a minute. Output kept for the failure message is capped. Draining still never stops, since stopping is what blocks the process, but a program that loops while printing would otherwise exhaust the worker before the timeout fires. Capping meant success could no longer be read back out of the retained text - testSuccess prints last, well past the limit - so the line is recognised while draining. Which is better regardless: whether a program succeeded no longer depends on how much of its output was kept.
slotFor says a key has nowhere to go by returning a slot below the instance's section, and get, has and remove all check for it. put did not, so a ninth distinct key wrote through the sentinel: an entry nothing can reach, a count of nine, and a negative array index on targets less forgiving than Lua. The test fills the map exactly - keys one to eight hash to the eight slots - and puts a ninth. It does not read the absent key back, because reading one of a type parameter returns a stand-in the interpreter cannot compare; that is a separate bug, fixed later in this stack.
The gate asked whether a dispatch sits in the closure itself. A closure whose body is () -> helper(x) has none - the dispatch is in helper - so the construction was skipped, and the bound reached the backend unresolved even though this is exactly the shape the change was for. It now uses the call-aware check that already exists for functions. classNeedsSpecialization is left as it was, because widening that would change what gets specialised on paths which have nothing to do with closures. Specialising the closure class then surfaced a second thing: the member access collector ran again for the copy, and a receiver still typed by the generic class cannot adapt to the specialised one, which is not a superclass of it. A class with no type variables left has nothing to select, so it is skipped.
|
Fixed, and it was reachable exactly as described. Repro: fails on the previous commit with The gate now uses the call-aware check that already exists for functions. I left Specialising the closure class then surfaced a second problem your suggestion would have hit too: the member access collector runs again for the copy, and a receiver still typed by the generic class cannot adapt to the specialised one, which is not a superclass of it — TypeClassTests, FastHashMapTests, GenericsWithTypeclassesTests, ClosureTests, LuaTranslationTests, FieldIterationTests and GenericsTests all pass. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5d9dfb2d4
ℹ️ 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".
| || typeArgumentsContainTypeVariable(memberAccess.getTypeArguments())) { | ||
| return; | ||
| } | ||
| genericsUses.add(new GenericMemberAccess(memberAccess)); |
There was a problem hiding this comment.
When a bounded generic creates an outer closure that creates an inner closure—e.g. the inner closure evaluates Q.toIndex(x)—this collector queues the inner capture of the generic outer closure. GenericMemberAccess.eliminate then tries to specialize that captured outer-receiver type with zero arguments and aborts with Rewrite generics with wrong sizes instead of compiling the valid Lua program; I reproduced this with the focused TypeClassTests harness, while the equivalent Jass path is valid. Handle the nested captured-closure type without enqueueing it with empty class arguments.
AGENTS.md reference: AGENTS.md:L215-L221
Useful? React with 👍 / 👎.
| public void dispatchInsideClosureLua() { | ||
| test().testLua(true).executeProg().lines( |
There was a problem hiding this comment.
This regression test only executes the result, so it does not verify the backend shape introduced here: that allocation uses the specialized closure class and that the Producer.produce slot binds the specialized implementation rather than the generic original. Add emitted-Lua assertions for those properties, as required for backend parity fixes involving fragile dispatch binding.
AGENTS.md reference: AGENTS.md:L250-L256
Useful? React with 👍 / 👎.
Neither of these belongs in a branch that gets merged. The loop configuration is machine-local state - an active session id, an iteration count, a path and a branch name from one checkout - and another checkout picking it up would inherit an automation session that has nothing to do with it. The brief beside it is one run's instructions, not documentation of anything. Both came in with the branch this work was based on rather than with the work. Ignored as well, so they do not come back the next time either is written.
…dispatch # Conflicts: # de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java
…ass-dispatch # Conflicts: # BACKLOG.md # de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaRunnerTests.java # de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java # de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java
A closure written inside another one reaches its captured environment through a receiver belonging to the enclosing closure, which has been specialised by then. Specialising the owner again with what is left over fails inside the rewrite, so the program came out with an error about generics of the wrong size where it previously said the bound could not be resolved. It never compiled either way, but the second of those is the one worth reading. They are kept out of the gate now, which restores what is said, and nestedClosuresInsideBoundedGenericAreRejectedForLua pins it. Making them work is a further step rather than a patch. The closure test also asserts the emitted shape rather than only the answer: the allocation names the specialised class, and that class binds the dispatched produce slot to its own implementation and not to the generic original. Running alone did not distinguish those, which is the whole thing this change alters.
|
Updated from master, both addressed. Nested closures (P1) — real, reproduced, fixed. Your repro gives Nested closures are now kept out of the gate, which restores the previous wording, and Emitted shape (P1) — taken. TypeClassTests, ClosureTests, GenericsWithTypeclassesTests, FastHashMapTests and LuaTranslationTests all pass. @codex review |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
1 participant
Stacked on #1232.
Lua specialises what it can reach from a concrete type, and it finds that type on calls carrying type arguments. A closure has no such call: it is reached through the interface it implements, which is not generic, so nothing at the call site says what the body dispatches on. Only the construction knows.
The backlog said the specialised class was built but nothing called it. That was wrong — no specialised class was being built at all, which is why this took a while to see.
What changed
Three pieces were missing, each of which already exists for the Jass path:
ImAlloc,The gate is deliberate
All three are restricted to closure-generated classes, which is the case where no call names the instantiation. Ungated, the object comes from the specialised class while its methods were bound to the erased one, and every FastHashMap Lua test fails — the disagreement AGENTS.md §9 describes. Widening this is a decision about Lua's erasure model, not a tweak, so I stopped at the claim I can defend.
dispatchInsideClosureIsRejectedForLuapinned the old diagnostic and becomesdispatchInsideClosureLua.WURST_LANGUAGE.mdandCHANGELOG.mdsaid "Jass only for now" and no longer do.Full test suite green.