Repository navigation
Fix the remaining audit bugs and close the test gaps - #1421
Conversation
…clear what used a removed or added unit (cherry picked from commit d3247f46edd8e7276654fec7c4594cca4b479743)
… CLI check gate (cherry picked from commit ef10435588e8f340eb408441d8248dc4a5776d2a)
(cherry picked from commit f6139e934ade695bd8f93499f4c5395dd4a09d55)
(cherry picked from commit 62d7c22dc10fcbb2cfd18e031877c20b95aaa7ee)
(cherry picked from commit f9895fce93a1ce56f2604fe898316acfb373a75c)
…ip and the liveness join (cherry picked from commit e852ff6fc6c3706fa500fdd8e75b77cc8da33c39)
(cherry picked from commit 95f58f9d4caa77541153636703e4affc355d1fef)
…nts only it reads (cherry picked from commit 1f06d76c926fd6f5e468a86c57d2da35f9acb543)
…packages (cherry picked from commit 9c1ac52b2a65efe7e89ff383543bf0ae5074d916)
…ide the interface, as on Jass Through the interface, Jass and the interpreter never take the method a class inherits from a superclass which does not implement the interface; Lua chose between the two by class name. Calls through the superclass's method get a slot of their own where one class would need both answers under one key. (cherry picked from commit e95142cddc9fafcdbb9348290a47b5fdcfc32f3b)
Since a class's own method is told by its class, it decides no binding: the Lua of LuaBackendAuditTests, LuaTranslationTests, DeterministicChecks, InterfaceTests and ClassesTests is byte-identical without it. (cherry picked from commit 04056283c24b4632fc907a4ce5444c165403fb7c)
Six OptimizerTests compared with Player(n) in the second condition, a native call, which the side-effect check refuses to move a statement across whatever the analysis says: they passed with an analysis which reports nothing. The native is read into a local first now, so each fails when the analysis misses its edge. Nine unit tests pin the edges which no test of the analysis caught when removed: the return value's tree parent, an exit under a local branch, vararg arguments and loop variables, every assigned left side, method receivers and implementations, the control edge into a return, and the stop at a nested loop. (cherry picked from commit ddc1e101f97c4107d875946037f59971ba2cae3a)
No test inlined an early return next to break or continue, a return-free loop after a returning one, a switch in a loop, a for-in loop closing before the return or a tuple return. Each callee has an @noinline twin with the same body; the program fails unless both give the same results and the same trace of effects. (cherry picked from commit fc8fa5d3fbc4eaba34c9c194692308f8049d3a64)
…tial build The first case waits for a completion which comes after the server's initial build; on a cold JVM it took 8 s of the 10 s limit and timed out once in a run of many test classes. The limit only guards against a hang.
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. |
…erits C extends Base implements Omega, with Omega's m abstract and Base, which is not an Omega, declaring m: the Jass dispatch over Omega's implementations follows the classes below Omega and takes the method declared in each, so C kept the abstract method and landed in another implementor's branch, and the interpreter called the abstract method. C gets a method of its own with Base's implementation now.
|
@codex review |
![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: 0616fa4518
ℹ️ 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".
The method a class gets of its own for an abstract interface method it inherits had no sub-methods, so a deeper override was reachable only through the interface method, not from the bridge (AGENTS.md section 8). Once every class is translated, the bridge takes the overrides of the inherited method below its class. Generic classes keep what they did: their methods are specialised with functions the class owns, and a generic class with an override in a non-generic subclass does not compile yet.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ffb32a5f0
ℹ️ 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 put what a dead assignment's value does in a statement expression, so the passes after it saw non-flat IM: a call (as before) and now a division which may stop the thread inside an expression. The effects become the statements a flatten makes of them; when that makes a local (the division), the liveness is computed again before the merge.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8baa23997
ℹ️ 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".
…ives it C extends Base implements Abstract, Default, with Abstract's m abstract and Default's m a default: a default beats an inherited method, and Lua binds one implementation for C to the calls through both interfaces, so with C's own method running Base's m a call through Default ran Base's m on Lua and the default on Jass. C's own method runs the default now, so every backend runs it through either interface.
…makes Merging two locals turns the copy between them into an assignment of the local to itself. The next run of the merger removed those, but nothing after the last run did: castle fight's optimised Lua had 103, zombie defense's 106. The merge removes them now.
|
@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
Fixes the high and medium bugs left from the audit of the 7-9 October master commits, and closes the test gaps it found. None of the bugs came from those commits; each predates them. Every fix starts from a test which fails on master and passes here.
Bugs fixed
Lua: old-generics values bound to an enum or int (
imtranslation/ExprTranslation)colors.get() castTo inton aBox<Color>gavemath.minintegerinstead of 0: the cast's from-type was the erasedImAnyType, so Lua applied the generic-storage encoding. The cast now converts from the type argument's type, and the value is normalised as it leaves erased storage.nilon Lua, not the first constant:wrapLuamatched type names andisPrimitiveTypeleft enums out. Enums are normalised like ints now.Language server: the model went stale before the reconcile (
ModelManagerImpl).wurst/.jurstfile is visible everywhere, but only.jfiles were treated so: editing or deleting such a file checked none of its users. Units with Jass declarations count as global now (Changes.jassNamesChangedcarries it to the reconcile).Optimiser: an unread division which may stop the thread was dropped (
Flatten,ImOptimizer,LocalMerger)int unused = 10 div dlost the division in every variant: the effect was kept but flattened to nothing, as Jass and Lua have no expression statement. Such an operator becomes an assignment to a local which the garbage removal and the local merger keep (one shared rule,Flatten.mayStopTheThread); the round bound still holds.Local optimisations: an uninitialised read pjass rejects (
LocalMerger,ControlFlowGraph)return a, removed every write toaand kept the read. The code no path reaches is removed first (ControlFlowGraph.unreachableStatements). Functions ending inendifwithout a final return are already in every map (stdlibLoglevel_getTag).Lua dispatch: an interface default against a method inherited from outside the interface (
LuaTranslator)class C extends Base implements Omegacalled throughOmegagave Base'smon Lua and Omega's default on Jass and in the interpreter, only because "Base" sorts before "Omega". Lua follows the rule Jass and the interpreter apply (a default passes over an implementation whose class is not below the interface), and where the call through the superclass reaches another implementation it gets its own slot. TheNoOpStatename tie-breaker is gone: the Lua of all affected test classes is byte-identical without it.Jass and interpreter: an abstract interface method implemented by an inherited method (
InterfaceTranslator)C extends Base implements Omega, with Omega'smabstract and Base, which is not an Omega, declaringm: the Jass dispatch over Omega's implementations follows the classes below Omega and takes the method declared in each, so C kept the abstract method and landed in another implementor's branch (3 instead of 1); the interpreter called the abstract method. C gets a method of its own with Base's implementation, which its subclasses inherit, linked to the overrides below C (filled in once every class is translated). Where another interface gives C a default for m, that method runs the default, as a call through that interface does on every backend. Lua was already right for the plain case. Generic classes keep their previous behaviour (see below).Local merger: assignments of a local to itself (
LocalMerger)x = x. The next run of the merger removed those, but nothing after the last run did: castle fight's optimised Lua had 103, zombie defense's 106. The merge removes them now (0 left). The later passes then see the code without those no-ops, which moves a few of their decisions: castle fight has 43 fewer assignments and 50 more local declarations (adjacentlocal x = nilcompile to one LOADNIL in Lua), zombie defense 49 fewer and 35 more; both scripts are smaller.Test gaps closed
OptimizerTestscompared withPlayer(n), a native call, so they passed with an analysis which reports nothing. The native is read into a local now: all six fail with the analysis switched off. NineLocalPlayerContextAnalyzerTestspin the edges no test caught when removed.@noinlinetwins..j/.jurstfiles parsed ahead, a legacy compilation not certifying the model, the CLI failing on a type error from its single check.LanguageWorkerTest: the wait behind the initial build is a 60 s hang guard (it took 8 s of 10 cold).Checks
x = xand the later passes' decisions described above.Found, not fixed here
EliminateGenerics.adaptSubmethods). Because of it, generic classes do not get the interface bridge above.A implements I(overriding the default),B extends A,C extends B implements I: interpreter 1, Jass 2 (Lua 2).