chore(type-utils): reuse newly added "is builtin symbol like" logic #8287
Conversation
|
Thanks for the PR, @arka1002! 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. |
| * let I = (callback: Function) => {} | ||
| * ^ FunctionLike | ||
| */ | ||
| export function isFunctionLike(program: ts.Program, type: ts.Type): boolean { |
There was a problem hiding this comment.
typescript package already exports the function with same name isFunctionLike. Having two functions with the same name may cause confusion.
| export function isFunctionLike(program: ts.Program, type: ts.Type): boolean { | ||
| return ( | ||
| isBuiltinSymbolLike(program, type, 'Function') || | ||
| isBuiltinSymbolLike(program, type, 'FunctionConstructor') |
There was a problem hiding this comment.
The original rule didn't check for symbols with name FunctionConstructor. Should we add tests that contain symbols with name FunctionConstructor? But that is probably not within the scope of this pr. IMO we shouldn't change the rule logic in this pr
There was a problem hiding this comment.
I just want to ask a small question -
This rule is checking FunctionConstructor right ? Like, in this case, the escapedName of the symbol is 'FunctionConstructor'.
typescript-eslint/packages/eslint-plugin/tests/rules/no-implied-eval.test.ts
Lines 876 to 883 in 920f909
There was a problem hiding this comment.
You're right Function here has FunctionConstructor type. I missed this because in the source code of rule both symbol name and callee identifier's name are compared to FUNCTION_CONSTRUCTOR
Then +1 on renaming isFunctionLike... Not sure what's the best name for it. Maybe isFunctionTypeLike or something like this?
Also we should consider that #8094 most likely will introduce changes to isBuiltinSymbolLike.
As per #8094 (comment):
isBuiltinSymbolLike(program, type, 'Function') || isBuiltinSymbolLike(program, type, 'FunctionConstructor') recursively visits all subtypes twice, we can optimize to not do the same job twice. #8094 does the following:
isBuiltinSymbolLike(
services.program,
calleeType,
name => name === 'PromiseConstructor' || name === 'Promise'
)Perhaps we can wait until #8094 is merged and then do the same
There was a problem hiding this comment.
But that is probably not within the scope of this pr. IMO we shouldn't change the rule logic in this pr
Yeah this is just a refactor (chore). If you want to change how it works that'd be a separate issue.
👍 I'll mark this as blocked on #8094. Thanks :)
There was a problem hiding this comment.
#8094 is closed - so we can now pretend it doesn't exist*. cc @arka1002, I'm un-blocking this PR. Sorry for the long wait!
*(though if you end up taking in code from it, we'll need a Co-authored-by: Timothy Moore <mtimothy984@gmail.com> in the PR description)
| // eslint-disable-next-line @typescript-eslint/no-unsafe-enum-comparison | ||
| if (symbol && symbol.escapedName === FUNCTION_CONSTRUCTOR) { | ||
| const declarations = symbol.getDeclarations() ?? []; | ||
| for (const declaration of declarations) { | ||
| const sourceFile = declaration.getSourceFile(); | ||
| if (services.program.isSourceFileDefaultLibrary(sourceFile)) { | ||
| return true; | ||
| } | ||
| } | ||
| return isFunctionLike(services.program, type); | ||
| } |
There was a problem hiding this comment.
isFunctionLike internally already checks that the type has a symbol, and that symbol is named Function, so the condition above is redundant.
| @@ -1,3 +1,4 @@ | |||
| import { isFunctionLike } from '@typescript-eslint/type-utils'; | |||
There was a problem hiding this comment.
@typescript-eslint/type-utils shouldn't be imported directly. This function should probably be imported from src/utils, as other rules do
typescript-eslint/packages/eslint-plugin/src/util/index.ts
Lines 18 to 19 in 920f909
|
👋 ping @arka1002, just checking in - is this PR still something you have the time and energy for? |
|
Closing it 👍 |
fixes: #8234
PR Checklist
type-utils#8234Overview