Skip to content

fix(string): cleanup code in String.prototype.repeat#5017

Merged
jedel1043 merged 6 commits intoboa-dev:mainfrom
Tanush576:ecma-conformance-pr
Mar 16, 2026
Merged

fix(string): cleanup code in String.prototype.repeat#5017
jedel1043 merged 6 commits intoboa-dev:mainfrom
Tanush576:ecma-conformance-pr

Conversation

@Tanush576
Copy link
Contributor

Overview
This PR refactors the String.prototype.repeat implementation to strictly align with Section 22.1.3.16 of the ECMA-262 specification. The previous implementation used a generic error handling match; this update introduces explicit checks for Infinity and negative values as required by the abstract operations.

Spec Compliance Audit
I have mapped the implementation to the following specification steps:

Step 3: Let n be ? ToIntegerOrInfinity(count).

Step 4: If n < 0, throw a RangeError exception. (Implemented via explicit match)

Step 5: If n is +∞, throw a RangeError exception. (Implemented via PositiveInfinity check)

Changes
Replaced the catch-all error handling with an exhaustive match on the IntegerOrInfinity enum.

Added specific RangeError messages that distinguish between negative counts and infinite counts.

Used variable shadowing to simplify the logic flow from an Enum to a primitive i64 after validation.

Verification
I have verified the fix using a local test script across the following edge cases:

JavaScript
"abc".repeat(0); // Returns "" (Success)
"abc".repeat(-1); // Throws RangeError: count must be non-negative (Success)
"abc".repeat(Infinity); // Throws RangeError: count must be less than infinity (Success)
Note to Maintainers
I am a prospective GSoC 2026 applicant interested in the ECMAScript Conformance project. I chose this task to demonstrate my ability to translate ECMA-262 algorithms into idiomatic Rust using Boa's internal types. I am eager to contribute more to the engine's compliance suite!

@Tanush576 Tanush576 requested a review from a team as a code owner March 12, 2026 20:21
@github-actions
Copy link

github-actions bot commented Mar 12, 2026

Test262 conformance changes

Test result main count PR count difference
Total 52,963 52,963 0
Passed 50,073 50,070 -3
Ignored 2,072 2,072 0
Failed 818 821 +3
Panics 0 0 0
Conformance 94.54% 94.54% -0.01%
Broken tests (3):
test/intl402/DateTimeFormat/prototype/toStringTag/toStringTag.js (previously Passed)
test/intl402/DateTimeFormat/prototype/toStringTag/toString.js (previously Passed)
test/intl402/Number/prototype/toLocaleString/throws-same-exceptions-as-NumberFormat.js (previously Passed)

Tested main commit: 0253916a4bca6a19be0a66f5b50842f3894d81dd
Tested PR commit: 91a7f8e1d0e3d8dd5148662dc6d58faeff5806d5
Compare commits: 0253916...91a7f8e

@Tanush576
Copy link
Contributor Author

Tanush576 commented Mar 16, 2026

Hi @jedel1043 ! I hope you had a great weekend!
Just a gentle ping on this PR and my other open one (#5018). It looks like both are waiting for a maintainer to approve the GitHub Actions CI workflows. Could you please trigger the checks for them when you have a moment?
I'm standing by to make any updates or fix any formatting issues once the CI runs. Thank you!

@Tanush576 Tanush576 force-pushed the ecma-conformance-pr branch from 1922eaa to dc51cdb Compare March 16, 2026 16:55
@github-actions github-actions bot added Waiting On Review Waiting on reviews from the maintainers C-Tests Issues and PRs related to the tests. C-Builtins PRs and Issues related to builtins/intrinsics labels Mar 16, 2026
@codecov
Copy link

codecov bot commented Mar 16, 2026

Codecov Report

❌ Patch coverage is 88.88889% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.15%. Comparing base (6ddc2b4) to head (91a7f8e).
⚠️ Report is 870 commits behind head on main.

Files with missing lines Patch % Lines
core/engine/src/builtins/string/mod.rs 88.88% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5017       +/-   ##
===========================================
+ Coverage   47.24%   59.15%   +11.90%     
===========================================
  Files         476      563       +87     
  Lines       46892    62779    +15887     
===========================================
+ Hits        22154    37134    +14980     
- Misses      24738    25645      +907     

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

Copy link
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.

Looks great!

@jedel1043 jedel1043 enabled auto-merge March 16, 2026 19:51
@jedel1043 jedel1043 changed the title feat(string): align String.prototype.repeat with ECMA-262 spec feat(string): cleanup code in String.prototype.repeat Mar 16, 2026
@jedel1043 jedel1043 changed the title feat(string): cleanup code in String.prototype.repeat fix(string): cleanup code in String.prototype.repeat Mar 16, 2026
@jedel1043 jedel1043 disabled auto-merge March 16, 2026 19:52
@jedel1043 jedel1043 enabled auto-merge March 16, 2026 19:52
auto-merge was automatically disabled March 16, 2026 22:19

Head branch was pushed to by a user without write access

@Tanush576
Copy link
Contributor Author

Hey @jedel1043, I pulled down your web edits, fixed the minor syntax/doc comment error, ran cargo fmt, and pushed it back up. The CI checks are all green and good to go!

@jedel1043 jedel1043 added this pull request to the merge queue Mar 16, 2026
@jedel1043
Copy link
Member

yeah that's what I get for trying to fix this through my phone 🤣🤣🤣
Thank you for the fix!

Merged via the queue into boa-dev:main with commit c7ee7e0 Mar 16, 2026
22 checks passed
@github-actions github-actions bot removed the Waiting On Review Waiting on reviews from the maintainers label Mar 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-Builtins PRs and Issues related to builtins/intrinsics C-Tests Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants