feat: add support for decorator metadata in scope analysis and in consistent-type-imports #2751

Frezc · 2020-11-10T11:49:17Z

Changes:

check import if referenced by decorator metadata if "emitDecoratorMetadata" if on.
fix type import to value import if it referenced by decorator metadata

typescript-eslint · 2020-11-10T11:49:23Z

Thanks for the PR, @Frezc!

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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

codecov · 2020-11-10T11:59:47Z

Codecov Report

Merging #2751 (b631e7a) into master (85c2720) will decrease coverage by 0.07%.
The diff coverage is 92.01%.

@@            Coverage Diff             @@
##           master    #2751      +/-   ##
==========================================
- Coverage   92.76%   92.68%   -0.08%     
==========================================
  Files         310      312       +2     
  Lines       10393    10584     +191     
  Branches     2943     3004      +61     
==========================================
+ Hits         9641     9810     +169     
- Misses        347      353       +6     
- Partials      405      421      +16

Flag	Coverage Δ
unittest	`92.68% <92.01%> (-0.08%)`	⬇️

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

Impacted Files	Coverage Δ
packages/scope-manager/src/analyze.ts	`70.96% <50.00%> (-1.45%)`	⬇️
...kages/scope-manager/src/referencer/ClassVisitor.ts	`91.34% <91.34%> (ø)`
...eslint-plugin/src/rules/consistent-type-imports.ts	`95.00% <93.06%> (-2.16%)`	⬇️
...ackages/scope-manager/src/referencer/Referencer.ts	`93.90% <100.00%> (-0.97%)`	⬇️
...ckages/scope-manager/src/referencer/TypeVisitor.ts	`89.24% <100.00%> (+0.11%)`	⬆️
.../src/rules/sort-type-union-intersection-members.ts	`92.30% <0.00%> (ø)`
packages/eslint-plugin/src/rules/comma-spacing.ts	`97.87% <0.00%> (+0.09%)`	⬆️

bradzacher

Thanks for looking into this!
Quick eyeball of the code - a few comments:

requiring type information in the rule is a breaking change.
We should not be making this change as it hugely limits the usability of the rule.

This is the wrong approach to fix this - this should be fixed in the scope analyser - not in the rule.
Fixing this in the scope analyser would benefit all consumers of scope information, and not just this specific rule.

Frezc · 2020-11-10T19:12:33Z

requiring type information in the rule is a breaking change.
We should not be making this change as it hugely limits the usability of the rule.

I set allowWithoutFullTypeInformation = true to getParserServices. This won't make tsconfig optional ?

This is the wrong approach to fix this - this should be fixed in the scope analyser - not in the rule.
Fixing this in the scope analyser would benefit all consumers of scope information, and not just this specific rule.

emm.. I have no idea what is scope analyser. Can you give me a brief explanation?

bradzacher · 2020-11-10T19:21:52Z

The scope analyser is the tooling which runs over the AST and figures out what's a variable and where it is used.

Right now it doesn't understand that the decorators can consume values when emitDecoratorMetadata is turned on.

So when it analyses

constructor(
  private readonly config: ConfigService,
  private readonly usersService: UsersService,
  private readonly logger: Logger,
  private readonly authService: AuthService,
  @InjectRepository(UserSessionRepository)
  private readonly userSessionRepository: UserSessionRepository,
)

It doesn't understand that private readonly config: ConfigService creates an implicit value reference on ConfigService because there is a decorator and emitDecoratorMetadata is turned on.

Because it doesn't understand this - it just creates a type-only reference to ConfigService.
This data then flows through to the lint rules (i.e. consistent-type-imports) which then look at import ConfigService from './ConfigService', and sees only type-only imports - and flags it.

Fix the source of the data (the scope analyser) and the rule will "just work".

bradzacher

Definitely heading in the right direction!
This is looking great so far!

Instead of intermingling all of this logic with Referencer, I think we should separate it out into its own ClassVisitor - similar to how we currently have TypeVisitor and PatternVisitor.

Classes are a pretty strict set of things, so we can more easily isolate the logic and setup the appropriate state there.

I mention this as there is one case that you haven't yet handled with your implementation code:

import Thing from 'thing';

@decorator
class Foo {
  constructor(
    arg: Thing,
  ) {}
}

In this example, the decorator on the class with a constructor creates a similar reference to that of a decorated method.

playground repl

Handling this case will involve another piece of local state and things could get messy at that point, because you can nest class declarations within methods:

@decorator
class Bar {
  constructor(
    arg: React.Component,
  ) {
    @decorator
    class Foo {
      constructor(
        arg: React.Component,
      ) {}
    }
  }
}

repl

So being able to do ClassVisitor.visit(clazz) will be much nicer as we can construct one ClassVisitor per class and thus isolate the state entirely.

It's also worth noting that decorators are only ever valid for class declarations - so we could do some cleaner separation of logic to ignore bad cases:

@decorator
class Foo {
  constructor(
    arg: React.Component,
  ) {}
}

@decorator // this is invalid (TS semantic error)
const classExpr = class {
  @decorator // this is invalid (TS semantic error)
  method(
    @decorator // this is invalid (TS semantic error)
    arg: React.Component,
  ) {}
}

const obj = {
  @decorator // this is invalid (syntax error)
  method(
    @decorator  // this is invalid (TS semantic error)
    arg: React.Component,
  ) {}
}

One other case I don't see tested, is this:

class Foo {
  method(
    @decorator
    arg1: Thing1,
    arg2: Thing2,
  ) {}
}

repl

In this instance - TS will emit metadata about both Thing1 and Thing2, even though only arg1 is decorated.

All of the cases that I found TS to emit metadata are listed in my comment here: #2559 (comment)

Frezc · 2020-12-15T10:36:37Z

I mention this as there is one case that you haven't yet handled with your implementation code:
import Thing from 'thing';

@decorator
class Foo {
  constructor(
    arg: Thing,
  ) {}
}
In this example, the decorator on the class with a constructor creates a similar reference to that of a decorated method.

This case at: https://github.com/typescript-eslint/typescript-eslint/pull/2751/files#diff-0cfd7f7e671424ab7fca326415a261843e64acdd13ecb12fc5133bd98b642948R184-R198

One other case I don't see tested, is this:
class Foo {
  method(
    @decorator
    arg1: Thing1,
    arg2: Thing2,
  ) {}
}
repl

In this instance - TS will emit metadata about both Thing1 and Thing2, even though only arg1 is decorated.

I missed this, thanks for hint.

bradzacher · 2020-12-15T16:20:17Z

One thing I didn't mention - please add some snapshot tests!

Your tests so far are great as they are explicit assertions of how it works, but as you've probably found out they're super cumbersome to write.

A snapshot test is less strict/safe as you have to manually eyeball the output, but they're nice because they're so easy to write. It means you can create isolated cases for each case and have a granular regression test.

They're also nice because they create a visual representation of the scope, which I found invaluable in figuring out how it all works.

https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/scope-manager/tests/fixtures/decorators

You can add the new scope manager option here

And then add this to the top of your fixture to turn on the option (example).

//// @emitDecoratorMetadata = true

class Fixture {...}

bradzacher

Almost there! The code's looking good I think.

could you also add some snapshots for the nested class cases:

function deco(...param: any) {}

@deco
class A {
  constructor(foo: b.Foo) {
    class B {
      constructor(bar: c.Foo) {}
    }
  }
}

function deco(...param: any) {}

class A {
  constructor(foo: b.Foo) {
    @deco
    class B {
      constructor(bar: c.Foo) {}
    }
  }
}

function deco(...param: any) {}

@deco
class A {
  constructor(foo: b.Foo) {
    @deco
    class B {
      constructor(bar: c.Foo) {}
    }
  }
}

They should all work fine - but a few extra snapshots to prevent regressions would be ace.

Thanks for sticking with this! We're almost there!

packages/scope-manager/tests/fixtures/class/declaration/emit-metadata.ts

bradzacher

LGTM - thanks heaps for all of your work here and following this through to the end.
You've done a great job!

packages/scope-manager/tests/fixtures/class/emit-metadata/method-deco.ts.shot

packages/scope-manager/tests/fixtures/class/emit-metadata/nested-class-both.ts.shot

bradzacher · 2021-01-18T03:26:20Z

packages/scope-manager/tests/fixtures/class/emit-metadata/accessor-deco.ts.shot

+        Reference$9 {
+          identifier: Identifier<"T">,
+          isRead: true,
+          isTypeReference: true,
+          isValueReference: false,
+          isWrite: false,
+          resolved: Variable$5,
+        },


From the looks of it - this one is wrong.

This reference is for the pair:

set [keyName](a: T) {} @deco get [keyName]() {}

TS will emit the following code:

__decorate([ deco, __metadata("design:type", T), __metadata("design:paramtypes", []) ], A.prototype, _a, null);

So whilst it hasn't referenced the param type - it did reference the return type.
I don't think it's worth spending any time fixing this - it's a super rare usecase. Also it'll be a pretty hard case to reliably fix I think.

I'm 100% okay with just marking this as a KP and only looking at fixing it if someone runs into the issue.

packages/scope-manager/tests/fixtures/class/emit-metadata/accessor-deco.ts.shot

packages/scope-manager/tests/fixtures/class/emit-metadata/nested-class-inner.ts.shot

packages/scope-manager/tests/fixtures/class/emit-metadata/nested-class-outer.ts.shot

packages/scope-manager/tests/fixtures/class/emit-metadata/parameters-deco.ts.shot

packages/scope-manager/tests/fixtures/class/emit-metadata/property-deco.ts.shot

…ass method default params Fixes #2941 Fixes #2942 This was a regression introduced by #2751

…ass method default params (#2943) Fixes #2941 Fixes #2942 This was a regression introduced by #2751

miluoshi · 2021-01-19T11:39:13Z

I think this change broke @typescript-eslint/no-unused-vars rule for me.

After updating @typescript-eslint packages from v4.13.0 to v4.14.0 the @typescript-eslint/no-unused-vars rule started showing false positive in my codebase. After explicitly disabling emitDecoratorMetadata in tsconfig.json the warning disappears.

See attached screenshot:

1:10  warning  'ChangeDetectorRef' is defined but never used                      @typescript-eslint/no-unused-vars

edit: There were related issues created #2946 #2947

bradzacher · 2021-01-19T16:17:58Z

And as linked above, this was fixed in #2943

pmstss · 2021-01-28T19:23:11Z

Seems something is still wrong with this change. After updating from 4.13.0 to 4.14.1 (or 4.14.0) @typescript-eslint/no-unused-vars reports false positives.

Single usage of imported MultiFactorState is marked with an arrow.

bradzacher · 2021-01-28T19:25:53Z

Don't comment on old, closed PRs. The comments are not easily discoverable by other users, and PRs do not show up in the "issues" tab - further hiding them from other users.

There is a pinned issue to follow, which is easily discoverable by using the issue search for no-unused-vars

fix(eslint-plugin): fix value import in decorator metadata

ed33caa

bradzacher requested changes Nov 10, 2020

View reviewed changes

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Nov 10, 2020

bradzacher changed the title ~~fix(eslint-plugin): [consistent-type-imports] fix passive value import in decorator metadata~~ feat(eslint-plugin): [consistent-type-imports] fix passive value import in decorator metadata Nov 10, 2020

bradzacher added the enhancement New feature or request label Nov 10, 2020

fix(eslint-plugin): test coverage

5b7de66

Frezc added 3 commits December 13, 2020 00:53

move logic

e24d78d

Merge remote-tracking branch 'origin/master' into fix/meta

364f4cb

fix ci

843f072

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Dec 12, 2020

bradzacher requested changes Dec 13, 2020

View reviewed changes

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Dec 13, 2020

Frezc added 2 commits January 3, 2021 22:47

class visitor

518d21a

Merge remote-tracking branch 'origin/master' into fix/meta

58324a0

Frezc requested a review from bradzacher January 3, 2021 15:05

bradzacher requested changes Jan 3, 2021

View reviewed changes

packages/scope-manager/tests/fixtures/class/declaration/emit-metadata.ts Outdated Show resolved Hide resolved

split snapshot

b631e7a

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jan 4, 2021

bradzacher approved these changes Jan 18, 2021

View reviewed changes

bradzacher changed the title ~~feat(eslint-plugin): [consistent-type-imports] fix passive value import in decorator metadata~~ feat: add support for decorator metadata in scope analysis and in consistent-type-imports Jan 18, 2021

bradzacher merged commit 445e416 into typescript-eslint:master Jan 18, 2021

bradzacher added a commit that referenced this pull request Jan 19, 2021

fix(scope-manager): fix incorrect handling of class decorators and cl…

cabfcf7

…ass method default params Fixes #2941 Fixes #2942 This was a regression introduced by #2751

bradzacher mentioned this pull request Jan 19, 2021

fix(scope-manager): fix incorrect handling of class decorators and class method default params #2943

Merged

bradzacher added a commit that referenced this pull request Jan 19, 2021

fix(scope-manager): fix incorrect handling of class decorators and cl…

e1eac83

…ass method default params (#2943) Fixes #2941 Fixes #2942 This was a regression introduced by #2751

mvnrhd mentioned this pull request Jan 19, 2021

[no-unused-vars] false positive for Type Import #2946

Closed

3 tasks

typescript-eslint locked as resolved and limited conversation to collaborators Jan 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for decorator metadata in scope analysis and in consistent-type-imports #2751

feat: add support for decorator metadata in scope analysis and in consistent-type-imports #2751

Frezc commented Nov 10, 2020

typescript-eslint bot commented Nov 10, 2020

codecov bot commented Nov 10, 2020 •

edited

Loading

bradzacher left a comment •

edited

Loading

Frezc commented Nov 10, 2020

bradzacher commented Nov 10, 2020

bradzacher left a comment •

edited

Loading

Frezc commented Dec 15, 2020

bradzacher commented Dec 15, 2020 •

edited

Loading

bradzacher left a comment

bradzacher left a comment

bradzacher Jan 18, 2021

miluoshi commented Jan 19, 2021 •

edited

Loading

bradzacher commented Jan 19, 2021

pmstss commented Jan 28, 2021

bradzacher commented Jan 28, 2021 •

edited

Loading

feat: add support for decorator metadata in scope analysis and in consistent-type-imports #2751

feat: add support for decorator metadata in scope analysis and in consistent-type-imports #2751

Conversation

Frezc commented Nov 10, 2020

typescript-eslint bot commented Nov 10, 2020

codecov bot commented Nov 10, 2020 • edited Loading

Codecov Report

bradzacher left a comment • edited Loading

Choose a reason for hiding this comment

Frezc commented Nov 10, 2020

bradzacher commented Nov 10, 2020

bradzacher left a comment • edited Loading

Choose a reason for hiding this comment

Frezc commented Dec 15, 2020

bradzacher commented Dec 15, 2020 • edited Loading

bradzacher left a comment

Choose a reason for hiding this comment

bradzacher left a comment

Choose a reason for hiding this comment

bradzacher Jan 18, 2021

Choose a reason for hiding this comment

miluoshi commented Jan 19, 2021 • edited Loading

bradzacher commented Jan 19, 2021

pmstss commented Jan 28, 2021

bradzacher commented Jan 28, 2021 • edited Loading

codecov bot commented Nov 10, 2020 •

edited

Loading

bradzacher left a comment •

edited

Loading

bradzacher left a comment •

edited

Loading

bradzacher commented Dec 15, 2020 •

edited

Loading

miluoshi commented Jan 19, 2021 •

edited

Loading

bradzacher commented Jan 28, 2021 •

edited

Loading