feat(eslint-plugin): [prefer-promise-reject-errors] add rule #8011

auvred · 2023-12-01T15:03:47Z

PR Checklist

Addresses an existing open issue: fixes Base rule extension: [prefer-promise-reject-errors] #7687
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

I tried to migrate the original rule with only type information added (it allows to detect PromiseConstructor like and Error like variables)

Also I moved isErrorLike function to the type-utils package (I hope it's the right place for this function) as its logic may be reused both in prefer-promise-reject-errors and no-throw-literal

I copied incorrect and correct examples from no-throw-literal because I found them applicable. But maybe there are too many examples for this rule

typescript-eslint · 2023-12-01T15:03:51Z

Thanks for the PR, @auvred!

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.

netlify · 2023-12-01T15:03:52Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`bd20782`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/659a6189a92e2f00080aa1f9
😎 Deploy Preview	https://deploy-preview-8011--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 90 (🔴 down 9 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 your Netlify site configuration.

packages/eslint-plugin/docs/rules/prefer-promise-reject-errors.md

packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts

JoshuaKGoldberg

Awesome, really solid work here - thanks for working on it @auvred!

It's interesting that this is the first rule to really need to check whether something is actually a 'PromiseConstructor'. Others have just approximated comparisons to Thenable.

A few touchups & refactors requested, but the general direction looks great to me! 🚀

packages/eslint-plugin/docs/rules/prefer-promise-reject-errors.md

JoshuaKGoldberg · 2023-12-05T19:54:38Z

packages/eslint-plugin/src/rules/prefer-promise-reject-errors.ts

+    function isPromiseConstructorLike(type: ts.Type): boolean {
+      const symbol = type.getSymbol();
+
+      if (symbol?.getName() === 'PromiseConstructor') {
+        const declarations = symbol.getDeclarations() ?? [];
+        for (const declaration of declarations) {
+          const sourceFile = declaration.getSourceFile();
+          if (program.isSourceFileDefaultLibrary(sourceFile)) {
+            return true;
+          }
+        }
+      }
+
+      return false;
+    }


[Refactor] This is roughly the same logic as isErrorLike's for checking if it's a built-in of a particular name. Could you extract into a shared helper for this too?

Not sure about the function name, but I extracted it: https://github.com/typescript-eslint/typescript-eslint/pull/8011/files#diff-75626015398ba4cc6293d6ec23077170a50f23e306b326a44966e44448c3edf2

TODO: refactor these places to use this function

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

Lines 82 to 90 in 705370a

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;

}

}

}

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

Lines 145 to 153 in 705370a

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;

}

}

LGTM! If you want to do that refactor in this PR I'd happily approve. Or we can leave it as a good first issue followup. Either way.

Let's open a followup with good first issue ;)

packages/eslint-plugin/src/rules/prefer-promise-reject-errors.ts

packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts

packages/eslint-plugin/src/rules/prefer-promise-reject-errors.ts

JoshuaKGoldberg

This looks lovely, thanks! 🙌

No blocking requests (other than the "guarantee" nit) from me. Just some touchup suggestions. Feel free to wontfix or do them, and then we can merge. Or if you don't have time I'll take another look soon. Cheers!

packages/eslint-plugin/docs/rules/prefer-promise-reject-errors.md

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

JoshuaKGoldberg

Sorry, two blocking requests:

The variable readability change actually introduced a performance regression - lmk if the suggestion doesn't make sense?
Apologies, I missed commenting on a couple of missing test cases

Since I already approved I do feel bad now requesting changes. If you don't have energy+time then no worries. I can apply these soon.

packages/eslint-plugin/src/rules/prefer-promise-reject-errors.ts

packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts

JoshuaKGoldberg

Super, thanks! This looks great. 🚀

auvred added 4 commits December 1, 2023 11:04

feat(eslint-plugin): [prefer-promise-reject-errors] new rule!

a4f7195

test: ~100% coverage

538323f

docs: add rule docs

f779d93

test: add some cases

a27930a

auvred marked this pull request as draft December 1, 2023 15:16

auvred added 4 commits December 1, 2023 15:37

chore: lint --fix

2b83ea4

chore: reformat tests

d86f5c0

feat: add support for literal computed reject name

dab6503

chore: lint --fix

8d8bbc3

auvred marked this pull request as ready for review December 2, 2023 09:04

refactor: get rid of one @ts-expect-error

ff307be

StyleShit suggested changes Dec 3, 2023

View reviewed changes

auvred added 2 commits December 3, 2023 19:37

docs: refer to the original rule description

ccde4a6

test: add few cases

d153d13

JoshuaKGoldberg requested changes Dec 5, 2023

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Dec 5, 2023

JoshuaKGoldberg reviewed Dec 5, 2023

View reviewed changes

packages/eslint-plugin/src/rules/prefer-promise-reject-errors.ts Show resolved Hide resolved

auvred added 11 commits December 6, 2023 15:17

docs: remove some examples

5263fa8

refactor: move check if symbol is from default lib or not to new fn

6981eb5

refactor: assert that rejectVariable is non-nullable

a680b8d

chore: remove assertion in skipChainExpression

1e2e771

test: specify error ranges for invalid test cases

e1db988

chore: merge main

1401910

chore: format tests

42e5c1f

chore: remove unused check if variable reference is read or not

47eb018

chore: include rule to strict-type-checked config

f2ee5d9

refactor: simplify isSymbolFromDefaultLibrary

0229ae7

chore: remove ts-expect-error comment

9db6d84

JoshuaKGoldberg previously approved these changes Dec 29, 2023

View reviewed changes

packages/eslint-plugin/docs/rules/prefer-promise-reject-errors.md Outdated Show resolved Hide resolved

JoshuaKGoldberg added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Dec 29, 2023

Update packages/eslint-plugin/docs/rules/prefer-promise-reject-errors.md

1c97219

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

auvred dismissed JoshuaKGoldberg’s stale review via 1c97219 December 29, 2023 19:42

auvred added 4 commits December 29, 2023 19:45

refactor: program -> services.program

42c5737

refactor: split unreadable if condition

184de0f

docs: simplify examples

eaab9eb

refactor: rename isBuiltinSymbolLike.ts -> builtinSymbolLikes.ts

895f1b5

auvred force-pushed the feat/prefer-promise-reject-errors-rule branch from ad4dc75 to 895f1b5 Compare December 29, 2023 19:46

Merge branch 'main' into feat/prefer-promise-reject-errors-rule

584c2e7

Tjstretchalot added a commit to Tjstretchalot/typescript-eslint that referenced this pull request Jan 2, 2024

Use latest helpers from typescript-eslint#8011

10ac860

JoshuaKGoldberg requested changes Jan 4, 2024

View reviewed changes

packages/eslint-plugin/src/rules/prefer-promise-reject-errors.ts Outdated Show resolved Hide resolved

packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts Show resolved Hide resolved

JoshuaKGoldberg added awaiting response Issues waiting for a reply from the OP or another party and removed 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labels Jan 4, 2024

auvred added 2 commits January 4, 2024 16:56

perf: get type of reject callee lazily

f186084

test: add cases with arrays,never,unknown

e4af6ed

auvred requested a review from JoshuaKGoldberg January 4, 2024 17:09

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Jan 4, 2024

auvred and others added 4 commits January 5, 2024 12:25

feat: add support for Readonly<Error> and similar

2326429

chore: fix lint issues

f33a84a

Merge branch 'main' into feat/prefer-promise-reject-errors-rule

7236787

Merge branch 'main' into feat/prefer-promise-reject-errors-rule

bd20782

JoshuaKGoldberg reviewed Jan 9, 2024

View reviewed changes

JoshuaKGoldberg approved these changes Jan 9, 2024

View reviewed changes

JoshuaKGoldberg merged commit 1aa8664 into typescript-eslint:main Jan 9, 2024

This was referenced Jan 9, 2024

Bug: [no-throw-literal] False positive report on Readonly<Error> #8233

Closed

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

Closed

Tjstretchalot added a commit to Tjstretchalot/typescript-eslint that referenced this pull request Jan 10, 2024

Use latest helpers from typescript-eslint#8011

30539a3

Tjstretchalot added a commit to Tjstretchalot/typescript-eslint that referenced this pull request Jan 12, 2024

Use latest helpers from typescript-eslint#8011

df8f3a4

github-actions bot locked as resolved and limited conversation to collaborators Jan 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): [prefer-promise-reject-errors] add rule #8011

feat(eslint-plugin): [prefer-promise-reject-errors] add rule #8011

auvred commented Dec 1, 2023

typescript-eslint bot commented Dec 1, 2023

netlify bot commented Dec 1, 2023 •

edited

Loading

JoshuaKGoldberg left a comment

JoshuaKGoldberg Dec 5, 2023

auvred Dec 6, 2023

JoshuaKGoldberg Dec 29, 2023

auvred Dec 29, 2023

JoshuaKGoldberg left a comment

JoshuaKGoldberg left a comment

JoshuaKGoldberg left a comment

	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 (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;
	}
	}

feat(eslint-plugin): [prefer-promise-reject-errors] add rule #8011

feat(eslint-plugin): [prefer-promise-reject-errors] add rule #8011

Conversation

auvred commented Dec 1, 2023

PR Checklist

Overview

typescript-eslint bot commented Dec 1, 2023

netlify bot commented Dec 1, 2023 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Dec 5, 2023

Choose a reason for hiding this comment

auvred Dec 6, 2023

Choose a reason for hiding this comment

JoshuaKGoldberg Dec 29, 2023

Choose a reason for hiding this comment

auvred Dec 29, 2023

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

netlify bot commented Dec 1, 2023 •

edited

Loading