'no-unless-return' rule's fix is not fix but destroying.

[uk-taniyama]

Tell us about your environment

• ESLint Version: 3.13.1
• Node Version: 7.4.0
• npm Version: 4.1.1

What parser (default, Babel-ESLint, etc.) are you using?

Please show your full configuration:

module.exports = {
  extends: 'airbnb',
  rules: {
    'no-unused-vars': ['error', { vars: "all", args: 'none' }],
  }
}

What did you do? Please include the actual source code causing the issue.

function foo(key) {
  var value = map[key];
  if(!value){
    return;
  }
  // do something....
}
=>
function foo(key) {
  var value = map[key];
  if(!value){
  }
  // do something....everytime???
}

What did you expect to happen?

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

I do not want you to change the logic by '--fix' option.

[not-an-aardvark]

Thanks for the report.

This seems like it's working as intended. You have the comment // do something... but you don't actually have any statements after the if block that are doing anything, so the return statement is correctly getting reported as useless. If you add statements here, the return statement will not get reported.

function foo(key) {
  var value = map[key];
  if(!value){
    return; // <-- This will not get reported
  }
  doSomething();
}

[uk-taniyama]

Thaks.

I can understand the rule.
BUT i dont want to remove a line.

Because....

• i use liter-eslint on Atom. it can do 'fix' on save. it is very useful.
• I want to save frequently during coding. so, 'fix' only format.

Request.

• 'fix' is only format. not break a logic.
• 'fix' has some level. like 'format-only', 'all'.

[not-an-aardvark]

Could you clarify what you mean by only fixing "format"? None of the fixers actually change any logic in the rule as written.

I suppose it could be useful to have an option to only apply whitespace fixes.

[uk-taniyama]

I understand....It's difficult.
but....

my writing sequence as follows......
write some condition check and TODO comment.

function foo(key) {
  var value = map[key];
  if(!value){
    return;
  }
  // TODO something
}

and write other code, and save...
go back to TODO comment.

I need 'return;' when writting a code.
so i changed 'warn' but removed by 'fix'.......
so i have to change 'off'......

by the way. this rule cannot fix source.
because fixed source "if(!value){}" does not need.(no-empty)

function foo(key) {
  var value = map[key];
  if(!value){
  }
  // TODO something
}

[platinumazure]

We do have some precedence for allowing comments to affect certain rules (e.g., default-case). In theory, we could create an option to allow a comment to count as a statement for the purpose of this rule. But I'm not sure it's necessarily worth the development/maintenance effort...

Anyone else from the team have thoughts?

[not-an-aardvark]

My personal opinion is that a change to the rule is not necessary here -- the issue is that the autofixer for the rule is being run before the code is complete. This should be fixed in the editor integration (e.g. by blacklisting/whitelisting certain rules from being autofixed), or we could make it easier to do partial autofixes in core (e.g. --fix=whitespace could fix all rules that have fixable: "whitespace").

However, I'm also interested in hearing what other team members think about this.

[nzakas]

This highlights some of the concerns I've mentioned around us doing too much auto fixing. When we first started, the intent was to only do white space fixes, and we've kept getting more aggressive with other types of fixes too. I think this is a case where we need to start pulling back and being a lot more conservative about applying fixes.

For this particular rule, I think it would be fine to consider a comment as a disqualifying statement, although I also think it would be fine to just remove autofixing from the rule entirely.

Partial auto fixes is a level of complexity I'd like to avoid, as it will make life harder in a lot of ways (case in point: editors would have to choose and likely would just fix everything, which leaves us in the same boat).

[not-an-aardvark]

The reason I'm unconvinced that autofixes like this are a problem is because I don't think ESLint can reasonably be expected to take into account that code is unfinished. There are a lot of rules where the decision of whether to report a piece of code is based on factors that appear after the code in question. If an editor integration is linting/fixing code before it's finished, it should be implied that the results might be different from what they would have been for finished code.

So I think filtering fixes to improve UX for unfinished code is in scope for an editor integration, because it's linting unfinished code with the understanding that the results might be different from the finished code. It's not in scope for a linter, because a linter assumes that all code it sees is finished.

[nzakas]

@not-an-aardvark while I understand your rationale, we do have to be sensitive to how ESLint is being used in all cases. We really can't just say, "oh well, editors should know better." While it's true ESLint will be a little weird in certain cases, I stand by my statement that I think we need to be more conservative with the auto fixes we do, especially in cases like this where we can unintentionally change logic.

[uk-taniyama]

I am interested in reading the discussion.
It is a personal opinion from one end user.

• Certainly, eslint as linter, 'eslint' is mission to do right.
• But I think that there is a level for "--fix" as well.
• But, if it is too complicated design then nobody can use it and not happy.
• Personally, I feel that the control is better with "severity" or "meta.fixable" by adding "--fix-lebel" (temporary).
• But, afterwards, it seems good to say that users are preparing to switch on rule basis.(SHOLD)
• "severity" is incorrect. But I think that it can be used as an alternative.

fix-level 1(for editor). fix only whitespace and error
fix-label 2(for release). fix all(whitespace/code and error/warn)

if user do not want fix 'whitespace' and 'error' then change 'error' to 'warn'.
but i think 'whitespace' is collect. user dont this.

[aboyton]

I agree that this is an editor problem. I think we either have the option of making ESLint do only very superficial automatic fixes or we make ESLint much more powerful and useful.

I think by only doing superficial fixes we force people to run separate tools to do other transformations which either makes things more complicated for users as multiple tools rarely play nicely together, or we end up pushing them to just using other tools exclusively. Currently I'm running TSLint and ESLint concurrently on a single codebase and having two tools is maddening.

[vitorbal]

The editor plugin could theoretically use the new getRules() API method to get the meta.fixable, and only apply fixes on file-save for the ones that are "whitespace".

[ilyavolodin]

@vitorbal Since we don't expose API methods for aufofixing (or rather, the methods we expose are not configurable), that means that editors will have to manually implement autofixing logic themselves. Maybe that's something we can look into. Basically enhance API methods to allow for filtering rules before autofixes are applied.

[IanVS]

FWIW, Atom's linter-eslint now supports specifying rules not to auto-fix in version 8.1.0.

[vitorbal]

@IanVS Wow, that's awesome! Great job! Do you think if we implement such an option to filter rules before applying autofixing it would help simplify the logic from linter-eslint's side?

[IanVS]

To be honest, the logic from linter-eslint's side was pretty simple. In fact most of it already existed because we allow specifying rules to ignore while you're typing. That said, we are using the cli executable directly (not using CLIEngine).

[not-an-aardvark]

In the TSC meeting last month, the TSC discussed cases where in-progress autofixes act unexpectedly. The consensus was to go ahead with those fixes and have a better way for editors to filter autofixes (see #8018).

So I'm going to close this issue since the no-useless-return autofixer is working as intended.