Skip to content
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.

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

[no-unused-vars] Generic type params are covered by varsIgnorePattern rather than argsIgnorePattern #2803

Closed
3 tasks done
jonaskello opened this issue Nov 24, 2020 · 9 comments
Labels
package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin working as intended Issues that are closed as they are working as intended

Comments

@jonaskello
Copy link

  • 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

{
  "rules": {
    "@typescript-eslint/no-unused-vars": ["error", { argsIgnorePattern: "^_" }],
  }
}
// your repro code case

export interface Bar<_A> {
  readonly foo: string;
}

Expected Result

I expected the _A parameter not being used to be ignored becuase it is prefixed with an underscore and therefore part of argsIgnorePattern.

Actual Result

error @typescript-eslint/no-unused-vars : '_A' is defined but never used.

Additional Info

Versions

package version
@typescript-eslint/eslint-plugin 4.8.2
@typescript-eslint/parser 4.8.2
TypeScript 4.0.2
ESLint 7.10.0
node 12.18.3
@jonaskello jonaskello added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Nov 24, 2020
@jonaskello
Copy link
Author

jonaskello commented Nov 24, 2020

By further testing I found that it seems generic type parameters are covered by varsIgnorePattern rather than argsIgnorePattern which seems a but unintuitive to me because if I use the tsc compiler rules then they are coverd by noUnusedParameters and you can ignore them by adding an underscore just like for other parameters.

@jonaskello jonaskello changed the title [no-unused-vars] The argsIgnorePattern option does not work for generic types template parameters/args [no-unused-vars] Generic type params are covered by varsIgnorePattern rather than argsIgnorePattern Nov 24, 2020
@jonaskello
Copy link
Author

jonaskello commented Nov 24, 2020

I want to emulate these tsconfig.json settings:

    "noUnusedLocals": true,
    "noUnusedParameters": true,

If you use the above settings there is no way allowed to ignore local vars but you can still ignore both regular parameters and generic type template parameters by prefixing them with underscore. I find no way to do this with how varsIgnorePattern and argsIgnorePattern works today becuase if I add a varsIgnorePattern to cover unused type params then I open up the possibility to ignore local vars too.

@bradzacher
Copy link
Member

TypeScript's unused vars checker simply ignores any name prefixed with an underscore.

You can do the same with both varsIgnorePattern and argsIgnorePattern set to ^_.


Type parameters matching the varsIgnorePattern is intentional. The only things not handled by this option are function parameters (yes, they misnamed the option).

Could there have been one option per type of thing? Sure. But each option increases configuration complexity and rule complexity.

In my experience across many codebases - the most common config is to set all the ignore patterns to the same thing (usually ^_) or not at all. Which is what we optimised for.

@bradzacher bradzacher added working as intended Issues that are closed as they are working as intended and removed triage Waiting for maintainers to take a look labels Nov 24, 2020
@jonaskello
Copy link
Author

jonaskello commented Nov 24, 2020

TypeScript's unused vars checker simply ignores any name prefixed with an underscore.

@bradzacher From my experience, tsc does not work the way you describe. Try this example:

tsconfig.json:

{ 
    "noUnusedLocals": true,
    "noUnusedParameters": true,
}

Example code:

export function youCanWorkAroundParamsWithUnderscoreInTsc<_AndTemplateParams>(_unusedParam: string): void {
  const _butYouCanNeverWorkAroundLocalsWIthUnderscore = 1;
}

Build result

error TS6133: '_butYouCannotWorkAroundLocalsWIthUnderscore' is declared but its value is never read.

9   const _butYouCanNeverWorkAroundLocalsWIthUnderscore = 1;

You can see how tsc only allow underscores on parameters (including type parameters) but never on locals.

This is what I want to replicate, I don't want to open a hole for developers to have ignored locals, only ignored parameters becuase that cannot be avoided sometimes. I guess this is why tsc is designed this way.

Are you saying you intentionally designed the rule to work differently than tsc? If so I don't understand the reasoning behind that.

@bradzacher
Copy link
Member

From my experience, tsc does not work the way you describe.

Apologies, I was misremembering how it works and playground doesn't work well on mobile.

Are you saying you intentionally designed the rule to work differently than tsc? If so I don't understand the reasoning behind that.

Simply put: because this isn't tsc, nor is the rule based on how tsc works.

This rule (and the entirety of ESLint) has existed for much longer than TS even.
This project merely augments the rule and the underlying infra to understand TypeScript.
We purposely do not seek to change how the underlying rule works.

@jonaskello
Copy link
Author

Yes, I agree that the options should work the same way as for the plain javascript rule, otherwise it would be confusing :-)

However, types and therefore parameters to generic types is not part of javascript. So treating them as parameters rather than local vars in the extended typescript rule should not affect the underlying javascript rule?

@bradzacher
Copy link
Member

The base rule's code simply says "if the variable is an argument, use argsIgnorePattern, else use varsIgnorePattern".

Up until yesterday (literally), the rule was purely an extension rule that built on top of the base rule's logic to teach it about TS.

Hence we could not make a distinction between types and variables - because we couldn't access the logic that made that branch.


Can we do it now? Yes - as off yesterday it's now technically possible.

Should we? I don't think so.
It's a breaking change for one.
That plus the clear lack of a need from the greater community - I don't think it's worth the effort / maintenance cost.

@jonaskello
Copy link
Author

Running my example code above at astexplorer with @typescript-eslint/parser I can see that the type parameters are of type TSTypeParameterDeclaration so I'm not sure what classifies them as variables. I took a quick look at the implementation of the rule to find why they were considered variables but nothing jumped out at me directly :-).

Regarding the effort I could do a PR for the change but I do understand about the maintenance cost. I maintain a few open source projects myself and time is always a constraint.

Regarding breaking change I understand that too. Perhaps type parameters could be covered by both ignore options to make it more backwards compatbile. This way it would solve our problem (stated below). It would of course break some more unusual cases. It could of course also be put on hold for a major release.

The problem we have is that we used the tsc options previously but that requires us to comment out a lot of code each time we want to try some temporary thing to make it build. So we want to switch to linting rules instead. However we do not in any way want to allow any unused local vars to sneak in. We do however need to allow unused params and type params since they are unavoidable sometimes. I think many teams in the community has the same needs but of course I have no empirical data to back that up so I am just speculating :-).

For what it is worth, I still think treating type parameters as parameters is the "correct" way to do it. I know that sounds opinionated but to justify it I will add it is the opinion of tsc and not my own :-).

@jonaskello
Copy link
Author

The work-around we use at the moment is to allow local ignored local vars but make them more explicit than just underscore:

    "@typescript-eslint/no-unused-vars": ["error", { argsIgnorePattern: "^_", varsIgnorePattern: "_[iI]gnored" }],

The reasoning is that you can easily name something underscore without thinking about it and then by accident leave it as unused. The downside is that all unused type params also need this explcit annotation but it is managable as we don't have that many of them. This solution is OK in my opinion however I would still consider it a work-around needed because lack of "correct" behavior from the rule :-).

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin working as intended Issues that are closed as they are working as intended
Projects
None yet
Development

No branches or pull requests

2 participants