Skip to content

[consistent-type-imports] False positive with decorator metadata #4608

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Closed
3 tasks done
TomMettam opened this issue Feb 28, 2022 · 7 comments · Fixed by #4646
Closed
3 tasks done

[consistent-type-imports] False positive with decorator metadata #4608

TomMettam opened this issue Feb 28, 2022 · 7 comments · Fixed by #4646
Labels
awaiting response Issues waiting for a reply from the OP or another party package: scope-manager Issues related to @typescript-eslint/scope-manager

Comments

@TomMettam
Copy link

TomMettam commented Feb 28, 2022 •
edited
Loading

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have read the FAQ and my problem is not listed.

Repro

(no rule configuration, using plugin:@typescript-eslint/all)

import type { Ref } from '@typegoose/typegoose';
import { prop } from '@typegoose/typegoose';

export class Test
{
    @prop({ ref: () => Test })
    public test?: Ref<Test>;
}

Expected Result

Ref is only used as a type, and is not used by the decorator, no issue should be raised here.

Actual Result

ESLint: Type import "Ref" is used by decorator metadata.(@typescript-eslint/consistent-type-imports)

However, typescript compiles the code fine with 'importsNotUsedAsValues'.

When removing type, typescript compilation fails:

../../src/backend/models/Test.ts:1:1 - error TS1371: This import is never used as a value and must use 'import type' 
because 'importsNotUsedAsValues' is set to 'error'.

1 import { Ref } from '@typegoose/typegoose';
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Versions

package version
@typescript-eslint/eslint-plugin 13.1.0
@typescript-eslint/parser 5.12.1
TypeScript 4.5.5
ESLint 8.10.0
node 16.13.0
@TomMettam TomMettam added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Feb 28, 2022
@JoshuaKGoldberg
Copy link
Member

Thanks for the report! It's good to know that this is a case that happens outside of Vue (#3108).

cc @bradzacher - tentatively I think we're stuck here and will just have to take a breaking change on consistent-type-imports to use the type checker?

@JoshuaKGoldberg JoshuaKGoldberg added breaking change This change will require a new major version to be released bug Something isn't working and removed triage Waiting for team members to take a look labels Feb 28, 2022
@bradzacher
Copy link
Member

The lint rule mostly just relies upon what every is reported by the scope analysis.
#2751 added support for it to the scope analyser.

However, right now we only derive that config from the tsconfig - meaning you need type information to turn on this code path.

typescript-eslint/packages/parser/src/parser.ts

Lines 167 to 170 in 9d47a8b

if (compilerOptions.emitDecoratorMetadata === true) {
analyzeOptions.emitDecoratorMetadata =
compilerOptions.emitDecoratorMetadata;
}

We could definitely add a parserOption for this so that you can override turn it on without type information.

@bradzacher bradzacher added enhancement New feature or request package: scope-manager Issues related to @typescript-eslint/scope-manager accepting prs Go ahead, send a pull request that resolves this issue package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin and removed bug Something isn't working breaking change This change will require a new major version to be released package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin labels Feb 28, 2022
@TomMettam
Copy link
Author

I don't quite understand the above, but I am using emitDecoratorMetadata in my tsconfig

@bradzacher
Copy link
Member

I don't quite understand the above, but I am using emitDecoratorMetadata in my tsconfig

if you are using type-awawre linting then it will work out of the box.

@TomMettam
Copy link
Author

The above issue was logged with type-aware linting enabled

@bradzacher
Copy link
Member

Have you got a more complete example of the types you've imported?
It's hard to provide much guidance with just one file.

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed enhancement New feature or request accepting prs Go ahead, send a pull request that resolves this issue labels Mar 7, 2022
@bradzacher bradzacher mentioned this issue Mar 7, 2022
Closed
@xboy2012 xboy2012 mentioned this issue Mar 8, 2022
Merged
3 tasks
@bradzacher
Copy link
Member

I believe this is a duplicate of #4268 (comment) slash #3108 (comment)

The TL;DR is that TS uses type information to resolve that Ref is an object type, and emits __metadata("design:type", Object).
We can't resolve this without using type information - so we cannot do this.

In general the decorator support is not amazing. We've done the best we can with the information we've got - but it's not perfect by any means.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
awaiting response Issues waiting for a reply from the OP or another party package: scope-manager Issues related to @typescript-eslint/scope-manager
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(parser): add parserOptions.emitDecoratorMetadata xboy2012/typescript-eslint
3 participants
@TomMettam @JoshuaKGoldberg @bradzacher