Skip to content

Conversation

A-D-E-A
Copy link

@A-D-E-A A-D-E-A commented May 21, 2025

Removes the impossible "if (lenB === null)" check, since it is impossible for it to be null because of the "& 0xff" on the previous line.

According the the spec (https://262.ecma-international.org/#sec-numberbitwiseop), the result of this._buf[offset++] & 0xff (call to the internal NumberBitwiseOp(&, this._buf[offset++], 0xff)) must be an integer unless it throws. It may throw because ToInt32 (https://262.ecma-international.org/#sec-toint32) calls ToNumber (https://262.ecma-international.org/#sec-tonumber) which throws if the argument is a Symbol or BigInt. I tested many combinations of thing & 0xff in multiple versions of Node; and have not yet found a case where this part of the spec is false.

Since it may never be null, the lines 93 and 94 that say if (lenB === null) return null are basically a no-op.
Removing it should not be a breaking change for anyone.

It's not much, but it may shave off a few bytes and CPU cycles.

removes the impossible "if (lenB === null)" check, since it is impossible for it to be null because of the "& 0xff" on the previous line.
@danmcd
Copy link

danmcd commented May 21, 2025

1.) This reads like an AI hallucination; it might even be one.

2.) Even if 1 is an overblown concern, I see no testing notes here.

3.) Speaking of testing, how did you encounter this problem in the first place?

@A-D-E-A
Copy link
Author

A-D-E-A commented May 21, 2025

This is not an AI hallunication, sorry, I'm just not that good at writing.

I'm not sure what testing notes you'd like. Basically, my tests consisted of the following:

var toTest = [
 [],
 {},
 Buffer.alloc(0),
 0,
 -0,
 NaN,
 null,
 undefined,
 [,][0],
 /_/,
 true,
 false,
 "str",
 ... // Many other things except Symbol and BigInt which crash
];
for (var v of toTest) {
  if (v & 0xff === null)
    console.log("Found a specific case in which the '=== null' is important");
}

I tested that for many versions of node (I don't remember exactly which as I did it manually, but at least each major since 10 worked on my computer); none of which logged. I could make a setup to test each node version since 1 to be absolutely sure.

I encountered the "problem" (not really a problem, just a weird and seemingly useless line) by reading the code. I don't know a lot about ASN.1 and BER, and I have to use it for an internal project. I often try to rewrite a library (my code and the official library side-by-side) in my codestyle to better understand the internals.
That's when I saw that line and tried to understand it. I thought that it should never be null, so I checked the spec to confirm that (went on the ecma spec, ctrl+f "Bitwise" and went up my way from there to what I put in the first comment). Then I tested it like I said above.

I don't think a new test case/file is needed, because it would be more of a language implementation issue than a library one. But I might be wrong.

@danmcd
Copy link

danmcd commented May 22, 2025

We use this on versions of node well before 10 internally, so 10 or later is not sufficient.

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.

2 participants