Skip to content

Improve TypeError diagnostics in WeakMap, WeakSet and Object builtins and update tests#4863

Merged
jedel1043 merged 7 commits intoboa-dev:mainfrom
mrhapile:improve-typeerror-diagnostics
Mar 13, 2026
Merged

Improve TypeError diagnostics in WeakMap, WeakSet and Object builtins and update tests#4863
jedel1043 merged 7 commits intoboa-dev:mainfrom
mrhapile:improve-typeerror-diagnostics

Conversation

@mrhapile
Copy link
Copy Markdown
Contributor

@mrhapile mrhapile commented Mar 4, 2026

Improve TypeError diagnostics in WeakMap, WeakSet and Object builtins and update tests

This Pull Request fixes/closes #.

It changes the following:

  • Improve TypeError messages in WeakMap, WeakSet, and Object builtins to provide clearer diagnostics.
  • Replace generic error messages such as "called with non-object value" and "Expected an object" with explicit messages indicating the expected receiver type.
  • Standardize error message format to:
    <Builtin>.<method>: expected 'this' to be a <Type> object
  • Update unit tests to match the improved error messages.

Before

Some errors used generic messages that did not clearly indicate which builtin method failed or what type was expected.

Examples:

WeakMap.get: called with non-object value
WeakMap.getOrInsertComputed: called with non-object value
Object prototype may only be an Object or null: undefined
Expected an object

These messages lacked clear context about the failing method or the expected this value.

After

Error messages now explicitly describe the expected receiver type and the builtin method where the error occurred.

Examples:

WeakMap.prototype.get: expected 'this' to be a WeakMap object
WeakMap.prototype.getOrInsert: expected 'this' to be a WeakMap object
WeakMap.prototype.getOrInsertComputed: expected 'this' to be a WeakMap object
Object.create: expected 'proto' to be an Object or null, got `undefined`
Object.defineProperty: expected 'this' to be an Object
Object.defineProperties: expected 'this' to be an Object

Files Modified

Engine changes

  • core/engine/src/builtins/weak_map/mod.rs
  • core/engine/src/builtins/weak_set/mod.rs
  • core/engine/src/builtins/object/mod.rs

These files were updated to replace generic TypeError messages with more descriptive diagnostics.

Test updates

To keep the test suite consistent with the improved diagnostics, expected error strings were updated in:

  • core/engine/src/builtins/weak_map/tests.rs
  • core/engine/src/builtins/object/tests.rs

No behavioral changes were introduced; only error message diagnostics were improved and tests updated accordingly.

… and update tests

Signed-off-by: mrhapile <allinonegaming3456@gmail.com>
@mrhapile mrhapile requested a review from a team as a code owner March 4, 2026 22:21
Copilot AI review requested due to automatic review settings March 4, 2026 22:21
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves TypeError diagnostics for receiver/argument validation across the WeakMap, WeakSet, and Object builtins, and updates unit tests to assert the new, more explicit error messages.

Changes:

  • Update WeakMap/WeakSet prototype methods to emit clearer “expected 'this' …” TypeErrors when called with an invalid receiver.
  • Improve Object builtin TypeError messages (e.g., Object.create, Object.setPrototypeOf, legacy __defineGetter__/__defineSetter__, etc.).
  • Update unit tests to match the revised diagnostics.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
core/engine/src/builtins/weak_set/mod.rs Updates WeakSet prototype receiver TypeError messages to be more explicit.
core/engine/src/builtins/weak_map/mod.rs Updates WeakMap prototype receiver TypeError messages to be more explicit.
core/engine/src/builtins/weak_map/tests.rs Adjusts WeakMap tests to assert the new receiver TypeError strings.
core/engine/src/builtins/object/mod.rs Refines TypeError messages for several Object builtins (create/setPrototypeOf/defineProperty/defineProperties/legacy accessors).
core/engine/src/builtins/object/tests.rs Updates Object.create error-message assertions to match the new wording.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread core/engine/src/builtins/object/mod.rs Outdated
Comment on lines 780 to 782
Err(JsNativeError::typ()
.with_message("Object.defineProperty called on non-object")
.with_message("Object.defineProperty: expected 'this' to be an Object")
.into())
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TypeError message here refers to "'this'" being an Object, but this check is validating the first argument (args[0]) is an object. This makes the diagnostic misleading (the call receiver is ignored for this builtin). Consider updating the message to refer to the object/target parameter (and ideally include the received type) instead of 'this'.

Copilot uses AI. Check for mistakes.
Comment thread core/engine/src/builtins/object/mod.rs Outdated
} else {
Err(JsNativeError::typ()
.with_message("Expected an object")
.with_message("Object.defineProperties: expected 'this' to be an Object")
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as Object.defineProperty: the error message says it expected 'this' to be an Object, but the code is rejecting non-object first arguments. Updating the message to reference the first parameter (and possibly the received type) would make the diagnostics accurate.

Suggested change
.with_message("Object.defineProperties: expected 'this' to be an Object")
.with_message("Object.defineProperties: expected first argument to be an object")

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 4, 2026

Test262 conformance changes

Test result main count PR count difference
Total 52,963 52,963 0
Passed 49,902 49,902 0
Ignored 2,222 2,222 0
Failed 839 839 0
Panics 0 0 0
Conformance 94.22% 94.22% 0.00%

Tested main commit: 55aed13ba6afc4c1f93b74348b225549e5ed713c
Tested PR commit: fef881c2ae389c8d5d8ad4861ed879f5a539441c
Compare commits: 55aed13...fef881c

@hansl
Copy link
Copy Markdown
Contributor

hansl commented Mar 4, 2026

Have you thought refactoring to use the js_error! macro instead?

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 4, 2026

Codecov Report

❌ Patch coverage is 32.69231% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.37%. Comparing base (6ddc2b4) to head (fef881c).
⚠️ Report is 829 commits behind head on main.

Files with missing lines Patch % Lines
core/engine/src/builtins/object/mod.rs 12.00% 22 Missing ⚠️
core/engine/src/builtins/weak_map/mod.rs 57.89% 8 Missing ⚠️
core/engine/src/builtins/weak_set/mod.rs 37.50% 5 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #4863       +/-   ##
===========================================
+ Coverage   47.24%   58.37%   +11.12%     
===========================================
  Files         476      559       +83     
  Lines       46892    61409    +14517     
===========================================
+ Hits        22154    35846    +13692     
- Misses      24738    25563      +825     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: mrhapile <allinonegaming3456@gmail.com>
@mrhapile
Copy link
Copy Markdown
Contributor Author

mrhapile commented Mar 5, 2026

Have you thought refactoring to use the js_error! macro instead?

I’ve refactored the error construction to use the js_error! macro instead of JsNativeError::typ().with_message(...) across the affected files. This also allowed removing the now-unused JsNativeError imports. All tests pass after the refactor. @hansl

@mrhapile
Copy link
Copy Markdown
Contributor Author

mrhapile commented Mar 5, 2026

@hansl Could you please review this? I’d like to make any necessary changes based on your feedback.

Copy link
Copy Markdown
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the error messages look much better!

@jedel1043 jedel1043 added this pull request to the merge queue Mar 13, 2026
@jedel1043 jedel1043 added A-Enhancement New feature or request A-Internal Changes that don't modify execution behaviour labels Mar 13, 2026
Merged via the queue into boa-dev:main with commit 2e6d209 Mar 13, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Enhancement New feature or request A-Internal Changes that don't modify execution behaviour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants