Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

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

Already on GitHub? # to your account

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
{ "selector": "enumMember", "format": ["camelCase", "PascalCase"], "leadingUnderscore": "allow", "filter": { "regex": "^[A-Za-z]+_[A-Za-z]+$", "match": false } },
{ "selector": "property", "format": null }
],
"@typescript-eslint/no-unused-vars": ["error", { "argsIgnorePattern": "^_", "varsIgnorePattern": "^_" }],

// Rules enabled in typescript-eslint configs that are not applicable here
"@typescript-eslint/ban-ts-comment": "off",
Expand All @@ -91,6 +92,7 @@
"@typescript-eslint/no-namespace": "off",
"@typescript-eslint/no-non-null-asserted-optional-chain": "off",
"@typescript-eslint/no-var-requires": "off",

"@typescript-eslint/no-empty-interface": "off",
"@typescript-eslint/no-explicit-any": "off",
"@typescript-eslint/ban-types": [
Expand All @@ -106,9 +108,6 @@
}
],

// Todo: For each of these, investigate whether we want to enable them ✨
"@typescript-eslint/no-unused-vars": "off",

// Pending https://github.com/typescript-eslint/typescript-eslint/issues/4820
"@typescript-eslint/prefer-optional-chain": "off",

Expand All @@ -125,6 +124,8 @@
"local/debug-assert": "error",
"local/no-keywords": "error",
"local/jsdoc-format": "error",
// Solely for marking things as used for the purposes of the no-unused-vars rule.
"local/mark-jsdoc-types-used": "error",

// eslint-plugin-no-null
"no-null/no-null": "error",
Expand Down
102 changes: 102 additions & 0 deletions scripts/eslint/rules/mark-jsdoc-types-used.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
const { TSESTree, ESLintUtils } = require("@typescript-eslint/utils");
const { createRule } = require("./utils.cjs");
const ts = require("typescript");

/**
* @param {ts.Identifier} node
*/
function isBareIdentifier(node) {
if (ts.isPropertyAccessExpression(node.parent)) {
return node.parent.expression === node;
}

if (ts.isQualifiedName(node.parent)) {
return node.parent.left === node;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

¿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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return true;
}

module.exports = createRule({
name: "mark-jsdoc-types-used",
meta: {
docs: {
description: ``,
},
messages: {},
schema: [],
type: "problem",
fixable: "whitespace",
},
defaultOptions: [],

create(context) {
/** @type {(node: TSESTree.Node) => void} */
const checkProgram = node => {
const parserServices = ESLintUtils.getParserServices(context, /*allowWithoutFullTypeInformation*/ true);
const ast = parserServices.esTreeNodeToTSNodeMap.get(node);

// TODO(jakebailey): I almost guarantee this is overzealous and wrong in some way.

/**
* @param {ts.Node} node
*/
function markIdentifiersUsed(node) {
if (ts.isIdentifier(node) && isBareIdentifier(node)) {
context.markVariableAsUsed(node.text);
return;
}
ts.forEachChild(node, markIdentifiersUsed);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
* @param {ts.Node} node
*/
function visit(node) {
const jsDoc = ts.getJSDocTags(node);
for (const tag of jsDoc) {
if (ts.isJSDocTypeTag(tag)) {
markIdentifiersUsed(tag.typeExpression);
}
else if (ts.isJSDocTypedefTag(tag) || ts.isJSDocPropertyLikeTag(tag) || ts.isJSDocReturnTag(tag) || ts.isJSDocThrowsTag(tag)) {
if (tag.typeExpression) {
markIdentifiersUsed(tag.typeExpression);
}
}
else if (ts.isJSDocTemplateTag(tag)) {
if (tag.constraint) {
markIdentifiersUsed(tag.constraint);
}
// tag.typeParameters?
}
else if (ts.isJSDocEnumTag(tag)) {
markIdentifiersUsed(tag.typeExpression);
}
else if (ts.isJSDocUnknownTag(tag)) {
// Ignore.
// TODO(jakebailey): Something's wrong here, though, becuase we're getting @parameter tags as unknown. See new code in parser.
}
else if (ts.isJSDocOverrideTag(tag) || ts.isJSDocDeprecatedTag(tag)) {
// Ignore.
}
else if (ts.isJSDocSeeTag(tag)) {
markIdentifiersUsed(tag.tagName);
}
else {
throw new Error(`Unexpected node kind: ${ts.SyntaxKind[tag.kind]} ${tag.getText()}}`);
}
}
ts.forEachChild(node, visit);
}

visit(ast);
};

return {
Program: checkProgram,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

};
},
});
6 changes: 3 additions & 3 deletions src/compiler/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -286,15 +286,15 @@ export function filter<T>(array: T[], f: (x: T) => boolean): T[];
/** @internal */
export function filter<T, U extends T>(array: readonly T[], f: (x: T) => x is U): readonly U[];
/** @internal */
export function filter<T, U extends T>(array: readonly T[], f: (x: T) => boolean): readonly T[];
export function filter<T>(array: readonly T[], f: (x: T) => boolean): readonly T[];
/** @internal */
export function filter<T, U extends T>(array: T[] | undefined, f: (x: T) => x is U): U[] | undefined;
/** @internal */
export function filter<T>(array: T[] | undefined, f: (x: T) => boolean): T[] | undefined;
/** @internal */
export function filter<T, U extends T>(array: readonly T[] | undefined, f: (x: T) => x is U): readonly U[] | undefined;
/** @internal */
export function filter<T, U extends T>(array: readonly T[] | undefined, f: (x: T) => boolean): readonly T[] | undefined;
export function filter<T>(array: readonly T[] | undefined, f: (x: T) => boolean): readonly T[] | undefined;
/** @internal */
export function filter<T>(array: readonly T[] | undefined, f: (x: T) => boolean): readonly T[] | undefined {
if (array) {
Expand Down Expand Up @@ -830,7 +830,7 @@ export function insertSorted<T>(array: SortedArray<T>, insert: T, compare: Compa
}

/** @internal */
export function sortAndDeduplicate<T>(array: readonly string[]): SortedReadonlyArray<string>;
export function sortAndDeduplicate(array: readonly string[]): SortedReadonlyArray<string>;
/** @internal */
export function sortAndDeduplicate<T>(array: readonly T[], comparer: Comparer<T>, equalityComparer?: EqualityComparer<T>): SortedReadonlyArray<T>;
/** @internal */
Expand Down
1 change: 1 addition & 0 deletions src/compiler/emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4131,6 +4131,7 @@ export function createPrinter(printerOptions: PrinterOptions = {}, handlers: Pri
writeSpace();
nextPos = emitTokenWithComment(SyntaxKind.AsKeyword, nextPos, writeKeyword, node);
writeSpace();
// eslint-disable-next-line @typescript-eslint/no-unused-vars
nextPos = emitTokenWithComment(SyntaxKind.NamespaceKeyword, nextPos, writeKeyword, node);
writeSpace();
emit(node.name);
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/moduleNameResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ interface PackageJson extends PackageJsonPathFields {
version?: string;
}

function readPackageJsonField<TMatch, K extends MatchingKeys<PackageJson, string | undefined>>(jsonContent: PackageJson, fieldName: K, typeOfTag: "string", state: ModuleResolutionState): PackageJson[K] | undefined;
function readPackageJsonField<K extends MatchingKeys<PackageJson, string | undefined>>(jsonContent: PackageJson, fieldName: K, typeOfTag: "string", state: ModuleResolutionState): PackageJson[K] | undefined;
function readPackageJsonField<K extends MatchingKeys<PackageJson, object | undefined>>(jsonContent: PackageJson, fieldName: K, typeOfTag: "object", state: ModuleResolutionState): PackageJson[K] | undefined;
function readPackageJsonField<K extends keyof PackageJson>(jsonContent: PackageJson, fieldName: K, typeOfTag: "string" | "object", state: ModuleResolutionState): PackageJson[K] | undefined {
if (!hasProperty(jsonContent, fieldName)) {
Expand Down
1 change: 1 addition & 0 deletions src/compiler/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9043,6 +9043,7 @@ namespace Parser {
case "arg":
case "argument":
case "param":
case "parameter": // TODO(jakebailey): why is this missing?
return parseParameterOrPropertyTag(start, tagName, PropertyLikeParse.Parameter, margin);
case "return":
case "returns":
Expand Down
14 changes: 7 additions & 7 deletions src/compiler/watchPublic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -258,18 +258,18 @@ export interface ProgramHost<T extends BuilderProgram> {
getModuleResolutionCache?(): ModuleResolutionCache | undefined;

jsDocParsingMode?: JSDocParsingMode;
}
/**
* Internal interface used to wire emit through same host
*
* @internal
*/
export interface ProgramHost<T extends BuilderProgram> {

// Internal interface used to wire emit through same host

// TODO: GH#18217 Optional methods are frequently asserted
/** @internal */
createDirectory?(path: string): void;
/** @internal */
writeFile?(path: string, data: string, writeByteOrderMark?: boolean): void;
// For testing
/** @internal */
storeFilesChangingSignatureDuringEmit?: boolean;
/** @internal */
now?(): Date;
}

Expand Down
1 change: 1 addition & 0 deletions src/server/editorServices.ts
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Traces back to 03bb5d170ec in #26716.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

return userPreferences;
}
Expand Down
2 changes: 0 additions & 2 deletions src/tsconfig-base.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@
"useUnknownInCatchVariables": false,
"noImplicitOverride": true,

"noUnusedLocals": true,
"noUnusedParameters": true,
"allowUnusedLabels": false,

"skipLibCheck": true,
Expand Down