GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — lib: fix modulo sign in webidl convertToInt by deepview-autofix · Pull Request #62940 · nodejs/node

Conversation

@deepview-autofix

@deepview-autofix deepview-autofix commented Apr 24, 2026 •
edited by ChALkeR
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 24, 2026
ChALkeR
ChALkeR reviewed Apr 24, 2026
View reviewed changes

@ChALkeR ChALkeR left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is mine

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ChALkeR
ChALkeR marked this pull request as ready for review April 24, 2026 23:12
@ChALkeR
ChALkeR force-pushed the deepview/415d17df0c branch from a9f91ee to 65d51ec Compare April 24, 2026 23:15
@ChALkeR ChALkeR mentioned this pull request Apr 24, 2026
fix(webidl): wrap negative values in ConvertToInt via two's complement nodejs/undici#5107
Open
ChALkeR
ChALkeR previously requested changes Apr 24, 2026
View reviewed changes

@ChALkeR ChALkeR left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

https://github.com/jsdom/webidl-conversions/blob/1c40227f0875ab17fbf5df0ea7590e837e4365c0/lib/index.js#L50

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ChALkeR
ChALkeR force-pushed the deepview/415d17df0c branch from 65d51ec to cae7693 Compare April 24, 2026 23:56
@ChALkeR
ChALkeR dismissed their stale review April 24, 2026 23:56

fixed

@ChALkeR
ChALkeR force-pushed the deepview/415d17df0c branch 2 times, most recently from 34e138f to 0f245d2 Compare April 25, 2026 00:18
@ChALkeR
ChALkeR marked this pull request as draft April 25, 2026 01:14
@codecov

codecov Bot commented Apr 25, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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 lines Coverage Δ
lib/internal/webidl.js 96.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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ChALkeR

ChALkeR commented Apr 25, 2026

Copy link
Copy Markdown
Member

Calculation at 64 bits is still off, will redo

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

ChALkeR commented Apr 26, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

I rewrote the whole thing and added more tests

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ChALkeR
ChALkeR marked this pull request as ready for review April 26, 2026 10:12
@ChALkeR
ChALkeR force-pushed the deepview/415d17df0c branch from 0c87f49 to a5ec26f Compare April 26, 2026 10:16
@ChALkeR @deepview-autofix
lib: fix modulo sign in webidl convertToInt
76ad558
Co-Authored-By: DeepView Autofix <[email protected]>
Assisted-by: Claude <[email protected]>
Signed-off-by: Nikita Skovoroda <[email protected]>
@ChALkeR
ChALkeR force-pushed the deepview/415d17df0c branch from a5ec26f to 76ad558 Compare April 26, 2026 10:37
ExE-Boss
ExE-Boss reviewed Apr 26, 2026
View reviewed changes

@ExE-Boss ExE-Boss left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/internal/webidl.js
// 9. Set x to IntegerPart(x).
x = integerPart(x);

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

@ExE-Boss ExE-Boss Apr 26, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ChALkeR

ChALkeR commented Apr 26, 2026

Copy link
Copy Markdown
Member

Closing in favor of #62979

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Reviewers

@ChALkeR ChALkeR ChALkeR left review comments
+1 more reviewer
@ExE-Boss ExE-Boss ExE-Boss left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@deepview-autofix @ChALkeR @ExE-Boss @nodejs-github-bot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.