feat: Catch more unsafe numbers #71

hildjj · 2024-12-03T19:28:39Z

Prerequisites checklist

I have read the contributing guidelines.

What is the purpose of this pull request?

Catch subnormals, numbers that are flushed to zero,
and overly-large integers in no-unsafe-values.

What changes did you make? (Give an overview)

Added three numeric cases to no-unsafe-values.js:

Flushed to zero: the parsed value is zero, but the original text implies that zero was not intended. Example: 1e-400
Overly-large integers: the original text looks like an integer (no decimal point or exponent), but the value is outside the range [Number.MIN_SAFE_INTEGER, Number.MAX_SAFE_INTEGER]. Example: 9007199254740992
Subnormals: these are numbers that have reduced precision in an IEEE-754 f64. Example: 2.2250738585072009e-308

Related Issues

Refs #68

Is there anything you'd like reviewers to focus on?

Please double-check my error messages for your preferences.

nzakas

Nice job on this. I just left a few notes mostly for cleanup.

nzakas · 2024-12-03T20:19:37Z

src/rules/no-unsafe-values.js

@@ -3,6 +3,10 @@
 * @author Bradley Meck Farias
 */

+// These numbers evaluated to zero unexpectedly.  If the group's digits
+// are all zero, that's fine.
+const SMALL_NUMBERS = [/\.(\d+)/u, /((?:\d+\.)?\d+)e/iu];


Do we need to match the entire number sequence? If not, can you add some comments explaining what patterns these regexes are meant to match?

Suggested change

const SMALL_NUMBERS = [/\.(\d+)/u, /((?:\d+\.)?\d+)e/iu];

const SMALL_NUMBERS = [/^\.(\d+)$/u, /^((?:\d+\.)?\d+)e$/iu];

Those are intentionally not anchored. I'll add some comments and examples inline there to make it more clear.

I've just rewritten this whole section to make it more clear and less error-prone, I hope. I just translated the ABNF for number into an anchored regex.

nzakas · 2024-12-03T20:23:35Z

src/rules/no-unsafe-values.js

@@ -13,6 +17,8 @@ export default {

 		messages: {
 			unsafeNumber: "Number outside safe range found.",
+			unsafeInteger: "Integer outside safe range found.",
+			subnormal: "Subnormal numbers are outside the safe range.",


I think we need to give folks a bit more here because I'm guessing a lot of people don't know what "subnormal" means.

Suggested change

subnormal: "Subnormal numbers are outside the safe range.",

subnormal: "Unexpected subnormal number '{{value}}` found.",

Agree, almost nobody deals with these enough, which is why I think this particular lint is so valuable. Perhaps a link to https://en.wikipedia.org/wiki/Subnormal_number as well?

We generally don't put links in the error messages, but I think it would be good to add into the rule description. (At some point we'll create a page for each rule, and that would be a good place to go into more detail.)

Whoops, I didn't see this until I just sent the update. Willfix.

nzakas · 2024-12-03T20:26:34Z

src/rules/no-unsafe-values.js

+						for (const r of SMALL_NUMBERS) {
+							const m = txt.match(r);
+							// If the digits are all 0, it's ok.
+							if (m?.[1]?.match(/[1-9]/u)) {


Just eliminating one-character variables.

Suggested change

for (const r of SMALL_NUMBERS) {

const m = txt.match(r);

// If the digits are all 0, it's ok.

if (m?.[1]?.match(/[1-9]/u)) {

for (const pattern of SMALL_NUMBERS) {

const match = txt.match(pattern);

// If the digits are all 0, it's ok.

if (match?.[1]?.match(/[1-9]/u)) {

nzakas · 2024-12-03T20:27:47Z

src/rules/no-unsafe-values.js

+						const ab = new ArrayBuffer(8);
+						const dv = new DataView(ab);
+						dv.setFloat64(0, node.value, false);
+						const bi = dv.getBigUint64(0, false);
+						// Subnormals have an 11-bit exponent of 0 and a non-zero mantissa.
+						if ((bi & 0x7ff0000000000000n) === 0n) {


Expanding variable names.

Suggested change

const ab = new ArrayBuffer(8);

const dv = new DataView(ab);

dv.setFloat64(0, node.value, false);

const bi = dv.getBigUint64(0, false);

// Subnormals have an 11-bit exponent of 0 and a non-zero mantissa.

if ((bi & 0x7ff0000000000000n) === 0n) {

const buffer = new ArrayBuffer(8);

const view = new DataView(buffer);

view.setFloat64(0, node.value, false);

const bigInt = view.getBigUint64(0, false);

// Subnormals have an 11-bit exponent of 0 and a non-zero mantissa.

if ((bigInt & 0x7ff0000000000000n) === 0n) {

nzakas · 2024-12-03T21:43:08Z

src/rules/no-unsafe-values.js

@@ -13,6 +19,11 @@ export default {

 		messages: {
 			unsafeNumber: "Number outside safe range found.",
+			unsafeInteger: "Integer outside safe range found.",
+			unsafeZero:
+				"This number will evaluate to zero, which is unintended.",


Can we include the number to make this a bit clearer?

Suggested change

"This number will evaluate to zero, which is unintended.",

"The number '{{ value }}` will evaluate to zero.",

There's nothing interesting in the number. It's either going to be exactly the same as the highlighted text (if we use txt) or 0 (if we use node.value)

That's true when people are using ESLint in an editor. People also use ESLint in their CI system, where it just outputs messages onto the terminal. In that case, having the value in the message is really helpful.

nzakas · 2024-12-03T21:44:25Z

src/rules/no-unsafe-values.js

+			unsafeZero:
+				"This number will evaluate to zero, which is unintended.",
+			subnormal:
+				"Subnormal numbers (see https://en.wikipedia.org/wiki/Subnormal_number) are outside the safe range: '{{ value }}'.",


We don't put URLs in messages as that makes the message more difficult to read.

Suggested change

"Subnormal numbers (see https://en.wikipedia.org/wiki/Subnormal_number) are outside the safe range: '{{ value }}'.",

"Unexpected subnormal number '{{ value }}' found. Subnormal numbers are outside the safe range.",

nzakas · 2024-12-03T21:45:15Z

tests/rules/no-unsafe-values.test.js

Can you add some tests for unsafeZero?

Sorry, I had done that, but didn't get the file in the commit. :)

No worries, I do that all the time!

nzakas · 2024-12-04T15:37:32Z

tests/rules/no-unsafe-values.test.js

No worries, I do that all the time!

nzakas · 2024-12-04T15:39:45Z

src/rules/no-unsafe-values.js

@@ -13,6 +19,11 @@ export default {

 		messages: {
 			unsafeNumber: "Number outside safe range found.",
+			unsafeInteger: "Integer outside safe range found.",
+			unsafeZero:
+				"This number will evaluate to zero, which is unintended.",


That's true when people are using ESLint in an editor. People also use ESLint in their CI system, where it just outputs messages onto the terminal. In that case, having the value in the message is really helpful.

nzakas · 2024-12-04T15:40:47Z

src/rules/no-unsafe-values.js

@@ -14,7 +14,8 @@ export default {
 		type: /** @type {const} */ ("problem"),

 		docs: {
-			description: "Disallow JSON values that are unsafe for interchange",
+			description:
+				"Disallow JSON values that are unsafe for interchange.  This includes strings with unmatched surrogates, numbers that evaluate to Infinity, numbers that evaluate to zero unintentionally, numbers that look like integers but they are too large, and subnormal numbers (see: https://en.wikipedia.org/wiki/Subnormal_number)",


Descriptions are meant to be one sentence. I think the original description was fine. The rest of these details should go in the README.

Since this is going to modify the readme, I'm going to rebase on top of main. Hopefully the patch isn't large enough that creates a problem.

When the test was copied from a previous example, the name in the comments wasn't changed.

Flag subnormals, numbers that are flushed to zero, and overly-large integers. Refs eslint#68

hildjj · 2024-12-04T16:26:24Z

Sorry forgot to fix the value issue also. On it.

hildjj · 2024-12-04T16:39:36Z

I made the rest of the errors consistent at the same time. Tests modified.

nzakas

LGTM. Waiting for a second review before merging.

mdjermanovic · 2024-12-05T14:55:34Z

src/rules/no-unsafe-values.js

+								data: { value },
+							});
+						}
+					} else if (!value.match(/[.e]/iu)) {


Suggested change

} else if (!value.match(/[.e]/iu)) {

} else if (!/[.e]/iu.test(value)) {

Since we're only checking, so we don't need the match.

mdjermanovic · 2024-12-05T15:04:38Z

src/rules/no-unsafe-values.js

+						const match = value.match(NUMBER);
+						// assert(match, "If the regex is right, match is always truthy")
+
+						// If any part of the number other than the  has a non-zero digit


Looks like some text is missing between "the" and "has"?

"exponent". fixing.

nzakas

Re-approving. Waiting for @mdjermanovic to verify his comments have been addressed.

mdjermanovic

LGTM, thanks!

eslint-github-bot bot added the feature label Dec 3, 2024

nzakas requested changes Dec 3, 2024

View reviewed changes

nzakas reviewed Dec 4, 2024

View reviewed changes

hildjj added 6 commits December 4, 2024 09:19

test: Drive-by nit to fix test name

99c2c95

When the test was copied from a previous example, the name in the comments wasn't changed.

feat: Catch more unsafe numbers

f0a6cd0

Flag subnormals, numbers that are flushed to zero, and overly-large integers. Refs eslint#68

Rework unsafe zero. Fix code review issues.

1592e51

Add a few more tests

760eeca

Rework error text

3ced729

Move detailed description of no-unsafe-values to readme.

29e443d

hildjj force-pushed the fix-68 branch from b451198 to 29e443d Compare December 4, 2024 16:21

fmt

9711a5c

Add values to errors

1d53514

Make links in readme

216dae4

nzakas previously approved these changes Dec 4, 2024

View reviewed changes

mdjermanovic reviewed Dec 5, 2024

View reviewed changes

code review issues

521fce6

hildjj dismissed nzakas’s stale review via 521fce6 December 5, 2024 15:17

nzakas approved these changes Dec 6, 2024

View reviewed changes

nzakas added the contributor pool label Dec 6, 2024

mdjermanovic approved these changes Dec 7, 2024

View reviewed changes

mdjermanovic merged commit 5ffc7c0 into eslint:main Dec 7, 2024
16 checks passed

github-actions bot mentioned this pull request Dec 4, 2024

chore: release 0.9.0 🚀 #72

Merged

hildjj deleted the fix-68 branch December 7, 2024 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Catch more unsafe numbers #71

feat: Catch more unsafe numbers #71

hildjj commented Dec 3, 2024

nzakas left a comment

nzakas Dec 3, 2024

hildjj Dec 3, 2024

hildjj Dec 3, 2024

nzakas Dec 3, 2024

hildjj Dec 3, 2024

nzakas Dec 3, 2024

hildjj Dec 3, 2024

nzakas Dec 3, 2024

nzakas Dec 3, 2024

nzakas Dec 3, 2024

hildjj Dec 3, 2024

nzakas Dec 4, 2024

nzakas Dec 3, 2024

nzakas Dec 3, 2024

hildjj Dec 3, 2024

nzakas Dec 4, 2024

nzakas Dec 4, 2024

nzakas Dec 4, 2024

nzakas Dec 4, 2024

hildjj Dec 4, 2024

hildjj commented Dec 4, 2024

hildjj commented Dec 4, 2024

nzakas left a comment

mdjermanovic Dec 5, 2024

mdjermanovic Dec 5, 2024

hildjj Dec 5, 2024

nzakas left a comment

mdjermanovic left a comment

	const SMALL_NUMBERS = [/\.(\d+)/u, /((?:\d+\.)?\d+)e/iu];
	const SMALL_NUMBERS = [/^\.(\d+)$/u, /^((?:\d+\.)?\d+)e$/iu];

	subnormal: "Subnormal numbers are outside the safe range.",
	subnormal: "Unexpected subnormal number '{{value}}` found.",

	"This number will evaluate to zero, which is unintended.",
	"The number '{{ value }}` will evaluate to zero.",

	"Subnormal numbers (see https://en.wikipedia.org/wiki/Subnormal_number) are outside the safe range: '{{ value }}'.",
	"Unexpected subnormal number '{{ value }}' found. Subnormal numbers are outside the safe range.",

	} else if (!value.match(/[.e]/iu)) {
	} else if (!/[.e]/iu.test(value)) {

feat: Catch more unsafe numbers #71

feat: Catch more unsafe numbers #71

Conversation

hildjj commented Dec 3, 2024

Prerequisites checklist

What is the purpose of this pull request?

What changes did you make? (Give an overview)

Related Issues

Is there anything you'd like reviewers to focus on?

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hildjj commented Dec 4, 2024

hildjj commented Dec 4, 2024

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas left a comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment