-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix(eslint-plugin): [no-misused-promises] the inheritedMethods
and properties
options to check all statically analyzable declarations
#10310
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR, @ronami! 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. |
View your CI Pipeline Execution ↗ for commit 6c48db9.
☁️ Nx Cloud last updated this comment at |
inheritedMethods
and properties
to check all statically analyzable declarationsinheritedMethods
and properties
options to check all statically analyzable declarations
function getWellKnownStringOfSymbol(symbol: symbol): string | null { | ||
switch (symbol) { | ||
case Symbol.iterator: | ||
return 'iterator'; | ||
case Symbol.asyncIterator: | ||
return 'asyncIterator'; | ||
} | ||
|
||
return null; | ||
} |
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 not sure if there's a better way to map a symbol to its string counterpart tsutils.getWellKnownSymbolPropertyOfType
is expecting.
The list for well-known symbols isn't long, but this will still require maintaining this mapping.
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 this approach makes sense, this list can be expanded to support other well-known symbols.
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.
Is there a reason we need to stick to well-known symbols? I'd think we should go with any detected symbol, 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.
Hmm, I might be missing something, but I'm not entirely sure how to check if a JavaScript symbol
exists on another ts.Type
🤔
Here's how existing code checks if an inherited type has a string
symbol:
const escapedMemberName = ts.escapeLeadingUnderscores(staticAccessValue);
const symbolMemberMatch = type.getSymbol()?.members?.get(escapedMemberName);
return (
symbolMemberMatch ?? tsutils.getPropertyOfType(type, escapedMemberName)
);
I've looked into the SymbolObject
but couldn't find a way to check against an arbitrary symbol
(not to be confused with ts.Symbol
).
Because of this, I've created a function to map between known symbols and their string representation, which are passed to tsutils.getWellKnownSymbolPropertyOfType
to check if they exist on the inherited type.
Edit: I don't think it's feasible to iterate the inherited parent's nodes, as a function may be higher up the inheritance chain:
const staticSymbol = Symbol.for('static symbol');
interface Bar {
[staticSymbol]: () => void;
}
interface Foo extends Bar {}
class Clazz implements Foo {
// should be reported
async [staticSymbol]() {}
}
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.
Hmm gotcha thanks. cc @kirkwaiblinger - I'm fine with this as a patchwork helper, with as little hardcoding as possible.
function getWellKnownStringOfSymbol(symbol: symbol): string | null {
for (const [key, value] of Object.entries(Symbol)) {
if (symbol === value) {
return key;
}
}
return null;
}
And, sorry for dropping the ball on this again 🤦 there's something about symbols that makes me instinctively forget the conversation...
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've updated the PR to not hard-code well-known symbols, thanks!
The only alternative I had in mind for not hard-coding was to extract the value from Symbol.prototype.toString()
with a regex (which I really didn't want to do), but I didn't think of iterating the global Symbol
's keys.
This means that arbitrary symbols (e.g. Symbol.for('foo')
) won't be reported by the rule though.
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.
Yeah I don't see a way to work for arbitrary symbols 😞
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.
cross-linking discord thread https://discord.com/channels/1026804805894672454/1322853884292497418
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10310 +/- ##
==========================================
+ Coverage 86.86% 86.92% +0.06%
==========================================
Files 445 446 +1
Lines 15455 15514 +59
Branches 4507 4520 +13
==========================================
+ Hits 13425 13486 +61
+ Misses 1675 1673 -2
Partials 355 355
Flags with carried forward coverage won't be shown. Click here to find out more.
|
node.type !== AST_NODE_TYPES.MethodDefinition && | ||
node.type !== AST_NODE_TYPES.TSAbstractMethodDefinition && | ||
node.type !== AST_NODE_TYPES.TSMethodSignature && | ||
node.type !== AST_NODE_TYPES.TSPropertySignature && | ||
node.type !== AST_NODE_TYPES.PropertyDefinition |
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.
This is enough to pass the test suit but there are additional types available to satisfy getStaticMemberAccessValue
.
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.
This is necessary mainly for type-checking, and I don't know if this can realistically have a runtime scenario. Please let me know if this should be handled differently (currently, there is no coverage for this).
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 believe this can realistically be a runtime scenario. I think the real issue here is that although nodeMember
is type ts.TypeElement | ts.ClassElement
, node
is type TSESTree.Node
.
The two strategies I'd think could be good would be:
- Simpler: use an
as
to correct TypeScript's understanding ofnode
's type to beNodeWithKey
(which will need to be exported frommisc.ts
) - More aligned with how most of our typed lint rules go: instead of iterating over the
tsNode.members
, iterate over the members ofnode
The latter is probably a lot more work and changes the existing code. So I think the as
strategy would be reasonable.
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.
Yes, I've started with a type-assertion on my initial implementation but I wasn't sure if there's a realistic case where this would explode.
I updated the code to use a type assertion, as the second option seems to be a lot more work.
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 like where this is going!
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.
[Testing] One thing under test per case please (https://typescript-eslint.io/contributing/pull-requests#granular-unit-tests)
node.type !== AST_NODE_TYPES.MethodDefinition && | ||
node.type !== AST_NODE_TYPES.TSAbstractMethodDefinition && | ||
node.type !== AST_NODE_TYPES.TSMethodSignature && | ||
node.type !== AST_NODE_TYPES.TSPropertySignature && | ||
node.type !== AST_NODE_TYPES.PropertyDefinition |
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 believe this can realistically be a runtime scenario. I think the real issue here is that although nodeMember
is type ts.TypeElement | ts.ClassElement
, node
is type TSESTree.Node
.
The two strategies I'd think could be good would be:
- Simpler: use an
as
to correct TypeScript's understanding ofnode
's type to beNodeWithKey
(which will need to be exported frommisc.ts
) - More aligned with how most of our typed lint rules go: instead of iterating over the
tsNode.members
, iterate over the members ofnode
The latter is probably a lot more work and changes the existing code. So I think the as
strategy would be reasonable.
function getWellKnownStringOfSymbol(symbol: symbol): string | null { | ||
switch (symbol) { | ||
case Symbol.iterator: | ||
return 'iterator'; | ||
case Symbol.asyncIterator: | ||
return 'asyncIterator'; | ||
} | ||
|
||
return null; | ||
} |
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.
Is there a reason we need to stick to well-known symbols? I'd think we should go with any detected symbol, no?
👋 Hey @ronami, did you want to re-request review on this? No pressure, just asking because it's been a bit and I've seen other things from you in the meantime. 🙂 |
Hey @JoshuaKGoldberg, thanks for checking up on this ❤️ I've commented here on something I'm not sure how to implement (I might be missing something 🙈). I didn't want to ask for a re-review since it's still an open discussion. This is the only remaining open issue before the PR can be re-reviewed. |
context, | ||
); | ||
|
||
if (!staticAccessValue) { |
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.
(same as other comment)
if (!staticAccessValue) { | |
if (staticAccessValue == null) { |
@@ -492,9 +492,14 @@ export default createRule<Options, MessageId>({ | |||
if (objType == null) { | |||
return; | |||
} | |||
const propertySymbol = checker.getPropertyOfType( | |||
const staticAccessValue = getStaticMemberAccessValue(node, context); | |||
if (!staticAccessValue) { |
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.
(bug)
if (!staticAccessValue) { | |
if (staticAccessValue == null) { |
because
class MyClass {
''(): void { }
}
class MySubclass extends MyClass {
async ''(): Promise<void> { }
}
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.
Oh, nice catch! Is there any reason we don't have strict-boolean-expressions
enabled?
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.
Because IIRC both Joshs don't like it hehe. Brad and I both do like it. 🤷♂️
function getWellKnownStringOfSymbol(symbol: symbol): string | null { | ||
switch (symbol) { | ||
case Symbol.iterator: | ||
return 'iterator'; | ||
case Symbol.asyncIterator: | ||
return 'asyncIterator'; | ||
} | ||
|
||
return null; | ||
} |
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.
cross-linking discord thread https://discord.com/channels/1026804805894672454/1322853884292497418
Hey @ronami, do you plan to make further changes regarding #10310 (comment) or is this intended to be reviewed as is? |
Oh, I'm sorry; I've looked into it but completely forgot to get back to it. I didn't consider the issue with the current approach, which could result in different values on different machines and NodeJS versions (though looking into MDN, it seems pretty much all well-known symbols are supported on rather old Node versions already). I haven't noticed Having a way to get the key type of a property Thanks for your help with this ❤️ |
Ok, that definitely alleviates some of the concern 👍 Though I guess there's maybe still a forwards-compatibility question.
What I usually do is just ask questions on the TS discord and copy down the link on the issue/PR I'm working on - partly for visibility and partly just so I can find it again myself! 😅
❤️ |
PR Checklist
inheritedMethods
check should flag all statically analyzable declarations, not just identifiers and numbers. #10211Overview
This PR resolves #10211 and uses the existing
getStaticMemberAccessValue()
utility instead oftsutils.getPropertyOfType()
to handle most statically analyzable declarations/properties:I added some PR review comments on parts of the PR I'm not sure are correct, please let me know what you think.