Implement import.meta support#265
Conversation
- Add parser, evaluator, compiler, and VM support - Introduce per-module `import.meta` caching with `url` and `resolve()` - Add language tests and update docs/opcode metadata
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ES-style import.meta: parser recognition, new AST node and compiler emitter, OP_IMPORT_META bytecode and VM handler, runtime per-module import.meta objects with Changes
Sequence DiagramsequenceDiagram
participant Parser as Parser
participant AST as AST Node
participant Compiler as Compiler
participant VM as VM/Executor
participant MetaCache as ImportMeta Cache
Parser->>AST: recognize import.meta (TGocciaImportMetaExpression)
AST-->>Parser: return expression node
Compiler->>Compiler: dispatch on TGocciaImportMetaExpression
Compiler->>VM: emit OP_IMPORT_META in bytecode
VM->>VM: decode OP_IMPORT_META
VM->>MetaCache: GetOrCreateImportMeta(current module path)
MetaCache->>MetaCache: normalize path and check cache
alt Cache Hit
MetaCache-->>VM: return cached meta object
else Cache Miss
MetaCache->>MetaCache: create null-prototype object with url & resolve
MetaCache->>MetaCache: pin objects in GC (if available)
MetaCache-->>VM: return new meta object
end
VM->>VM: store meta object in result register
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
units/Goccia.Compiler.Expressions.pas (1)
2451-2455: Add the ES spec annotation above this implementation.This new
import.metapath introduces ECMAScript-defined behavior in the implementation section, but it is missing the requiredES2026reference comment.♻️ Suggested change
+// ES2026 §13.3.12 MetaProperty — import.meta procedure CompileImportMeta(const ACtx: TGocciaCompilationContext; const ADest: UInt8); begin EmitInstruction(ACtx, EncodeABC(OP_IMPORT_META, ADest, 0, 0)); end;As per coding guidelines "When implementing ECMAScript-specified behavior, annotate each function or method with a comment referencing the relevant spec section using the format
// ESYYYY §X.Y.Z SpecMethodName(specParams)..."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Compiler.Expressions.pas` around lines 2451 - 2455, Add the required ECMAScript spec annotation comment above the CompileImportMeta procedure to reference the spec section for import.meta (e.g., "// ES2026 §X.Y.Z ImportMeta(...)"); locate the CompileImportMeta procedure and insert the comment immediately above it so the implementation of import.meta (and use of OP_IMPORT_META) is annotated per the coding guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.ImportMeta.pas`:
- Around line 109-115: The code currently pins MetaObject, ResolveHelper, and
ResolveFunction via TGarbageCollector.Instance.PinObject but only unpins
MetaObject later, leaving ResolveHelper and ResolveFunction permanently pinned;
update the cleanup/unpin logic to call TGarbageCollector.Instance.UnpinObject
for each object previously pinned (MetaObject, ResolveHelper, and the cast
TGCManagedObject(ResolveFunction)) in the same scope where MetaObject is
unpinned, and apply the same fix to the other import.meta creation block (the
second occurrence around the 122–133 region) so that every PinObject has a
matching UnpinObject for those three symbols.
In `@units/Goccia.VM.pas`:
- Around line 5124-5125: OP_IMPORT_META currently calls
GetOrCreateImportMeta(FCurrentModuleSourcePath) which uses the executing module
rather than the defining module; change the argument to use the defining
module's source if available by passing Template.DebugInfo.SourceFile when
present and falling back to FCurrentModuleSourcePath for tests, i.e. call
GetOrCreateImportMeta(Template.DebugInfo.SourceFile or FCurrentModuleSourcePath)
in the OP_IMPORT_META handler so import.meta binds lexically to the function's
defining module.
---
Nitpick comments:
In `@units/Goccia.Compiler.Expressions.pas`:
- Around line 2451-2455: Add the required ECMAScript spec annotation comment
above the CompileImportMeta procedure to reference the spec section for
import.meta (e.g., "// ES2026 §X.Y.Z ImportMeta(...)"); locate the
CompileImportMeta procedure and insert the comment immediately above it so the
implementation of import.meta (and use of OP_IMPORT_META) is annotated per the
coding guidelines.
🪄 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: 5581d68b-d807-414c-a73a-ca2c764e3636
📒 Files selected for processing (17)
AGENTS.mddocs/bytecode-vm.mddocs/language-restrictions.mdtests/language/modules/helpers/import-meta-child.jstests/language/modules/import-meta.jsunits/Goccia.AST.Expressions.pasunits/Goccia.Bytecode.OpCodeNames.pasunits/Goccia.Bytecode.pasunits/Goccia.Compiler.Expressions.pasunits/Goccia.Compiler.pasunits/Goccia.Constants.PropertyNames.pasunits/Goccia.Engine.pasunits/Goccia.Error.Suggestions.pasunits/Goccia.ImportMeta.pasunits/Goccia.Keywords.Contextual.pasunits/Goccia.Parser.pasunits/Goccia.VM.pas
Suite Timing
Measured on ubuntu-latest x64. |
Benchmark Results364 benchmarks Interpreted: 🟢 10 improved · 🔴 261 regressed · 93 unchanged · avg -3.3% arraybuffer.js — Interp: 🟢 2, 🔴 11, 1 unch. · avg -3.2% · Bytecode: 🟢 1, 🔴 7, 6 unch. · avg -2.3%
arrays.js — Interp: 🟢 1, 🔴 12, 6 unch. · avg -2.4% · Bytecode: 🟢 4, 🔴 7, 8 unch. · avg +2.0%
async-await.js — Interp: 🔴 6 · avg -3.6% · Bytecode: 🔴 1, 5 unch. · avg -0.5%
base64.js — Interp: 🔴 3, 7 unch. · avg -2.2% · Bytecode: 🟢 2, 🔴 5, 3 unch. · avg -0.2%
classes.js — Interp: 🔴 31 · avg -4.4% · Bytecode: 🟢 2, 🔴 3, 26 unch. · avg -0.6%
closures.js — Interp: 🔴 10, 1 unch. · avg -5.4% · Bytecode: 🔴 2, 9 unch. · avg -0.6%
collections.js — Interp: 🟢 1, 🔴 10, 1 unch. · avg -3.3% · Bytecode: 🟢 3, 🔴 1, 8 unch. · avg +1.6%
destructuring.js — Interp: 🔴 14, 8 unch. · avg -3.7% · Bytecode: 🟢 5, 🔴 5, 12 unch. · avg -0.3%
fibonacci.js — Interp: 🟢 2, 🔴 1, 5 unch. · avg +0.4% · Bytecode: 🟢 1, 🔴 4, 3 unch. · avg -0.8%
float16array.js — Interp: 🟢 2, 🔴 22, 8 unch. · avg -2.6% · Bytecode: 🟢 6, 🔴 15, 11 unch. · avg -1.3%
for-of.js — Interp: 🔴 3, 4 unch. · avg -1.6% · Bytecode: 🔴 3, 4 unch. · avg -3.0%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 1, 🔴 32, 9 unch. · avg -4.5% · Bytecode: 🟢 19, 🔴 9, 14 unch. · avg +1.8%
json.js — Interp: 🔴 9, 11 unch. · avg -2.2% · Bytecode: 🟢 5, 🔴 3, 12 unch. · avg +1.4%
jsx.jsx — Interp: 🔴 9, 12 unch. · avg -2.3% · Bytecode: 🟢 7, 🔴 3, 11 unch. · avg +0.8%
modules.js — Interp: 🔴 4, 5 unch. · avg -1.3% · Bytecode: 🟢 3, 🔴 1, 5 unch. · avg +0.6%
numbers.js — Interp: 🔴 10, 1 unch. · avg -3.6% · Bytecode: 🟢 4, 🔴 4, 3 unch. · avg -0.5%
objects.js — Interp: 🔴 7 · avg -4.5% · Bytecode: 🟢 5, 🔴 1, 1 unch. · avg +4.7%
promises.js — Interp: 🔴 9, 3 unch. · avg -4.2% · Bytecode: 🟢 6, 6 unch. · avg +1.4%
regexp.js — Interp: 🔴 7, 4 unch. · avg -2.5% · Bytecode: 🟢 8, 🔴 2, 1 unch. · avg +4.2%
strings.js — Interp: 🟢 1, 🔴 16, 2 unch. · avg -2.6% · Bytecode: 🟢 9, 🔴 3, 7 unch. · avg +2.7%
typed-arrays.js — Interp: 🔴 21, 1 unch. · avg -4.9% · Bytecode: 🟢 5, 🔴 8, 9 unch. · avg -1.9%
uint8array-encoding.js — Interp: 🔴 14, 4 unch. · avg -3.7% · Bytecode: 🔴 12, 6 unch. · avg -2.4%
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. |
…notation - Fix GC unpin leak: track all pinned objects (meta object, resolve helper, resolve function) and unpin them all during ClearImportMetaCache - Use Template.DebugInfo.SourceFile for lexical import.meta binding in the bytecode VM, with FCurrentModuleSourcePath as fallback for tests without debug info — ensures exported functions report their defining module's metadata, not the calling module's - Add ES2026 §13.3.12 spec annotation to CompileImportMeta - Add test for exported function preserving lexical import.meta binding Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.ImportMeta.pas`:
- Around line 118-123: The pin-tracking logic uses bare numeric literals (3, 2,
8) in the block that manipulates PinnedCount and PinnedObjects; extract these
into named constants (e.g., PIN_ENTRY_SIZE = 3, GROW_MULTIPLIER = 2, MIN_GROW =
8) declared in the implementation section and replace the literals in the check,
SetLength calculation, indexing (PinnedObjects[PinnedCount +
0..PIN_ENTRY_SIZE-1] for MetaObject/ResolveHelper/ResolveFunction) and the
Inc(PinnedCount, 3) call with the constant (Inc(PinnedCount, PIN_ENTRY_SIZE)) so
intent is clear and the growth formula is centralized.
- Around line 75-77: The current resolution always prefixes Specifier with
BaseDirectory, which mis-resolves absolute specifiers; update the resolution in
the import.meta.resolve logic so that if Specifier is an absolute path (detect
via platform-appropriate check for IsAbsolutePath/starting with path separator
or drive letter) you call ExpandFileName(Specifier) directly, otherwise call
ExpandFileName(BaseDirectory + Specifier); keep using ResolvedPath and continue
to return TGocciaStringLiteralValue.Create(FilePathToUrl(ResolvedPath)).
🪄 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: bebe94fe-4f1a-4eb1-888d-681e2bafbeff
📒 Files selected for processing (5)
tests/language/modules/helpers/import-meta-function.jstests/language/modules/import-meta.jsunits/Goccia.Compiler.Expressions.pasunits/Goccia.ImportMeta.pasunits/Goccia.VM.pas
✅ Files skipped from review due to trivial changes (1)
- tests/language/modules/helpers/import-meta-function.js
🚧 Files skipped from review as they are similar to previous changes (2)
- units/Goccia.Compiler.Expressions.pas
- tests/language/modules/import-meta.js
- Handle absolute specifiers in import.meta.resolve correctly: paths starting with / or a drive letter now resolve directly instead of being concatenated with the module's base directory - Extract bare numeric literals in pin-tracking logic into named constants (PINNED_OBJECTS_PER_MODULE, PINNED_INITIAL_CAPACITY) - Add test for absolute path resolution Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
units/Goccia.ImportMeta.pas (1)
130-135:⚠️ Potential issue | 🟡 MinorExtract the remaining growth multiplier literal into a constant.
The resize formula still hardcodes
2(PinnedCount * 2 + PINNED_INITIAL_CAPACITY). Please promote it to a named constant for consistency with the other pin-tracking constants.As per coding guidelines: “Extract bare numeric literals in
implementationsections into named constants so the value is defined once and the name conveys intent.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.ImportMeta.pas` around lines 130 - 135, The resize logic for PinnedObjects uses a hardcoded growth multiplier (the literal 2) — extract that literal into a named constant (e.g. PINNED_GROWTH_MULTIPLIER) in the implementation section and replace the expression PinnedCount * 2 + PINNED_INITIAL_CAPACITY with PinnedCount * PINNED_GROWTH_MULTIPLIER + PINNED_INITIAL_CAPACITY; update any related comments and ensure the new constant is used consistently with PinnedCount, PINNED_OBJECTS_PER_MODULE, SetLength and PINNED_INITIAL_CAPACITY.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.ImportMeta.pas`:
- Around line 47-57: FilePathToUrl returns raw file paths so characters like
spaces, #, ?, and % break URL parsing; after computing AbsolutePath (via
ExpandFileName and normalizing slashes on Windows with StringReplace) you must
percent-encode the path portion (encode each path segment, preserving '/'
separators and unreserved chars per RFC3986) and avoid double-encoding
already-percent-encoded sequences; then prepend the platform-appropriate scheme
('file:///' on Windows, 'file://' on others). Implement this inside
FilePathToUrl so AbsolutePath is URL-encoded before being concatenated into
Result.
---
Duplicate comments:
In `@units/Goccia.ImportMeta.pas`:
- Around line 130-135: The resize logic for PinnedObjects uses a hardcoded
growth multiplier (the literal 2) — extract that literal into a named constant
(e.g. PINNED_GROWTH_MULTIPLIER) in the implementation section and replace the
expression PinnedCount * 2 + PINNED_INITIAL_CAPACITY with PinnedCount *
PINNED_GROWTH_MULTIPLIER + PINNED_INITIAL_CAPACITY; update any related comments
and ensure the new constant is used consistently with PinnedCount,
PINNED_OBJECTS_PER_MODULE, SetLength and PINNED_INITIAL_CAPACITY.
🪄 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: eb5060ee-2c67-4c0b-a807-3b69695c2711
📒 Files selected for processing (2)
tests/language/modules/import-meta.jsunits/Goccia.ImportMeta.pas
✅ Files skipped from review due to trivial changes (1)
- tests/language/modules/import-meta.js
- Add RFC 3986 §2.3 percent-encoding to FilePathToUrl so paths with spaces, #, ?, % and other reserved characters produce valid URLs (e.g. "file:///path%20with%20spaces/file%231.js") - Extract the remaining bare literal 2 into PINNED_GROWTH_MULTIPLIER - Add tests for percent-encoding in resolve and url properties Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
units/Goccia.ImportMeta.pas (1)
73-76:⚠️ Potential issue | 🟠 MajorHandle Windows rooted and UNC paths as absolute in both resolve and URL emission.
Line [102]-[109] only treat
/and<drive>:as absolute. Windows rooted (\foo\bar.js) and UNC (\\server\share\file.js) paths can fall into the relative branch and resolve incorrectly.
Line [75] also always prependsfile:///, which produces malformed UNC URLs (file://///server/...).🔧 Proposed fix
function TGocciaImportMetaResolveHelper.Resolve( const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; var Specifier, BaseDirectory, ResolvedPath: string; begin @@ - if (Length(Specifier) > 0) and ((Specifier[1] = '/') or - (Length(Specifier) >= 2) and (Specifier[2] = ':')) then + if (ExtractFileDrive(Specifier) <> '') or + ((Specifier <> '') and (Specifier[1] in ['/', '\'])) then ResolvedPath := ExpandFileName(Specifier) else begin @@ function FilePathToUrl(const AFilePath: string): string; var AbsolutePath: string; begin AbsolutePath := ExpandFileName(AFilePath); {$IFDEF MSWINDOWS} AbsolutePath := StringReplace(AbsolutePath, '\', '/', [rfReplaceAll]); - Result := 'file:///' + PercentEncodePath(AbsolutePath); + if (Length(AbsolutePath) >= 2) and (AbsolutePath[1] = '/') and (AbsolutePath[2] = '/') then + Result := 'file:' + PercentEncodePath(AbsolutePath) // UNC: //server/share -> file://server/share + else + Result := 'file:///' + PercentEncodePath(AbsolutePath); {$ELSE} Result := 'file://' + PercentEncodePath(AbsolutePath); {$ENDIF} end;Also applies to: 102-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.ImportMeta.pas` around lines 73 - 76, Treat Windows rooted paths (starting with '\' ) and UNC paths (starting with '\\') as absolute in both the path resolution logic and URL emission: update the absolute-path check in the resolver (the code handling '/' and '<drive>:' currently around the block at lines ~102-109) to also consider paths beginning with '\' and '\\' as absolute, and in the URL emission code that builds Result := 'file:///' + PercentEncodePath(AbsolutePath) (using AbsolutePath and PercentEncodePath) detect UNC paths and emit a correct file URL (e.g., 'file://server/share/...' for '\\server\share\file' by stripping the leading backslashes and not prepending an extra slash, and for rooted local paths use 'file:///' + PercentEncodePath with backslashes normalized to '/'). Ensure backslashes are normalized to '/' before percent-encoding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@units/Goccia.ImportMeta.pas`:
- Around line 73-76: Treat Windows rooted paths (starting with '\' ) and UNC
paths (starting with '\\') as absolute in both the path resolution logic and URL
emission: update the absolute-path check in the resolver (the code handling '/'
and '<drive>:' currently around the block at lines ~102-109) to also consider
paths beginning with '\' and '\\' as absolute, and in the URL emission code that
builds Result := 'file:///' + PercentEncodePath(AbsolutePath) (using
AbsolutePath and PercentEncodePath) detect UNC paths and emit a correct file URL
(e.g., 'file://server/share/...' for '\\server\share\file' by stripping the
leading backslashes and not prepending an extra slash, and for rooted local
paths use 'file:///' + PercentEncodePath with backslashes normalized to '/').
Ensure backslashes are normalized to '/' before percent-encoding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc8480a8-f57d-44b2-895c-a45411cfc6d8
📒 Files selected for processing (2)
tests/language/modules/import-meta.jsunits/Goccia.ImportMeta.pas
✅ Files skipped from review due to trivial changes (1)
- tests/language/modules/import-meta.js
- Detect backslash-rooted (\path) and UNC (\\server\share) specifiers as absolute in import.meta.resolve - Emit correct RFC 8089 §2 UNC file URLs (file://server/share/...) instead of the malformed file://///server/share/... Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Both Windows path issues from the latest review are addressed in c0c1ac3:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
units/Goccia.ImportMeta.pas (1)
93-94: Normalize ES spec annotations to the required exact signature formatLine 93 and Line 119 use descriptive prose. Please switch these to the required exact format:
// ESYYYY §X.Y.Z SpecMethodName(specParams)(and keep step-level annotations in-body where relevant).As per coding guidelines,
**/*.pas: Add ECMAScript spec annotations in the format// ESYYYY §X.Y.Z SpecMethodName(specParams)immediately above function/method bodies implementing ES-specified behavior, using the exact method name and parameter list from the spec's pseudo-code; and**/*.pas: When implementing an ECMAScript-specified feature, add spec annotations above each function body and at key algorithm steps within the body.Also applies to: 119-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.ImportMeta.pas` around lines 93 - 94, Replace the descriptive prose spec comments above the implementation(s) with the exact spec-annotation format; for the Resolve method change the comment to use the canonical spec method and parameter list like "// ES2026 §13.3.12.1.1 HostGetImportMetaProperties(specifier)" immediately above TGocciaImportMetaResolveHelper.Resolve, and likewise update the other occurrence (lines ~119-120) to the exact "// ES2026 §X.Y.Z SpecMethodName(params)" form that matches the ECMAScript pseudo-code name and parameter list for that implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@units/Goccia.ImportMeta.pas`:
- Around line 93-94: Replace the descriptive prose spec comments above the
implementation(s) with the exact spec-annotation format; for the Resolve method
change the comment to use the canonical spec method and parameter list like "//
ES2026 §13.3.12.1.1 HostGetImportMetaProperties(specifier)" immediately above
TGocciaImportMetaResolveHelper.Resolve, and likewise update the other occurrence
(lines ~119-120) to the exact "// ES2026 §X.Y.Z SpecMethodName(params)" form
that matches the ECMAScript pseudo-code name and parameter list for that
implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3903e4db-9587-4ecd-a730-a96e5adf7abf
📒 Files selected for processing (1)
units/Goccia.ImportMeta.pas
Align ES2026 annotations with the codebase convention of "// ESYYYY §X.Y.Z SpecMethodName(specParams)": - HostGetImportMetaProperties(moduleRecord) for the resolve helper - ImportMeta : import . meta for the evaluation entry point Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Spec annotation format fixed in 455724f. All
|
Summary
import.metaexpressions.import.meta.urlandimport.meta.resolve()behavior.import.metain the language restrictions and bytecode VM docs, and updated the import/meta suggestion text.Testing
tests/language/modules/import-meta.jsandtests/language/modules/helpers/import-meta-child.js.