Preserve externally visible names during optimization - #1277
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 3e1667ff98
ℹ️ 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 (func.isNative() || func.isBj() || func.isCompiletime() || func.isExtern() | ||
| || NamePreservation.isPreserved(func)) { |
There was a problem hiding this comment.
When an @preserveName function has no IM caller—which is the expected case for a callback invoked solely by Warcraft or other external code—ImOptimizer.removeGarbage() runs before this compressor and retains only functions reachable from main or config; Lua's RemoveGarbage likewise ignores the preservation flag. The function is therefore deleted before this check can preserve its name. Add preserved functions as reachability roots in both backend paths so the annotation can expose an external-only entry point.
AGENTS.md reference: AGENTS.md:L229-L235
Useful? React with 👍 / 👎.
| NameLink variable = de.peeeq.wurstscript.attributes.names.NameResolution | ||
| .lookupVarNoConfig(useSite, variableName, false); | ||
| if (variable != null && variable.getDef() instanceof GlobalVarDef) { | ||
| NamePreservation.preserve((GlobalVarDef) variable.getDef()); |
There was a problem hiding this comment.
When a TriggerRegisterVariableEvent call is in a scope with a local or parameter matching the target global's runtime name, this ordinary lexical lookup returns that non-global and the preservation marker is never attached; qualified emitted names for static fields similarly need not resolve as source identifiers. The old name-based protection still matched the emitted global, whereas optimization can now remove or rename it. Resolve the runtime spelling to a global AST identity without allowing local shadowing, then carry that node identity into IM metadata.
AGENTS.md reference: AGENTS.md:L217-L220
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ed1157af2
ℹ️ 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 (func.isNative() || func.isBj() || func.isCompiletime() || func.isExtern() | ||
| || NamePreservation.isPreserved(func)) { | ||
| // do not rename builtin an bj functions | ||
| continue; |
There was a problem hiding this comment.
When a preserved identifier matches a token produced by NameGenerator (for example, w, the first token), skipping it here does not prevent a later ordinary symbol from being renamed to that same identifier. With an earlier ordinary function and a later @preserveName function w, Jass name deduplication can consequently rename the preserved function to w_1, while Lua can emit colliding globals; either outcome defeats the externally visible name contract. Seed the compressor's reserved-name set with all names that must remain unchanged before assigning any tokens.
Useful? React with 👍 / 👎.
| } else if (isFixedEntryPoint(a) || NamePreservation.isPreserved(a)) { | ||
| usedNames.add(name); |
There was a problem hiding this comment.
In Lua builds without compressor-driven renaming, if a reachable ordinary function or global has the same IM spelling as a preserved function, the ordinary symbol can claim the name through uniqueName() before this lazy initializer runs. This usedNames.add(name) then silently fails, so both definitions are emitted under the same Lua global and one overwrites the other. Include preserved functions and globals in collectPredefinedNames() so ordinary symbols are uniqued around them before translation begins.
AGENTS.md reference: AGENTS.md:L229-L235
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de84a794c4
ℹ️ 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".
| private static String runtimeName(GlobalVarDef variable) { | ||
| if (variable.getParent() != null && variable.getParent().getParent() instanceof NamedScope scope) { | ||
| return runtimeName(scope) + "_" + variable.getName(); |
There was a problem hiding this comment.
When TriggerRegisterVariableEvent targets a lowered tuple component—for example, global p of tuple pair(real x, real y) via "pkg_p_x"—this computes only pkg_p. EliminateTuples later emits scalar globals such as pkg_p_x, so no source marker is attached and optimization may remove or rename the actual watched global; the previous emitted-name set handled this spelling. Resolve the component spelling to its structural source path and propagate that identity to the corresponding scalar IM variable.
AGENTS.md reference: AGENTS.md:L217-L220
Useful? React with 👍 / 👎.
| if (!a.isExtern() && !a.isBj() && !a.isNative() | ||
| && !isFixedEntryPoint(a) && !NamePreservation.isPreserved(a)) { | ||
| name = uniqueName(name); | ||
| } else if (isFixedEntryPoint(a)) { | ||
| } else if (isFixedEntryPoint(a) || NamePreservation.isPreserved(a)) { |
There was a problem hiding this comment.
When @preserveName is applied to a class/static function, this branch initially retains its IM name, but translateClass later unconditionally executes luaFunc.getFor(f).setName(uniqueName(c.getName() + "_" + f.getName())), so Lua still renames the externally invoked function while Jass skips compression. Fresh evidence beyond the earlier Lua collision comment is this later overwrite after lazy initialization; exempt preserved class functions from that final renaming and reserve their names.
AGENTS.md reference: AGENTS.md:L229-L235
Useful? React with 👍 / 👎.
| Annotation marker = Ast.Annotation(variable.getSource(), | ||
| Ast.Identifier(variable.getSource(), ANNOTATION.substring(1)), Ast.Arguments()); | ||
| variable.getModifiers().add(marker); |
There was a problem hiding this comment.
When an incremental LSP check marks a global from an unchanged compilation unit and a later edit removes the TRVE registration from another unit, this synthetic annotation remains attached to the reused AST node because nothing distinguishes or clears it. Subsequent builds therefore retain and reserve that global even though a clean build of the same sources would not, making optimized output depend on edit history; keep this metadata transient or remove synthetic markers before each validation.
AGENTS.md reference: AGENTS.md:L291-L294
Useful? React with 👍 / 👎.
| public static void preserveGlobalWithRuntimeName(WurstModel model, String runtimeName) { | ||
| model.accept(new Element.DefaultVisitor() { |
There was a problem hiding this comment.
For a project containing many TRVE registrations, every constant call invokes this method and traverses the entire WurstModel, making validation cost proportional to registrations times total AST size rather than a single pass as before. Build a runtime-name-to-global index once per validation run, or collect requested names and resolve them in one model traversal.
AGENTS.md reference: AGENTS.md:L88-L91
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1eb32b2d90
ℹ️ 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".
| @@ -488,7 +492,8 @@ private boolean isFixedEntryPoint(ImFunction function) { | |||
|
|
|||
| private void collectPredefinedNames() { | |||
| for (ImFunction function : prog.getFunctions()) { | |||
There was a problem hiding this comment.
When a class Foo has @preserveName function bar, its preserved Lua name is Foo_bar; if another class is named Foo_bar, the class-variable pass claims that name before translateClass() runs, and the preserved function is then emitted with the same global name, overwriting the sibling class table. Fresh evidence after the earlier class-function comment is that collectPredefinedNames() still scans only prog.getFunctions(), while these functions live in ImClass.getFunctions(). Include preserved class functions in this reservation pass before any Lua names are allocated.
AGENTS.md reference: AGENTS.md:L229-L235
Useful? React with 👍 / 👎.
| if (runtimeNameIndex != null) { | ||
| runtimeNameIndex.clearSyntheticMarkers(); | ||
| } |
There was a problem hiding this comment.
During incremental checking, this cleanup never sees markers from the preceding check because WurstChecker.checkProg() constructs a new WurstValidator for every invocation, so runtimeNameIndex is always null here initially. If a TRVE registration is removed while the referenced global's compilation unit is reused, its synthetic @preserveName annotation therefore remains and the optimized output continues retaining it based on edit history. Fresh evidence beyond the prior comment is the current per-check construction at WurstChecker.java:63; track or clear synthetic markers outside the validator instance.
AGENTS.md reference: AGENTS.md:L291-L294
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 781b39da46
ℹ️ 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 (ImFunction f : ImHelper.calculateFunctionsOfProg(prog)) { | ||
| if (f.getName().equals("main") | ||
| || f.getName().equals("config")) { | ||
| || f.getName().equals("config") | ||
| || NamePreservation.isPreserved(f)) { | ||
| visitFunction(f, used); |
There was a problem hiding this comment.
When @preserveName is applied to a static class function whose body does not otherwise reference its declaring class, rooting the function here only adds it to used.functions. Unlike an instance function, it has no this parameter whose type would retain the owning ImClass, so the class is removed at line 124 and the preserved function disappears with it from Lua output. Retain the function's owning class as part of this root traversal.
AGENTS.md reference: AGENTS.md:L229-L235
Useful? React with 👍 / 👎.
| NamePreservation.clearSyntheticMarkers(prog); | ||
| runtimeNameIndex = NamePreservation.indexGlobals(prog); | ||
|
|
||
| lightValidation(toCheck); |
There was a problem hiding this comment.
During a partial LSP check where an unrelated compilation unit changes but a TRVE call remains in an unchanged unit, this clears synthetic markers across the entire model while lightValidation(toCheck) only revisits the selected units. The unchanged registration therefore does not recreate its marker, and translating that current model can remove or rename the watched global even though a clean check of the same sources preserves it. Either recompute TRVE preservation from the full model or limit cleanup to registrations that are actually reprocessed.
AGENTS.md reference: AGENTS.md:L291-L294
Useful? React with 👍 / 👎.
|
@codex review |
9a4887f to
f578b88
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f578b88008
ℹ️ 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".
| && !isFixedEntryPoint(a) && !NamePreservation.isPreserved(a)) { | ||
| name = uniqueName(name); |
There was a problem hiding this comment.
When a valid Wurst function uses a Lua-only keyword such as goto, adding @preserveName takes this branch and skips uniqueName(), which is the only path through LuaIdentifiers.toIdentifier; the Lua AST consequently emits function goto(...), which Lua 5.3 rejects. Jass accepts this identifier, so either emit such preserved globals through syntax that supports arbitrary keys or diagnose names that cannot be represented in Lua.
AGENTS.md reference: AGENTS.md:L229-L235
Useful? React with 👍 / 👎.
| if (NamePreservation.isPreserveAnnotation(annotation.getAnnotationType())) { | ||
| flags.add(PRESERVE_NAME); |
There was a problem hiding this comment.
When two legal top-level overloads with the same source name both carry @preserveName, this assigns PRESERVE_NAME to both IM functions. Lua then emits both definitions under the same global name, so the latter overwrites the former and calls to one overload execute the wrong body; Jass instead suffixes one in makeNamesUnique, violating its preservation contract. Diagnose multiple preserved functions sharing a runtime name rather than marking all of them preservable.
AGENTS.md reference: AGENTS.md:L229-L235
Useful? React with 👍 / 👎.
f578b88 to
fd7d4b4
Compare
1 participant
Summary
@preserveNamesupport for functions when-optcompresses identifiers.Acceptance criteria
-opt.Checks
./gradlew test— passed on this branch (BUILD SUCCESSFUL in 5m 11s).