Skip to content

chore(type-utils): reuse newly added "is builtin symbol like" logic #8287

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

chore(type-utils): reuse newly added "is builtin symbol like" logic #8287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d004c95
chore(type-utils): reuse newly added "is builtin symbol like" logic
arkapratimc Jan 22, 2024
59adffb
chore: lint errors
arkapratimc Jan 22, 2024
7c96190
chore: address reviews
arkapratimc Jan 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 5 additions & 17 deletions packages/eslint-plugin/src/rules/no-implied-eval.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { getScope } from '@typescript-eslint/utils/eslint-utils';
import * as tsutils from 'ts-api-utils';
import * as ts from 'typescript';

import { createRule, getParserServices } from '../util';
import { createRule, getParserServices, isFunctionSimilar } from '../util';

const FUNCTION_CONSTRUCTOR = 'Function';
const GLOBAL_CANDIDATES = new Set(['global', 'window', 'globalThis']);
Expand Down Expand Up @@ -78,15 +78,8 @@ export default createRule({
return true;
}

// 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;
}
}
if (isFunctionSimilar(services.program, type)) {
return true;
}

const signatures = checker.getSignaturesOfType(
Expand Down Expand Up @@ -143,13 +136,8 @@ export default createRule({
const type = services.getTypeAtLocation(node.callee);
const symbol = type.getSymbol();
if (symbol) {
const declarations = symbol.getDeclarations() ?? [];
for (const declaration of declarations) {
const sourceFile = declaration.getSourceFile();
if (services.program.isSourceFileDefaultLibrary(sourceFile)) {
context.report({ node, messageId: 'noFunctionConstructor' });
return;
}
if (isFunctionSimilar(services.program, type)) {
context.report({ node, messageId: 'noFunctionConstructor' });
}
} else {
context.report({ node, messageId: 'noFunctionConstructor' });
Expand Down
12 changes: 12 additions & 0 deletions packages/type-utils/src/builtinSymbolLikes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,18 @@ export function isReadonlyTypeLike(
);
});
}
/**
* let F = new Function("foo");
* ^ FunctionSimilar
* let I = (callback: Function) => {}
* ^ FunctionSimilar
*/
export function isFunctionSimilar(program: ts.Program, type: ts.Type): boolean {
return (
isBuiltinSymbolLike(program, type, 'Function') ||
isBuiltinSymbolLike(program, type, 'FunctionConstructor')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@arkapratimc arkapratimc Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

code: "const fn = new Function('a', 'b', 'return a + b');",
errors: [
{
messageId: 'noFunctionConstructor',
line: 1,
column: 12,
},
],

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg Apr 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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 <[email protected]> in the PR description)

);
}
export function isBuiltinTypeAliasLike(
program: ts.Program,
type: ts.Type,
Expand Down