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

[Frezc]

Fixes #2559

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]

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]

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]

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]

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]

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]

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!

[bradzacher]

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

[bradzacher]

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.

[miluoshi]

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]

And as linked above, this was fixed in #2943

[pmstss]

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]

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