Skip to content

lib: fix modulo sign in webidl convertToInt - #62940

Closed
deepview-autofix wants to merge 1 commit into
nodejs:mainfrom
deepview-autofix:deepview/415d17df0c
Closed

lib: fix modulo sign in webidl convertToInt#62940
deepview-autofix wants to merge 1 commit into
nodejs:mainfrom
deepview-autofix:deepview/415d17df0c

Conversation

@deepview-autofix

@deepview-autofixdeepview-autofix commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

The modulo() helper in lib/internal/webidl.js is documented to implement ECMA-262's "x modulo y" (result has the same sign as y), but it used the JS % operator directly, whose result takes the sign of x. convertToInt step 10 relies on modulo(x, 2^bitLength) producing a non-negative representative in [0, 2^bitLength); step 11 only handles the signed upper half, so negative results of % were returned unchanged.

As a result, negative inputs to unsigned WebIDL integer types returned the original negative value instead of the two's-complement wrap (e.g. convertToInt('v', -3, 8) returned -3 instead of 253), and signed inputs in the one-wrap band below the lower bound were returned as-is (convertToInt('v', -200, 8, { signed: true }) returned -200 instead of 56). This diverged from typed-array coercion.

@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 24, 2026

@ChALkeRChALkeR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is mine

ChALkeR
ChALkeR previously requested changes Apr 24, 2026

@ChALkeRChALkeR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ChALkeR
ChALkeRforce-pushed the deepview/415d17df0c branch from 65d51ec to cae7693CompareApril 24, 2026 23:56
@ChALkeR
ChALkeRforce-pushed the deepview/415d17df0c branch 2 times, most recently from 34e138f to 0f245d2CompareApril 25, 2026 00:18
@ChALkeR
ChALkeR marked this pull request as draft April 25, 2026 01:14
@codecov

codecovBot commented Apr 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.63%. Comparing base (4744070) to head (76ad558).
⚠️ Report is 18 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #62940 +/- ##
==========================================
- Coverage 89.66% 89.63% -0.03% 
==========================================
Files 706 706 Lines 219370 219424 +54 Branches 42065 42074 +9 ==========================================
- Hits 196694 196681 -13 - Misses 14586 14633 +47 - Partials 8090 8110 +20 
Files with missing linesCoverage Δ
lib/internal/webidl.js96.78% <100.00%> (+0.05%)⬆️

... and 55 files with indirect coverage changes

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

@ChALkeR

Copy link
Copy Markdown
Member

Calculation at 64 bits is still off, will redo

@ChALkeR
ChALkeRforce-pushed the deepview/415d17df0c branch 4 times, most recently from 598cfec to 0c87f49CompareApril 26, 2026 10:12
@ChALkeR

ChALkeR commented Apr 26, 2026

Copy link
Copy Markdown
Member

I rewrote the whole thing and added more tests

@ChALkeR
ChALkeR marked this pull request as ready for review April 26, 2026 10:12
@ChALkeR
ChALkeRforce-pushed the deepview/415d17df0c branch from 0c87f49 to a5ec26fCompareApril 26, 2026 10:16
Co-Authored-By: DeepView Autofix <276251120+deepview-autofix@users.noreply.github.com>
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Nikita Skovoroda <chalkerx@gmail.com>
@ChALkeR
ChALkeRforce-pushed the deepview/415d17df0c branch from a5ec26f to 76ad558CompareApril 26, 2026 10:37

@ExE-BossExE-Boss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

loose: opposite of tight
lose: the correct verb for this

// 9. Set x to IntegerPart(x).
x = integerPart(x);

// Following the exact algorithm looses precision on signed modulo in 64-bit

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Following the exact algorithm looses precision on signed modulo in 64-bit
// Following the exact algorithm loses precision on signed modulo in 64-bit

@ChALkeR

Copy link
Copy Markdown
Member

Closing in favor of #62979

@ChALkeRChALkeR closed this Apr 26, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@deepview-autofix@ChALkeR@ExE-Boss@nodejs-github-bot