Centralize source pipeline parser policy#682
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds TGocciaSourcePipeline; centralizes compatibility flags and new T* CLI option primitives; migrates parsing, timing, warnings, and source-map handling across engine, loaders, bundler, REPL, TestRunner, and BenchmarkRunner; infers ChangesCombined Change Cohort
Sequence DiagramsequenceDiagram
participant CLI_Host as CLI Host
participant SourcePipeline as TGocciaSourcePipeline
participant Compiler as Compiler/ModuleBuilder
participant Runtime as Executor/VM
CLI_Host->>SourcePipeline: Parse(source, options: Preprocessors, Compatibility, SourceType)
SourcePipeline-->>CLI_Host: ProgramNode + Warnings + Lex/Parse times + SourceMap + GeneratedLines
CLI_Host->>Compiler: Compile(ProgramNode, SourceType, Compatibility)
Compiler-->>CLI_Host: Compiled module/program
CLI_Host->>Runtime: Execute(compiled)
Runtime-->>CLI_Host: Result + coverage + timings
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
|
Contributor
Suite TimingTest Runner (interpreted: 9,798 passed; bytecode: 9,798 passed)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Test runner worker shutdown frees thread-local heaps in bulk; that shutdown reclamation is not counted as GC collections or collected objects.
Benchmarks (interpreted: 407; bytecode: 407)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Benchmark runner performs explicit between-file collections, so collection and collected-object counts can be much higher than the test runner.
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results407 benchmarks Interpreted: 🟢 397 improved · 🔴 4 regressed · 6 unchanged · avg +41.4% arraybuffer.js — Interp: 🟢 14 · avg +41.3% · Bytecode: 🔴 3, 11 unch. · avg -0.6%
arrays.js — Interp: 🟢 19 · avg +41.3% · Bytecode: 🟢 4, 🔴 1, 14 unch. · avg +2.0%
async-await.js — Interp: 🟢 6 · avg +44.6% · Bytecode: 🟢 1, 🔴 1, 4 unch. · avg +0.8%
async-generators.js — Interp: 🟢 2 · avg +48.7% · Bytecode: 🟢 2 · avg +6.6%
base64.js — Interp: 🟢 10 · avg +29.2% · Bytecode: 🟢 2, 🔴 2, 6 unch. · avg +1.0%
classes.js — Interp: 🟢 31 · avg +38.1% · Bytecode: 🟢 3, 🔴 15, 13 unch. · avg -4.8%
closures.js — Interp: 🟢 11 · avg +43.4% · Bytecode: 🟢 1, 🔴 1, 9 unch. · avg +1.6%
collections.js — Interp: 🟢 12 · avg +41.4% · Bytecode: 🟢 11, 1 unch. · avg +7.8%
csv.js — Interp: 🟢 13 · avg +43.2% · Bytecode: 🔴 9, 4 unch. · avg -3.6%
destructuring.js — Interp: 🟢 22 · avg +40.3% · Bytecode: 🟢 4, 🔴 1, 17 unch. · avg +2.3%
fibonacci.js — Interp: 🟢 8 · avg +43.4% · Bytecode: 🟢 8 · avg +9.1%
float16array.js — Interp: 🟢 32 · avg +38.3% · Bytecode: 🟢 9, 🔴 13, 10 unch. · avg -3.1%
for-of.js — Interp: 🟢 7 · avg +44.3% · Bytecode: 🟢 6, 1 unch. · avg +8.9%
generators.js — Interp: 🟢 4 · avg +31.8% · Bytecode: 🔴 3, 1 unch. · avg -5.0%
iterators.js — Interp: 🟢 42 · avg +44.3% · Bytecode: 🟢 18, 🔴 3, 21 unch. · avg +3.7%
json.js — Interp: 🟢 20 · avg +36.5% · Bytecode: 🟢 3, 17 unch. · avg +2.1%
jsx.jsx — Interp: 🟢 21 · avg +42.9% · Bytecode: 🔴 20, 1 unch. · avg -9.8%
modules.js — Interp: 🟢 9 · avg +42.5% · Bytecode: 🟢 4, 🔴 1, 4 unch. · avg +3.4%
numbers.js — Interp: 🟢 11 · avg +41.3% · Bytecode: 🟢 3, 8 unch. · avg +2.4%
objects.js — Interp: 🟢 7 · avg +43.4% · Bytecode: 🟢 1, 🔴 6 · avg -9.7%
promises.js — Interp: 🟢 12 · avg +34.0% · Bytecode: 🔴 7, 5 unch. · avg -5.7%
regexp.js — Interp: 🟢 11 · avg +27.7% · Bytecode: 🔴 3, 8 unch. · avg -2.4%
strings.js — Interp: 🟢 19 · avg +42.4% · Bytecode: 🔴 13, 6 unch. · avg -4.8%
tsv.js — Interp: 🟢 9 · avg +39.9% · Bytecode: 🟢 1, 🔴 3, 5 unch. · avg -1.6%
typed-arrays.js — Interp: 🟢 15, 🔴 4, 3 unch. · avg +23.4% · Bytecode: 🟢 4, 🔴 10, 8 unch. · avg -11.1%
uint8array-encoding.js — Interp: 🟢 15, 3 unch. · avg +41.7% · Bytecode: 🟢 3, 🔴 11, 4 unch. · avg -7.8%
weak-collections.js — Interp: 🟢 15 · avg +96.1% · Bytecode: 🟢 7, 🔴 3, 5 unch. · avg +7.1%
Deterministic profile diffDeterministic profile diff: no significant changes. Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
test262 Conformance
Areas closest to 100%
Per-test deltas (+1 / -0)Newly passing (1):
Steady-state failures are non-blocking; regressions vs the cached main baseline (lower total pass count, or any PASS → non-PASS transition) fail the conformance gate. Measured on ubuntu-latest x64, bytecode mode. Areas grouped by the first two test262 path components; minimum 25 attempted tests, areas already at 100% excluded. Δ vs main compares against the most recent cached |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/app/Goccia.CLI.Application.pas (1)
516-527:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep invalid per-file
source-typevalues from bypassing the root config.Line 516 currently falls straight back to the file-extension default when a per-file
source-typeis malformed, even if the root config already supplied a validsource-type. That skips the documentedCLI > per-file > root config > extension defaultprecedence and can silently flip a file back to script/.mjsdefaults instead of the inherited root setting.Suggested fix
if FindConfigEntry(AFileConfig, 'source-type', ValueStr) then begin NormalizedValue := LowerCase(Trim(ValueStr)); if NormalizedValue = 'module' then Exit(Goccia.Engine.stModule); if NormalizedValue = 'script' then Exit(Goccia.Engine.stScript); + if AOption.Present then + begin + WriteLn(StdErr, Format( + 'Warning: invalid per-file config value for "source-type": %s ' + + '(valid: script, module). Falling back to root config value.', + [ValueStr])); + if AOption.Matches(Goccia.CLI.Options.stModule) then + Exit(Goccia.Engine.stModule); + Exit(Goccia.Engine.stScript); + end; WriteLn(StdErr, Format( 'Warning: invalid per-file config value for "source-type": %s ' + '(valid: script, module). Falling back to file extension default.', [ValueStr])); Exit(DefaultSourceTypeForFileName(AFileName)); end;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/app/Goccia.CLI.Application.pas` around lines 516 - 527, The per-file branch treats an invalid ValueStr as a signal to immediately fall back to DefaultSourceTypeForFileName(AFileName), which bypasses any valid root config; change the logic so that when NormalizedValue is not "module" or "script" you attempt to read the root-level 'source-type' (using FindConfigEntry on the root config structure) and if that root value is valid return the corresponding Goccia.Engine.stModule/stScript, otherwise log the warning and then return DefaultSourceTypeForFileName(AFileName); use the existing symbols ValueStr, NormalizedValue, FindConfigEntry, and DefaultSourceTypeForFileName(AFileName) to locate and implement this check.
🧹 Nitpick comments (1)
scripts/build-logo.ts (1)
74-74: 💤 Low valueConsider extracting the ImageMagick floodfill command for readability.
The background-removal command on line 74 is quite long with multiple ImageMagick operations. While correct, it could be more maintainable by extracting parameters into named constants or breaking into logical steps with comments.
♻️ Example refactor for readability
+// Remove solid background: add temp border, flood-fill from corner, remove border +const fillCmd = "color 0,0 floodfill"; if (await sourceHasTransparency(source)) { await $`${magick} ${source} -resize ${traceSize} ${cutout}`; } else { - await $`${magick} ${source} -resize ${traceSize} -alpha set -bordercolor none -border 1x1 -fill none -fuzz ${opaqueBackgroundFuzz} -draw ${"color 0,0 floodfill"} -shave 1x1 ${cutout}`; + await $`${magick} ${source} + -resize ${traceSize} + -alpha set + -bordercolor none -border 1x1 + -fill none -fuzz ${opaqueBackgroundFuzz} + -draw ${fillCmd} + -shave 1x1 + ${cutout}`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build-logo.ts` at line 74, Extract the long ImageMagick invocation into a named variable or helper to improve readability: build a descriptive constant or function (e.g., buildFloodfillArgs or floodfillCommand) that assembles the sequence of parameters currently in the template literal (`${magick} ${source} -resize ${traceSize} -alpha set -bordercolor none -border 1x1 -fill none -fuzz ${opaqueBackgroundFuzz} -draw ${"color 0,0 floodfill"} -shave 1x1 ${cutout}`), then call await $`${magick} ${floodfillArgs}` (or pass the args array) from the place where the command is run; keep the same variables (magick, source, traceSize, opaqueBackgroundFuzz, cutout) and add brief comments for each logical step (resize, alpha/border setup, fuzz/draw floodfill, shave) to document intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/units/Goccia.Engine.pas`:
- Around line 1266-1305: The module-execution path assigns FLastTiming.Result
from FExecutor.EvaluateModuleBody and then calls WaitForRuntimeIdle without
protecting that value; temp-root the EvaluateModuleBody return before
WaitForRuntimeIdle (as RunModule/RunModuleInScope do) by storing the result in a
local rooted variable (or using the interpreter's temp-root API) e.g. capture
the value from EvaluateModuleBody into a temp-rooted local (TempResult), call
WaitForRuntimeIdle, then set FLastTiming.Result from TempResult and continue to
set timing fields.
- Around line 1557-1580: BodyParseResult (returned by
TGocciaSourcePipeline.ParseFunctionBodyWrapper) is an owned object that is never
freed, leaking on every Function(...) call including error paths; ensure
BodyParseResult is freed on all exit paths by taking ownership and releasing it
in a try..finally (or equivalent) around the logic that uses it (the validity
check, ThrowSyntaxError call, the call to ParseDynamicFunctionWrapper, execution
via FExecutor.ExecuteDynamicFunction and the final ProgramNode.Free), i.e., free
BodyParseResult in a finally block after you've finished using it so both
success and error paths release the object.
In `@source/units/Goccia.Evaluator.pas`:
- Around line 6367-6378: The ParseExpression calls currently pass the hard-coded
source label 'template-expression', losing original source location; update both
branches in EvaluateTemplateExpression so TGocciaSourcePipeline.ParseExpression
receives a source identifier built from the current file path and the caller's
ALine/AColumn (e.g. combine AContext's file/path property with ALine and
AColumn) instead of the literal 'template-expression', and keep the existing use
of DeclaredPrivateNames and PipelineOptions unchanged so parsed
nodes/diagnostics remain anchored to the original ${...} location.
In `@source/units/Goccia.SourcePipeline.pas`:
- Around line 373-399: The parser helpers TGocciaParser.Expression and
TGocciaParser.ParseExpressionWithPrivateNames only parse a single expression and
do not enforce EOF, so update TGocciaSourcePipeline.ParseExpression to reject
trailing tokens: after calling Parser.Expression or
Parser.ParseExpressionWithPrivateNames, verify the parser/token stream is at EOF
(or the next token kind is EOF) and if not, free Result and return nil (or
raise/handle as existing error flow) so inputs like "foo() bar" or "a, b; c" are
rejected; use the parser's current token/position API (or inspect
Tokens/Parser.Token.Kind) to detect non-EOF trailing tokens and handle
accordingly.
- Around line 462-479: ParseDynamicFunctionWrapper currently constructs
TGocciaLexer/TGocciaParser directly and ignores AOptions.Preprocessors; modify
it so the generated Source is passed through AOptions.Preprocessors (or the same
preprocessing step used by Parse/ParseModuleSource) before creating
TGocciaLexer. Specifically, after building Source in
ParseDynamicFunctionWrapper, run each preprocessor in AOptions.Preprocessors
(preserving filename/context) to produce the final source string, then create
TGocciaLexer with that processed source and continue creating
TGocciaParser/ConfigureParser as before so JSX and other preprocessing options
are honored.
- Around line 149-151: The NonStrictModeEnabled flag is being forced on for
modules; change the logic in the assignment for Result.NonStrictModeEnabled so
it is only true when cfNonStrictMode is present in AOptions.Compatibility
(remove the OR check against AOptions.SourceType = stModule). Update the line
that sets Result.NonStrictModeEnabled to rely solely on (cfNonStrictMode in
AOptions.Compatibility) so module sources are not parsed in non-strict mode.
---
Outside diff comments:
In `@source/app/Goccia.CLI.Application.pas`:
- Around line 516-527: The per-file branch treats an invalid ValueStr as a
signal to immediately fall back to DefaultSourceTypeForFileName(AFileName),
which bypasses any valid root config; change the logic so that when
NormalizedValue is not "module" or "script" you attempt to read the root-level
'source-type' (using FindConfigEntry on the root config structure) and if that
root value is valid return the corresponding Goccia.Engine.stModule/stScript,
otherwise log the warning and then return
DefaultSourceTypeForFileName(AFileName); use the existing symbols ValueStr,
NormalizedValue, FindConfigEntry, and DefaultSourceTypeForFileName(AFileName) to
locate and implement this check.
---
Nitpick comments:
In `@scripts/build-logo.ts`:
- Line 74: Extract the long ImageMagick invocation into a named variable or
helper to improve readability: build a descriptive constant or function (e.g.,
buildFloodfillArgs or floodfillCommand) that assembles the sequence of
parameters currently in the template literal (`${magick} ${source} -resize
${traceSize} -alpha set -bordercolor none -border 1x1 -fill none -fuzz
${opaqueBackgroundFuzz} -draw ${"color 0,0 floodfill"} -shave 1x1 ${cutout}`),
then call await $`${magick} ${floodfillArgs}` (or pass the args array) from the
place where the command is run; keep the same variables (magick, source,
traceSize, opaqueBackgroundFuzz, cutout) and add brief comments for each logical
step (resize, alpha/border setup, fuzz/draw floodfill, shave) to document
intent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7cbb57a1-52cb-463c-b998-966f730ec850
⛔ Files ignored due to path filters (3)
logo.pngis excluded by!**/*.pnglogo.svgis excluded by!**/*.svgwebsite/public/logo.pngis excluded by!**/*.png
📒 Files selected for processing (58)
.agents/skills/grill-with-docs/ADR-FORMAT.md.agents/skills/grill-with-docs/CONTEXT-FORMAT.md.agents/skills/grill-with-docs/SKILL.md.agents/skills/handoff/SKILL.md.agents/skills/improve-codebase-architecture/DEEPENING.md.agents/skills/improve-codebase-architecture/HTML-REPORT.md.agents/skills/improve-codebase-architecture/INTERFACE-DESIGN.md.agents/skills/improve-codebase-architecture/LANGUAGE.md.agents/skills/improve-codebase-architecture/SKILL.mdAGENTS.mdCONTEXT.mdREADME.mddocs/architecture.mddocs/build-system.mddocs/built-ins.mddocs/bytecode-vm.mddocs/decision-log.mddocs/embedding.mddocs/interpreter.mddocs/language-tables.mddocs/language.mddocs/spikes/lexer-performance-simplification.mddocs/spikes/parser-hot-dispatch-performance.mddocs/testing.mdscripts/build-logo.tsscripts/fetch-e2e.shscripts/run_test262_suite.tsscripts/test-cli-apps.tsscripts/test-cli-config.tsscripts/test-cli-lexer.tsscripts/test-cli-parser.tsscripts/test-cli.tsskills-lock.jsonsource/app/Goccia.CLI.Application.passource/app/Goccia.CLI.EngineSetup.passource/app/Goccia.CLI.Help.passource/app/Goccia.CLI.Options.passource/app/GocciaBenchmarkRunner.dprsource/app/GocciaBundler.dprsource/app/GocciaREPL.dprsource/app/GocciaScriptLoader.dprsource/app/GocciaScriptLoaderBare.dprsource/app/GocciaTestRunner.dprsource/shared/CLI.ConfigFile.Test.passource/shared/CLI.ConfigFile.passource/shared/CLI.Options.passource/shared/CLI.Parser.Test.passource/shared/CLI.Parser.passource/units/Goccia.Engine.passource/units/Goccia.Evaluator.passource/units/Goccia.FileExtensions.passource/units/Goccia.Modules.Loader.passource/units/Goccia.Parser.passource/units/Goccia.Shims.passource/units/Goccia.SourcePipeline.pastests/language/asi/goccia.jsontests/language/modules/mjs-entry-source-type.mjstests/language/with/asi/goccia.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/units/Goccia.SourcePipeline.pas (1)
394-425:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor preprocessors in
ParseExpression.This helper still lexes
AExpressionTextdirectly, so JSX-enabled callers will fail here even thoughParseandParseDynamicFunctionWrappernow go throughApplyPreprocessors.Suggested fix
class function TGocciaSourcePipeline.ParseExpression(const AExpressionText, AFileName: string; const AOptions: TGocciaSourcePipelineOptions; const ADeclaredPrivateNames: TStrings): TGocciaExpression; var Lexer: TGocciaLexer; Parser: TGocciaParser; SourceLines: TStringList; Tokens: TObjectList<TGocciaToken>; + ProcessedExpressionText: string; + PreprocessorSourceMap: TGocciaSourceMap; begin Result := nil; if AExpressionText = '' then Exit; - SourceLines := CreateECMAScriptSourceLines(AExpressionText); + ProcessedExpressionText := ApplyPreprocessors(AExpressionText, AOptions, + PreprocessorSourceMap); + SourceLines := CreateECMAScriptSourceLines(ProcessedExpressionText); try - Lexer := TGocciaLexer.Create(AExpressionText, AFileName); + Lexer := TGocciaLexer.Create(ProcessedExpressionText, AFileName); try Tokens := Lexer.ScanTokens; if not Assigned(Tokens) then Exit; Parser := TGocciaParser.Create(Tokens, AFileName, SourceLines); @@ finally Lexer.Free; end; finally SourceLines.Free; + PreprocessorSourceMap.Free; end; end;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/units/Goccia.SourcePipeline.pas` around lines 394 - 425, ParseExpression currently lexes AExpressionText directly (creating SourceLines and TGocciaLexer with the raw text) so preprocessors (e.g., JSX handling) are skipped; update TGocciaSourcePipeline.ParseExpression to run the expression text through ApplyPreprocessors (using the provided AOptions) before calling CreateECMAScriptSourceLines and TGocciaLexer.Create, then proceed to ConfigureParser and parsing as before so Parser.ParseExpressionWithPrivateNames / Parser.Expression operate on the preprocessed text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/units/Goccia.Engine.pas`:
- Around line 1240-1241: Initialize the local TGocciaValue variable ModuleResult
to nil before calling FExecutor.EvaluateModuleBody so that the finally block
cannot pass an uninitialized pointer to GC.RemoveTempRoot; additionally, in the
finally ensure you only call GC.RemoveTempRoot(ModuleResult) when
Assigned(ModuleResult) (or leave that guard in place if already present) to
avoid removing an unrelated temp root; relevant symbols: ModuleResult,
FExecutor.EvaluateModuleBody, GC.RemoveTempRoot,
TGarbageCollector.RemoveTempRoot.
In `@source/units/Goccia.Evaluator.pas`:
- Around line 6158-6159: The code creates a new TGocciaSourcePipeline but resets
PipelineOptions.Preprocessors, PipelineOptions.Compatibility and
PipelineOptions.SourceType to defaults, causing template expressions to be
parsed with different rules; instead reuse the existing active source-pipeline
options by copying the current pipeline’s TGocciaSourcePipelineOptions into the
new PipelineOptions (e.g., assign PipelineOptions :=
CurrentPipeline.PipelineOptions or clone the active
TGocciaSourcePipelineOptions) and only adjust any fields that must differ,
ensuring Preprocessors, Compatibility and SourceType are preserved; apply the
same change in the other occurrence that constructs a TGocciaSourcePipeline so
both sites inherit the surrounding parser policy.
---
Outside diff comments:
In `@source/units/Goccia.SourcePipeline.pas`:
- Around line 394-425: ParseExpression currently lexes AExpressionText directly
(creating SourceLines and TGocciaLexer with the raw text) so preprocessors
(e.g., JSX handling) are skipped; update TGocciaSourcePipeline.ParseExpression
to run the expression text through ApplyPreprocessors (using the provided
AOptions) before calling CreateECMAScriptSourceLines and TGocciaLexer.Create,
then proceed to ConfigureParser and parsing as before so
Parser.ParseExpressionWithPrivateNames / Parser.Expression operate on the
preprocessed text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57d65d58-371a-47a7-ae68-b1611ff6b05e
📒 Files selected for processing (6)
scripts/test-cli.tssource/shared/CLI.Parser.passource/units/Goccia.Engine.passource/units/Goccia.Evaluator.passource/units/Goccia.Parser.passource/units/Goccia.SourcePipeline.pas
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
source/app/GocciaScriptLoader.dpr (1)
630-647:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegister coverage against the generated source, not
ASource.This path now parses through
TGocciaSourcePipeline, but coverage still counts executable lines from the originalASource. For JSX/preprocessed inputs that can disagree with the cloned source map and produce incorrect coverage metadata. ThreadGeneratedSourceLinesout ofParseSource(or keep the pipeline result alive here) and use that forRegisterSourceFile.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/app/GocciaScriptLoader.dpr` around lines 630 - 647, ParseSource is returning a transformed/generated source via the pipeline but the coverage registration is still using the original ASource (used in CountExecutableLines and RegisterSourceFile), which yields wrong line counts for preprocessed/JSX inputs; update ParseSource (or its caller) to return or expose the generated source lines (e.g., GeneratedSourceLines or keep the pipeline result alive) and replace uses of ASource in TGocciaCoverageTracker.Instance.RegisterSourceFile and CountExecutableLines with the generated source (and pass SourceMap.Clone as before) so coverage is counted against the generated output rather than the original ASource.source/shared/CLI.Parser.pas (1)
92-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAllow dash-prefixed separate values.
Line 96 treats any next token starting with
-as “missing”, so valid argv like--timeout -1or--output --tmp-filenever reachesOption.Apply. That makes separate-value options parse incorrectly for a real class of inputs.Suggested fix
- if (I >= Count) or - (Copy(ParamStr(I + 1), 1, 1) = SHORT_FLAG_CHAR) then + if I >= Count then raise TParseError.CreateFmt( '--%s requires a value', [Name]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/shared/CLI.Parser.pas` around lines 92 - 100, The parser currently rejects any separate value that begins with SHORT_FLAG_CHAR (treating e.g. "--timeout -1" as missing); update the check in the block handling Option.ConsumesSeparateValue so the next token is considered a flag only when it really looks like one (e.g. starts with "--" or starts with "-" followed by a non-digit), otherwise accept it as the option's value; specifically change the condition that inspects ParamStr(I+1) to treat "--..." as a flag but allow "-1" or "-tmp" as values by checking Copy(ParamStr(I+1),1,2) = '--' or (Copy(...,1,1)=SHORT_FLAG_CHAR and not (ParamStr(I+1)[2] in ['0'..'9'])), then proceed to Inc(I); set Value := ParamStr(I) and call Option.Apply as before (symbols: Option.ConsumesSeparateValue, ParamStr, SHORT_FLAG_CHAR, Value, Inc(I), Option.Apply, TParseError.CreateFmt).source/units/Goccia.SourcePipeline.pas (1)
549-550:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't free the preprocessor source map before parse errors are translated.
SourceMap.Freeruns beforeParseUnchecked, so anyTGocciaErrorraised here reports positions in the transformed wrapper source when preprocessing is active. That regresses diagnostics for JSX-enabled dynamicFunctionparsing compared withParse, which keeps the map alive until after error translation.Also applies to: 552-558
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/units/Goccia.SourcePipeline.pas` around lines 549 - 550, The SourceMap created by ApplyPreprocessors is being freed too early (SourceMap.Free runs before ParseUnchecked), causing ParseUnchecked to raise TGocciaError with positions in the transformed wrapper instead of original source; update the flow so that the SourceMap instance returned by ApplyPreprocessors is not freed until after ParseUnchecked completes and after any error translation/position remapping logic has run (mirror how Parse keeps the map alive); specifically, delay or move the SourceMap.Free call to after the error-translation step that follows ParseUnchecked (also apply the same change to the analogous block around the 552–558 region) so diagnostics use the original-source positions.
♻️ Duplicate comments (1)
source/units/Goccia.Engine.pas (1)
1269-1327:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTemp-root the script result across
WaitForRuntimeIdle.The module branch now protects its return value before draining pending work, but the script branch still does
FExecutor.ExecuteProgram(...)followed byWaitForRuntimeIdlewithout rooting that value first. If a pending job triggers GC,FLastTiming.Resultcan be reclaimed beforeExecutereturns it.Suggested fix
CheckTopLevelRedeclarations(PipelineResult.ProgramNode, FInterpreter.GlobalScope, FSourcePath); - FLastTiming.Result := FExecutor.ExecuteProgram( - PipelineResult.ProgramNode); - WaitForRuntimeIdle; + ModuleResult := FExecutor.ExecuteProgram( + PipelineResult.ProgramNode); + GC := TGarbageCollector.Instance; + if Assigned(ModuleResult) and Assigned(GC) then + GC.AddTempRoot(ModuleResult); + try + WaitForRuntimeIdle; + FLastTiming.Result := ModuleResult; + finally + if Assigned(ModuleResult) and Assigned(GC) then + GC.RemoveTempRoot(ModuleResult); + end; ExecEnd := GetNanoseconds;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/units/Goccia.Engine.pas` around lines 1269 - 1327, The non-module (script) branch must protect the execution result across WaitForRuntimeIdle the same way the module branch does: capture ExecuteProgram's return into a local (e.g., ScriptResult), get GC := TGarbageCollector.Instance, if Assigned(ScriptResult) and Assigned(GC) call GC.AddTempRoot(ScriptResult) before WaitForRuntimeIdle, then assign FLastTiming.Result and finally remove the temp root in a finally block (GC.RemoveTempRoot(ScriptResult)); mirror the module-branch pattern around FExecutor.ExecuteProgram, WaitForRuntimeIdle, and FLastTiming.Result using the same GC checks.
🧹 Nitpick comments (2)
source/shared/CLI.Parser.Test.pas (1)
142-156: ⚡ Quick winTest parser behavior here, not the option internals.
This only proves
TOptionalStringOptionreportsConsumesSeparateValue = False; it does not proveParseCommandLineactually leaves the following argv token untouched. A parser-level test for--source-map,--source-map=out.map, and a trailing positional arg would cover the changed contract much better.As per coding guidelines,
**/*.pas: Keep implementation details private; test only public APIs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/shared/CLI.Parser.Test.pas` around lines 142 - 156, The current unit TestOptionalStringOptionDoesNotConsumeSeparateValue inspects TOptionalStringOption internals instead of exercising the parser contract; replace or augment it with a parser-level test that calls ParseCommandLine (or the public CLI parsing entrypoint used in tests) using argv scenarios: ["--source-map", "positional"], ["--source-map=out.map", "positional"], and ensure the parser marks the option present with the correct Value (empty for bare --source-map, "out.map" for the = form) and that the trailing positional argument remains in the parser's positional results; target the public API (ParseCommandLine) and avoid asserting on TOptionalStringOption.source/units/Goccia.SourcePipeline.pas (1)
31-35: 🏗️ Heavy liftKeep the active-options stack off the public surface.
TGocciaSourcePipelineOptionsFrameplusPushActiveOptions/PopActiveOptionsexposes the thread-local bookkeeping protocol as a supported API. That looks like an internal propagation mechanism, not something callers should couple to directly. A scoped helper/internal hook would keep this unit's public surface narrower.As per coding guidelines,
**/*.pas: Keep implementation details private; test only public APIs`.Also applies to: 100-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/units/Goccia.SourcePipeline.pas` around lines 31 - 35, TGocciaSourcePipelineOptionsFrame and the PushActiveOptions/PopActiveOptions protocol are implementation details and should be removed from the unit's public interface; move the TGocciaSourcePipelineOptionsFrame type and the PushActiveOptions/PopActiveOptions routines into the implementation section (or hide them inside a non-exported helper), and replace any external usage with a scoped/internal RAII-style helper (e.g. a local record/class with constructor/destructor that calls PushActiveOptions/PopActiveOptions) so callers only see the public API and not the thread-local bookkeeping symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/shared/CLI.Options.pas`:
- Around line 76-80: TOptionalStringOption must clear the inherited
TStringOption stored value when the flag is present without an explicit "=..."
value; to fix, implement ConsumesSeparateValue and the option-handling path so
that when the parser recognizes the option with no separate value you explicitly
set the underlying string storage to empty (clear TStringOption's FValue or call
its setter to set '') and mark the option as present, and keep FormatForHelp
as-is for help text; locate TOptionalStringOption and add the no-value handling
in the code path the parser uses (the override of ConsumesSeparateValue and/or
the SetValue/assign hook used by TStringOption) so a bare "--source-map" clears
the inherited value instead of leaving it.
---
Outside diff comments:
In `@source/app/GocciaScriptLoader.dpr`:
- Around line 630-647: ParseSource is returning a transformed/generated source
via the pipeline but the coverage registration is still using the original
ASource (used in CountExecutableLines and RegisterSourceFile), which yields
wrong line counts for preprocessed/JSX inputs; update ParseSource (or its
caller) to return or expose the generated source lines (e.g.,
GeneratedSourceLines or keep the pipeline result alive) and replace uses of
ASource in TGocciaCoverageTracker.Instance.RegisterSourceFile and
CountExecutableLines with the generated source (and pass SourceMap.Clone as
before) so coverage is counted against the generated output rather than the
original ASource.
In `@source/shared/CLI.Parser.pas`:
- Around line 92-100: The parser currently rejects any separate value that
begins with SHORT_FLAG_CHAR (treating e.g. "--timeout -1" as missing); update
the check in the block handling Option.ConsumesSeparateValue so the next token
is considered a flag only when it really looks like one (e.g. starts with "--"
or starts with "-" followed by a non-digit), otherwise accept it as the option's
value; specifically change the condition that inspects ParamStr(I+1) to treat
"--..." as a flag but allow "-1" or "-tmp" as values by checking
Copy(ParamStr(I+1),1,2) = '--' or (Copy(...,1,1)=SHORT_FLAG_CHAR and not
(ParamStr(I+1)[2] in ['0'..'9'])), then proceed to Inc(I); set Value :=
ParamStr(I) and call Option.Apply as before (symbols:
Option.ConsumesSeparateValue, ParamStr, SHORT_FLAG_CHAR, Value, Inc(I),
Option.Apply, TParseError.CreateFmt).
In `@source/units/Goccia.SourcePipeline.pas`:
- Around line 549-550: The SourceMap created by ApplyPreprocessors is being
freed too early (SourceMap.Free runs before ParseUnchecked), causing
ParseUnchecked to raise TGocciaError with positions in the transformed wrapper
instead of original source; update the flow so that the SourceMap instance
returned by ApplyPreprocessors is not freed until after ParseUnchecked completes
and after any error translation/position remapping logic has run (mirror how
Parse keeps the map alive); specifically, delay or move the SourceMap.Free call
to after the error-translation step that follows ParseUnchecked (also apply the
same change to the analogous block around the 552–558 region) so diagnostics use
the original-source positions.
---
Duplicate comments:
In `@source/units/Goccia.Engine.pas`:
- Around line 1269-1327: The non-module (script) branch must protect the
execution result across WaitForRuntimeIdle the same way the module branch does:
capture ExecuteProgram's return into a local (e.g., ScriptResult), get GC :=
TGarbageCollector.Instance, if Assigned(ScriptResult) and Assigned(GC) call
GC.AddTempRoot(ScriptResult) before WaitForRuntimeIdle, then assign
FLastTiming.Result and finally remove the temp root in a finally block
(GC.RemoveTempRoot(ScriptResult)); mirror the module-branch pattern around
FExecutor.ExecuteProgram, WaitForRuntimeIdle, and FLastTiming.Result using the
same GC checks.
---
Nitpick comments:
In `@source/shared/CLI.Parser.Test.pas`:
- Around line 142-156: The current unit
TestOptionalStringOptionDoesNotConsumeSeparateValue inspects
TOptionalStringOption internals instead of exercising the parser contract;
replace or augment it with a parser-level test that calls ParseCommandLine (or
the public CLI parsing entrypoint used in tests) using argv scenarios:
["--source-map", "positional"], ["--source-map=out.map", "positional"], and
ensure the parser marks the option present with the correct Value (empty for
bare --source-map, "out.map" for the = form) and that the trailing positional
argument remains in the parser's positional results; target the public API
(ParseCommandLine) and avoid asserting on TOptionalStringOption.
In `@source/units/Goccia.SourcePipeline.pas`:
- Around line 31-35: TGocciaSourcePipelineOptionsFrame and the
PushActiveOptions/PopActiveOptions protocol are implementation details and
should be removed from the unit's public interface; move the
TGocciaSourcePipelineOptionsFrame type and the
PushActiveOptions/PopActiveOptions routines into the implementation section (or
hide them inside a non-exported helper), and replace any external usage with a
scoped/internal RAII-style helper (e.g. a local record/class with
constructor/destructor that calls PushActiveOptions/PopActiveOptions) so callers
only see the public API and not the thread-local bookkeeping symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 34572e79-5635-4145-b287-5ed4b32a7c0b
📒 Files selected for processing (9)
source/app/GocciaBundler.dprsource/app/GocciaScriptLoader.dprsource/shared/CLI.Options.passource/shared/CLI.Parser.Test.passource/shared/CLI.Parser.passource/units/Goccia.Engine.passource/units/Goccia.Evaluator.passource/units/Goccia.Modules.Loader.passource/units/Goccia.SourcePipeline.pas
- Let required options accept dash-prefixed values - Preserve generated source lines for coverage and source maps - Make pipeline options scope-based and keep script results rooted
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/units/Goccia.Modules.Loader.pas (1)
402-410:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't print module-parse warnings directly from the loader.
These
WriteLncalls bypass the CLI's warning suppression/capture path, so an imported-module warning will still hit stdout in--output=jsonflows and can corrupt the JSON stream. Please route these warnings through the same sink used by entry-source parsing instead of emitting them here.Based on learnings: For GocciaScript CLI tools, suppress human-readable status/progress WriteLn output when running in JSON output mode.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/units/Goccia.Modules.Loader.pas` around lines 402 - 410, The loader currently prints ModuleParseResult warnings directly via WriteLn (iterating ModuleParseResult.Warnings and printing ModuleWarning.Message/Suggestion and ResolvedPath), which bypasses the CLI's JSON-safe warning sink; replace those WriteLn calls with the same warning-emission API used by entry-source parsing (i.e. call the centralized warning/logger method rather than WriteLn), forwarding the message, suggestion (if any), and location (ResolvedPath, ModuleWarning.Line, ModuleWarning.Column) so warnings respect JSON/output modes and suppression.
♻️ Duplicate comments (1)
source/units/Goccia.SourcePipeline.pas (1)
481-499:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply preprocessors in
ParseExpressiontoo.This helper still lexes
AExpressionTextdirectly, soAOptions.Preprocessorsare ignored here. JSX expressions will parse throughParse/ParseDynamicFunctionWrapperbut fail throughParseExpression, which makes the new pipeline API inconsistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/units/Goccia.SourcePipeline.pas` around lines 481 - 499, ParseExpression currently lexes AExpressionText directly and ignores AOptions.Preprocessors, causing JSX and other preprocessed syntax to fail; run the same preprocessing step used by Parse/ParseDynamicFunctionWrapper on AExpressionText before creating SourceLines and the TGocciaLexer so preprocessors are applied. Specifically, replace uses of AExpressionText in CreateECMAScriptSourceLines and TGocciaLexer.Create inside ParseExpression with the preprocessed text (apply AOptions.Preprocessors to AExpressionText first), then continue to call ConfigureParser(Parser, AOptions) and the existing parser methods (ParseExpressionWithPrivateNames / Expression) as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/units/Goccia.Engine.pas`:
- Line 1242: The temporary rooting of the ScriptResult must be moved into the
public TGocciaEngine.ExecuteProgram so the TGocciaValue returned across
WaitForRuntimeIdle is protected from GC; locate ExecuteProgram and add a
Root/Unroot (or equivalent TempRoot/TempUnroot) around the value returned
(reusing the same rooting logic currently next to ScriptResult), ensure
WaitForRuntimeIdle is called while the value is rooted, and remove the duplicate
local rooting where ScriptResult is declared so both internal and external
callers are protected; reference the ExecuteProgram method, the ScriptResult
variable, and the WaitForRuntimeIdle call when making the change.
---
Outside diff comments:
In `@source/units/Goccia.Modules.Loader.pas`:
- Around line 402-410: The loader currently prints ModuleParseResult warnings
directly via WriteLn (iterating ModuleParseResult.Warnings and printing
ModuleWarning.Message/Suggestion and ResolvedPath), which bypasses the CLI's
JSON-safe warning sink; replace those WriteLn calls with the same
warning-emission API used by entry-source parsing (i.e. call the centralized
warning/logger method rather than WriteLn), forwarding the message, suggestion
(if any), and location (ResolvedPath, ModuleWarning.Line, ModuleWarning.Column)
so warnings respect JSON/output modes and suppression.
---
Duplicate comments:
In `@source/units/Goccia.SourcePipeline.pas`:
- Around line 481-499: ParseExpression currently lexes AExpressionText directly
and ignores AOptions.Preprocessors, causing JSX and other preprocessed syntax to
fail; run the same preprocessing step used by Parse/ParseDynamicFunctionWrapper
on AExpressionText before creating SourceLines and the TGocciaLexer so
preprocessors are applied. Specifically, replace uses of AExpressionText in
CreateECMAScriptSourceLines and TGocciaLexer.Create inside ParseExpression with
the preprocessed text (apply AOptions.Preprocessors to AExpressionText first),
then continue to call ConfigureParser(Parser, AOptions) and the existing parser
methods (ParseExpressionWithPrivateNames / Expression) as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7cdfacff-ade1-4024-a588-0059ee9e25b4
⛔ Files ignored due to path filters (32)
tmp/logo-detail/cut.pngis excluded by!**/*.pngtmp/logo-detail/final-balanced.pngis excluded by!**/*.pngtmp/logo-detail/manual-balanced.pngis excluded by!**/*.pngtmp/logo-detail/manual-balanced.svgis excluded by!**/*.svgtmp/logo-detail/manual-max.pngis excluded by!**/*.pngtmp/logo-detail/manual-max.svgis excluded by!**/*.svgtmp/logo-detail/photo-preset.pngis excluded by!**/*.pngtmp/logo-detail/photo-preset.svgis excluded by!**/*.svgtmp/logo-large.svgis excluded by!**/*.svgtmp/logo-medium.svgis excluded by!**/*.svgtmp/logo-poster.svgis excluded by!**/*.svgtmp/logo-small.svgis excluded by!**/*.svgtmp/logo-tune/current-script-output.pngis excluded by!**/*.pngtmp/logo-tune/cut-detail-no-blur.pngis excluded by!**/*.pngtmp/logo-tune/cut-detail-soft.pngis excluded by!**/*.pngtmp/logo-tune/cut-floodfill.pngis excluded by!**/*.pngtmp/logo-tune/cut-max-detail.pngis excluded by!**/*.pngtmp/logo-tune/cut-no-blur.pngis excluded by!**/*.pngtmp/logo-tune/cut-preserve-alpha.pngis excluded by!**/*.pngtmp/logo-tune/detail-no-blur.pngis excluded by!**/*.pngtmp/logo-tune/detail-no-blur.svgis excluded by!**/*.svgtmp/logo-tune/detail-soft.pngis excluded by!**/*.pngtmp/logo-tune/detail-soft.svgis excluded by!**/*.svgtmp/logo-tune/final-logo-svg-render.pngis excluded by!**/*.pngtmp/logo-tune/floodfill.pngis excluded by!**/*.pngtmp/logo-tune/floodfill.svgis excluded by!**/*.svgtmp/logo-tune/max-detail.pngis excluded by!**/*.pngtmp/logo-tune/max-detail.svgis excluded by!**/*.svgtmp/logo-tune/no-blur.pngis excluded by!**/*.pngtmp/logo-tune/no-blur.svgis excluded by!**/*.svgtmp/logo-tune/preserve-alpha.pngis excluded by!**/*.pngtmp/logo-tune/preserve-alpha.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
source/app/GocciaScriptLoader.dprsource/shared/CLI.Parser.Test.passource/shared/CLI.Parser.passource/units/Goccia.Engine.passource/units/Goccia.Modules.Loader.passource/units/Goccia.SourcePipeline.pas
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/units/Goccia.Engine.pas (1)
1249-1269:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the module loader in sync with the engine’s parser policy.
This entry path now parses and activates
FPreprocessors/FCompatibility, but imported modules still readTGocciaModuleLoader.FPreprocessors/FCompatibility. Those loader fields are never initialized from the engine defaults andSetPreprocessors/SetCompatibilitydo not update them, so imports can silently parse with stale policy. The default case already regresses JSX: the entry file uses[ppJSX], while imported modules stay at[].🩹 Suggested sync points
procedure TGocciaEngine.Initialize(const AFileName: string; const ASourceLines: TStringList; const AModuleLoader: TGocciaModuleLoader; const AOwnsModuleLoader: Boolean); begin ... FPreprocessors := DefaultPreprocessors; FCompatibility := DefaultCompatibility; + FModuleLoader.Preprocessors := FPreprocessors; + FModuleLoader.Compatibility := FCompatibility; ... end; procedure TGocciaEngine.SetPreprocessors(const AValue: TGocciaPreprocessors); begin FPreprocessors := AValue; + if Assigned(FModuleLoader) then + FModuleLoader.Preprocessors := AValue; FInterpreter.JSXEnabled := ppJSX in AValue; end; procedure TGocciaEngine.SetCompatibility(const AValue: TGocciaCompatibilityFlags); begin FCompatibility := AValue; + if Assigned(FModuleLoader) then + FModuleLoader.Compatibility := AValue; FInterpreter.ASIEnabled := cfASI in AValue; ... end;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/units/Goccia.Engine.pas` around lines 1249 - 1269, The module loader still uses TGocciaModuleLoader.FPreprocessors/FCompatibility which are never initialized/updated, so imported modules can parse with stale policy; to fix, propagate the engine's FPreprocessors and FCompatibility into the module loader before parsing and whenever the engine options change: assign TGocciaModuleLoader.FPreprocessors := FPreprocessors and TGocciaModuleLoader.FCompatibility := FCompatibility right before calling TGocciaSourcePipeline.Parse (or when creating the loader) and also update SetPreprocessors and SetCompatibility to write through to TGocciaModuleLoader so imports always use the current policy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/units/Goccia.Engine.pas`:
- Around line 1314-1316: The runtime cleanup (clearing microtask queue and
calling DiscardRuntimePending -> TGocciaEngineExtension.DiscardPending) is
executed in both Execute and ExecuteProgram, causing duplicate cleanup; pick a
single owner by removing the cleanup from the delegated routine: keep the
finally cleanup in Execute (the top-level caller) and remove the finally block
that clears microtasks and calls DiscardRuntimePending from ExecuteProgram (also
update the other duplicate region around the second occurrence), ensuring only
Execute performs the microtask queue clear and calls DiscardRuntimePending so
DiscardPending runs exactly once per script execution.
In `@source/units/Goccia.SourcePipeline.pas`:
- Around line 97-98: The ActivateOptions function must stop using a simple
restore-by-snapshot IInterface and instead push the AOptions onto a thread-local
stack; return an IInterface wrapper (e.g. a small TInterfacedObject scope token)
whose destructor performs a guarded pop: it only restores the previous options
if the token is the current top of the stack, otherwise it removes itself from
the middle without touching the current top. Update ActivateOptions to Push the
new options onto the TLS stack and return that token; implement the token's
Release/Destroy to check identity against the stack top (and unlink if
out-of-order) so out-of-order scope releases cannot clobber active options.
Ensure the same pattern is applied at the other restore-by-snapshot call sites
noted (lines 137-149, 217-246, 341-345) so every scope uses the top-of-stack
guard/unlink strategy.
---
Outside diff comments:
In `@source/units/Goccia.Engine.pas`:
- Around line 1249-1269: The module loader still uses
TGocciaModuleLoader.FPreprocessors/FCompatibility which are never
initialized/updated, so imported modules can parse with stale policy; to fix,
propagate the engine's FPreprocessors and FCompatibility into the module loader
before parsing and whenever the engine options change: assign
TGocciaModuleLoader.FPreprocessors := FPreprocessors and
TGocciaModuleLoader.FCompatibility := FCompatibility right before calling
TGocciaSourcePipeline.Parse (or when creating the loader) and also update
SetPreprocessors and SetCompatibility to write through to TGocciaModuleLoader so
imports always use the current policy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 041a3056-0316-4d22-8647-56ee64511657
⛔ Files ignored due to path filters (32)
tmp/logo-detail/cut.pngis excluded by!**/*.pngtmp/logo-detail/final-balanced.pngis excluded by!**/*.pngtmp/logo-detail/manual-balanced.pngis excluded by!**/*.pngtmp/logo-detail/manual-balanced.svgis excluded by!**/*.svgtmp/logo-detail/manual-max.pngis excluded by!**/*.pngtmp/logo-detail/manual-max.svgis excluded by!**/*.svgtmp/logo-detail/photo-preset.pngis excluded by!**/*.pngtmp/logo-detail/photo-preset.svgis excluded by!**/*.svgtmp/logo-large.svgis excluded by!**/*.svgtmp/logo-medium.svgis excluded by!**/*.svgtmp/logo-poster.svgis excluded by!**/*.svgtmp/logo-small.svgis excluded by!**/*.svgtmp/logo-tune/current-script-output.pngis excluded by!**/*.pngtmp/logo-tune/cut-detail-no-blur.pngis excluded by!**/*.pngtmp/logo-tune/cut-detail-soft.pngis excluded by!**/*.pngtmp/logo-tune/cut-floodfill.pngis excluded by!**/*.pngtmp/logo-tune/cut-max-detail.pngis excluded by!**/*.pngtmp/logo-tune/cut-no-blur.pngis excluded by!**/*.pngtmp/logo-tune/cut-preserve-alpha.pngis excluded by!**/*.pngtmp/logo-tune/detail-no-blur.pngis excluded by!**/*.pngtmp/logo-tune/detail-no-blur.svgis excluded by!**/*.svgtmp/logo-tune/detail-soft.pngis excluded by!**/*.pngtmp/logo-tune/detail-soft.svgis excluded by!**/*.svgtmp/logo-tune/final-logo-svg-render.pngis excluded by!**/*.pngtmp/logo-tune/floodfill.pngis excluded by!**/*.pngtmp/logo-tune/floodfill.svgis excluded by!**/*.svgtmp/logo-tune/max-detail.pngis excluded by!**/*.pngtmp/logo-tune/max-detail.svgis excluded by!**/*.svgtmp/logo-tune/no-blur.pngis excluded by!**/*.pngtmp/logo-tune/no-blur.svgis excluded by!**/*.svgtmp/logo-tune/preserve-alpha.pngis excluded by!**/*.pngtmp/logo-tune/preserve-alpha.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
source/app/GocciaScriptLoader.dprsource/shared/CLI.Parser.Test.passource/shared/CLI.Parser.passource/units/Goccia.Engine.passource/units/Goccia.Modules.Loader.passource/units/Goccia.SourcePipeline.pas
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
1 participant
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.
Summary
TGocciaSourcePipeline, with class result objects that return warnings as data and narrow parse APIs for modules, expressions, dynamicFunction, and parser probes.Goccia.CLI.Options; table-drive compatibility flags and resolve them intoTGocciaEngine.Compatibility/ source-pipeline options instead of per-flag convenience properties.--compat-asi/"compat-asi") with no legacy alias, infer module source type for.mjs, and update source-pipeline / CLI-host documentation and tests.Testing
Commands run:
./build.pas clean testrunner./build/GocciaTestRunner tests --no-progress./build/GocciaTestRunner tests/language/modules --no-progress./build/GocciaTestRunner tests/language/source-type --no-progress./build/GocciaTestRunner tests/built-ins/Function/constructor --no-progress./build/GocciaTestRunner tests/language/expressions/string --no-progress./build/GocciaTestRunner tests/language/classes/private-fields-in-template-literals.js --no-progress./build/GocciaTestRunner tests/language/expressions/loose-equality/loose-equality.js --no-progress./build.pas testrunner loader bundler benchmarkrunner && ./format.pas --check./build.pas tests./format.pas --check