typescript-eslint/typescript-eslintPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star15.8k

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

arkapratimc wants to merge3 commits intotypescript-eslint:mainfromarkapratimc:issue-8234

Closed

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

arkapratimc wants to merge3 commits intotypescript-eslint:mainfromarkapratimc:issue-8234

Conversation

Copy link

Contributor

arkapratimc commentedJan 22, 2024•
edited
Loading

fixes:#8234

PR Checklist

Addresses an existing open issue:fixesRepo: reuse newly added "is builtin symbol like" logic fromtype-utils #8234
That issue was marked asaccepting prs
Steps inContributing were taken

Overview

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

d004c95

fixes:typescript-eslint#8234

Copy link

Contributor

typescript-eslintbot commentedJan 22, 2024

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.

Copy link

netlifybot commentedJan 22, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`7c96190`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/65ae788bf15b9f0008692838
😎 Deploy Preview	https://deploy-preview-8287--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (🟢 up 2 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to yourNetlify site configuration.

chore: lint errors

59adffb

auvred requested changes

Jan 22, 2024

View reviewed changes

packages/type-utils/src/builtinSymbolLikes.ts Outdated

		* let I = (callback: Function) => {}
		* ^ FunctionLike
		*/
		export function isFunctionLike(program: ts.Program, type: ts.Type): boolean {

Copy link

Member

Choose a reason for hiding this comment

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

typescript package already exports the function with same nameisFunctionLike. Having two functions with the same name may cause confusion.

packages/type-utils/src/builtinSymbolLikes.ts

		export function isFunctionLike(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 nameFunctionConstructor. Should we add tests that contain symbols with nameFunctionConstructor? But that is probably not within the scope of this pr. IMO we shouldn't change the rule logic in this pr

Copy link

ContributorAuthor

arkapratimcJan 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 in920f909

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

Copy link

Member

auvredJan 22, 2024

Choose a reason for hiding this comment

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

You're rightFunction here hasFunctionConstructor type. I missed this because in the source code of rule both symbol name and callee identifier's name are compared toFUNCTION_CONSTRUCTOR

Then +1 on renamingisFunctionLike... Not sure what's the best name for it. MaybeisFunctionTypeLike or something like this?

Also we should consider that#8094 most likely will introduce changes toisBuiltinSymbolLike.
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

ContributorAuthor

arkapratimcJan 22, 2024

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

JoshuaKGoldbergJan 24, 2024

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

JoshuaKGoldbergApr 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 aCo-authored-by: Timothy Moore <mtimothy984@gmail.com> in the PR description)

packages/eslint-plugin/src/rules/no-implied-eval.ts Outdated

Comment on lines 82 to 85

		// 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);
		}

Copy link

Member

auvredJan 22, 2024

Choose a reason for hiding this comment

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

isFunctionLike internally already checks that the type has a symbol, and that symbol is namedFunction, so the condition above is redundant.

packages/eslint-plugin/src/rules/no-implied-eval.ts Outdated

		@@ -1,3 +1,4 @@
		import { isFunctionLike } from '@typescript-eslint/type-utils';

Copy link

Member

auvredJan 22, 2024

Choose a reason for hiding this comment

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

@typescript-eslint/type-utils shouldn't be imported directly. This function should probably be imported fromsrc/utils, as other rules do

typescript-eslint/packages/eslint-plugin/src/util/index.ts

Lines 18 to 19 in920f909

	// this is done for convenience - saves migrating all of the old rules
	export*from'@typescript-eslint/type-utils';

chore: address reviews

7c96190

JoshuaKGoldberg added the blocked by another PRPRs which are ready to go but waiting on another PR label

Jan 24, 2024

JoshuaKGoldberg mentioned this pull request

Feb 9, 2024

Repo: reuse newly added "is builtin symbol like" logic fromtype-utils#8234

Closed

JoshuaKGoldberg added awaiting responseIssues waiting for a reply from the OP or another party and removed blocked by another PRPRs which are ready to go but waiting on another PR labels

Apr 23, 2024