Skip to content

Implement upsert methods for Map#4436

Merged
hansl merged 3 commits intoboa-dev:mainfrom
jasonmilad:main
Sep 26, 2025
Merged

Implement upsert methods for Map#4436
hansl merged 3 commits intoboa-dev:mainfrom
jasonmilad:main

Conversation

@jasonmilad
Copy link
Contributor

This Pull Request implements part of the Upsert proposal for Map.

It changes the following:

  • Adds Map.prototype.getOrInsert method.
  • Adds Map.prototype.getOrInsertComputed method.
  • Implements canonicalization of keys (-0+0) per the spec draft.
  • Includes new tests for both methods to ensure correct behavior.

@jasonmilad
Copy link
Contributor Author

#4436

@codecov
Copy link

codecov bot commented Sep 25, 2025

Codecov Report

❌ Patch coverage is 93.75000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.46%. Comparing base (6ddc2b4) to head (d1920c6).
⚠️ Report is 540 commits behind head on main.

Files with missing lines Patch % Lines
core/engine/src/builtins/map/mod.rs 92.85% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4436      +/-   ##
==========================================
+ Coverage   47.24%   51.46%   +4.22%     
==========================================
  Files         476      504      +28     
  Lines       46892    51343    +4451     
==========================================
+ Hits        22154    26424    +4270     
- Misses      24738    24919     +181     

☔ 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.

@nekevss nekevss added A-Enhancement New feature or request C-Builtins PRs and Issues related to builtins/intrinsics labels Sep 25, 2025
@nekevss nekevss requested a review from a team September 25, 2025 21:07
@hansl hansl self-requested a review September 25, 2025 21:11
Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

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

Good job so far! Two minor nits and one blocker which panics in the compliance tests.

This seems to fix some test262 compliance tests, so that's great! But also, we ignored the Upsert-specific tests, to re-enable them you need to remove these lines: https://github.com/boa-dev/boa/blob/main/test262_config.toml#L51-L54

And run the test262 suite (it might take a while):

cargo run --bin boa_tester -- run

You can also add the --suite test/built-ins/Map/prototype argument to only run Map-specific compliance tests.

Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

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

Two nits, one non-blocking and one I'd like to see fixed before merging. Otherwise LGTM and ready to merge (outside of those nits).

Also, note: force pushing the whole PR makes it hard to review. We don't use commit messages when merging into main (only the PR description), so just push new commits to PRs and I can see the diff clearer.

Thanks a lot!

r#"
const m = new Map();
let seenThis, seenKey;
const v = m.getOrInsertComputed(-0, function(k) { 'use strict'; seenThis = this; seenKey = k; return 'ok'; });
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking nit: no need for use strict.

Copy link
Contributor Author

@jasonmilad jasonmilad Sep 26, 2025

Choose a reason for hiding this comment

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

Without use strict, is this undefined?


features = [
### Unimplemented features:
### Unimplemented features:
Copy link
Contributor

Choose a reason for hiding this comment

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

Just realized that our editorconfig didn't have 4 spaces for indentation. I made #4446, but in the meantime, could you reconfigure your editor to have 4 spaces, then re-format this file? That way we can actually see what changed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely

Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

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

LGTM. To answer your question, it should not, no. Unless the function is bound of course.

Not a blocker IMO.

@hansl
Copy link
Contributor

hansl commented Sep 26, 2025

Good work @jasonmilad ! Thanks so much for the PR and the patience going through the review process.

@hansl hansl added this pull request to the merge queue Sep 26, 2025
Merged via the queue into boa-dev:main with commit 87fded1 Sep 26, 2025
16 checks passed
@nekevss nekevss mentioned this pull request Sep 26, 2025
23 tasks
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 C-Builtins PRs and Issues related to builtins/intrinsics

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants