Enable strict types by default for bytecode executor#336
Merged
Conversation
- Add executor hook for default strict type mode - Let engine initialize strict typing from the active executor - Keep interpreter-backed execution defaulting to false
Contributor
📝 WalkthroughWalkthroughExecutors now expose a virtual Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant Engine as TGocciaEngine
participant Executor as TGocciaExecutor
participant BExecutor as TGocciaBytecodeExecutor
Test->>Engine: Create(Executor=nil) / Create(Executor=instance)
Engine->>Executor: DefaultStrictTypes()
alt Executor is TGocciaBytecodeExecutor (override)
BExecutor-->>Engine: True
Engine->>Engine: FStrictTypes := True
else Default executor implementation
Executor-->>Engine: False
Engine->>Engine: FStrictTypes := False
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/units/Goccia.Engine.pas (1)
589-592: Add regression coverage for executor-driven strict defaults.Lines 589-592 are correct, but this behavior shift should be locked with tests for both paths: interpreter/default executor (
False) and bytecode executor (True).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Engine.pas` around lines 589 - 592, Add regression tests that assert the FStrictTypes behavior when an executor is present vs nil: create two unit tests exercising the constructor/initialization logic that reads FExecutor and sets FStrictTypes (the code that checks Assigned(FExecutor) and uses FExecutor.DefaultStrictTypes). One test should leave FExecutor nil (or use the interpreter/default executor) and assert FStrictTypes = False; the other should attach a bytecode executor mock/instance where DefaultStrictTypes returns True and assert FStrictTypes = True. Use the engine class creating path that initializes FExecutor/FStrictTypes so the tests fail if the executor-driven default behavior changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@source/units/Goccia.Engine.pas`:
- Around line 589-592: Add regression tests that assert the FStrictTypes
behavior when an executor is present vs nil: create two unit tests exercising
the constructor/initialization logic that reads FExecutor and sets FStrictTypes
(the code that checks Assigned(FExecutor) and uses
FExecutor.DefaultStrictTypes). One test should leave FExecutor nil (or use the
interpreter/default executor) and assert FStrictTypes = False; the other should
attach a bytecode executor mock/instance where DefaultStrictTypes returns True
and assert FStrictTypes = True. Use the engine class creating path that
initializes FExecutor/FStrictTypes so the tests fail if the executor-driven
default behavior changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f291db8f-8dd9-4830-b866-7e9ef29cbfd1
📒 Files selected for processing (3)
source/units/Goccia.Engine.Backend.passource/units/Goccia.Engine.passource/units/Goccia.Executor.pas
Contributor
Benchmark Results364 benchmarks Interpreted: 🟢 28 improved · 🔴 102 regressed · 234 unchanged · avg -1.4% arraybuffer.js — Interp: 🔴 4, 10 unch. · avg -2.1% · Bytecode: 🟢 1, 🔴 10, 3 unch. · avg -6.9%
arrays.js — Interp: 🟢 1, 🔴 2, 16 unch. · avg -0.6% · Bytecode: 🔴 17, 2 unch. · avg -11.1%
async-await.js — Interp: 🔴 1, 5 unch. · avg -2.0% · Bytecode: 🔴 4, 2 unch. · avg -7.2%
base64.js — Interp: 🟢 1, 🔴 1, 8 unch. · avg -1.3% · Bytecode: 🔴 9, 1 unch. · avg -8.6%
classes.js — Interp: 🔴 6, 25 unch. · avg -1.1% · Bytecode: 🟢 1, 🔴 18, 12 unch. · avg -5.2%
closures.js — Interp: 🟢 3, 8 unch. · avg +1.5% · Bytecode: 🔴 10, 1 unch. · avg -8.3%
collections.js — Interp: 🟢 3, 🔴 5, 4 unch. · avg -0.4% · Bytecode: 🔴 12 · avg -6.0%
destructuring.js — Interp: 🟢 3, 🔴 1, 18 unch. · avg +0.3% · Bytecode: 🔴 19, 3 unch. · avg -9.5%
fibonacci.js — Interp: 🔴 2, 6 unch. · avg -0.5% · Bytecode: 🔴 5, 3 unch. · avg -6.4%
float16array.js — Interp: 🟢 4, 🔴 11, 17 unch. · avg -1.8% · Bytecode: 🟢 6, 🔴 19, 7 unch. · avg +0.9%
for-of.js — Interp: 🟢 2, 🔴 2, 3 unch. · avg -0.3% · Bytecode: 🔴 6, 1 unch. · avg -9.0%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 1, 🔴 17, 24 unch. · avg -1.4% · Bytecode: 🔴 36, 6 unch. · avg -6.9%
json.js — Interp: 🟢 2, 🔴 6, 12 unch. · avg -1.7% · Bytecode: 🔴 19, 1 unch. · avg -10.3%
jsx.jsx — Interp: 🔴 4, 17 unch. · avg -1.5% · Bytecode: 🔴 21 · avg -9.4%
modules.js — Interp: 🟢 1, 🔴 2, 6 unch. · avg -0.0% · Bytecode: 🔴 9 · avg -14.8%
numbers.js — Interp: 🟢 3, 🔴 2, 6 unch. · avg +0.1% · Bytecode: 🔴 10, 1 unch. · avg -9.2%
objects.js — Interp: 🔴 1, 6 unch. · avg -2.1% · Bytecode: 🔴 7 · avg -13.0%
promises.js — Interp: 🔴 1, 11 unch. · avg -1.0% · Bytecode: 🔴 12 · avg -13.1%
regexp.js — Interp: 🔴 2, 9 unch. · avg -2.3% · Bytecode: 🔴 11 · avg -5.7%
strings.js — Interp: 🔴 11, 8 unch. · avg -10.4% · Bytecode: 🟢 7, 🔴 10, 2 unch. · avg +14.6%
typed-arrays.js — Interp: 🔴 19, 3 unch. · avg -2.8% · Bytecode: 🟢 13, 🔴 3, 6 unch. · avg +8.7%
uint8array-encoding.js — Interp: 🟢 4, 🔴 2, 12 unch. · avg +3.8% · Bytecode: 🟢 3, 🔴 15 · avg -24.2%
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. |
- Verify interpreter engines default StrictTypes to false - Verify bytecode executors enable StrictTypes
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.StrictTypes.Test.pas`:
- Around line 45-47: Initialize object references to nil and open the try block
before calling constructors to make test setup exception-safe: set Source :=
nil; Executor := nil (or Engine := nil) at declaration, then begin try
immediately, then call Source := CreateEmptySource and Engine :=
TGocciaEngine.Create('<strict-test>', Source, []); finally free in the finally
block; similarly update the other occurrence (the block around lines 61-64) to
follow the same pattern so constructed objects are not leaked if a constructor
raises.
🪄 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: 564a8dbd-3561-49ce-a3dd-410b10f4ec5a
📒 Files selected for processing (1)
source/units/Goccia.Engine.StrictTypes.Test.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
DefaultStrictTypeshook to executors so engine initialization can derive the strict-type default from the active backend.Falsedefault for other executors.