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.

Sign up for GitHub

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

Jump to bottom

Fix: add better TS generic support for arrow-parens (fixes #12570) #12587

Closed
wants to merge 6 commits into from
Closed

Fix: add better TS generic support for arrow-parens (fixes #12570) #12587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7945e7a
Chore: Adds typescript generics tests for arrow-parens (refs #12570)
matmalkowski Nov 21, 2019
d33c7f0
Fix: add better TS generic support for arrow-parens (fixes #12570)
matmalkowski Nov 23, 2019
d2084cd
Fix: code review comments
matmalkowski Dec 26, 2019
1bf3ff0
Fix: add additional test case for TS generic
matmalkowski Dec 28, 2019
444dc14
Revert "Fix: add additional test case for TS generic"
matmalkowski Jan 1, 2020
eb30038
Fix: code review comments
matmalkowski Jan 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
95 changes: 60 additions & 35 deletions lib/rules/arrow-parens.js
View file
Open in desktop
Expand Up @@ -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))
Copy link
Member

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.

: sourceCode.getTokenBefore(arrowToken, t => isTokenPartOfNode(t, node) && astUtils.isOpeningParenToken(t));
Copy link
Member

Choose a reason for hiding this comment

The 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 params[0], and make sure that the rule works only on single-param arrow function by changing the ArrowFunctionExpression selector to:

"ArrowFunctionExpression[params.length=1]"


/**
* 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
Expand All @@ -111,15 +141,17 @@ module.exports = {
node.params.length === 1 &&
node.params[0].type === "Identifier" &&
!node.params[0].typeAnnotation &&
!node.typeParameters &&
Copy link
Member

Choose a reason for hiding this comment

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

This works, but we shouldn't use node.typeParameters because it isn't defined in estree extensions.

There is a simple alternative using the tokens only: Once we have openingParenToken, we could take the first token of the function (the second if the function is async) and check by reference whether it is the openingParenToken. If it isn't, then the rule should not warn to remove parens. That way, we are not using undocumented AST properties. It's a simple check, though it doesn't make sense for JS-only input so the rule becomes aware that there can be something unknown before the params (I don't think there is a way to avoid that).

@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) => {};

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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;
Expand All @@ -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;
Expand All @@ -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
});
}
}

Expand Down
10 changes: 10 additions & 0 deletions lib/rules/utils/ast-utils.js
View file
Open in desktop
Expand Up @@ -271,6 +271,15 @@ function isArrowToken(token) {
return token.value === "=>" && token.type === "Punctuator";
}

/**
* Checks if the given token is an async token or not.
* @param {Token} token The token to check.
* @returns {boolean} `true` if the token is an async token.
*/
function isAsyncToken(token) {
return token.value === "async" && token.type === "Identifier";
}

/**
* Checks if the given token is a comma token or not.
* @param {Token} token The token to check.
Expand Down Expand Up @@ -451,6 +460,7 @@ module.exports = {
equalTokens,

isArrowToken,
isAsyncToken,
isClosingBraceToken,
isClosingBracketToken,
isClosingParenToken,
Expand Down