address copilot review on #111: robust luacParse + looser local-count check#112
Merged
Conversation
- test/.../Lua/Golden/Spec.hs:207 — relax the unflattened local-count check from `== 1` to `< 50`, robust to unrelated module/runtime-setup locals; the "too many syntax levels" error message is the real cause-discriminator (#111 (comment)) - test/.../Lua/Golden/Spec.hs:250 — luacParse now uses withSystemTempFile + a `proc "luac" ["-p", file]` argv (no hard-coded /tmp, no shell concat), so it is portable and safe against spaces / concurrent runs (#111 (comment))
There was a problem hiding this comment.
Pull request overview
Updates the Lua golden spec regression test added in #111 to be more robust and to invoke luac more safely/portably, while keeping the test’s intent (pinning the failure to Lua parser nesting) intact.
Changes:
- Relaxes the “unflattened local-count” assertion to be less brittle against unrelated codegen changes.
- Reworks
luacParseto use a unique system temp file andtyped-process’sproc(argv-based) invocation rather than a hard-coded/tmppath + shell string.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- test/.../Lua/Golden/Spec.hs — luacParse writes through the handle that withSystemTempFile provides and hFlushes it, instead of hClose-ing early and re-opening the file by path. Avoids the manual-close-of-a-bracket-managed- handle smell (and the redundant re-open, which is unportable on Windows). (#112 (comment) comments)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #111, applying both Copilot review comments on the parser-nesting reproduction test.
Spec.hs:207— the unflattened local-count assertion was== 1, which is brittle to unrelated codegen changes (e.g. if the emitter later adds a module/runtime-setuplocal). Relaxed to< 50: still far below Lua's 200-locals cap (so it corroborates "the crash isn't the locals cap"), but robust. The real cause-discriminator is unchanged —luacreportstoo many syntax levels, a distinct message from its locals error.Spec.hs:250—luacParsewrote to a hard-coded/tmp/...path and ranluacviashellstring concatenation. Switched towithSystemTempFile(unique, auto-cleaned) +proc "luac" ["-p", file](argv list, no shell), so it is portable and safe against paths with spaces and concurrent test runs.Test-only; the assertion still passes and still pins the failure to parser nesting.