Fix console.log TypeError on prototype-less objects, add --inspect-depth#578
Merged
Conversation
Extract shared Goccia.Values.Formatting unit with FormatForDisplay that renders values structurally without invoking user toString/valueOf, matching V8/Node/Bun/Deno behavior. Console and TestingLibrary both delegate to this single formatter. Remove ToDebugString from TGocciaObjectValue since FormatForDisplay subsumes it. Closes #536 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
📝 WalkthroughWalkthroughAdds a configurable inspect-depth option and new Goccia.Values.Formatting unit (depth-limited value formatter); wires the option into CLI and app init; refactors console and testing code to use FormatForDisplay; removes TGocciaObjectValue.ToDebugString; adds console tests and updates docs/examples. ChangesInspect Depth Configuration and Formatting Refactor
Sequence DiagramsequenceDiagram
participant UserCLI
participant AppInit
participant EngineOptions
participant Formatting
UserCLI->>AppInit: start with CLI/config
AppInit->>EngineOptions: read InspectDepth
AppInit->>Formatting: SetInspectDepth(value)
Formatting->>Formatting: set GInspectDepth
UserCLI->>Formatting: console/test formatting calls -> FormatForDisplay
Formatting->>UserCLI: formatted string
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Contributor
Suite TimingTest Runner (interpreted: 8,954 passed; bytecode: 8,954 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: 🟢 15 improved · 🔴 220 regressed · 172 unchanged · avg -2.9% arraybuffer.js — Interp: 🔴 7, 7 unch. · avg -3.0% · Bytecode: 🟢 4, 🔴 2, 8 unch. · avg +1.4%
arrays.js — Interp: 🔴 10, 9 unch. · avg -3.2% · Bytecode: 🔴 1, 18 unch. · avg +0.3%
async-await.js — Interp: 🔴 5, 1 unch. · avg -3.6% · Bytecode: 6 unch. · avg -0.6%
async-generators.js — Interp: 2 unch. · avg -2.8% · Bytecode: 2 unch. · avg +0.3%
base64.js — Interp: 🔴 3, 7 unch. · avg -2.0% · Bytecode: 🟢 2, 8 unch. · avg +1.9%
classes.js — Interp: 🔴 29, 2 unch. · avg -4.7% · Bytecode: 🟢 4, 🔴 2, 25 unch. · avg +0.6%
closures.js — Interp: 🔴 1, 10 unch. · avg -0.9% · Bytecode: 🟢 2, 🔴 1, 8 unch. · avg +0.4%
collections.js — Interp: 🟢 2, 🔴 4, 6 unch. · avg -1.4% · Bytecode: 🔴 3, 9 unch. · avg -0.9%
csv.js — Interp: 🔴 9, 4 unch. · avg -3.9% · Bytecode: 🟢 1, 12 unch. · avg +1.0%
destructuring.js — Interp: 🔴 16, 6 unch. · avg -4.1% · Bytecode: 🟢 5, 17 unch. · avg +1.4%
fibonacci.js — Interp: 🔴 1, 7 unch. · avg +0.1% · Bytecode: 🔴 1, 7 unch. · avg -1.6%
float16array.js — Interp: 🟢 2, 🔴 16, 14 unch. · avg -2.3% · Bytecode: 🟢 8, 🔴 2, 22 unch. · avg +1.5%
for-of.js — Interp: 🔴 3, 4 unch. · avg -2.6% · Bytecode: 🟢 2, 🔴 1, 4 unch. · avg +0.9%
generators.js — Interp: 🔴 1, 3 unch. · avg -1.0% · Bytecode: 🔴 2, 2 unch. · avg -1.9%
iterators.js — Interp: 🔴 20, 22 unch. · avg -2.5% · Bytecode: 🟢 19, 🔴 1, 22 unch. · avg +2.3%
json.js — Interp: 🟢 1, 🔴 12, 7 unch. · avg -4.4% · Bytecode: 🟢 3, 🔴 1, 16 unch. · avg +1.0%
jsx.jsx — Interp: 🔴 15, 6 unch. · avg -4.5% · Bytecode: 🟢 1, 🔴 3, 17 unch. · avg -0.4%
modules.js — Interp: 🔴 2, 7 unch. · avg -1.8% · Bytecode: 🔴 7, 2 unch. · avg -3.6%
numbers.js — Interp: 11 unch. · avg +0.2% · Bytecode: 🟢 6, 5 unch. · avg +2.3%
objects.js — Interp: 🔴 5, 2 unch. · avg -3.4% · Bytecode: 🟢 1, 🔴 1, 5 unch. · avg -0.8%
promises.js — Interp: 🔴 11, 1 unch. · avg -5.3% · Bytecode: 🟢 2, 10 unch. · avg +1.7%
regexp.js — Interp: 🔴 6, 5 unch. · avg -3.0% · Bytecode: 11 unch. · avg -0.9%
strings.js — Interp: 🔴 10, 9 unch. · avg -3.6% · Bytecode: 🟢 3, 🔴 2, 14 unch. · avg -0.7%
tsv.js — Interp: 🔴 6, 3 unch. · avg -4.9% · Bytecode: 🟢 1, 🔴 1, 7 unch. · avg -0.1%
typed-arrays.js — Interp: 🟢 6, 🔴 7, 9 unch. · avg +9.3% · Bytecode: 🟢 9, 🔴 2, 11 unch. · avg +3.3%
uint8array-encoding.js — Interp: 🟢 2, 🔴 14, 2 unch. · avg -2.8% · Bytecode: 🟢 3, 🔴 6, 9 unch. · avg -7.2%
weak-collections.js — Interp: 🟢 2, 🔴 7, 6 unch. · avg -15.1% · Bytecode: 🟢 5, 🔴 3, 7 unch. · avg -0.9%
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 |
Prevent SIGSEGV on circular or deeply nested objects by capping
recursion at 5 levels, rendering {...} / [...] at the limit.
Remove now-unused Goccia.Values.SymbolValue import from TestingLibrary.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rename the generic DoFormat helper to FormatRecursive for clarity per the project's explicitness convention. Switch FormatObject from GetEnumerablePropertyNames + per-key GetProperty to GetEnumerablePropertyEntries, avoiding N redundant hash lookups. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Match V8/Node/Deno output style:
- Truncation markers: [Object] and [Array] instead of {...} and [...]
- Spaced output: { a: 1 } instead of {a: 1} (empty objects stay {})
- Configurable depth via --inspect-depth CLI flag and goccia.json
(default: 5, stored in module-level var, set in InitializeSingletons)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Empty arrays now render as [] (matching Node) instead of [ ]. Add --inspect-depth to the run commands reference. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
frostney
changed the title
coderabbitai
Bot
added
bug
![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: 1
🧹 Nitpick comments (1)
source/units/Goccia.Values.Formatting.pas (1)
114-115: 💤 Low valueConsider escaping single quotes in nested string values.
When a nested string contains a single quote (e.g.,
"it's"), the current output will be'it's', which visually breaks the quoting. Consider escaping inner quotes as\'for clearer output:'it\'s'.This is a minor display issue and can be deferred if the current behavior is acceptable for console output.
Optional escaping improvement
else if ANested and (AValue is TGocciaStringLiteralValue) then - Result := '''' + AValue.ToStringLiteral.Value + '''' + Result := '''' + StringReplace(AValue.ToStringLiteral.Value, '''', '\''', [rfReplaceAll]) + ''''Note: This would require adding
StrUtilsto theusesclause forStringReplace.🤖 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.Values.Formatting.pas` around lines 114 - 115, The branch that generates nested string output in Goccia.Values.Formatting (the else-if handling ANested and AValue is TGocciaStringLiteralValue) must escape single quotes inside the string literal before surrounding it with outer quotes; update the logic that uses AValue.ToStringLiteral.Value to run a replace of ' with \' (e.g. via StringReplace) and then return the quoted escaped value, and add StrUtils to the uses clause so StringReplace is available.
🤖 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.Builtins.TestingLibrary.pas`:
- Around line 307-310: FormatForMessage currently calls FormatForDisplay and
returns quoted/display-formatted strings which breaks matcher logic (used in
toMatch, toContain, toHaveLength). Update FormatForMessage so it preserves raw
string semantics for string-typed TGocciaValue (return the underlying unquoted
string or length-sensitive representation) and only uses FormatForDisplay for
non-string types (or create a separate FormatForMatcher helper and switch
callers to it). Modify the implementation of FormatForMessage (and replace
usages in matcher code paths) to ensure substring and length checks operate on
the raw string value rather than the quoted/display format.
---
Nitpick comments:
In `@source/units/Goccia.Values.Formatting.pas`:
- Around line 114-115: The branch that generates nested string output in
Goccia.Values.Formatting (the else-if handling ANested and AValue is
TGocciaStringLiteralValue) must escape single quotes inside the string literal
before surrounding it with outer quotes; update the logic that uses
AValue.ToStringLiteral.Value to run a replace of ' with \' (e.g. via
StringReplace) and then return the quoted escaped value, and add StrUtils to the
uses clause so StringReplace is available.
🪄 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: 1b53fb1a-a2b8-4637-86ee-89a19c9c7626
📒 Files selected for processing (10)
AGENTS.mddocs/build-system.mdsource/app/Goccia.CLI.Application.passource/shared/CLI.Options.passource/units/Goccia.Builtins.Console.passource/units/Goccia.Builtins.TestingLibrary.passource/units/Goccia.Values.Formatting.passource/units/Goccia.Values.ObjectValue.Test.passource/units/Goccia.Values.ObjectValue.pastests/built-ins/console/log.js
💤 Files with no reviewable changes (1)
- source/units/Goccia.Values.ObjectValue.pas
Replace with existing First flag — after the loop, not First is equivalent to HasEntries. One fewer local to track. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove the trivial one-line wrapper and replace all 73 callsites in TestingLibrary with direct FormatForDisplay calls. FormatThrowValueDetail is kept — it has real logic (stack/message extraction from error objects). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
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.Builtins.TestingLibrary.pas (1)
311-324:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid reading arbitrary
message/stackproperties while formatting thrown values.This helper still calls
GetProperty(PROP_MESSAGE/PROP_STACK)on any thrown object. For non-error objects, that can execute user accessors while building the failure text, and a throwing getter will mask the original exception. Restrict the special case to real error objects (HasErrorData) or fall back toFormatForDisplayif property access fails.💡 Minimal hardening
function FormatThrowValueDetail(const AValue: TGocciaValue): string; var MsgValue, StackValue: TGocciaValue; begin - if AValue is TGocciaObjectValue then + if (AValue is TGocciaObjectValue) and TGocciaObjectValue(AValue).HasErrorData then begin - MsgValue := TGocciaObjectValue(AValue).GetProperty(PROP_MESSAGE); - StackValue := TGocciaObjectValue(AValue).GetProperty(PROP_STACK); + try + MsgValue := TGocciaObjectValue(AValue).GetProperty(PROP_MESSAGE); + StackValue := TGocciaObjectValue(AValue).GetProperty(PROP_STACK); + except + Exit(FormatForDisplay(AValue)); + end; if Assigned(StackValue) and (StackValue is TGocciaStringLiteralValue) and (TGocciaStringLiteralValue(StackValue).Value <> '') then Result := TGocciaStringLiteralValue(StackValue).Value else if Assigned(MsgValue) and (MsgValue is TGocciaStringLiteralValue) then Result := TGocciaStringLiteralValue(MsgValue).Value else Result := FormatForDisplay(AValue); end else Result := FormatForDisplay(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.Builtins.TestingLibrary.pas` around lines 311 - 324, The code calls TGocciaObjectValue.GetProperty(PROP_MESSAGE/PROP_STACK) for any thrown object which can execute user getters and mask the original exception; update the block that handles TGocciaObjectValue (in this file/function) to only attempt reading PROP_MESSAGE/PROP_STACK for bona fide error objects (e.g. when TGocciaObjectValue.HasErrorData returns true) and/or wrap each GetProperty call in a try..except that on exception falls back to FormatForDisplay(AValue); ensure you still prefer stack then message when safe but always return FormatForDisplay if property access fails or HasErrorData is false.
🤖 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.Values.Formatting.pas`:
- Around line 31-34: SetInspectDepth currently only enforces a minimum and can
be set arbitrarily large; change it to clamp ADepth between 1 and a reasonable
upper bound (e.g. INSPECT_DEPTH_MAX) before assigning to GInspectDepth.
Introduce a constant (INSPECT_DEPTH_MAX) near the existing declarations and
update procedure SetInspectDepth to use Min(Max(1, ADepth), INSPECT_DEPTH_MAX)
(referencing SetInspectDepth and GInspectDepth) so very large --inspect-depth
values cannot bypass the recursion guard.
- Around line 111-113: The current branch that handles nested
TGocciaStringLiteralValue in the formatting routine returns '''' +
AValue.ToStringLiteral.Value + '''' without escaping interior characters; update
the branch that checks "ANested and (AValue is TGocciaStringLiteralValue)" to
escape single quotes, backslashes and control characters in the string content
before surrounding with single quotes (use or add a small helper such as
EscapeForSingleQuoted or reuse an existing ToEscapedString/ToStringLiteral
method on TGocciaStringLiteralValue), ensuring the function that formats values
(the routine containing that branch) calls the escape helper on
AValue.ToStringLiteral.Value and then returns the quoted escaped result.
---
Outside diff comments:
In `@source/units/Goccia.Builtins.TestingLibrary.pas`:
- Around line 311-324: The code calls
TGocciaObjectValue.GetProperty(PROP_MESSAGE/PROP_STACK) for any thrown object
which can execute user getters and mask the original exception; update the block
that handles TGocciaObjectValue (in this file/function) to only attempt reading
PROP_MESSAGE/PROP_STACK for bona fide error objects (e.g. when
TGocciaObjectValue.HasErrorData returns true) and/or wrap each GetProperty call
in a try..except that on exception falls back to FormatForDisplay(AValue);
ensure you still prefer stack then message when safe but always return
FormatForDisplay if property access fails or HasErrorData is false.
🪄 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: 97365227-5ccc-4421-82c4-19c6f6da6727
📒 Files selected for processing (10)
AGENTS.mddocs/build-system.mdsource/app/Goccia.CLI.Application.passource/shared/CLI.Options.passource/units/Goccia.Builtins.Console.passource/units/Goccia.Builtins.TestingLibrary.passource/units/Goccia.Values.Formatting.passource/units/Goccia.Values.ObjectValue.Test.passource/units/Goccia.Values.ObjectValue.pastests/built-ins/console/log.js
💤 Files with no reviewable changes (1)
- source/units/Goccia.Values.ObjectValue.pas
A very large --inspect-depth on a circular object could overflow the native call stack. Cap at 64 which is far beyond practical use while keeping recursion safe. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use single quotes by default, double quotes when the string contains a single quote, backticks when it contains both. Matches V8/Node util.inspect output exactly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
Goccia.Values.Formattingunit withFormatForDisplaythat renders values structurally (like V8/Nodeutil.inspect) without invoking usertoString()/valueOf(). This fixesconsole.log(Object.create(null))throwingTypeErrorinstead of printing{}.FormatArgs,ConsoleAssert,ConsoleDir,ConsoleTable) and TestingLibrary (FormatForMessage) both delegate to the single shared formatter, eliminating duplicated formatting logic.TGocciaObjectValue.ToDebugString— its only consumer wasFormatForMessage, now superseded byFormatForDisplay.{ a: 1 }not{a: 1}),[Object]/[Array]truncation markers at depth limit, empty containers stay compact ({},[]).--inspect-depth=NCLI flag and"inspect-depth"config key (default: 5) to control maximum object inspection depth.Closes #536
Testing