Enable eslint rule @typescript-eslint/no-unused-vars, disable compiler rule #55139

jakebailey · 2023-07-24T20:03:15Z

Follow-up to #54693.

As much as I want to do this (because I really want to be able to not have errors when I don't use a variable), typescript-eslint/typescript-eslint#5017 + gajus/eslint-plugin-jsdoc#858 means that we have to bring back eslint-plugin-jsdoc to have something mark JSDoc as using variables.

Adding eslint-plugin-jsdoc back adds a whopping 14 seconds of lint time (33% of the entire lint run); this was the original reason I removed this plugin in favor of a small custom JSDoc parser instead. Hopefully there are some performance improvements that can be done over there to help everyone.

We could add workarounds, disable the rule in places, etc, but that's unfortunate.

…r rule

sheetalkamat · 2023-07-26T17:04:37Z

src/server/editorServices.ts

@@ -477,6 +477,7 @@ export function convertScriptKindName(scriptKindName: protocol.ScriptKindName) {

 /** @internal */
 export function convertUserPreferences(preferences: protocol.UserPreferences): UserPreferences {
+    // eslint-disable-next-line @typescript-eslint/no-unused-vars
    const { lazyConfiguredProjectsFromExternalProject, ...userPreferences } = preferences;


Do we really need to filter out lazyConfiguredProjectsFromExternalProject option. Can we instead pass it as is ?

I have no idea; I should probably go git blame this back to where it was added to see if there was a reason this was written this way.

Traces back to 03bb5d170ec in #26716.

i think it should be ok to pass it as is and avoid having to destructure object all together when its not needed.

Yeah, I'll give it a shot, though there are other fundamental problems with this PR that probably mean we're not going to do this, e.g. the 14s extra lint time because ts-eslint doesn't support jsdoc itself, and the only available parser is very slow (for some reason... but TS already parses things, so I don't understand what's happening).

src/compiler/core.ts

jakebailey · 2023-08-11T22:14:24Z

In talking to @JoshuaKGoldberg and @sandersn this week, it sounded like there should be a way to do better here; TS already parses out these JSDoc trees, so the AST should be walkable... it's just not brought into the ESTree format used by lint rules.

It'd be really great to figure out how to make at least someone aware of the symbols present in the JSDoc AST we produce. That and, figure out why the jsdoc plugin is slow (or alternatively, if it could use the TS AST when present?).

bradzacher · 2023-08-12T01:54:09Z

We make the TS AST available on the parser services provided to the rule - so it's entirely possible for you guys to build a rule which jumps into the TS AST, accesses the JSDoc, extracts referenced names, and calls context.markVariableAsUsed.

This will stop the no-unused-vars rule from reporting on those names.

We generally avoid touching JSDoc because it's not a standard thing to have real code marked as used by a comment (evidenced by the comparatively low DL count of the JSDoc plugin and by the lack of requests for us to build it).

Also I thought you guys had changes the JSDoc parsing to be lazily done? If yes then accessing the JSDoc data would slow down your lint run anyways. IDK if it would be as slow as the JSDoc plugin's parser - but it would be a hit none the less.

jakebailey · 2023-08-12T02:19:45Z

Also I thought you guys had changes the JSDoc parsing to be lazily done? If yes then accessing the JSDoc data would slow down your lint run anyways. IDK if it would be as slow as the JSDoc plugin's parser - but it would be a hit none the less.

We haven't made it lazier, no, just more efficient and WIP change to not do it for most comments in tsc... But the ones we still have to parse are those with the tags like see/link which mark as used (as tsc tracks that), which is the main thing I'm talking about with JSDoc here.

bradzacher · 2023-08-13T02:45:08Z

#52959
I knew I'd seen discussion about it which must have been what I was thinking about!

jakebailey · 2023-08-13T05:31:10Z

Yeah; the info I'm talking about re: this lint rule is in fact the info we must parse no matter the circumstance, for the same reasons as the lint rule.

bradzacher · 2023-08-13T08:33:50Z

Does it always need to be parsed no matter the circumstances? JSDoc isn't used for typechecking TS files so surely it needn't be parsed by default in TS files?
It powers intelisense yeah but surely it can be lazily parsed for those usecases? 🤷‍♂️ I'm no TS internals expert though.

The JSDoc plugin has 1m DLs (compared to our 22m) and we (@typescript-eslint) have only ever had like 3 or 4 issues over the years suggesting support across any rule (and I think just one for no-unused-vars?).

Its just not something most people want or need I think - people either use imports within the JSDoc or they don't annotate things like that.

I think most people would not want the behaviour as described where a comment marks real runtime code as used. I understand the niche utility but it seems counter-intuitive for most usecases - so I don't think it really belongs in our no-unused-vars rule.

However there clearly is some desire for this functionality - so it does belong somewhere. The JSDoc plugin provides it - albeit slowly with its own parser. I know nothing of their codebase but I wonder if their rule(s) could be improved by using the TS AST from the parser services if they exist to avoid the separate parser?

That would be a "best of both worlds" IMO - centralise the JSDoc logic to the JSDoc plugin and improve its perf.

Failing that yeah - definitely a local rule to call context.markVariableAsUsed(name: string) would work fine for your internal usecase without the perf hit.

jakebailey · 2023-08-13T14:55:13Z

Does it always need to be parsed no matter the circumstances? JSDoc isn't used for typechecking TS files so surely it needn't be parsed by default in TS files?

It does, because tsc's no-unused-var equivalent also includes @see and @link references. Hence #52921 which is able to make tsc faster by skipping JSDoc parsing, except when it sees those two tags might be present.

What I'm trying to say is that we're talking about exactly the same rule, except implemented in tsc versus eslint; for them to have parity, the latter has to walk the JSDoc. But, the latter also gets this info "for free" because TypeScript must parse comments containing @see and @link because they have semantic meaning during checking.

jakebailey · 2023-08-13T15:00:02Z

I'll try and make a little rule that can fix this, but it does feel a little silly to need to do so. After all, these nodes are in the AST already, it's just that unlike every other node, they aren't automatically walked.

bradzacher · 2023-08-13T23:51:11Z

because tsc's no-unused-var equivalent also includes @see and @link references

But the unused local diagnostic is lazily computed - so it shouldn't be needed to be parsed up-front!

I definitely understand what you're saying - the point of difference is that you believe that it should be part of @typescript-eslint/no-unused-vars because it is part of noUnusedLocals and is available on the TS AST, whilst I don't believe that parity with TS is correct here because the base eslint rule doesn't care for JSDoc and I don't believe enough users want this.

bradzacher · 2023-08-14T22:03:58Z

scripts/eslint/rules/mark-jsdoc-types-used.cjs

+    }
+
+    // TODO(jakebailey): surely I'm missing something here.
+    // Also, how does eslint deal with type space versus value space? Or does it not?


It does definitely - which is why we forked eslint-scope! Was a big piece of work.

ESLint core has no concept of it - it just knows values. Our forked scope manager tracks it and exposes the state via the .isTypeVariable and .isValueVariable properties on the Variable.

Using context.markVariableAsUsed eslint will just find the first variable matching the name and mark it as used (because it has no concept of type vs value). TBH this probably fine for this usecase - I doubt you guys have too much scope shadowing - but if you want you can pretty easily reimplement it with awareness (it's just a few lines).

https://github.com/eslint/eslint/blob/9e9edf93ecfa0658e8b79e71bc98530ade150081/lib/source-code/source-code.js#L690-L724

The weird thing about jsdoc is that it doesn't really care if it's a type or value for some cases, right?

Is your forked one usable somewhere?

I know that what I've done is definitely going to be wrong because I'm pretty sure one can write a function type that introduces a new name and now it should refer to that one, but eslint's not going to see it.

I don't think JSDoc is any different in terms of type/value, though.

I know that this PR is also wrong in other ways, e.g. it probably marks parameters as used. There's some call I should be making that checks if a node is in type space or not but I forgot it before pushing my WIP change.

Is your forked one usable somewhere?

You get it for free by using our parser :)

————

I know that what I've done is definitely going to be wrong because I'm pretty sure one can write a function type that introduces a new name and now it should refer to that one, but eslint's not going to see it.

¿que? Can you show me an example?

I don't think JSDoc is any different in terms of type/value, though.

Yeah I meant in terms of "does an identifier in a JSDoc comment refer to a type or a value". Most places are unambiguous and can only reference one!

¿que? Can you show me an example?

I mean that my method is naive, so given:

/** @typedef {number} T */ /** @type {{ <T>(x: T) => T }} */ const identity = x => x;

This is going to mark T as used because it's referenced in the param/return statement. So without someone managing the scopes, it'll mark that outer typedef as used. And given the API I'm using (where I'm just giving it names and no position info), I don't see how I can do it properly.

It won't actually do that because JSDoc typedefs don't create variables in the scope analyser.

Our scope analyser only understands concrete TS syntax.
We don't support this because there's no sane reason to use a JSDoc typedef within a TS file.

If you want JSDoc typedefs marked as unused then yeah you'll need the JSDoc plugin (it does this I think?). You can scope the plugin to just JS files for that usecase to reduce the impact of it (unless there's some reason you guys use typedef within TS files..?)

Yeah. I realized earlier that I should really be skipping most of these tags in TS files.

I'm mainly trying to avoid the JSDoc plugin because it's so slow, so I'll have to see if the scope manager is in any way available for use, or something else.

Yeah so the problem is that comment nodes aren't attached to the AST - so you can't acquire the scope for it (context.getSourceCode().getScope(node)).

If you can figure out the node that contains the comment then you can acquire the scope and go from there based on the markVariableAsUsed code I linked above.
This isn't super easy to do though, sadly, because it involves traversing the AST to try and find the tightest containing node.

So I guess it really depends on how many violations there are in the codebase and what the violations look like exactly.

If you guys have test fixtures for your intellisense services or noUnusedLocals option then I'd just turn off the no-unused-vars rule on those files. (TBH generally you would turn off linting on test fixtures anyways).

But you can see what I was saying about why we don't support this in our rule - it's a super complicated problem to solve on top of it being a niche ask!

As soon as josh mentioned it to me I was like "oh god no" heh.

bradzacher · 2023-08-14T22:06:16Z

scripts/eslint/rules/mark-jsdoc-types-used.cjs

+                    context.markVariableAsUsed(node.text);
+                    return;
+                }
+                ts.forEachChild(node, markIdentifiersUsed);


Idk exactly how this traverses - but wouldn't this mark both x and y as used in x.y?

It shouldn't; markIdentifiersUsed checks for that case and only marks things on the left side.

bradzacher · 2023-08-14T23:44:36Z

scripts/eslint/rules/mark-jsdoc-types-used.cjs

+        };
+
+        return {
+            Program: checkProgram,


One problem I can see here is that you're accessing the comments from the program root.

Scopes are ofc scoped - so context.markVariableAsUsed is scoped too. So by doing all the work from the Program node you're accessing the top-level scope (eg the module scope) and thus will only ever mark top-level variables as used.

If you want to be able to handle nested scopes then we need to find the containing scope for the comment - which will require more effort.

Yeah, this is what I was noting above; I do have another JSDoc rule which instead walks specifically only declarations and then asks for their JSDoc, but I wasn't sure if it was possible to have JSDoc hanging around somewhere else. e.g. I know in that other rule I'm definitely missing JSDoc on EndOfFileToken but for that rule it doesn't matter because it's about d.ts files and the bundler won't deal with it.

But, with this API, I'm not super sure how this scoping is supposed to work; a JSDoc node can go arbitrarily deep and introduce new scopes, which eslint is not going to know about because it's not seeing the full tree.

Of course, this begs the question; how is the jsdoc plugin doing this? Or is it bugged too? Is this hopeless?

It's a bit of a black box I think based on however their parser package works and traverses jsdoc comments

https://github.com/gajus/eslint-plugin-jsdoc/blob/eed807edd9a14750ae4e16279cafeb27064ecd59/src/rules/noUndefinedTypes.js#L288

But essentially if (type === 'JsdocTypeName') { mark_as_used(value) }.
So it probably doesn't handle all these edge-cases you're thinking of?

jakebailey · 2024-01-26T19:17:35Z

Closing in favor of #57123, in which we're just going to use ignores when needed.

Enable eslint rule @typescript-eslint/no-unused-vars, disable compile…

3c71898

…r rule

typescript-bot assigned jakebailey Jul 24, 2023

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jul 24, 2023

sheetalkamat reviewed Jul 26, 2023

View reviewed changes

rubiesonthesky reviewed Jul 26, 2023

View reviewed changes

src/compiler/core.ts Show resolved Hide resolved

jakebailey added 2 commits July 27, 2023 16:45

Merge branch 'main' into eslint-no-unused-var

5b94d73

Merge branch 'main' into eslint-no-unused-var

32c1b32

Attempt to DIY the jsdoc stuff using a local plugin

00f2237

bradzacher reviewed Aug 14, 2023

View reviewed changes

jakebailey mentioned this pull request Sep 13, 2023

Skip parsing JSDoc when not needed #52921

Merged

bradzacher mentioned this pull request Sep 13, 2023

Make JSDoc skipping public, plus more configurable #55739

Merged

This was referenced Jan 22, 2024

enable @typescript-eslint/no-unused-vars #57123

Merged

enable @typescript-eslint/no-unused-vars #57124

Closed

jakebailey added 2 commits January 22, 2024 12:03

Merge branch 'main' into eslint-no-unused-var

e21539d

fmt

bf6a46b

jakebailey closed this Jan 26, 2024

jakebailey deleted the eslint-no-unused-var branch March 15, 2024 22:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable eslint rule @typescript-eslint/no-unused-vars, disable compiler rule #55139

Enable eslint rule @typescript-eslint/no-unused-vars, disable compiler rule #55139

jakebailey commented Jul 24, 2023 •

edited

Loading

sheetalkamat Jul 26, 2023

jakebailey Jul 26, 2023

jakebailey Jul 26, 2023

sheetalkamat Jul 26, 2023

jakebailey Jul 26, 2023

jakebailey commented Aug 11, 2023

bradzacher commented Aug 12, 2023

jakebailey commented Aug 12, 2023

bradzacher commented Aug 13, 2023

jakebailey commented Aug 13, 2023

bradzacher commented Aug 13, 2023

jakebailey commented Aug 13, 2023

jakebailey commented Aug 13, 2023

bradzacher commented Aug 13, 2023

bradzacher Aug 14, 2023

jakebailey Aug 14, 2023

bradzacher Aug 14, 2023

jakebailey Aug 14, 2023

bradzacher Aug 15, 2023

jakebailey Aug 15, 2023

bradzacher Aug 15, 2023

bradzacher Aug 15, 2023

bradzacher Aug 14, 2023

jakebailey Aug 14, 2023

bradzacher Aug 14, 2023

jakebailey Aug 14, 2023

jakebailey Aug 14, 2023

bradzacher Aug 15, 2023

jakebailey commented Jan 26, 2024

Enable eslint rule @typescript-eslint/no-unused-vars, disable compiler rule #55139

Enable eslint rule @typescript-eslint/no-unused-vars, disable compiler rule #55139

Conversation

jakebailey commented Jul 24, 2023 • edited Loading

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

jakebailey commented Aug 11, 2023

bradzacher commented Aug 12, 2023

jakebailey commented Aug 12, 2023

bradzacher commented Aug 13, 2023

jakebailey commented Aug 13, 2023

bradzacher commented Aug 13, 2023

jakebailey commented Aug 13, 2023

jakebailey commented Aug 13, 2023

bradzacher commented Aug 13, 2023

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

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

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

jakebailey commented Jan 26, 2024

jakebailey commented Jul 24, 2023 •

edited

Loading