Skip to content

fix(webidl): correct signed integer bounds in ConvertToInt#5038

Merged
KhafraDev merged 1 commit intonodejs:mainfrom
deepview-autofix:deepview/37f241ab70
Apr 16, 2026
Merged

fix(webidl): correct signed integer bounds in ConvertToInt#5038
KhafraDev merged 1 commit intonodejs:mainfrom
deepview-autofix:deepview/37f241ab70

Conversation

@deepview-autofix
Copy link
Copy Markdown
Contributor

The signed integer lower bound was computed as Math.pow(-2, bitLength) - 1, which evaluates to (-2)^bitLength - 1 instead of the spec-required -2^(bitLength - 1). The step 11 overflow threshold similarly used 2^bitLength - 1 instead of 2^(bitLength - 1), causing values in [2^(bitLength-1), 2^bitLength - 2] to skip the signed wrap. Fix both expressions and update the surrounding comments to match the WebIDL spec.

The signed integer lower bound was computed as `Math.pow(-2, bitLength) - 1`,
which evaluates to `(-2)^bitLength - 1` instead of the spec-required
`-2^(bitLength - 1)`. The step 11 overflow threshold similarly used
`2^bitLength - 1` instead of `2^(bitLength - 1)`, causing values in
`[2^(bitLength-1), 2^bitLength - 2]` to skip the signed wrap. Fix both
expressions and update the surrounding comments to match the WebIDL spec.
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: DeepView Autofix <276251120+deepview-autofix@users.noreply.github.com>
Co-Authored-By: Nikita Skovoroda <chalkerx@gmail.com>
Signed-off-by: Nikita Skovoroda <chalkerx@gmail.com>
@deepview-autofix deepview-autofix marked this pull request as draft April 16, 2026 15:57
@deepview-autofix deepview-autofix marked this pull request as ready for review April 16, 2026 15:59
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.03%. Comparing base (bc0a19c) to head (90d3c3d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5038      +/-   ##
==========================================
- Coverage   93.03%   93.03%   -0.01%     
==========================================
  Files         110      110              
  Lines       35793    35793              
==========================================
- Hits        33301    33299       -2     
- Misses       2492     2494       +2     

☔ 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
Copy Markdown
Member

@KhafraDev KhafraDev left a comment

Choose a reason for hiding this comment

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

the comments come from https://webidl.spec.whatwg.org/#abstract-opdef-converttoint, the changes to them should be reverted

@ChALkeR
Copy link
Copy Markdown
Member

ChALkeR commented Apr 16, 2026

@KhafraDev No.

The comments were copy-pasted incorrectly originally due to them including formatting.

It's the main that has comments mismatching the spec, not the PR.

Please check the spec you linked

@KhafraDev
Copy link
Copy Markdown
Member

KhafraDev commented Apr 16, 2026

I did, for example in the reference image, there are no parenthesis:
image

The comments here are meant to match the spec -- not to dictate the correct behavior necessarily, but to make it easier (mostly for me) to match what code matches the instruction in the spec. Makes it easier to refactor whenever the specs change.

@KhafraDev
Copy link
Copy Markdown
Member

I see what you mean though

@ChALkeR
Copy link
Copy Markdown
Member

ChALkeR commented Apr 16, 2026

@KhafraDev 2^bitLength - 1 - 1 (as was in main) does not correctly represent that screenshot.

Also, you already added ^, why omit parenthesis?

@KhafraDev
Copy link
Copy Markdown
Member

Yeah it appears as though it is wrong in both cases unfortunately

@ChALkeR
Copy link
Copy Markdown
Member

ChALkeR commented Apr 16, 2026

it is wrong in both cases unfortunately

WDYM?

@KhafraDev
Copy link
Copy Markdown
Member

When refactoring (especially in the fetch spec) one of the best parts of the undici code base is the ability to copy a comment and CTRL+F to find where exactly a step was moved/changed to/from in the spec. It makes refactoring much easier from what I've found. In this case, I'd prefer to keep webidl comments the same, but they should also be readable (and the spec doesn't really ever change, due to its nature).

@KhafraDev KhafraDev merged commit d2e8178 into nodejs:main Apr 16, 2026
34 checks passed
@ChALkeR
Copy link
Copy Markdown
Member

ChALkeR commented Apr 16, 2026

and CTRL+F to find where exactly a step was moved

That already doesn't work as ^ was inserted.

@KhafraDev
Copy link
Copy Markdown
Member

Which is why I approved, main is wrong, it's not your responsibility to fix an unrelated 'issue' in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants