Add unsafe Function constructor support#386
Merged
Conversation
- Gate dynamic Function construction behind a new CLI/config flag - Implement Function constructor creation and add coverage tests
Contributor
📝 WalkthroughWalkthroughAdds opt-in dynamic Function constructor support: new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant App as Application
participant Engine as Engine
participant FC as FunctionConstructor
participant Compiler as Parser/Compiler
participant Exec as Executor
CLI->>App: parse flags & per-file/root config
App->>Engine: apply resolved unsafe-function-constructor flag
Engine->>FC: set Enabled and assign CompileDynamicFunction delegate
FC->>Engine: BuildFunction(request args → param/ body sources)
Engine->>Compiler: parse/validate param/body → TGocciaProgram
Compiler-->>Engine: return TGocciaProgram
Engine->>Exec: ExecuteDynamicFunction(TGocciaProgram)
Exec-->>Engine: return TGocciaValue (function) — module retained for closures
Engine-->>FC: return TGocciaFunctionBase or raise SyntaxError/TypeError
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
new feature
![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: 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 `@tests/built-ins/Function/constructor-disabled.js`:
- Around line 7-13: Update the two assertions in the tests titled "new
Function() throws TypeError when disabled" and "Function() throws TypeError when
disabled" to assert the specific exception class instead of any error; change
expect(() => new Function("return 1")).toThrow() to expect(() => new
Function("return 1")).toThrow(TypeError) and likewise change expect(() =>
Function("return 1")).toThrow() to expect(() => Function("return
1")).toThrow(TypeError) so the tests fail if a different error type is thrown.
In `@tests/built-ins/Function/constructor/errors.js`:
- Around line 7-13: Tests that construct new Function with invalid source use
bare toThrow(), which may allow wrong error types; update the two assertions
that call expect(() => new Function("}{")) and expect(() => new Function("@bad",
"return 1")) to assert the specific error class by using toThrow(SyntaxError)
(or toThrowError(SyntaxError)) so the test explicitly requires a SyntaxError for
parse failures in the Function constructor.
🪄 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: 46807403-1a48-483e-ab19-e3fd56b60d11
📒 Files selected for processing (11)
source/app/Goccia.CLI.Application.passource/shared/CLI.Options.passource/units/Goccia.Engine.passource/units/Goccia.Runtime.Bootstrap.passource/units/Goccia.Values.ClassValue.pastests/built-ins/Function/constructor-disabled.jstests/built-ins/Function/constructor/basic.jstests/built-ins/Function/constructor/errors.jstests/built-ins/Function/constructor/goccia.jsontests/built-ins/Function/constructor/properties.jstests/built-ins/Function/constructor/scope.js
Use toThrow(TypeError) and toThrow(SyntaxError) instead of bare toThrow() so wrong exception types don't silently pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Benchmark Results386 benchmarks Interpreted: 🟢 218 improved · 🔴 7 regressed · 161 unchanged · avg +3.2% arraybuffer.js — Interp: 🟢 11, 3 unch. · avg +4.0% · Bytecode: 🟢 1, 🔴 11, 2 unch. · avg -7.1%
arrays.js — Interp: 🟢 7, 12 unch. · avg +1.8% · Bytecode: 🔴 18, 1 unch. · avg -10.5%
async-await.js — Interp: 🟢 4, 2 unch. · avg +2.0% · Bytecode: 🔴 5, 1 unch. · avg -8.2%
base64.js — Interp: 🟢 6, 4 unch. · avg +3.5% · Bytecode: 🔴 10 · avg -10.8%
classes.js — Interp: 🟢 20, 11 unch. · avg +3.8% · Bytecode: 🔴 17, 14 unch. · avg -4.6%
closures.js — Interp: 🟢 7, 4 unch. · avg +3.4% · Bytecode: 🔴 10, 1 unch. · avg -6.6%
collections.js — Interp: 🟢 3, 9 unch. · avg +1.7% · Bytecode: 🔴 12 · avg -8.8%
csv.js — Interp: 🟢 11, 2 unch. · avg +3.2% · Bytecode: 🔴 13 · avg -6.7%
destructuring.js — Interp: 🟢 13, 9 unch. · avg +3.3% · Bytecode: 🔴 20, 2 unch. · avg -7.4%
fibonacci.js — Interp: 🟢 1, 7 unch. · avg +1.6% · Bytecode: 🔴 6, 2 unch. · avg -7.8%
float16array.js — Interp: 🟢 19, 🔴 2, 11 unch. · avg +3.3% · Bytecode: 🟢 4, 🔴 25, 3 unch. · avg -4.4%
for-of.js — Interp: 🟢 7 · avg +5.6% · Bytecode: 🔴 6, 1 unch. · avg -10.1%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 24, 18 unch. · avg +3.5% · Bytecode: 🟢 2, 🔴 31, 9 unch. · avg -5.4%
json.js — Interp: 🟢 11, 9 unch. · avg +3.1% · Bytecode: 🔴 20 · avg -8.0%
jsx.jsx — Interp: 🟢 21 · avg +5.2% · Bytecode: 🔴 17, 4 unch. · avg -5.0%
modules.js — Interp: 9 unch. · avg +0.4% · Bytecode: 🔴 9 · avg -7.1%
numbers.js — Interp: 🟢 8, 3 unch. · avg +5.3% · Bytecode: 🟢 1, 🔴 8, 2 unch. · avg -3.5%
objects.js — Interp: 🟢 1, 6 unch. · avg +0.7% · Bytecode: 🔴 7 · avg -11.5%
promises.js — Interp: 🟢 5, 7 unch. · avg +3.4% · Bytecode: 🔴 12 · avg -10.5%
regexp.js — Interp: 🟢 5, 6 unch. · avg +2.4% · Bytecode: 🔴 10, 1 unch. · avg -4.4%
strings.js — Interp: 🟢 14, 5 unch. · avg +2.3% · Bytecode: 🟢 1, 🔴 17, 1 unch. · avg -3.2%
tsv.js — Interp: 🟢 1, 🔴 1, 7 unch. · avg +0.4% · Bytecode: 🔴 4, 5 unch. · avg -4.4%
typed-arrays.js — Interp: 🟢 15, 🔴 1, 6 unch. · avg +2.9% · Bytecode: 🟢 9, 🔴 8, 5 unch. · avg -0.7%
uint8array-encoding.js — Interp: 🟢 4, 🔴 3, 11 unch. · avg +5.0% · Bytecode: 🟢 2, 🔴 15, 1 unch. · avg -3.0%
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
Suite Timing
Measured on ubuntu-latest x64. |
- Route Function constructor through executor-level dynamic compilation - Preserve anonymous naming and this-binding semantics for generated functions - Remove the old global-scope-backed Function constructor path
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Engine.pas`:
- Around line 1533-1537: The parser created for dynamic functions
(TGocciaParser.Create) does not inherit the engine compatibility flags (cfASI,
cfVar), causing Function(...) bodies to be parsed more strictly than top-level
code; fix by copying the engine's compatibility flags into the parser instance
(i.e., set the parser's CompatibilityFlags or equivalent to include cfASI and
cfVar from the engine/host before calling Parser.ParseUnchecked) so the parser
used in the dynamic-function path honors the same flags as the main engine.
In `@source/units/Goccia.Values.ClassValue.pas`:
- Around line 1244-1271: BuildFunction currently calls the compiler callback
FCompileDynamicFunction unguarded (after checking FEnabled) which can AV if the
bootstrap forgot to wire it; modify BuildFunction to check
Assigned(FCompileDynamicFunction) before calling it and, if nil, raise a JS
error (e.g., via ThrowTypeError or an appropriate engine error) with a clear
message like "Dynamic function compiler not initialized" so callers get a
JS-level exception instead of a crash.
🪄 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: 2a7d6bb4-7063-4129-afcd-1a08eaf09afa
📒 Files selected for processing (5)
source/units/Goccia.Engine.Backend.passource/units/Goccia.Engine.passource/units/Goccia.Executor.passource/units/Goccia.Runtime.Bootstrap.passource/units/Goccia.Values.ClassValue.pas
Resolve conflict in ApplyFileConfigToEngine: adopt main's ResolveFlagOption helper for the unsafe-function-constructor flag, matching the ASI and compat-var precedence pattern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
source/units/Goccia.Values.ClassValue.pas (1)
1278-1315:⚠️ Potential issue | 🟠 MajorGuard
FCompileDynamicFunctionbefore invocation to prevent runtime AV.
FCompileDynamicFunctionis initialized tonilat Line 1278 and called unconditionally at Line 1314. If wiring is missed in any bootstrap path, this crashes instead of throwing a JS error.Proposed fix
function TGocciaFunctionConstructorClassValue.BuildFunction( const AArguments: TGocciaArgumentsCollection): TGocciaFunctionBase; @@ if not FEnabled then ThrowTypeError('Dynamic code generation is disabled. ' + 'Pass --unsafe-function-constructor to enable the Function constructor'); + if not Assigned(FCompileDynamicFunction) then + ThrowTypeError('Function constructor is not available in this runtime'); @@ Result := FCompileDynamicFunction(Source); end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Values.ClassValue.pas` around lines 1278 - 1315, BuildFunction currently calls FCompileDynamicFunction unconditionally which can be nil; add a guard in TGocciaFunctionConstructorClassValue.BuildFunction before calling FCompileDynamicFunction to detect nil and raise a JS-level error (use ThrowTypeError or the existing runtime error helper) explaining the Function constructor is unavailable or not wired, then only call FCompileDynamicFunction(Source) when it's not nil; reference FCompileDynamicFunction and TGocciaFunctionConstructorClassValue.BuildFunction to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@source/units/Goccia.Values.ClassValue.pas`:
- Around line 1278-1315: BuildFunction currently calls FCompileDynamicFunction
unconditionally which can be nil; add a guard in
TGocciaFunctionConstructorClassValue.BuildFunction before calling
FCompileDynamicFunction to detect nil and raise a JS-level error (use
ThrowTypeError or the existing runtime error helper) explaining the Function
constructor is unavailable or not wired, then only call
FCompileDynamicFunction(Source) when it's not nil; reference
FCompileDynamicFunction and TGocciaFunctionConstructorClassValue.BuildFunction
to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a9f4189-eaba-4c0c-a165-250a4efae106
📒 Files selected for processing (3)
source/app/Goccia.CLI.Application.passource/shared/CLI.Options.passource/units/Goccia.Values.ClassValue.pas
Add a nil check before calling FCompileDynamicFunction in BuildFunction. If the callback was never wired (e.g. via TGocciaRuntimeBootstrap), this now throws a clear TypeError instead of crashing with an access violation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The parser created by CompileDynamicFunction now inherits cfASI and cfVar from the engine's compatibility flags, so Function(...) bodies are parsed with the same rules as top-level code. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Values.ClassValue.pas`:
- Around line 1313-1317: The current code builds Source by interpolating
ParamStr and BodyStr into a wrapper string and calls
FCompileDynamicFunction(Source), which allows crafted inputs to escape the
wrapper; instead add a dedicated parsing path that accepts params+body directly
(e.g., change FCompileDynamicFunction to accept ParamStr and BodyStr or add a
new FCompileDynamicFunctionFromParts), update the call site that currently sets
Source (the '({ anonymous(' + ParamStr + ') {' + BodyStr + '}}).anonymous'
construction) to call the new parser entry, and implement the parser change in
TGocciaEngine so it parses a dynamic function node from the two pieces rather
than by embedding them in a synthetic program string; ensure downstream callers
(TGocciaEngine.CompileDynamicFunction) use the new safe entry point.
🪄 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: 40a24f22-a8d0-45a8-993d-0b8f97c6740e
📒 Files selected for processing (2)
source/units/Goccia.Engine.passource/units/Goccia.Values.ClassValue.pas
Parse params and body as separate, self-contained programs before
interpolating into the method shorthand wrapper. This prevents
crafted input from escaping the wrapper and executing arbitrary
code during construction.
Params are validated via `(params) => {}` and body via
`() => { body }`. Both must parse as exactly one statement
(the arrow function). If either contains tokens that break
out of its context, the parser rejects it as a SyntaxError
before the wrapper is ever assembled.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Engine.pas`:
- Around line 1564-1568: ProgramNode validation is too weak and leaks on error:
after Parser.Parse ensure the AST wrapper shape is exactly a single expression
statement whose expression is a TGocciaArrowFunctionExpression (e.g., check
ProgramNode.Body.Count = 1 AND ProgramNode.Body[0] is TGocciaExpressionStatement
AND (ProgramNode.Body[0] as TGocciaExpressionStatement).Expression is
TGocciaArrowFunctionExpression) before calling ParseUnchecked; if validation
fails call ProgramNode.Free first then ThrowSyntaxError to avoid leaking the
temporary AST; apply the same stricter validation and free-before-throw fix to
the other occurrence referenced (around the later block using ThrowSyntaxError).
🪄 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: cf10246b-2317-4810-81de-db2805941bf5
📒 Files selected for processing (2)
source/units/Goccia.Engine.passource/units/Goccia.Values.ClassValue.pas
Tighten the pre-validation shape check: instead of only checking Body.Count = 1, verify the statement is a TGocciaExpressionStatement whose expression is a TGocciaArrowFunctionExpression. This rejects crafted inputs that produce a single non-arrow statement. Wrap ProgramNode in try/finally so it is freed even when ThrowSyntaxError raises, preventing an AST leak on invalid input. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolve conflicts in Engine.pas and Runtime.Bootstrap.pas: keep TGocciaFunctionConstructorClassValue while adopting the new Function.prototype chain setup from main (shared prototype wiring, SetDefaultPrototype, PatchDefaultPrototype, FTypedArrayIntrinsic). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/units/Goccia.Engine.pas`:
- Around line 1617-1628: The parser warnings from Parser.Parse are currently
discarded, allowing unsupported syntax to be silently ignored; after calling
Parser.Parse (where ProgramNode is examined and ParseUnchecked used later) you
should surface any parser warnings by invoking the parser's warning reporting
routine (e.g. PrintParserWarnings or equivalent) before freeing
ProgramNode/Parser so that warnings for unsupported syntax in parameter
lists/defaults are emitted; update the blocks around ProgramNode := Parser.Parse
and the corresponding ParseUnchecked usage (references: ProgramNode,
Parser.Parse, ParseUnchecked, PrintParserWarnings, TGocciaExpressionStatement,
TGocciaArrowFunctionExpression, ThrowSyntaxError) to call the warning printer
after validation and before freeing resources, and apply the same change to the
other occurrence noted (lines ~1647-1658).
🪄 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: f27ae757-f6a0-457c-b1b6-53e4de66181d
📒 Files selected for processing (3)
source/units/Goccia.Engine.passource/units/Goccia.Runtime.Bootstrap.passource/units/Goccia.Values.ClassValue.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
--unsafe-function-constructorengine option and matchinggoccia.jsonconfig key.Functionconstructor with a dedicated implementation that parses parameter and body strings into callable functions.TypeErrorunless explicitly enabled.