-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(eslint-plugin): [no-misused-spread] add new rule #8509
base: main
Are you sure you want to change the base?
feat(eslint-plugin): [no-misused-spread] add new rule #8509
Conversation
Thanks for the PR, @StyleShit! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8509 +/- ##
==========================================
- Coverage 88.13% 87.27% -0.87%
==========================================
Files 411 252 -159
Lines 14291 12363 -1928
Branches 4171 3899 -272
==========================================
- Hits 12596 10790 -1806
+ Misses 1387 1303 -84
+ Partials 308 270 -38
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Not sure why the tests are failing, seems to pass locally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Progress! 🚀
Sorry, I'm only now realizing the class instance stuff might need an option. I think I've look at this PR too much - we should get someone else from @typescript-eslint/triage-team to look too. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[tests] After #8497 we render snapshots of ESLint output for each code sample in docs. Could you please merge main
and update the snapshots with yarn test docs -u
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks!
|
||
## Examples | ||
|
||
<!--tabs--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[docs] The documentation is now being built with Docusaurus v3: #8209. Could you please update these <!--tabs-->
comments to <Tabs>
MDX tags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
function isString(type: ts.Type): boolean { | ||
return tsutils | ||
.typeParts(type) | ||
.some(t => !!isTypeFlagSet(t, ts.TypeFlags.StringLike)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function isString(type: ts.Type): boolean { | |
return tsutils | |
.typeParts(type) | |
.some(t => !!isTypeFlagSet(t, ts.TypeFlags.StringLike)); | |
} | |
function isString(type: ts.Type): boolean { | |
return tsutils | |
.unionTypeParts(type) | |
.some(unionPart => tsutils.intersectionTypeParts(unionPart) | |
.some(intersectionPart => !!isTypeFlagSet(t, ts.TypeFlags.StringLike)) | |
); | |
} |
typeParts
iterates through the first level of .types
property only. Looks like a bug:
declare const aa: (string & { foo: 1 }) | (string & { bar: 1 })
[...aa] // not reported - bug
declare const bb: (string & { foo: 1 })
[...bb] // reported - ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch!
function isPromise(program: ts.Program, type: ts.Type): boolean { | ||
return tsutils.typeParts(type).some(t => isPromiseLike(program, t)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function isPromise(program: ts.Program, type: ts.Type): boolean { | |
return tsutils.typeParts(type).some(t => isPromiseLike(program, t)); | |
} |
isPromiseLike
already contains the type part iteration logic 😄 I think there is no need to do typeParts
here!
typescript-eslint/packages/type-utils/src/builtinSymbolLikes.ts
Lines 132 to 141 in f248e68
if (type.isIntersection()) { | |
return type.types.some(t => | |
isBuiltinSymbolLikeRecurser(program, t, predicate), | |
); | |
} | |
if (type.isUnion()) { | |
return type.types.every(t => | |
isBuiltinSymbolLikeRecurser(program, t, predicate), | |
); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like it doesn't catch this case:
declare const maybePromise: Promise<number> | { a: number };
const o = { ...maybePromise };
and I think we should catch it, because it means we might spread a promise in runtime, right?
anyway, see this comment:
#8509 (comment)
I'll be glad to get some help here with removing this check completely 😄
Wasn't able to figure out why isClassOrInterface
didn't work here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (isPromise(services.program, type)) { | ||
context.report({ | ||
node, | ||
messageId: 'noSpreadInObject', | ||
data: { | ||
type: 'Promise', | ||
}, | ||
}); | ||
|
||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question - do we want to handle this with this rule?
no-misused-promises
also handles this case
It's okay if the answer is yes! Just wanna make sure we consciously say "yes we're okay with both rules reporting this". I'm personally not super bothered by the dual reporting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
honestly? I'm not sure about it
I don't have any strong opinion in this case, and had the same thought about no-misused-promises
.
@JoshuaKGoldberg said we should remove it, and that it should be reported by the isClassInstance
function (which doesn't work for some reason... should we call this bug a feature and call it a day? 😅)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My 2c:
I'm okay with having such a check here as it's possible to have one rule but not the other.
If we are going to special-case promise in this rule then the error should also be special-cased as well - eg by adding "did you forget to await the promise?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me
done :)
if (isIterable(type, checker) && !isString(type)) { | ||
context.report({ | ||
node, | ||
messageId: 'noSpreadInObject', | ||
data: { | ||
type: 'Iterable', | ||
}, | ||
}); | ||
|
||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: what about cases like this:
const iterator = {
*[Symbol.iterator]() {
yield 1;
yield 2;
yield 3;
},
prop: 1,
};
({ ...iterator });
should this be allowed?
spreading iterables is quite weird because it creates a clone of the Symbol.iterator
function itself - but ofc it doesn't account for the fact that they're sharing implementation details.
for example
let outerState = 1;
const iterator = {
*[Symbol.iterator]() {
if (outerState > 1) {
return;
}
yield outerState++;
},
};
const x = ({ ...iterator });
for (const y of x) { console.log(y) }
// -> 1
for (const y of iterator) { console.log(y) }
// no logs
I'm not sure what the correct behaviour should be here for the rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... that's a great question actually
Regarding the first case - I still think the result you get is weird, who would expect to get an object with the first value of the iterator, and a key-value pair of the iterator function?
const iterator = {
*[Symbol.iterator]() {
yield 1;
yield 2;
yield 3;
},
prop: 1,
};
console.log({ ...iterator }); // { prop: 1, Symbol(Symbol.iterator): * Symbol.iterator() }
Regarding the second one - That's a great catch, haven't thought about it, but again, you'd get a (subjectively?) weird result from spreading the iterator in the first place, so... 🤷♂️
Should we make it an option? (if we're already there, should we make everything an option?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just not familiar enough with custom iterables to know for sure what the "idiomatic" usages are.
Are there many cases where you do want to clone an iterables via an object spread?
Having an option to control the case it is probably the best option?
Cc @typescript-eslint/triage-team
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: It looks like the "array in object spread" case is handled via this "iterator" handling.
If we do decide to remove this handling or we put this handling behind an option -- then we should special-case arrays and tuples so they're always banned, given that that's a concrete, well-defined cases that you'd never ever want, and because it's easy to catch from the type system side of things (there's utils to detect the array type)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoah, that's a good catch, thank you!
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what I know: you would almost never have a custom iterable that actually spreads and remains iterable. This is because usually [Symbol.iterator]()
is defined on the class (such as Set.prototype[@@iterator]
), not as an own property of each instance, and it's usually bad practice to make methods enumerable in any case.
I would suggest forbidding all iterables from being spreaded as objects, with an option that allows certain types. There's probably no reason you want to spread maps or sets but there might be reasons to spread other iterables.
'Using the spread operator on a function without additional properties can cause unexpected behavior. Did you forget to call the function?', | ||
|
||
noClassInstanceSpreadInObject: | ||
'Using the spread operator on class instances without `[Symbol.iterator]` can cause unexpected behavior.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RC:
I'm not sure I understand this error message.
Symbol.iterator
isn't used for object spreads.
class C { prop = 1; *[Symbol.iterator]() { yield 2 } }
({...(new C)})
// -> {prop: 1}
This error probably should mention the loss of the super class / prototype chain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I... don't know why I wrote that 😂
yup, that's wrong, I don't even check that in the rule...
changed the message
properties: { | ||
allowClassInstances: { | ||
description: | ||
'Whether to allow spreading class instances in objects.', | ||
type: 'boolean', | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question:
do we want to add options for each of the cases here just to stave off future issues from people asking to allow specific cases?
I'm personally okay with being lazy and not - just worth calling out so we have the decision logged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, I just proposed the same in this comment 😄
will do
noClassDeclarationSpreadInObject: | ||
'Using the spread operator on class declarations can cause unexpected behavior. Did you forget to instantiate the class?', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spreading a class declaration will spread the static properties of the class.
it does carry the same risks as spreading a class instance (losing the prototype chain / the "static this
" context).
Should this similarly be covered by an option?
Again I'm okay with the answer being no :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added an option, and clarified the message
if (node.argument.type === AST_NODE_TYPES.ClassExpression) { | ||
context.report({ | ||
node, | ||
messageId: 'noClassDeclarationSpreadInObject', | ||
}); | ||
|
||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RC:
we should use types for this case to trace the type back to determine if it's a class declaration or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure that I fully understand what you mean, can you explain please?
EDIT: seems like my code reports wrongly in a real codebase (this repo). I'm assuming it's related to what you're saying?
on a side not: what does "RC" mean? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RC = "request change"
Currently this check is a pure syntactic check. It is specifically only looking for this case: ({...(class C {})})
This is, obviously, fairly limited and very specific. Instead we should be looking at the type of the thing being spread to determine if it is a class declaration or not so we can support cases like
class A {}
const B = class {};
const C = Set<string>;
({...A});
({...B});
({...C});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
allowPromises: false, | ||
allowFunctions: false, | ||
allowMaps: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My 2c: I think that we don't want allowPromises
or allowMaps
.
Like the Array case - these cases are special cases that are always going to be bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha, done
### `allowClassDeclarations` | ||
|
||
By default, this rule disallows using the spread operator on class declarations: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the name of this option, as the class could also be passed as a reference or be generated by a class factory.
I asked ChatGPT for a better name and it suggested "class reference" as an alternative, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't have any strong opinion about either of the names
Maybe let's wait for one of the maintainers to decide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest allowClasses
. Any concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Josh-Cena It's better than declaration, but allowClasses
can be understood (or confused) as class instances which is another option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but "class instances" and "classes" are obviously different things and the fact that we already have an "instances" option further mitigates that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, yes, assuming you read about both in proximity, seeing an eslint-disable comment or config option can lose the context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't find "allowClasses
allows classes to be spread" as being ambiguous. Sure maybe someone will use the same name for an option that checks instances, but there's no way this is wrong.
Also classes are just sugared classes so I'm not sure if we want to treat them any more specially than functions or anything with a new
signature...
Closes #748