Skip to content

Commit 1686e8a

Browse files
bakkotaduh95
bakkot
authored and
aduh95
committed
tools: require value before done in iterator results
V8 has a fast path for `{ value, done }` literals, but it must be that order. Also, the ES spec technically requires that order. Assisted-by: claude:opus-5.5 Signed-off-by: Kevin Gibbons <bakkot@gmail.com> PR-URL: #66837 Reviewed-By: Jordan Harband <ljharb@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name>
1 parent 81d2321 commit 1686e8a

4 files changed

Lines changed: 68 additions & 64 deletions

File tree

‎lib/eslint.config_partial.mjs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -435,7 +435,7 @@ export default [
435435
'node-core/alphabetize-errors': 'error',
436436
'node-core/alphabetize-primordials': 'error',
437437
'node-core/avoid-prototype-pollution': 'error',
438-
'node-core/iterator-result-done-first': 'error',
438+
'node-core/iterator-result-value-first': 'error',
439439
'node-core/lowercase-name-for-primitive': 'error',
440440
'node-core/non-ascii-character': 'error',
441441
'node-core/no-array-destructuring': 'error',

‎test/parallel/test-eslint-iterator-result-done-first.js‎

Lines changed: 0 additions & 60 deletions
This file was deleted.

‎test/parallel/test-eslint-iterator-result-value-first.js‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if ((!common.hasCrypto) || (!common.hasIntl)) {
5+
common.skip('ESLint tests require crypto and Intl');
6+
}
7+
8+
common.skipIfEslintMissing();
9+
10+
const { RuleTester } = require('../../tools/eslint/node_modules/eslint');
11+
const rule = require('../../tools/eslint-rules/iterator-result-value-first');
12+
13+
const message = 'Iterator result objects should place `value` before `done`.';
14+
15+
new RuleTester().run('iterator-result-value-first', rule, {
16+
valid: [
17+
'function next() { return { value: undefined, done: true }; }',
18+
'function next() { return { __proto__: null, value: chunk, done: false }; }',
19+
'function next() { return { value, done }; }',
20+
'function next() { return { value }; }',
21+
'function next() { return { done }; }',
22+
'function next() { return { done: 1, other: 2 }; }',
23+
'function next() { return { [done]: 1, value: true }; }',
24+
'function next() { return { done: 1, [value]: true }; }',
25+
'function next() { return { "value": undefined, "done": true }; }',
26+
'function next() { return { ["value"]: undefined, ["done"]: true }; }',
27+
],
28+
invalid: [
29+
{
30+
code: 'function next() { return { done: true, value: undefined }; }',
31+
errors: [{ message }],
32+
output: 'function next() { return { value: undefined, done: true }; }',
33+
},
34+
{
35+
code: 'function next() { return { __proto__: null, done: false, value: chunk }; }',
36+
errors: [{ message }],
37+
output: 'function next() { return { __proto__: null, value: chunk, done: false }; }',
38+
},
39+
{
40+
code: 'function next() { return { done, value }; }',
41+
errors: [{ message }],
42+
output: 'function next() { return { value, done }; }',
43+
},
44+
{
45+
code: 'function next() { return { "done": true, "value": undefined }; }',
46+
errors: [{ message }],
47+
output: 'function next() { return { "value": undefined, "done": true }; }',
48+
},
49+
{
50+
code: 'function next() { return { ["done"]: true, ["value"]: undefined }; }',
51+
errors: [{ message }],
52+
output: 'function next() { return { ["value"]: undefined, ["done"]: true }; }',
53+
},
54+
{
55+
code: 'function next() { return { done: false, extra: true, value: result }; }',
56+
errors: [{ message }],
57+
output: 'function next() { return { value: result, extra: true, done: false }; }',
58+
},
59+
],
60+
});

tools/eslint-rules/iterator-result-done-first.js renamed to tools/eslint-rules/iterator-result-value-first.js

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
'use strict';
22

3-
const MESSAGE = 'Iterator result objects should place `done` before `value`.';
3+
// V8 has a fast path for `{ value, done }` literals, but it must be that order.
4+
// https://source.chromium.org/chromium/chromium/src/+/main:v8/src/init/bootstrapper.cc;l=746-754;drc=e8ac4914ee6b24442aed773d3a041451692b6a9c
5+
// Also, the ES spec technically requires that order.
6+
// https://tc39.es/ecma262/multipage/abstract-operations.html#sec-createiterresultobject
7+
const MESSAGE = 'Iterator result objects should place `value` before `done`.';
48

59
function getStaticPropertyName(property) {
610
const { key } = property;
@@ -48,9 +52,9 @@ module.exports = {
4852
}
4953
}
5054

51-
if (doneProperty && valueProperty && valueProperty.range[0] < doneProperty.range[0]) {
55+
if (doneProperty && valueProperty && doneProperty.range[0] < valueProperty.range[0]) {
5256
context.report({
53-
node: valueProperty,
57+
node: doneProperty,
5458
message: MESSAGE,
5559
fix(fixer) {
5660
return [

0 commit comments

Comments
 (0)