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
Fix: add better TS generic support for arrow-parens (fixes #12570) #12587
Changes from all commits
7945e7a
d33c7f0
d2084cd
1bf3ff0
444dc14
eb30038
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 |
---|---|---|
|
@@ -74,35 +74,65 @@ module.exports = { | |
|
||
const sourceCode = context.getSourceCode(); | ||
|
||
/** | ||
* Checks if token is part of the given node. | ||
* @param {ASTNode} token A node to check. | ||
* @param {ASTNode} node The arrow function node. | ||
* @returns {boolean} Whether or not the token is part of node. | ||
*/ | ||
function isTokenPartOfNode(token, node) { | ||
return token.range[0] >= node.range[0] && token.range[1] <= node.range[1]; | ||
} | ||
|
||
/** | ||
* Determines whether a arrow function argument end with `)` | ||
* @param {ASTNode} node The arrow function node. | ||
* @returns {void} | ||
*/ | ||
function parens(node) { | ||
const isAsync = node.async; | ||
const firstTokenOfParam = sourceCode.getFirstToken(node, isAsync ? 1 : 0); | ||
const arrowToken = sourceCode.getTokenBefore(node.body, astUtils.isArrowToken); | ||
|
||
const openingParenToken = node.params.length >= 1 && node.params[0].type === "Identifier" | ||
? sourceCode.getTokenBefore(node.params[0], t => isTokenPartOfNode(t, node) && astUtils.isOpeningParenToken(t)) | ||
: sourceCode.getTokenBefore(arrowToken, t => isTokenPartOfNode(t, node) && astUtils.isOpeningParenToken(t)); | ||
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 could find an opening paren in default values for params. Maybe we should just always get the first token before
|
||
|
||
/** | ||
* Remove the parenthesis around a parameter | ||
* @param {Fixer} fixer Fixer | ||
* @returns {string} fixed parameter | ||
*/ | ||
function fixParamsWithParenthesis(fixer) { | ||
const paramToken = sourceCode.getTokenAfter(firstTokenOfParam); | ||
function fixUnwrap(fixer) { | ||
const firstParamToken = sourceCode.getTokenAfter(openingParenToken); | ||
|
||
/* | ||
* ES8 allows Trailing commas in function parameter lists and calls | ||
* https://github.com/eslint/eslint/issues/8834 | ||
*/ | ||
const closingParenToken = sourceCode.getTokenAfter(paramToken, astUtils.isClosingParenToken); | ||
const asyncToken = isAsync ? sourceCode.getTokenBefore(firstTokenOfParam) : null; | ||
const shouldAddSpaceForAsync = asyncToken && (asyncToken.range[1] === firstTokenOfParam.range[0]); | ||
// /* | ||
// * ES8 allows Trailing commas in function parameter lists and calls | ||
// * https://github.com/eslint/eslint/issues/8834 | ||
// */ | ||
const closingParenToken = sourceCode.getTokenAfter(firstParamToken, astUtils.isClosingParenToken); | ||
|
||
const adjacentToken = sourceCode.getTokenBefore(openingParenToken); | ||
|
||
const shouldAddSpace = | ||
adjacentToken && | ||
!astUtils.canTokensBeAdjacent(adjacentToken, firstParamToken) && | ||
adjacentToken.range[1] === openingParenToken.range[0]; | ||
|
||
return fixer.replaceTextRange([ | ||
firstTokenOfParam.range[0], | ||
openingParenToken.range[0], | ||
closingParenToken.range[1] | ||
], `${shouldAddSpaceForAsync ? " " : ""}${paramToken.value}`); | ||
], `${shouldAddSpace ? " " : ""}${firstParamToken.value}`); | ||
} | ||
|
||
|
||
/** | ||
* Adds the parenthesis around a parameter | ||
* @param {Fixer} fixer Fixer | ||
* @returns {string} fixed parameter | ||
*/ | ||
function fixWrap(fixer) { | ||
const firstParamToken = sourceCode.getFirstToken(node, node.async ? 1 : 0); | ||
|
||
return fixer.replaceText(firstParamToken, `(${firstParamToken.value})`); | ||
} | ||
|
||
// "as-needed", { "requireForBlockBody": true }: x => x | ||
|
@@ -111,15 +141,17 @@ module.exports = { | |
node.params.length === 1 && | ||
node.params[0].type === "Identifier" && | ||
!node.params[0].typeAnnotation && | ||
!node.typeParameters && | ||
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 works, but we shouldn't use There is a simple alternative using the tokens only: Once we have @platinumazure @kaicataldo would this be acceptable? This is to avoid removing necessary parens in cases such as this: /* eslint arrow-parens: ["error", "as-needed"] */
<T>(x) => {}; 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'm no longer on the team, but I would be okay with this approach. @kaicataldo? 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. Agreed! |
||
node.body.type !== "BlockStatement" && | ||
!node.returnType | ||
) { | ||
if (astUtils.isOpeningParenToken(firstTokenOfParam)) { | ||
if (openingParenToken) { | ||
context.report({ | ||
node, | ||
messageId: "unexpectedParensInline", | ||
loc: getLocation(node), | ||
fix: fixParamsWithParenthesis | ||
fix: fixUnwrap | ||
|
||
}); | ||
} | ||
return; | ||
|
@@ -129,14 +161,12 @@ module.exports = { | |
requireForBlockBody && | ||
node.body.type === "BlockStatement" | ||
) { | ||
if (!astUtils.isOpeningParenToken(firstTokenOfParam)) { | ||
if (!openingParenToken) { | ||
context.report({ | ||
node, | ||
messageId: "expectedParensBlock", | ||
loc: getLocation(node), | ||
fix(fixer) { | ||
return fixer.replaceText(firstTokenOfParam, `(${firstTokenOfParam.value})`); | ||
} | ||
fix: fixWrap | ||
}); | ||
} | ||
return; | ||
|
@@ -147,33 +177,28 @@ module.exports = { | |
node.params.length === 1 && | ||
node.params[0].type === "Identifier" && | ||
!node.params[0].typeAnnotation && | ||
!node.typeParameters && | ||
!node.returnType | ||
) { | ||
if (astUtils.isOpeningParenToken(firstTokenOfParam)) { | ||
if (openingParenToken) { | ||
context.report({ | ||
node, | ||
messageId: "unexpectedParens", | ||
loc: getLocation(node), | ||
fix: fixParamsWithParenthesis | ||
fix: fixUnwrap | ||
}); | ||
} | ||
return; | ||
} | ||
|
||
if (firstTokenOfParam.type === "Identifier") { | ||
const after = sourceCode.getTokenAfter(firstTokenOfParam); | ||
|
||
// (x) => x | ||
if (after.value !== ")") { | ||
context.report({ | ||
node, | ||
messageId: "expectedParens", | ||
loc: getLocation(node), | ||
fix(fixer) { | ||
return fixer.replaceText(firstTokenOfParam, `(${firstTokenOfParam.value})`); | ||
} | ||
}); | ||
} | ||
// "always" | ||
if (!openingParenToken) { | ||
context.report({ | ||
node, | ||
messageId: "expectedParens", | ||
loc: getLocation(node), | ||
fix: fixWrap | ||
}); | ||
} | ||
} | ||
|
||
|
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.
If there is no opening paren for params (e.g.,
x => x
), I think this would go through all tokens before the param until it reaches the beginning of the source code.Maybe we could just unconditionally get the first token before
params[0]
and then check if it is an opening paren that is part of the node.Also, I think there is no need to check
node.params[0].type === "Identifier"
here.