Skip to content

Commit ecd44ee

Browse files
authored
new rule: no-object-spread-of-iterable (#750)
1 parent f86cda1 commit ecd44ee

File tree

20 files changed

+356
-112
lines changed

20 files changed

+356
-112
lines changed

baselines/packages/mimir/api/packlist.txt

+5
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ src/formatters/json.js.map
1010
src/formatters/stylish.d.ts
1111
src/formatters/stylish.js
1212
src/formatters/stylish.js.map
13+
src/iteration.js
14+
src/iteration.js.map
1315
src/restricted-property.js
1416
src/restricted-property.js.map
1517
src/rules/async-function-assignability.d.ts
@@ -57,6 +59,9 @@ src/rules/no-misused-generics.js.map
5759
src/rules/no-nan-compare.d.ts
5860
src/rules/no-nan-compare.js
5961
src/rules/no-nan-compare.js.map
62+
src/rules/no-object-spread-of-iterable.d.ts
63+
src/rules/no-object-spread-of-iterable.js
64+
src/rules/no-object-spread-of-iterable.js.map
6065
src/rules/no-octal-escape.d.ts
6166
src/rules/no-octal-escape.js
6267
src/rules/no-octal-escape.js.map
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
import { TypedRule } from '@fimbul/ymir';
2+
export declare class Rule extends TypedRule {
3+
apply(): void;
4+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
export {};
2+
3+
[...[]];
4+
[...[1]];
5+
[...new Set()];
6+
[...new Map()];
7+
8+
declare function doStuff(...args: any[]): void;
9+
doStuff(...[]);
10+
11+
declare const arrayLike: ArrayLike<number>;
12+
({...arrayLike});
13+
let {...a} = [];
14+
({...a} = []);
15+
let b: Array<number>;
16+
({...b} = [1]);
17+
18+
({...[]});
19+
~~ [error no-object-spread-of-iterable: Spreading an Iterable type into an object is most likely a mistake. Did you intend to use array spread instead?]
20+
({...[1]});
21+
~~~ [error no-object-spread-of-iterable: Spreading an Iterable type into an object is most likely a mistake. Did you intend to use array spread instead?]
22+
({...new Set()});
23+
~~~~~~~~~ [error no-object-spread-of-iterable: Spreading an Iterable type into an object is most likely a mistake. Did you intend to use array spread instead?]
24+
({...new Map()});
25+
~~~~~~~~~ [error no-object-spread-of-iterable: Spreading an Iterable type into an object is most likely a mistake. Did you intend to use array spread instead?]
26+
27+
({a: 1, ...[1] as const});
28+
({..."..."});
29+
~~~~~ [error no-object-spread-of-iterable: Spreading an Iterable type into an object is most likely a mistake. Did you intend to use array spread instead?]
30+
31+
<span {...new Map()}></span>;
32+
~~~~~~~~~ [error no-object-spread-of-iterable: Spreading an Iterable type into an object is most likely a mistake. Did you intend to use array spread instead?]

baselines/packages/mimir/test/prefer-for-of/es3/types.ts.lint

+13
Original file line numberDiff line numberDiff line change
@@ -213,3 +213,16 @@ declare let privateIterable: PrivateIterable;
213213
for (let i = 0; i < privateIterable.length; ++i) {
214214
privateIterable[i]
215215
}
216+
217+
declare let record: Record<string, number>;
218+
for (let i = 0; i < record.length; ++i) {
219+
record[i];
220+
}
221+
222+
{
223+
interface Array extends ArrayLike {}
224+
const customArrayType = null! as Array;
225+
for (let i = 0; i < customArrayType.length; ++i) {
226+
customArrayType[i];
227+
}
228+
}

baselines/packages/mimir/test/prefer-for-of/es5-iteration/types.ts.lint

+13
Original file line numberDiff line numberDiff line change
@@ -223,3 +223,16 @@ declare let privateIterable: PrivateIterable;
223223
for (let i = 0; i < privateIterable.length; ++i) {
224224
privateIterable[i]
225225
}
226+
227+
declare let record: Record<string, number>;
228+
for (let i = 0; i < record.length; ++i) {
229+
record[i];
230+
}
231+
232+
{
233+
interface Array extends ArrayLike {}
234+
const customArrayType = null! as Array;
235+
for (let i = 0; i < customArrayType.length; ++i) {
236+
customArrayType[i];
237+
}
238+
}

baselines/packages/mimir/test/prefer-for-of/es5/types.ts.lint

+13
Original file line numberDiff line numberDiff line change
@@ -214,3 +214,16 @@ declare let privateIterable: PrivateIterable;
214214
for (let i = 0; i < privateIterable.length; ++i) {
215215
privateIterable[i]
216216
}
217+
218+
declare let record: Record<string, number>;
219+
for (let i = 0; i < record.length; ++i) {
220+
record[i];
221+
}
222+
223+
{
224+
interface Array extends ArrayLike {}
225+
const customArrayType = null! as Array;
226+
for (let i = 0; i < customArrayType.length; ++i) {
227+
customArrayType[i];
228+
}
229+
}

baselines/packages/mimir/test/prefer-for-of/es6/types.ts.lint

+13
Original file line numberDiff line numberDiff line change
@@ -223,3 +223,16 @@ declare let privateIterable: PrivateIterable;
223223
for (let i = 0; i < privateIterable.length; ++i) {
224224
privateIterable[i]
225225
}
226+
227+
declare let record: Record<string, number>;
228+
for (let i = 0; i < record.length; ++i) {
229+
record[i];
230+
}
231+
232+
{
233+
interface Array extends ArrayLike {}
234+
const customArrayType = null! as Array;
235+
for (let i = 0; i < customArrayType.length; ++i) {
236+
customArrayType[i];
237+
}
238+
}

packages/mimir/README.md

+1
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ Rule | Description | Difference to TSLint rule / Why you should use it
4141
[`no-invalid-assertion`](docs/no-invalid-assertion.md) | :mag: Disallows asserting a literal type to a different literal type of the same widened type, e.g. `'foo' as 'bar'`.| TSLint has no similar rule.
4242
[`no-misused-generics`](docs/no-misused-generics.md) | Detects generic type parameters that cannot be inferred from the functions parameters. It also detects generics that don't enforce any constraint between types. | There's no similar TSLint rule.
4343
[`no-nan-compare`](docs/no-nan-compare.md) | Disallows comparing with `NaN`, use `isNaN(number)` or `Number.isNaN(number)` instead. | Performance!
44+
[`no-object-spread-of-iterable`](docs/no-object-spread-of-iterable.md) | :mag: Disallows spreading iterable types into an object. |
4445
[`no-octal-escape`](docs/no-octal-escape.md) | :wrench: Disallows octal escape sequences in strings and template strings. | No such rule in TSLint.
4546
[`no-restricted-property-access`](docs/no-restricted-property-access.md) | :mag: Disallows accessing properties via computed name that would not be accessible using a static name. | TSLint has no similar rule.
4647
[`no-return-await`](docs/no-return-await.md) | :wrench: Disallows unnecesary `return await foo;` when you can simply `return foo;` | The same as TSLint's rule. I wrote both, but this one is faster.
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
# no-object-spread-of-iterable
2+
3+
:mag: requires type information
4+
5+
Disallows spreading iterable types into an object.
6+
7+
## Rationale
8+
9+
Spreading an iterable type like `Array` via object spread gives an object with the same numeric properties as the original array, but it no longer behaves like an array. There's no `length` property and all methods are not available. Unfortunately TypeScript cannot correctly represent that. So it tells you that object spread of an array gives you an array.
10+
11+
Most of the time this is a typo, as you need to use array spread to copy an array (square brackets instead of curly brackets): `[...arr]` instead of `{...arr}`.
12+
13+
Another possible mistake is a naive attempt to copy complex types like `Map` or `Set` via object spread. This gives you an empty object at runtime, but once again TypeScript doesn't warn you about that.
14+
15+
## Examples
16+
17+
:thumbsdown: Examples of incorrect code
18+
19+
```ts
20+
declare const arr: Array<number>;
21+
declare const set: Set<number>;
22+
declare const map: Map<number>;
23+
24+
console.log({...arr}.reverse()); // will throw at runtime, because 'reverse' does not exist on the resulting object
25+
console.log({...set}); // this is not how you copy Set
26+
console.log({...map}); // this is not how you copy Map
27+
```
28+
29+
:thumbsup: Examples of correct code
30+
31+
```ts
32+
declare const arr: Array<number>;
33+
declare const set: Set<number>;
34+
declare const map: Map<number>;
35+
36+
console.log([...arr].reverse()); // works as expected with square brackets
37+
console.log(new Set(set)); // this is how you copy Set
38+
console.log([...set]); // creates a new Array from the values in the Set
39+
console.log(new Map(map)); // this is not how you copy Map
40+
console.log([...map]); // creates a new Array with tuples representing the Map's entries
41+
```
42+
43+
## Further Reading
44+
45+
* MDN: [Spread Syntax](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax)
46+
47+
## Related Rules
48+
49+
* [`no-duplicate-spread-property`](no-duplicate-spread-property.md)
50+
* [`no-useless-spread`](no-useless-spread.md)
51+
* [`prefer-object-spread`](prefer-object-spread.md)
52+

packages/mimir/docs/no-useless-spread.md

+1
Original file line numberDiff line numberDiff line change
@@ -69,4 +69,5 @@ Math.max(1, ...arr, 2);
6969
## Related Rules
7070

7171
* [`no-duplicate-spread-property`](no-duplicate-spread-property.md)
72+
* [`no-object-spread-of-iterable`](no-object-spread-of-iterable.md)
7273
* [`prefer-object-spread`](prefer-object-spread.md)

packages/mimir/docs/prefer-object-spread.md

+1
Original file line numberDiff line numberDiff line change
@@ -56,4 +56,5 @@ function copy<T>(obj: T) {
5656
## Related Rules
5757

5858
* [`no-duplicate-spread-property`](no-duplciate-spread-property.md)
59+
* [`no-object-spread-of-iterable`](no-object-spread-of-iterable.md)
5960
* [`no-useless-spread`](no-useless-spread.md)

packages/mimir/recommended.yaml

+1
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ rules:
1717
no-return-await: error
1818
no-misused-generics: error
1919
no-nan-compare: error
20+
no-object-spread-of-iterable: error
2021
no-octal-escape: error
2122
no-unassigned-variable: error
2223
no-uninferred-type-parameter: error

packages/mimir/src/iteration.ts

+123
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
import {
2+
getIteratorYieldResultFromIteratorResult,
3+
getPropertyOfType,
4+
isCompilerOptionEnabled,
5+
isIntersectionType,
6+
isModifierFlagSet,
7+
isSymbolFlagSet,
8+
isTypeFlagSet,
9+
isTypeReference,
10+
isUnionType,
11+
removeOptionalityFromType,
12+
someTypePart,
13+
unionTypeParts,
14+
} from 'tsutils';
15+
import * as ts from 'typescript';
16+
import { tryGetBaseConstraintType, typesAreEqual } from './utils';
17+
18+
function isIterationProtocolAvailable(compilerOptions: ts.CompilerOptions) {
19+
return compilerOptions.target! >= ts.ScriptTarget.ES2015 || isCompilerOptionEnabled(compilerOptions, 'downlevelIteration');
20+
}
21+
22+
export function isExpressionIterable(
23+
node: ts.Expression,
24+
checker: ts.TypeChecker,
25+
compilerOptions: ts.CompilerOptions,
26+
matchIndexSignature: boolean,
27+
): boolean {
28+
const type = checker.getTypeAtLocation(node);
29+
return isIterationProtocolAvailable(compilerOptions)
30+
? isIterable(checker.getApparentType(type), checker, node, matchIndexSignature)
31+
: unionTypeParts(tryGetBaseConstraintType(type, checker)).every((t) => isArrayLike(t, compilerOptions));
32+
}
33+
34+
function isArrayLike(type: ts.Type, compilerOptions: ts.CompilerOptions): boolean {
35+
if (isTypeReference(type))
36+
type = type.target;
37+
if (type.getNumberIndexType() === undefined)
38+
return false;
39+
if (type.flags & ts.TypeFlags.StringLike)
40+
return compilerOptions.target! >= ts.ScriptTarget.ES5; // iterating string is only possible starting from ES5
41+
if (type.symbol !== undefined && /^(?:Readonly)?Array$/.test(<string>type.symbol.escapedName) &&
42+
type.symbol.declarations?.some((node) => node.getSourceFile().hasNoDefaultLib))
43+
return true;
44+
if (isIntersectionType(type))
45+
return type.types.some((t) => isArrayLike(t, compilerOptions));
46+
return !!type.getBaseTypes()?.some((t) => isArrayLike(t, compilerOptions));
47+
}
48+
49+
function isIterable(type: ts.Type, checker: ts.TypeChecker, node: ts.Node, matchIndexSignature: boolean): boolean {
50+
const indexType = type.getNumberIndexType() || type.getStringIndexType();
51+
if (indexType === undefined && matchIndexSignature)
52+
return false;
53+
const iteratorFn = getPropertyOfType(type, <ts.__String>'__@iterator');
54+
if (!isPresentPublicAndRequired(iteratorFn))
55+
return false;
56+
return checkReturnTypeAndRequireZeroArity(checker.getTypeOfSymbolAtLocation(iteratorFn, node), (iterator) => {
57+
const next = iterator.getProperty('next');
58+
return isPresentPublicAndRequired(next) &&
59+
checkReturnTypeAndRequireZeroArity(checker.getTypeOfSymbolAtLocation(next, node), (iteratorResult) => {
60+
const done = iteratorResult.getProperty('done');
61+
if (
62+
!isPresentAndPublic(done) ||
63+
someTypePart(
64+
removeOptionalityFromType(checker, checker.getTypeOfSymbolAtLocation(done, node)),
65+
isUnionType,
66+
(t) => !isTypeFlagSet(t, ts.TypeFlags.BooleanLike),
67+
)
68+
)
69+
return false;
70+
const value = getIteratorYieldResultFromIteratorResult(iteratorResult, node, checker).getProperty('value');
71+
return isPresentAndPublic(value) &&
72+
(
73+
!matchIndexSignature ||
74+
typesAreEqual(checker.getTypeOfSymbolAtLocation(value, node), indexType!, checker)
75+
);
76+
});
77+
78+
});
79+
}
80+
81+
function checkReturnTypeAndRequireZeroArity(type: ts.Type, cb: (type: ts.Type) => boolean): boolean {
82+
let zeroArity = false;
83+
for (const signature of type.getCallSignatures()) {
84+
if (!cb(signature.getReturnType()))
85+
return false;
86+
if (signatureHasArityZero(signature))
87+
zeroArity = true;
88+
}
89+
return zeroArity;
90+
}
91+
92+
function signatureHasArityZero(signature: ts.Signature): boolean {
93+
if (signature.parameters.length === 0)
94+
return true;
95+
const decl = <ts.ParameterDeclaration | undefined>signature.parameters[0].declarations![0];
96+
return decl !== undefined && isOptionalParameter(decl);
97+
}
98+
99+
function isOptionalParameter(node: ts.ParameterDeclaration): boolean {
100+
if (node.questionToken !== undefined || node.dotDotDotToken !== undefined)
101+
return true;
102+
if (node.flags & ts.NodeFlags.JavaScriptFile && ts.getJSDocParameterTags(node).some((tag) => tag.isBracketed))
103+
return true;
104+
if (node.initializer === undefined)
105+
return false;
106+
const parameters = node.parent!.parameters;
107+
const nextIndex = parameters.indexOf(node) + 1;
108+
if (nextIndex === parameters.length)
109+
return true;
110+
return isOptionalParameter(parameters[nextIndex]);
111+
}
112+
113+
function isPresentPublicAndRequired(symbol: ts.Symbol | undefined): symbol is ts.Symbol {
114+
return isPresentAndPublic(symbol) && !isSymbolFlagSet(symbol, ts.SymbolFlags.Optional);
115+
}
116+
117+
function isPresentAndPublic(symbol: ts.Symbol | undefined): symbol is ts.Symbol {
118+
return symbol !== undefined &&
119+
(
120+
symbol.declarations === undefined ||
121+
symbol.declarations.every((d) => !isModifierFlagSet(d, ts.ModifierFlags.NonPublicAccessibilityModifier))
122+
);
123+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
import { TypedRule, excludeDeclarationFiles } from '@fimbul/ymir';
2+
import * as ts from 'typescript';
3+
import {
4+
WrappedAst,
5+
getWrappedNodeAtPosition,
6+
isReassignmentTarget,
7+
} from 'tsutils';
8+
import { isExpressionIterable } from '../iteration';
9+
10+
@excludeDeclarationFiles
11+
export class Rule extends TypedRule {
12+
public apply() {
13+
const re = /\.{3}/g;
14+
let wrappedAst: WrappedAst | undefined;
15+
for (let match = re.exec(this.sourceFile.text); match !== null; match = re.exec(this.sourceFile.text)) {
16+
const {node} = getWrappedNodeAtPosition(wrappedAst ??= this.context.getWrappedAst(), re.lastIndex)!;
17+
if (node.pos !== re.lastIndex)
18+
continue;
19+
switch (node.parent!.kind) {
20+
case ts.SyntaxKind.SpreadAssignment:
21+
if (isReassignmentTarget(<ts.ObjectLiteralExpression>node.parent.parent))
22+
continue;
23+
// falls through
24+
case ts.SyntaxKind.JsxSpreadAttribute:
25+
this.checkObjectSpread(<ts.Expression>node);
26+
}
27+
}
28+
}
29+
30+
private checkObjectSpread(node: ts.Expression) {
31+
if (isExpressionIterable(node, this.checker, this.context.compilerOptions, false))
32+
this.addFindingAtNode(node, 'Spreading an Iterable type into an object is most likely a mistake. Did you intend to use array spread instead?');
33+
}
34+
}

0 commit comments

Comments
 (0)