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

captain-yossarian · 2018-11-11T17:13:17Z

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[X ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

#11069

What changes did you make? (Give an overview)
I've add the function which checks if ELSE block includes block scope local variable declaration [let or const]

Is there anything you'd like reviewers to focus on?

jsf-clabot · 2018-11-11T17:13:18Z

All committers have signed the CLA.

…11069

captain-yossarian · 2018-11-11T18:43:49Z

I had wrong email on my git account, that's why CI couldn't not to approve. How to properly remove all of this previous commits? Thank you

platinumazure

Hi @SerhiiBilyk, thanks for contributing!

As the PR template notes, we require unit tests for all changes (where unit tests are applicable). Could you please add some tests?

Rule unit test resources:

platinumazure

Thanks for adding tests!

I think the important test case in the issue is when there is a shadowing of an existing variable in the else clause. For example:

function foo() {
    let bar = true;
    if (baz) {
        return;
    } else {
        let bar = false;    // Shadows "bar" from top of function
        return;
    ]

Could you please add this test case? Then I can review the general approach 😄

captain-yossarian · 2018-11-12T19:26:32Z

@platinumazure done

tests/lib/rules/no-else-return.js

captain-yossarian · 2018-11-13T07:29:00Z

@ajafff @platinumazure what I'm doing wrong?

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

this test is not failing. Like I understand, [output] must be:

output : "function foo() { let bar = true; if (baz) {  } else { { let bar = false; }  } }"

not-an-aardvark · 2018-11-18T01:35:43Z

I think @ajafff meant that the output should be

function foo() {
  let bar = true;
  if (baz) {
    return;
  }
    {
      let bar = false; 
    }
}

...since it would be safe to remove the else block in that case because all the variables declared the let block would be scoped off, and there would be no risk of having the variables redeclared elsewhere.

ajafff · 2018-11-18T08:07:15Z

I meant exactly what @not-an-aardvark wrote. Thank you for clarifying.

captain-yossarian · 2018-11-18T11:32:15Z

@ajafff @not-an-aardvark
This is the inital code:

When I saving it (trigger auto fix), I get the next version , without first [return] keyword:

But! When I trying to test it:

I'm getting the next error:

My question:
Why do my test code does not trigger autofix, just like my IDE?

P.S. After two fixes, my IDE (with my code) return next result:

So, it is mean that my code has not any side effects.

not-an-aardvark · 2018-11-18T22:48:22Z

@SerhiiBilyk I think this is happening because the autofixer for the no-useless-return rule is also getting applied at the same time, which causes the no-else-return rule autofixer to temporarily not get applied because the VSCode integration sometimes doesn't fix all problems. I wouldn't worry about this.

captain-yossarian · 2018-11-19T06:18:56Z

@not-an-aardvark @ajafff Is it will be ok?

aladdin-add · 2018-11-20T11:04:43Z

tests/lib/rules/no-else-return.js

@@ -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; } }",


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

@aladdin-add Does my test code correct?

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

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.

captain-yossarian · 2018-12-02T15:17:14Z

@platinumazure
Hi, my PR still have 'requested changes' status
Can you please check what else I have to do?

platinumazure

I've left some comments. Let me know if you have any questions.

Also, your current approach does not take into account any nested scopes that may be introduced within an else block. It might be worth using context.getScope on the BlockStatement node in the IfStatement's alternate property, getting that scope's upper scope, and comparing the declared variables of both scopes. If the else scope declares a new variable with the same name as a variable in the upper scope, then that variable is being shadowed in the pre-autofix code and the fix isn't safe. Using this approach should also allow for the nested scope declarations (i.e., the last test cases) to be fixed safely.

platinumazure · 2018-12-02T15:39:02Z

lib/rules/no-else-return.js

+                     * If else block includes block scope local variable declaration [let or const],
+                     * then it is not safe to remove else keyword [issue 11069]
+                     */
+                    const isDeclarationInside = sourceCode.getTokensBetween(startToken, endToken).findIndex(isVariableDeclaration) > -1;


Nit: You could use .some here to simplify

@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

@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

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

platinumazure · 2018-12-02T15:40:35Z

tests/lib/rules/no-else-return.js

@@ -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; } }",


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

platinumazure · 2018-12-02T15:42:33Z

tests/lib/rules/no-else-return.js

+            errors: [{ messageId: "unexpected", type: "BlockStatement" }]
+        },
+        {
+            code: "function foo() { let bar = true; if (baz) { return; } else { { let bar = false; }  return; } }",


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.

platinumazure · 2018-12-02T15:48:03Z

Also, I apologize for losing track of this.

captain-yossarian · 2018-12-14T09:23:03Z

@platinumazure can you check my last commit?
I've created [ childScopeVars ] function which gather all variables from child scopes of [else] block, and then [ shadowedVariables ] checks is there any shadows.

ajafff · 2018-12-17T07:07:34Z

lib/rules/no-else-return.js

+
+                    /**
+                     * If else block includes block scope local variable declaration [let or const],
+                     * then it is not safe to remove else keyword [issue 11069]


it's safe to remove the else keyword. it's not safe to remove the Block of the else branch

ajafff · 2018-12-17T07:11:17Z

lib/rules/no-else-return.js

+                    const shadowed = shadowedVariables(context.getScope());
+                    console.log('test', shadowed)
+
+                    if (isDeclarationInside) {


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

@ajafff
Do you mean

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

by "block scoped declarations" ?

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

"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); } } }

ajafff · 2018-12-17T07:17:19Z

lib/rules/no-else-return.js

+                        const result = childScopeVars(scope.childScopes);
+                        return scope.variables.filter(elem => result[0].findIndex(vars => vars.name === elem.name) > -1)
+                    };
+                    const shadowed = shadowedVariables(context.getScope());


you probably don't need to compute the shadowed names. Consider the following example, where the shadowed declaration is in a parent scope:

let foo = 1; function test() { if (condition) { return foo; } else { let foo = 2; console.log(foo) } }

removing the else-block changes the reference of foo in the then-branch (which will throw as the use is in the temporal dead zone)

@ajafff Thank you very much for your example and patience. I made another one commit. Can you check it?
Still have problem with variable shadowing,Don't know how to handle it.
P.S. Happy holidays!

ajafff · 2018-12-17T07:18:39Z

lib/rules/no-else-return.js

+
+                    const shadowedVariables = scope => {
+                        const result = childScopeVars(scope.childScopes);
+                        return scope.variables.filter(elem => result[0].findIndex(vars => vars.name === elem.name) > -1)


consider using some instead of findIndex if you're only interested in a match and don't use the index

captain-yossarian · 2018-12-18T06:36:16Z

@ajafff Thank you very much for your comments!!!!!!!!!

ajafff · 2019-01-02T20:52:10Z

lib/rules/no-else-return.js

+                             const elseBranch =
+                                 start === startToken.start && end === endToken.end;
+
+                             return elseBranch ? childScope.variables.some(variable => BLOCK_SCOPED_DECLARATION.test(variable.defs[0].node.type)) : false


seems like there's a missing semicolon at the end of the line

Please fix the missing semicolon on this line, as well as any other lint errors. Thanks!

ajafff · 2019-01-02T20:53:53Z

lib/rules/no-else-return.js

+                             const elseBranch =
+                                 start === startToken.start && end === endToken.end;
+
+                             return elseBranch ? childScope.variables.some(variable => BLOCK_SCOPED_DECLARATION.test(variable.defs[0].node.type)) : false


I don't know exactly how ESLint's scope handling works, but it should work if you only check if childScope.variables.length !== 0. That's because function scoped declarations like var or function are hoisted to the containing function scope.

I agree that it's worth trying out this simplification, although I think the real problem is that we need to track all variables available in the containing scope and look for declarations with the same name (which are shadowing before the autofix, but redeclaring after).

platinumazure

I've left a comment about fixing lint errors. Additionally, I tried to explain more about what I think the logic should look like, but I expect we might need to go back and forth a bit to figure out the best way to handle it.

Let's start by fixing the lint errors first... Thanks!

platinumazure · 2019-02-09T20:57:32Z

lib/rules/no-else-return.js

+                             const elseBranch =
+                                 start === startToken.start && end === endToken.end;
+
+                             return elseBranch ? childScope.variables.some(variable => BLOCK_SCOPED_DECLARATION.test(variable.defs[0].node.type)) : false


Please fix the missing semicolon on this line, as well as any other lint errors. Thanks!

platinumazure · 2019-02-09T21:09:29Z

lib/rules/no-else-return.js

+                             const elseBranch =
+                                 start === startToken.start && end === endToken.end;
+
+                             return elseBranch ? childScope.variables.some(variable => BLOCK_SCOPED_DECLARATION.test(variable.defs[0].node.type)) : false


I agree that it's worth trying out this simplification, although I think the real problem is that we need to track all variables available in the containing scope and look for declarations with the same name (which are shadowing before the autofix, but redeclaring after).

captain-yossarian · 2019-02-10T11:15:05Z

@platinumazure I have fixed eslint errors.
Thank you for your help.

I agree that it's worth trying out this simplification, although I think the real problem is that we need to track all variables available in the containing scope and look for declarations with the same name (which are shadowing before the autofix, but redeclaring after).

Do you mean, that I should try this (0b3a3a4) approach?

ilyavolodin · 2019-02-15T20:03:59Z

@platinumazure What's the status of this PR? Anything else needs to be done here?

platinumazure

Hi @SerhiiBilyk, I apologize for having let this slip.

I have a few simple changes, and then I was hoping we could look at the 2 test cases that could potentially be autofixable. It might be okay to merge this as is since it's better for our autofixer to be cautious than overeager, but I wanted to see what you thought about trying to make those 2 cases fixable.

Thanks so much for your patience and all of your effort so far. On behalf of the ESLint team, I greatly appreciate the hard work!

platinumazure · 2019-03-30T02:19:10Z

lib/rules/no-else-return.js

+ * @param {Token} endToken s
+ * @returns {boolean} s
+ */
+function blockScopeDeclaration(scope, startToken, endToken) {


I'm a little confused by this function name. What are we checking here? It looks like we might be trying to check if an else block has any block scope declarations?

I would suggest prefixing the function name with "is" or "has", if it is returning a boolean.

I have renamed it.
Yes, I'm checking here if else block has any scope declarations,

platinumazure · 2019-03-30T02:19:30Z

lib/rules/no-else-return.js

+ * @param {*} scope  current scope
+ * @param {Token} startToken s
+ * @param {Token} endToken s
+ * @returns {boolean} s


Could you please fill in these comments (this line, and the two lines above it)?

platinumazure · 2019-03-30T02:21:04Z

tests/lib/rules/no-else-return.js

@@ -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; } }",


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.

platinumazure · 2019-03-30T02:21:12Z

tests/lib/rules/no-else-return.js

+        },
+        {
+            code: "function foo() { if (true) { return bar; } else { let baz = 1;  return baz; } }",
+            output: null,


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.

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

captain-yossarian · 2019-04-03T18:00:05Z

Hi @platinumazure ,
Thank you for your answer, I will check all your comment tomorrow.

platinumazure · 2019-05-14T21:08:16Z

Friendly ping @SerhiiBilyk: Are you still planning to work on this? Is there anything I can do to help? Thanks!

captain-yossarian · 2019-05-17T07:26:12Z

@platinumazure I'm sorry, but unfortunately I have no time to work on this bug. Sorry for the delay

aladdin-add · 2019-05-22T12:44:50Z

@SerhiiBilyk no worries, thanks for the contributing regardless!

@eslint/eslint-team shall we close it?

ilyavolodin · 2019-05-25T19:36:31Z

I'm going to close this PR, since it's been open for a while, and original owner doesn't have time to work on it anymore. Thanks again for the PR!

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

9a8d8d6

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Nov 11, 2018

captain-yossarian added 4 commits November 11, 2018 19:15

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

a640595

Merge branch 'issue11069' of github.com:SerhiiBilyk/eslint into issue…

20f8998

…11069

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

28eb7f8

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

cc00be7

captain-yossarian force-pushed the issue11069 branch from 7f6912c to cc00be7 Compare November 11, 2018 18:39

platinumazure added bug ESLint is working incorrectly rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Nov 11, 2018

platinumazure requested changes Nov 11, 2018

View reviewed changes

Add unit tests

1945162

captain-yossarian force-pushed the issue11069 branch from 302d094 to 1945162 Compare November 12, 2018 18:06

platinumazure requested changes Nov 12, 2018

View reviewed changes

Add specific test case

51ed101

ajafff reviewed Nov 12, 2018

View reviewed changes

tests/lib/rules/no-else-return.js Show resolved Hide resolved

Add one more unit test

62289ee

aladdin-add added the accepted There is consensus among the team that this change meets the criteria for inclusion label Nov 19, 2018

aladdin-add reviewed Nov 20, 2018

View reviewed changes

platinumazure requested changes Dec 2, 2018

View reviewed changes

Get all variables from childScope and compare them

0772a0a

ajafff suggested changes Dec 17, 2018

View reviewed changes

Compute block scope declaration

0b3a3a4

ajafff reviewed Jan 2, 2019

View reviewed changes

platinumazure requested changes Feb 9, 2019

View reviewed changes

Fix eslint errors

425f041

platinumazure requested changes Mar 30, 2019

View reviewed changes

ilyavolodin closed this May 25, 2019

eslint-deprecated bot locked and limited conversation to collaborators Nov 22, 2019

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Nov 22, 2019

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

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

Conversation

captain-yossarian commented Nov 11, 2018

jsf-clabot commented Nov 11, 2018 • edited

captain-yossarian commented Nov 11, 2018

platinumazure left a comment

Choose a reason for hiding this comment

platinumazure left a comment

Choose a reason for hiding this comment

captain-yossarian commented Nov 12, 2018

captain-yossarian commented Nov 13, 2018 • edited

not-an-aardvark commented Nov 18, 2018

ajafff commented Nov 18, 2018

captain-yossarian commented Nov 18, 2018

not-an-aardvark commented Nov 18, 2018

captain-yossarian commented Nov 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captain-yossarian commented Dec 2, 2018

platinumazure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

platinumazure commented Dec 2, 2018

captain-yossarian commented Dec 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captain-yossarian Dec 20, 2018 • edited

Choose a reason for hiding this comment

captain-yossarian Dec 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captain-yossarian commented Dec 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

platinumazure left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captain-yossarian commented Feb 10, 2019 • edited

ilyavolodin commented Feb 15, 2019

platinumazure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captain-yossarian commented Apr 3, 2019

platinumazure commented May 14, 2019

captain-yossarian commented May 17, 2019

aladdin-add commented May 22, 2019

ilyavolodin commented May 25, 2019

jsf-clabot commented Nov 11, 2018 •

edited

captain-yossarian commented Nov 13, 2018 •

edited

captain-yossarian Dec 20, 2018 •

edited

captain-yossarian Dec 20, 2018 •

edited

platinumazure left a comment •

edited

captain-yossarian commented Feb 10, 2019 •

edited