Fix: no-extra-label autofix should not remove labels used elsewhere #7885

Merged
merged 1 commit into from Jan 9, 2017

Projects

None yet

6 participants

@not-an-aardvark
Member

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

[x] Bug fix

Tell us about your environment

  • ESLint Version: 3.13.0
  • Node Version: 7.3.0
  • npm Version: 3.10.10

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

default

Please show your full configuration:

(none)

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

/* eslint no-extra-label: error */

A: while (true) {
  break A;
  while (true) {
    break A;
  }
}

What did you expect to happen?

I expected an error to be reported on line 2, and the code to be fixed to

/* eslint no-extra-label: error */

A: while (true) {
  break;
  while (true) {
    break A;
  }
}

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

An error was reported on line 2 as expected, but the code was fixed to

/* eslint no-extra-label: error */

while (true) {
  break;
  while (true) {
    break A;
  }
}

...which is a syntax error, because the A label no longer exists even though it's used elsewhere.

What changes did you make? (Give an overview)

This changes the behavior of the no-extra-label autofixer. Previously, the autofixer would behave like this:

A: while (true) {
  break A;
}

// gets fixed to

while (true) {
  break;
}

However, it now behaves like this:

A: while (true) {
  break A;
}

// gets fixed to

A: while (true) {
  break;
}

In other words, it now only fixes the break/continue statement that got reported, but doesn't touch the loop. In my opinion, this is a better fix, because the no-extra-label rule is only concerned about extra break/continue labels, not unused labels on loops. (The no-unused-labels rule can be used to enforce no missing loop labels, and it will be able to do this automatically when/if #7841 is merged.)

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

It would be good to verify that this change is a good idea. My only concern is that there was some opposition to adding a fixer for no-unused-labels in #7841, so if that PR stalls or is rejected, people might have more unused labels in their code after autofixing.

@not-an-aardvark not-an-aardvark Fix: no-extra-label autofix should not remove labels used elsewhere
d3e5ba7
@eslintbot

LGTM

@mention-bot

@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vitorbal, @mysticatea and @gyandeeps to be potential reviewers.

@ilyavolodin ilyavolodin merged commit f90462e into master Jan 9, 2017

5 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@not-an-aardvark not-an-aardvark deleted the no-extra-label-multi-use branch Jan 9, 2017
@keylocation-bot keylocation-bot referenced this pull request in singapore/lint-condo Jan 11, 2017
Merged

Update dependency eslint to version 3.13.1 #215

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment