Sanitise Lua method names where they are assigned - #1230
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.
![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: cc994da12d
ℹ️ 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".
| for (int i = 0; i < name.length(); i++) { | ||
| if (!isIdentifierPart(name.charAt(i))) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
When a generated dispatch alias is a Lua keyword—for example, a closure implementing a Wurst interface method named goto, repeat, or nil—this character-only check returns true. Consequently toIdentifier preserves the keyword and the new assertion also accepts it, allowing the printer to emit invalid method/table-key syntax. Include the existing LuaReservedNames.LUA_KEYWORDS set in this validation and map reserved words to a safe identifier.
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.
|
Addressed: keywords are now rejected by Confirmed the path first — it is reachable. Method names were already safe, because The sharper half of your point was the second one: the assertion added in this PR accepted the name, and catching this before the syntax check is the whole reason it exists. It rejects keywords now. Mapping is by trailing underscore 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. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7871731728
ℹ️ 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".
| active: true | ||
| iteration: 1 | ||
| session_id: 0413fc86-e9a2-4954-bc47-1620ed81d95e | ||
| max_iterations: 0 |
There was a problem hiding this comment.
Checking in this session-specific file with active: true, max_iterations: 0, a stale session ID, and a developer-local path/branch can cause Ralph/Claude loop tooling in another checkout to inherit an unbounded automation session unrelated to this compiler change. Remove the file and add it to the ignore rules so repository users do not activate another developer's local loop state.
AGENTS.md reference: AGENTS.md:L64-L64
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.
|
Removed, and you were right that it should never have been there. Both For the record on how they got in: neither file was touched by any commit in this work. They arrived with @codex review |
|
Codex Review: Didn't find any major issues. Delightful! 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
A method name becomes a Lua table key, so it has to be an identifier. A method specialised with two type arguments was named after them, commas included, and emitted
which is valid Lua assigning to two targets — so the syntax check passed and the wrong thing happened quietly. A tuple argument produced characters
luacrejects outright.What changed
LuaDispatchPreparation.normalizeMethodNamesis 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 everyLuaMethodname through the same function, so call sites and class tables keep agreeing. Lua's identifier rule now has one home,LuaIdentifiers.Why the assertion is part of this
The luac check never caught this, because the broken output parses. So the check is on the names themselves: every emitted function, method, variable, field and call-by-name must match
[A-Za-z_][A-Za-z0-9_]*, verified for everytestLuacompile. That is what stops this class of bug returning rather than the fix itself.Repro that failed before and passes after:
FastHashMapTests.tupleKeyLua.Full test suite green.