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: prevent fixing to incorrect code (fixes #11069) #11076

Closed
wants to merge 11 commits into from
Closed

Fix: prevent fixing to incorrect code (fixes #11069) #11076

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9a8d8d6
Fix: prevent fixing to incorrect code (fixes #11069)
captain-yossarian Nov 11, 2018
a640595
Fix: prevent fixing to incorrect code (fixes #11069)
captain-yossarian Nov 11, 2018
20f8998
Merge branch 'issue11069' of github.com:SerhiiBilyk/eslint into issue…
captain-yossarian Nov 11, 2018
28eb7f8
Fix: prevent fixing to incorrect code (fixes #11069)
captain-yossarian Nov 11, 2018
cc00be7
Fix: prevent fixing to incorrect code (fixes #11069)
captain-yossarian Nov 11, 2018
1945162
Add unit tests
captain-yossarian Nov 12, 2018
51ed101
Add specific test case
captain-yossarian Nov 12, 2018
62289ee
Add one more unit test
captain-yossarian Nov 19, 2018
0772a0a
Get all variables from childScope and compare them
captain-yossarian Dec 14, 2018
0b3a3a4
Compute block scope declaration
captain-yossarian Dec 27, 2018
425f041
Fix eslint errors
captain-yossarian Feb 10, 2019
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
25 changes: 25 additions & 0 deletions lib/rules/no-else-return.js
View file
Open in desktop
Expand Up @@ -12,6 +12,20 @@
const astUtils = require("../util/ast-utils");
const FixTracker = require("../util/fix-tracker");

//------------------------------------------------------------------------------
// Helpers
//------------------------------------------------------------------------------

/**
* @param {Token} token - The token to check
* @returns {boolean} `true` if keyword let or const exist
*/
function isVariableDeclaration(token) {
const variableDeclaration = token.value === "let" || token.value === "const";

return token.type === "Keyword" && variableDeclaration;
}

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -88,6 +102,17 @@ module.exports = {
}

const endToken = sourceCode.getLastToken(node);

/**
* If else block includes block scope local variable declaration [let or const],
* then it is not safe to remove else keyword [issue 11069]
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 safe to remove the else keyword. it's not safe to remove the Block of the else branch

*/
const isDeclarationInside = sourceCode.getTokensBetween(startToken, endToken).findIndex(isVariableDeclaration) > -1;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You could use .some here to simplify

Copy link
Author

Choose a reason for hiding this comment

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

@platinumazure I've used .findIndex, because it will stop iteration if it will find an element and .some method will iterate through whole array, even it will find an element

Copy link
Member

Choose a reason for hiding this comment

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

@SerhiiBilyk Not right. The some method also stops iteration once the callback returns truthy value. You can check MDN for detail: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/some

Copy link
Author

Choose a reason for hiding this comment

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

@g-plane you are right, thank you. I will change it!


if (isDeclarationInside) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this will evaluate to true if there's a let or const somewhere in a nested child scope. instead it should only abort here if the else-block contains block scoped declarations

Copy link
Author

Choose a reason for hiding this comment

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

@ajafff
Do you mean

else {
    {
      const x = false;
    }
    return null;
  }

by "block scoped declarations" ?

Copy link
Author

Choose a reason for hiding this comment

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

@ajafff Does it mean, that if else contain block scoped declarations , fix method has to return null (abort)?
Can you please provide an example?

Copy link
Contributor

Choose a reason for hiding this comment

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

"Block scoped declaration" means const, let and class as they are not hoisted to the containing function.

let foo = 1;
function canNotBeFixed() {
  if (condition) {
    return foo;
  } else {
    let foo = 2; // else-block contains block scoped declaration, can not be fixed
    console.log(foo);
  }
}

function canBeFixed() {
  if (condition) {
    return foo;
  } else {
    {
      let foo = 2; // block scoped declaration is not directly in else-block, but in a nested block
      console.log(foo);
    }
  }
}

return null;
}

const lastTokenOfElseBlock = sourceCode.getTokenBefore(endToken);

if (lastTokenOfElseBlock.value !== ";") {
Expand Down
26 changes: 26 additions & 0 deletions tests/lib/rules/no-else-return.js
View file
Open in desktop
Expand Up @@ -182,6 +182,32 @@ ruleTester.run("no-else-return", rule, {
output: "function foo21() { var x = true; if (x) { return x; } if (x === false) { return false; } }",
options: [{ allowElseIf: false }],
errors: [{ messageId: "unexpected", type: "IfStatement" }]
},
{
code: "function foo() { if (true) { return bar; } else { const baz = 1; return baz; } }",
Copy link
Member

Choose a reason for hiding this comment

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

why this case cannot be fixed? IMHO, it seems safe to do the fix.

Copy link
Author

Choose a reason for hiding this comment

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

@aladdin-add Does my test code correct?

Copy link
Member

Choose a reason for hiding this comment

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

This case should be fixable because the declaration in the else block doesn't shadow any other variable.

Copy link
Member

Choose a reason for hiding this comment

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

It might be okay to just not autofix any block scope declarations to be safe, but I think this one could be autofixed since none of the declarations share a name with the variable in the immediate upper scope.

output: null,
parserOptions: { ecmaVersion: 6 },
errors: [{ messageId: "unexpected", type: "BlockStatement" }]
},
{
code: "function foo() { if (true) { return bar; } else { let baz = 1; return baz; } }",
output: null,
Copy link
Member

Choose a reason for hiding this comment

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

It might be okay to just not autofix any block scope declarations to be safe, but I think this one could be autofixed since none of the declarations share a name with the variable in the immediate upper scope.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @platinumazure , I'm sorry for the delay. I have started to work with this issue.
What the expected output here ?

parserOptions: { ecmaVersion: 6 },
errors: [{ messageId: "unexpected", type: "BlockStatement" }]
},
{
code: "function foo() { let bar = true; if (baz) { return; } else { let bar = false; return; } }",
captain-yossarian marked this conversation as resolved.
Show resolved Hide resolved
output: null,
parserOptions: { ecmaVersion: 6 },
errors: [{ messageId: "unexpected", type: "BlockStatement" }]
},
{
code: "function foo() { let bar = true; if (baz) { return; } else { { let bar = false; } return; } }",
Copy link
Member

Choose a reason for hiding this comment

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

This test case could be fixed as well. After removing the else and associated braces, let bar = false is in its own scope due to the extra braces. So this fix is safe.

output: null,
parserOptions: { ecmaVersion: 6 },
errors: [
{ messageId: "unexpected", type: "BlockStatement" }
]
}
]
});