-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Changes from 28 commits
b4429fa
535145f
09c2548
b8629b8
db07b13
f1e06ed
5dfa6a2
cda5c40
3ae67d9
b39be75
90badae
1df2cea
981991e
9c6f33e
e2492ca
6b7bdfb
f3596ba
88334c0
0875de4
4690507
541e808
81e4208
1f67de8
2ad916f
1fa5786
4d21516
c3496df
9930669
0c9e58e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,203 @@ | ||||||||||||
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; | ||||||||||||
} | ||||||||||||
|
||||||||||||
switch (methodName) { | ||||||||||||
case "apply": | ||||||||||||
case "call": | ||||||||||||
case "equal": | ||||||||||||
case "stringify": | ||||||||||||
case "push": | ||||||||||||
return true; | ||||||||||||
} | ||||||||||||
|
||||||||||||
return false; | ||||||||||||
} | ||||||||||||
|
||||||||||||
if (node.callee && node.callee.type === AST_NODE_TYPES.Identifier) { | ||||||||||||
const functionName = node.callee.name; | ||||||||||||
|
||||||||||||
if (isSetOrAssert(functionName)) { | ||||||||||||
return true; | ||||||||||||
} | ||||||||||||
|
||||||||||||
switch (functionName) { | ||||||||||||
case "contains": | ||||||||||||
return true; | ||||||||||||
} | ||||||||||||
|
||||||||||||
return false; | ||||||||||||
} | ||||||||||||
|
||||||||||||
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) { | ||||||||||||
const name = ts.unescapeLeadingUnderscores(expectedName); | ||||||||||||
// If a parameter is unused, we prepend an underscore. Ignore this | ||||||||||||
// so that we can switch between used and unused without modifying code, | ||||||||||||
// requiring that arugments are tagged with the non-underscored name. | ||||||||||||
return name.startsWith("_") ? name.slice(1) : name; | ||||||||||||
} | ||||||||||||
} | ||||||||||||
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; | ||||||||||||
if (got !== expectedName) { | ||||||||||||
context.report({ | ||||||||||||
messageId: "argumentTriviaArgumentNameError", | ||||||||||||
data: { got, want: expectedName }, | ||||||||||||
node: comment, | ||||||||||||
fix: (fixer) => { | ||||||||||||
return fixer.replaceText(comment, `/*${expectedName}*/`); | ||||||||||||
}, | ||||||||||||
}); | ||||||||||||
return; | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
const hasNewLine = sourceCodeText.slice(commentRangeEnd, argRangeStart).indexOf("\n") >= 0; | ||||||||||||
if (argRangeStart !== commentRangeEnd + 1 && !hasNewLine) { | ||||||||||||
// TODO(jakebailey): range should be whitespace | ||||||||||||
context.report({ | ||||||||||||
messageId: "argumentTriviaArgumentSpaceError", | ||||||||||||
node, | ||||||||||||
fix: (fixer) => { | ||||||||||||
return fixer.replaceTextRange([commentRangeEnd, argRangeStart], " "); | ||||||||||||
} | ||||||||||||
}); | ||||||||||||
} | ||||||||||||
}; | ||||||||||||
|
||||||||||||
/** @type {(node: TSESTree.CallExpression | TSESTree.NewExpression) => void} */ | ||||||||||||
const checkArgumentTrivia = (node) => { | ||||||||||||
if (shouldIgnoreCalledExpression(node)) { | ||||||||||||
return; | ||||||||||||
} | ||||||||||||
|
||||||||||||
const getSignature = memoize(() => { | ||||||||||||
if (context.parserServices?.hasFullTypeInformation) { | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only Now, if we get There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Suggested change
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||||||||||||
}; | ||||||||||||
}, | ||||||||||||
}); |
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: { | ||
|
@@ -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: ` | ||
|
@@ -48,6 +48,12 @@ const fn = (prop: boolean) => {}; | |
fn.apply(null, true); | ||
`, | ||
}, | ||
{ | ||
code: ` | ||
const fn = (prop: boolean) => {}; | ||
fn(/* first comment */ /* second comment */ false); | ||
`, | ||
}, | ||
], | ||
|
||
invalid: [ | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
` | ||
}, | ||
], | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? What's this mean?
There was a problem hiding this comment.
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.