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

Upgrade "boolean-trivia" lint to new "argument-trivia" lint that uses type info, has quick fixes, etc. #53002

Merged
merged 29 commits into from
Mar 23, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
b4429fa
Remove some special cases
jakebailey Feb 27, 2023
535145f
Also check NewExpressions
jakebailey Feb 27, 2023
09c2548
Use type info
jakebailey Feb 27, 2023
b8629b8
Rename, quickfix
jakebailey Feb 27, 2023
db07b13
Merge branch 'main' into fixup-trivia-lint
jakebailey Feb 27, 2023
f1e06ed
Fix rename
jakebailey Feb 27, 2023
5dfa6a2
Allow JSDoc for now
jakebailey Feb 27, 2023
cda5c40
Move again
jakebailey Feb 27, 2023
3ae67d9
Merge branch 'main' into fixup-trivia-lint
jakebailey Feb 27, 2023
b39be75
Merge branch 'main' into fixup-trivia-lint
jakebailey Feb 27, 2023
90badae
Remove TODO
jakebailey Feb 27, 2023
1df2cea
Update tests
jakebailey Feb 27, 2023
981991e
Fix all lints
jakebailey Feb 27, 2023
9c6f33e
Remove unused
jakebailey Feb 27, 2023
e2492ca
Try and preserve whitespace
jakebailey Feb 27, 2023
6b7bdfb
Revert "Fix all lints"
jakebailey Feb 27, 2023
f3596ba
Just replace name
jakebailey Feb 27, 2023
88334c0
Re-fix
jakebailey Feb 27, 2023
0875de4
Do replace later
jakebailey Feb 27, 2023
4690507
Remove underscore at start
jakebailey Feb 27, 2023
541e808
PR feedback
jakebailey Feb 28, 2023
81e4208
Cleanup
jakebailey Feb 28, 2023
1f67de8
Merge branch 'main' into fixup-trivia-lint
jakebailey Mar 7, 2023
2ad916f
Merge branch 'main' into fixup-trivia-lint
jakebailey Mar 21, 2023
1fa5786
Merge branch 'main' into fixup-trivia-lint
jakebailey Mar 22, 2023
4d21516
Per design meeting, disallow spaces
jakebailey Mar 22, 2023
c3496df
Simplify
jakebailey Mar 22, 2023
9930669
Merge branch 'main' into fixup-trivia-lint
jakebailey Mar 22, 2023
0c9e58e
Merge branch 'main' into fixup-trivia-lint
jakebailey Mar 23, 2023
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
2 changes: 1 addition & 1 deletion .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@
"allowDeclarations": true
}],
"local/no-double-space": "error",
"local/boolean-trivia": "error",
"local/argument-trivia": "error",
"local/no-in-operator": "error",
"local/simple-indent": "error",
"local/debug-assert": "error",
Expand Down
187 changes: 187 additions & 0 deletions scripts/eslint/rules/argument-trivia.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
const { AST_NODE_TYPES, TSESTree, ESLintUtils } = require("@typescript-eslint/utils");
const { createRule } = require("./utils.cjs");
const ts = require("typescript");

const unset = Symbol();
/**
* @template T
* @param {() => T} fn
* @returns {() => T}
*/
function memoize(fn) {
/** @type {T | unset} */
let value = unset;
return () => {
if (value === unset) {
value = fn();
}
return value;
};
}


module.exports = createRule({
name: "argument-trivia",
meta: {
docs: {
description: ``,
recommended: "error",
},
messages: {
argumentTriviaArgumentError: `Tag argument with parameter name`,
argumentTriviaArgumentSpaceError: `There should be 1 space between an argument and its comment`,
argumentTriviaArgumentNameError: `Argument name "{{ got }}" does not match expected name "{{ want }}"`,
},
schema: [],
type: "problem",
fixable: "code",
},
defaultOptions: [],

create(context) {
const sourceCode = context.getSourceCode();
const sourceCodeText = sourceCode.getText();

/** @type {(name: string) => boolean} */
const isSetOrAssert = (name) => name.startsWith("set") || name.startsWith("assert");
/** @type {(node: TSESTree.Node) => boolean} */
const isTrivia = (node) => {
if (node.type === AST_NODE_TYPES.Identifier) {
return node.name === "undefined";
}

if (node.type === AST_NODE_TYPES.Literal) {
// eslint-disable-next-line no-null/no-null
return node.value === null || node.value === true || node.value === false;
}

return false;
};

/** @type {(node: TSESTree.CallExpression | TSESTree.NewExpression) => boolean} */
const shouldIgnoreCalledExpression = (node) => {
if (node.callee && node.callee.type === AST_NODE_TYPES.MemberExpression) {
const methodName = node.callee.property.type === AST_NODE_TYPES.Identifier
? node.callee.property.name
: "";

if (isSetOrAssert(methodName)) {
return true;
}

return ["apply", "call", "equal", "fail", "isTrue", "output", "stringify", "push"].indexOf(methodName) >= 0;
jakebailey marked this conversation as resolved.
Show resolved Hide resolved
}

if (node.callee && node.callee.type === AST_NODE_TYPES.Identifier) {
const functionName = node.callee.name;

if (isSetOrAssert(functionName)) {
return true;
}

return ["contains"].indexOf(functionName) >= 0;
}

return false;
};


/** @type {(node: TSESTree.Node, i: number, getSignature: () => ts.Signature | undefined) => void} */
const checkArg = (node, i, getSignature) => {
if (!isTrivia(node)) {
return;
}

const getExpectedName = memoize(() => {
const signature = getSignature();
if (signature) {
const expectedName = signature.parameters[i]?.escapedName;
if (expectedName) {
return ts.unescapeLeadingUnderscores(expectedName);
}
}
return undefined;
});

const comments = sourceCode.getCommentsBefore(node);
/** @type {TSESTree.Comment | undefined} */
const comment = comments[comments.length - 1];

if (!comment || comment.type !== "Block") {
const expectedName = getExpectedName();
if (expectedName) {
context.report({
messageId: "argumentTriviaArgumentError",
node,
fix: (fixer) => {
return fixer.insertTextBefore(node, `/*${expectedName}*/ `);
}
});
}
else {
context.report({ messageId: "argumentTriviaArgumentError", node });
}
return;
}

const argRangeStart = node.range[0];
const commentRangeEnd = comment.range[1];
const expectedName = getExpectedName();
if (expectedName) {
const got = comment.value.trim();
if (got !== expectedName) {
context.report({
messageId: "argumentTriviaArgumentNameError",
data: { got, want: expectedName },
node: comment,
fix: (fixer) => {
// Try and preserve whitespace.
const newComment = comment.value.replace(got, expectedName);
return fixer.replaceText(comment, `/*${newComment}*/`);
},
});
return;
}
}

const hasNewLine = sourceCodeText.slice(commentRangeEnd, argRangeStart).indexOf("\n") >= 0;
if (argRangeStart !== commentRangeEnd + 1 && !hasNewLine) {
// TODO(jakebailey): range should be whitespace
Copy link
Member

Choose a reason for hiding this comment

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

? What's this mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, this is the lint for when you have /*param*/ true, with extra whitespace, and this lint rule apparently determines that style and deletes the extra spaces.

I wanted the error range to just be on the spaces but had trouble calculating it.

context.report({
messageId: "argumentTriviaArgumentSpaceError",
node,
fix: (fixer) => {
return fixer.replaceTextRange([commentRangeEnd, argRangeStart], " ");
}
});
}
};

/** @type {(node: TSESTree.CallExpression | TSESTree.NewExpression) => void} */
const checkargumentTrivia = (node) => {
jakebailey marked this conversation as resolved.
Show resolved Hide resolved
if (shouldIgnoreCalledExpression(node)) {
return;
}

const getSignature = memoize(() => {
if (context.parserServices?.hasFullTypeInformation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, do you not always lint with full type information? Is there context around here? Just curious 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Only src is set up for that. Reasonably, we could also add scripts to this list, but any root-level file like Herebyfile.mjs is not going to be a part of any project.

Now, if we get ts.ProjectService into ts-eslint... then everything could have type info, as TS would automagically figure out what project each file needs to be in, and allocate it to the implicit project where needed, without tsconfig being configured in eslintrc at all.

Copy link
Member Author

@jakebailey jakebailey Mar 21, 2023

Choose a reason for hiding this comment

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

Also, I've actually been trying to drop type info, because it's just really expensive, but, we get too much value out of the singular type-using lint we have enabled, the one about unused type assertions.

That's a little funny from the POV that this new lint needs type info to be useful. So, oops.

const parserServices = ESLintUtils.getParserServices(context);
const checker = parserServices.program.getTypeChecker();
const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node);
return checker.getResolvedSignature(tsNode);
Comment on lines +185 to +187
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh we're adding in a little set of utilities for calling these checker APIs directly from services. getResolvedSignature doesn't exist right now, but if it did, we could simplify this to:

Suggested change
const checker = parserServices.program.getTypeChecker();
const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node);
return checker.getResolvedSignature(tsNode);
return parserServices.getResolvedSignature(node);

How does that make you feel? And if it's not negative, are there any other APIs you think would be good to make available?

Right now we just have getSymbolAtLocation and getTypeAtLocation wrapped.

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 personally don't mind the couple of lines to make this work, but, I'm also not writing rules all day.

}
return undefined;
});

for (let i = 0; i < node.arguments.length; i++) {
const arg = node.arguments[i];
checkArg(arg, i, getSignature);
}
};

return {
CallExpression: checkargumentTrivia,
NewExpression: checkargumentTrivia,
};
},
});
110 changes: 0 additions & 110 deletions scripts/eslint/rules/boolean-trivia.cjs

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const { RuleTester } = require("./support/RuleTester.cjs");
const rule = require("../rules/boolean-trivia.cjs");
const rule = require("../rules/argument-trivia.cjs");

const ruleTester = new RuleTester({
parserOptions: {
Expand All @@ -8,7 +8,7 @@ const ruleTester = new RuleTester({
parser: require.resolve("@typescript-eslint/parser"),
});

ruleTester.run("boolean-trivia", rule, {
ruleTester.run("argument-trivia", rule, {
valid: [
{
code: `
Expand Down Expand Up @@ -48,6 +48,12 @@ const fn = (prop: boolean) => {};
fn.apply(null, true);
`,
},
{
code: `
const fn = (prop: boolean) => {};
fn(/* first comment */ /* second comment */ false);
`,
},
],

invalid: [
Expand All @@ -56,28 +62,25 @@ fn.apply(null, true);
const fn = (prop: null) => {};
fn(null);
`,
errors: [{ messageId: "booleanTriviaArgumentError" }],
errors: [{ messageId: "argumentTriviaArgumentError" }],
},
{
code: `
const fn = (prop: boolean) => {};
fn(false);
`,
errors: [{ messageId: "booleanTriviaArgumentError" }],
errors: [{ messageId: "argumentTriviaArgumentError" }],
},
{
code: `
const fn = (prop: boolean) => {};
fn(/* boolean arg */false);
`,
errors: [{ messageId: "booleanTriviaArgumentSpaceError" }],
},
{
code: `
errors: [{ messageId: "argumentTriviaArgumentSpaceError" }],
output:`
const fn = (prop: boolean) => {};
fn(/* first comment */ /* second comment */ false);
`,
errors: [{ messageId: "booleanTriviaArgumentError" }],
Comment on lines -78 to -80
Copy link
Member Author

Choose a reason for hiding this comment

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

This case is now "okay" because the lint takes the last comment as the hint, which I think is correct and is needed to handle cases where we put other comments before the parameter.

fn(/* boolean arg */ false);
`
},
],
});
4 changes: 2 additions & 2 deletions src/compiler/binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3306,7 +3306,7 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void {
}
else if (hasDynamicName(node)) {
bindAnonymousDeclaration(node, SymbolFlags.Property | SymbolFlags.Assignment, InternalSymbolName.Computed);
const sym = bindPotentiallyMissingNamespaces(parentSymbol, node.left.expression, isTopLevelNamespaceAssignment(node.left), /*isPrototype*/ false, /*containerIsClass*/ false);
const sym = bindPotentiallyMissingNamespaces(parentSymbol, node.left.expression, isTopLevelNamespaceAssignment(node.left), /*isPrototypeProperty*/ false, /*containerIsClass*/ false);
addLateBoundAssignmentDeclarationToSymbol(node, sym);
}
else {
Expand Down Expand Up @@ -3477,7 +3477,7 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void {
function bindCallExpression(node: CallExpression) {
// We're only inspecting call expressions to detect CommonJS modules, so we can skip
// this check if we've already seen the module indicator
if (!file.commonJsModuleIndicator && isRequireCall(node, /*checkArgumentIsStringLiteralLike*/ false)) {
if (!file.commonJsModuleIndicator && isRequireCall(node, /*requireStringLiteralLikeArgument*/ false)) {
setCommonJsModuleIndicator(node);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1623,7 +1623,7 @@ export function createBuilderProgram(kind: BuilderProgramKind, { newProgram, hos
}
else {
// When whole program is affected, get all semantic diagnostics (eg when --out or --outFile is specified)
result = state.program.getSemanticDiagnostics(/*targetSourceFile*/ undefined, cancellationToken);
result = state.program.getSemanticDiagnostics(/*sourceFile*/ undefined, cancellationToken);
state.changedFilesSet.clear();
state.programEmitPending = getBuilderFileEmit(state.compilerOptions);
}
Expand Down
Loading