fix: fix illegal decorator check #6723

fisker · 2023-03-21T07:17:34Z

PR Checklist

Addresses an existing open issue: fixes #000
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

Copied from prettier https://github.com/prettier/prettier/blob/ddf3b43c33e2e98f6413b5232ad623876d96738e/src/language-js/parse/postprocess/typescript.js#L46

typescript-eslint · 2023-03-21T07:17:37Z

Thanks for the PR, @fisker!

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-03-21T07:17:40Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`e7743d4`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/648d0d26b3171f00080ba75d
😎 Deploy Preview	https://deploy-preview-6723--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

nx-cloud · 2023-03-21T07:22:00Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit e7743d4. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 31 targets

Sent with 💌 from NxCloud.

packages/typescript-estree/src/node-utils.ts

codecov · 2023-03-21T09:43:29Z

Codecov Report

Merging #6723 (1bbf432) into v6 (89b14f2) will decrease coverage by 0.48%.
The diff coverage is 17.94%.

❗ Current head 1bbf432 differs from pull request most recent head e7743d4. Consider uploading reports for the commit e7743d4 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##               v6    #6723      +/-   ##
==========================================
- Coverage   87.75%   87.28%   -0.48%     
==========================================
  Files         372      376       +4     
  Lines       12814    12974     +160     
  Branches     3800     3833      +33     
==========================================
+ Hits        11245    11324      +79     
- Misses       1193     1262      +69     
- Partials      376      388      +12

Flag	Coverage Δ
unittest	`87.28% <17.94%> (-0.48%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/typescript-estree/src/node-utils.ts	`54.77% <0.00%> (-6.92%)`	⬇️
packages/typescript-estree/src/convert.ts	`31.11% <16.66%> (-0.21%)`	⬇️
packages/typescript-estree/src/getModifiers.ts	`69.23% <100.00%> (+1.04%)`	⬆️

... and 35 files with indirect coverage changes

bradzacher · 2023-03-21T11:06:23Z

packages/typescript-estree/src/convert.ts

    if (nodeHasIllegalDecorators(node)) {
      this.#throwError(
        node.illegalDecorators[0],
        'Decorators are not valid here.',
      );
    }

-    for (const modifier of getModifiers(node) ?? []) {
+    // @ts-expect-error -- this is safe as it's guarded
+    const modifiers: ts.ModifierLike[] = node.modifiers;


instead of using .modifiers can we use getModifiers here?

getModifiers only returns ts.Modifier, but we need ts.Decorators, unless we call getDecorators again.

In that same file there is the getDecorators util which should do what you want I think?

I know.

unless we ~~can~~ call getDecorators again.

But I checked both modifiers and decorators here, and getModifiers() + getDecorators() is node.modifiers

I'm not quite sure I understand - your code below does "for each modifier, only action the decorators"

So couldn't the code be

for (const decorator of getDecorators(node)) { // your code } for (const modifier of getModifiers(node)) { // old code }

Ideally, it should work, but I still can't get why you want 4 loops when you can do it in 1 loop.

Anyway, I tried, but turns out getModifiers() + getDecorators() not equals to node.modifiers.
ts.canHaveModifiers() and ts.canHaveDecorators() are stopping me to access modifiers on some node. You can try it to see the failed tests.

Should we

Remove those check

Add option to skip check

?

The problem here is that they purposely removed node.modifiers from the types in TSv5 - so we definitely don't want to be accessing it directly if we can avoid it as it's clear that their intention is for it to be a truly private property.

I guess we could add an option to skip the checks?

bradzacher · 2023-03-21T12:08:24Z

packages/typescript-estree/src/node-utils.ts

+    useLegacyDecorators =>
+      // @ts-expect-error -- internal api
+      // eslint-disable-next-line @typescript-eslint/no-unsafe-call
+      ts.nodeCanBeDecorated?.(


we can't use this function, or at least not without version checks.
in versions before 5.0 this function did not take boolean as the first argument - so by calling it like this we will cause a crash.

Given it's an internal API - I think we might want to consider rewriting it for ourselves? The logic will be pretty static given the spec doesn't change much if at all.

in versions before 5.0 this function did not take boolean as the first argument

Didn't know that.

I think we might want to consider rewriting it for ourselves?

I can try.

Added acaee6b, but I don't know the equivalent of ts.hasAmbientModifier, can you help?

hasAmbientModifier was only added in TS 4.3, and currently on v6 we are supporting 4.2
However based on the 2y lifecycle of versions that DefinitelyTyped supports (which we want to roughly align with) we will actually now be able to bump the minimum version to 4.3.

We can queue a separate PR for that then update this PR to use ts.hasAmbientModifier (#6923)

ts.hasAmbientModifier seems not available https://github.com/typescript-eslint/typescript-eslint/actions/runs/4739172024/jobs/8413765640?pr=6723#step:4:98

it's still a private, internal API!

Let's use a fake one for now? a84fa78

If we're going with a fake () => false one then I just removed it altogether. Seems like everything is working well without it? I posted a link in microsoft/TypeScript#52727 (comment).

This reverts commit 04e3b73.

JoshuaKGoldberg · 2023-06-15T22:22:12Z

@fisker just checking in - are you waiting for us on anything for this one? I'm not sure from reading through the comments, sorry.

fisker · 2023-06-16T00:27:16Z

I don't know how ts.hasAmbientModifier works, and can't access ts.hasAmbientModifier directly, so I suggest use () => false for now. #6723 (comment)

JoshuaKGoldberg

Swell, thanks @fisker! I'll merge this now. 🚀 Thanks for the informative discussions!

Fix illegal decorator check

282e115

fisker changed the title ~~Fix illegal decorator check~~ fix: fix illegal decorator check Mar 21, 2023

Fix

35304da

fisker commented Mar 21, 2023

View reviewed changes

packages/typescript-estree/src/node-utils.ts Outdated Show resolved Hide resolved

fisker added 5 commits March 21, 2023 17:02

Fix scope-manager tests

07b03ee

Fix member-ordering.test.ts

1a4b800

Fix no-unused-vars.test.ts

a2e2a42

Fix explicit-member-accessibility.test.ts

63f0c3f

Fix member-ordering-alphabetically-order.test.ts

01c197f

fisker added 2 commits March 21, 2023 17:44

Linting

af3a56f

Don't check setter getters

6360076

fisker marked this pull request as ready for review March 21, 2023 10:21

bradzacher added this to the 6.0.0 milestone Mar 21, 2023

bradzacher reviewed Mar 21, 2023

View reviewed changes

fisker added 5 commits March 21, 2023 20:57

Implement nodeCanBeDecorated

acaee6b

Fix comment

ee03f74

Fix type

28a43e7

Revert

9a83be1

Fix

229c4ce

fisker force-pushed the decorator-check branch from a443e5c to 229c4ce Compare March 21, 2023 13:31

armano2 added the package: typescript-estree Issues related to @typescript-eslint/typescript-estree label Mar 22, 2023

fisker mentioned this pull request Apr 7, 2023

Try out v6 beta on various important community repos #6760

Closed

JoshuaKGoldberg requested a review from bradzacher April 7, 2023 15:11

bradzacher mentioned this pull request Apr 17, 2023

chore: bump minimum TS to 4.3.5 #6923

Merged

Merge branch 'v6' into decorator-check

aea5e0d

fisker added 3 commits April 19, 2023 11:14

Use ts.hasAmbientModifier

04e3b73

Revert "Use ts.hasAmbientModifier"

a84fa78

This reverts commit 04e3b73.

refactor: use getModifiers and getDecorators

1bbf432

Merge branch 'v6' into decorator-check

00e9b42

JoshuaKGoldberg mentioned this pull request Jun 17, 2023

Expose more node checking / type guard API functions for developers microsoft/TypeScript#52727

Open

5 tasks

Remove stub hasAmbientModifier

e7743d4

JoshuaKGoldberg approved these changes Jun 17, 2023

View reviewed changes

JoshuaKGoldberg merged commit c456f8c into typescript-eslint:v6 Jun 17, 2023
42 of 44 checks passed

fisker deleted the decorator-check branch June 17, 2023 01:51

github-actions bot locked as resolved and limited conversation to collaborators Jun 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix illegal decorator check #6723

fix: fix illegal decorator check #6723

fisker commented Mar 21, 2023

typescript-eslint bot commented Mar 21, 2023

netlify bot commented Mar 21, 2023 •

edited

nx-cloud bot commented Mar 21, 2023 •

edited

codecov bot commented Mar 21, 2023 •

edited

bradzacher Mar 21, 2023

fisker Mar 21, 2023 •

edited

bradzacher Mar 21, 2023

fisker Mar 21, 2023 •

edited

bradzacher Apr 17, 2023

fisker Apr 19, 2023 •

edited

bradzacher Apr 19, 2023

bradzacher Mar 21, 2023

fisker Mar 21, 2023

fisker Mar 21, 2023

bradzacher Apr 17, 2023 •

edited

fisker Apr 19, 2023

bradzacher Apr 19, 2023

fisker Apr 19, 2023

JoshuaKGoldberg Jun 17, 2023

JoshuaKGoldberg commented Jun 15, 2023

fisker commented Jun 16, 2023

JoshuaKGoldberg left a comment

fix: fix illegal decorator check #6723

fix: fix illegal decorator check #6723

Conversation

fisker commented Mar 21, 2023

PR Checklist

Overview

typescript-eslint bot commented Mar 21, 2023

netlify bot commented Mar 21, 2023 • edited

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Mar 21, 2023 • edited

☁️ Nx Cloud Report

codecov bot commented Mar 21, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

fisker Mar 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisker Mar 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisker Apr 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradzacher Apr 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Jun 15, 2023

fisker commented Jun 16, 2023

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

netlify bot commented Mar 21, 2023 •

edited

nx-cloud bot commented Mar 21, 2023 •

edited

codecov bot commented Mar 21, 2023 •

edited

fisker Mar 21, 2023 •

edited

fisker Mar 21, 2023 •

edited

fisker Apr 19, 2023 •

edited

bradzacher Apr 17, 2023 •

edited