no-else-return fixed to incorrect code

[aladdin-add]

Tell us about your environment

• ESLint Version: 5.8.0/master
• Node Version: n/a
• npm Version: n/a

What parser (default, Babel-ESLint, etc.) are you using?
default
Please show your full configuration:

Configuration

{
  rules: {"no-else-return": 2}
}

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

function foo(){
    let bar = 0;
    if(cond) {
        return bar;
    } else {
        let bar = 1;
        return bar;
    }
}

What did you expect to happen?
not fix to incorrect code.

What actually happened? Please include the actual, raw output from ESLint.

fixed code:

function foo(){
    let bar = 0;
    if(cond) {
        return bar;
    } 
        let bar = 1;
        return bar;
    
}

7:13  error  Parsing error: Identifier 'bar' has already been declared

Are you willing to submit a pull request to fix this bug?
yes, but I don't have time to do it in the following days. please feel free to take it if someone else have interests.

[captain-yossarian]

I can take it if it is not too complicated, because it will be my first issue. So, if somebody can give me the start point I will appreciate it

[aladdin-add]

@SerhiiBilyk thanks for reaching out!

the related code is the fixer function:

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

Lines 63 to 64 in 5da378a

	fix: fixer => {
	const sourceCode = context.getSourceCode();

it should return null, if not safe to do the fix.

[captain-yossarian]

@aladdin-add thank you for the answer.
Do I need to remove last [let] keyword in case if same variable exist in current scope?

[aladdin-add]

I think just as-is would be fine. return null; in fixer function means not to fix it. 😄

[captain-yossarian]

Aladdin-ADD , ok I have understand, thank you again!

[captain-yossarian]

@aladdin-add can you check the algorithm?
I'm cheking if there block scope local variable declaration inside [else] statement.
If [true], fix is unsafe => return null

Is this correct?
P.S. startToken && endToken of ELSE block

const isScopeDeclaration = (token) => {
  const keyword = token.type === 'Keyword';
  const scopeDeclaration = token.value === 'let' || token.value === 'const';
  return keyword && scopeDeclaration;
};
const isDeclarationInside = sourceCode.getTokensBetween(startToken, endToken).findIndex(isScopeDeclaration) > -1;

[aladdin-add]

SGTM! Please feel free to visit us in the ESLint Gitter if you have some more questions!

and thanks for doing this!

[eslint-deprecated]

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that accepted issues failing to be implemented after 90 days tend to
never be implemented, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

[platinumazure]

Reopening since we have an open PR.