Add Map#getOrInsert[Computed], Error.isError, Math.sumPrecise#34
Merged
Conversation
…ath.sumPrecise`
Contributor
📝 WalkthroughWalkthroughAdds three built-in APIs: Math.sumPrecise (compensated summation), Error.isError (detect Error instances via error metadata), and Map.prototype.getOrInsert / getOrInsertComputed (upsert helpers). Implements runtime support, updates object error-data tracking, and adds comprehensive tests and docs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
frostney
changed the title
Map#getOrInsert, Map#getOrInsertComputed, Error.isError, `M…Map#getOrInsert[Computed], Error.isError, Math.sumPrecise
![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
🧹 Nitpick comments (1)
tests/built-ins/Error/isError.js (1)
6-6: Consider adding GocciaScript feature gate for consistency.The
Map.prototype.getOrInsertandMap.prototype.getOrInsertComputedtests usedescribe.runIf(isGocciaScript)to conditionally run tests in GocciaScript environments. This test file lacks that pattern.If
Error.isErroris intended to be a GocciaScript-specific feature (ES2026 proposal), consider adding the feature gate for consistency:💡 Suggested addition
+const isGocciaScript = typeof GocciaScript !== "undefined"; + -describe("Error.isError", () => { +describe.runIf(isGocciaScript)("Error.isError", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Error/isError.js` at line 6, The test suite for Error.isError should be gated behind the GocciaScript feature flag like other ES2026-specific tests; wrap or replace the top-level describe("Error.isError", ...) with describe.runIf(isGocciaScript)("Error.isError", ...) (or add a surrounding conditional using isGocciaScript) so the tests only run when the feature is enabled, using the same pattern as Map.prototype.getOrInsert/Map.prototype.getOrInsertComputed and referencing describe.runIf and isGocciaScript to locate where to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/built-ins.md`:
- Around line 142-146: Update the Math.sumPrecise documentation to explicitly
state its behavior for non-iterable inputs: mention that
Math.sumPrecise(iterable) throws a TypeError when the provided argument is not
an iterable (e.g., null, undefined, numbers, plain objects), keeping this
alongside the existing contract lines about non-number elements and empty
iterables; refer to the Math.sumPrecise symbol in the sentence so readers can
find the behavior in the API docs.
In `@tests/built-ins/Math/sumPrecise.js`:
- Around line 56-67: Update the tests for Math.sumPrecise to assert TypeError
explicitly instead of a generic throw: replace the generic expect(...).toThrow()
calls in the "throws TypeError for non-number elements" and "throws TypeError
for non-iterable argument" tests with expect(...).toThrow(TypeError) (or the
equivalent explicit TypeError assertion) for each case touching Math.sumPrecise
so the test verifies the contract requires TypeError for invalid inputs.
---
Nitpick comments:
In `@tests/built-ins/Error/isError.js`:
- Line 6: The test suite for Error.isError should be gated behind the
GocciaScript feature flag like other ES2026-specific tests; wrap or replace the
top-level describe("Error.isError", ...) with
describe.runIf(isGocciaScript)("Error.isError", ...) (or add a surrounding
conditional using isGocciaScript) so the tests only run when the feature is
enabled, using the same pattern as
Map.prototype.getOrInsert/Map.prototype.getOrInsertComputed and referencing
describe.runIf and isGocciaScript to locate where to change.
Contributor
Benchmark Results179 benchmarks · 🟢 62 improved · 🔴 20 regressed · 97 unchanged · avg +4.6% arrays.js — 🟢 14 improved, 5 unchanged · avg +15.1%
classes.js — 🟢 4 improved, 11 unchanged · avg +4.9%
closures.js — 🟢 6 improved, 5 unchanged · avg +7.6%
collections.js — 🟢 1 improved, 11 unchanged · avg +2.7%
destructuring.js — 🟢 12 improved, 2 unchanged · avg +18.2%
fibonacci.js — 🟢 4 improved, 2 unchanged · avg +9.7%
iterators.js — 🟢 10 improved, 10 unchanged · avg +5.1%
json.js — 🟢 3 improved, 17 unchanged · avg +2.2%
jsx.jsx — 🔴 13 regressed, 8 unchanged · avg -7.3%
numbers.js — 🟢 6 improved, 5 unchanged · avg +7.2%
objects.js — 🟢 2 improved, 🔴 1 regressed, 4 unchanged · avg +3.2%
promises.js — 12 unchanged · avg +0.2%
strings.js — 🔴 6 regressed, 5 unchanged · avg -6.4%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/built-ins.md (1)
454-455: Consider expanding the Map upsert methods documentation.The current documentation is brief but functional. For better developer experience, consider adding details about:
- Return values (does
getOrInsertreturn the existing value when key is present, or the default?)- Callback signature for
getOrInsertComputed(is the key passed as an argument?)- Error handling (what happens if the callback throws?)
- Mutation semantics (clarify these are mutating operations)
📝 Example expanded documentation
-| `map.getOrInsert(key, default)` | Return value for key if present; otherwise insert default and return it (TC39 proposal-upsert) | -| `map.getOrInsertComputed(key, cb)` | Return value for key if present; otherwise call `cb(key)`, insert result, and return it (TC39 proposal-upsert) | +| `map.getOrInsert(key, default)` | Return the value for `key` if present; otherwise insert `default` as the value for `key` and return `default`. Mutates the map. (TC39 proposal-upsert) | +| `map.getOrInsertComputed(key, cb)` | Return the value for `key` if present; otherwise call `cb(key)`, insert the result as the value for `key`, and return it. Mutates the map. Throws if `cb` is not callable or if `cb` throws. (TC39 proposal-upsert) |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/built-ins.md` around lines 454 - 455, Update the Map upsert method docs for `map.getOrInsert(key, default)` and `map.getOrInsertComputed(key, cb)` to explicitly state that `getOrInsert` returns the existing value if the key is present and otherwise inserts and returns the provided default; that `getOrInsertComputed` receives the key as its single argument (`cb(key)`), its return value is inserted and returned; clarify that both methods mutate the Map in-place; describe error behavior (if `cb` throws, no insertion occurs and the exception propagates); and add a brief usage example showing both success and error cases to make semantics clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/built-ins.md`:
- Around line 326-331: Update the docs for Error.isError to reflect the actual
brand-check used by the implementation: change the description for
Error.isError(arg) to state it returns true when arg has the internal
[[ErrorData]] slot (per ES2026 §20.5.2.1) rather than saying its prototype chain
includes Error.prototype, and mention this is a resilient brand check that
cannot be spoofed.
---
Nitpick comments:
In `@docs/built-ins.md`:
- Around line 454-455: Update the Map upsert method docs for
`map.getOrInsert(key, default)` and `map.getOrInsertComputed(key, cb)` to
explicitly state that `getOrInsert` returns the existing value if the key is
present and otherwise inserts and returns the provided default; that
`getOrInsertComputed` receives the key as its single argument (`cb(key)`), its
return value is inserted and returned; clarify that both methods mutate the Map
in-place; describe error behavior (if `cb` throws, no insertion occurs and the
exception propagates); and add a brief usage example showing both success and
error cases to make semantics clear.
Comment on lines
+326
to
+331
| **Static methods:** | ||
|
|
||
| | Method | Description | | ||
| |--------|-------------| | ||
| | `Error.isError(arg)` | Returns `true` if `arg` is an Error instance (prototype chain includes `Error.prototype`), `false` otherwise (ES2026) | | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the Error.isError implementation
rg -n 'ErrorIsError|error.*isError' -i --type=pascal
# Search in builtins for Error methods
fd -e pas | xargs grep -l 'Error' | grep -i builtin
# Look for Error object creation and methods
rg -n 'class Error|Error.*method' -i --type=pascalRepository: frostney/GocciaScript
Length of output: 4024
🏁 Script executed:
# Read the ErrorIsError implementation
sed -n '202,220p' units/Goccia.Builtins.Globals.pasRepository: frostney/GocciaScript
Length of output: 799
🏁 Script executed:
# Verify HasErrorData property in TGocciaObjectValue
rg -n 'HasErrorData|FErrorData' units/Goccia.Values.ObjectValue.pas | head -20Repository: frostney/GocciaScript
Length of output: 173
Update Error.isError() documentation to match implementation.
The documentation describes the method as checking if arg's "prototype chain includes Error.prototype", but the actual implementation uses [[ErrorData]] internal slot for brand checking (as per ES2026 §20.5.2.1). Update line 330 to clarify that it returns true if arg has the [[ErrorData]] internal slot, which is the reliable brand check mechanism used in the codebase and cannot be spoofed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/built-ins.md` around lines 326 - 331, Update the docs for Error.isError
to reflect the actual brand-check used by the implementation: change the
description for Error.isError(arg) to state it returns true when arg has the
internal [[ErrorData]] slot (per ES2026 §20.5.2.1) rather than saying its
prototype chain includes Error.prototype, and mention this is a resilient brand
check that cannot be spoofed.
3 tasks
This was referenced Apr 12, 2026
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.
…ath.sumPrecise`
Summary by CodeRabbit
New Features
Tests
Documentation