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.

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

Fix: allow fallthrough comment inside block (fixes #14701) #14702

Merged
merged 2 commits into from Jun 18, 2021

Conversation

@bakkot
Copy link
Contributor

@bakkot bakkot commented Jun 14, 2021

Prerequisites checklist

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

[x] Bug fix
[x] Changes an existing rule

What rule do you want to change?

no-fallthrough

Does this change cause the rule to produce more or fewer warnings?

fewer

How will the change be implemented? (New option, new default behavior, etc.)?

new default

Please provide some example code that this change will affect:

switch (foo) {
  case 1: {
    let x = value;
    console.log(value);
    // falls through
  }
  case 2: {
    doSomething();
  }
}

What does the rule currently do for this code?

warn

What will the rule do after it's changed?

not warn

What changes did you make? (Give an overview)

Now when the case consists of a single BlockStatement the code will look for the fallthrough comment to be the final comment in the case, as well where it looked previously.

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

In the situation in question - i.e., when the case consists of exactly one statement, which is a BlockStatement - I've chosen to check for the fallthrough comment in both places. This means that it becomes possible to have a comment after the fallthrough comment, as long as it's after the block, as in

switch (foo) {
  case 1: {
    let x = value;
    console.log(value);
    // falls through
  }

  // some other comment
  case 2: {
    doSomething();
  }
}

I think that's correct, personally. However, I'm happy to be more restrictive and say that the fallthrough comment must also be the final comment: that is, to only look in the new place when there are no comments after the block.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jun 14, 2021

@bakkot thanks for the PR!

In the situation in question - i.e., when the case consists of exactly one statement, which is a BlockStatement - I've chosen to check for the fallthrough comment in both places. This means that it becomes possible to have a comment after the fallthrough comment, as long as it's after the block, as in

switch (foo) {
  case 1: {
    let x = value;
    console.log(value);
    // falls through
  }

  // some other comment
  case 2: {
    doSomething();
  }
}

I agree, I think it's fine to allow any comments after } when the fallsthrough comment is last in { ... }.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

All changes look good, I only have some small requests about the documentation and a few additional tests.

docs/rules/no-fallthrough.md Show resolved Hide resolved
tests/lib/rules/no-fallthrough.js Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

Copy link
Contributor

@snitin315 snitin315 left a comment

LGTM

Copy link
Member

@btmills btmills left a comment

Thanks, and congrats on your first ESLint pull request, @bakkot! This will be released today.

@btmills btmills merged commit 6a1c7a0 into eslint:master Jun 18, 2021
14 checks passed
14 checks passed
@github-actions
Verify Files
Details
@github-actions
Test (ubuntu-latest, 16.x)
Details
@github-actions
Test (ubuntu-latest, 15.x)
Details
@github-actions
Test (ubuntu-latest, 14.x)
Details
@github-actions
Test (ubuntu-latest, 13.x)
Details
@github-actions
Test (ubuntu-latest, 12.x)
Details
@github-actions
Test (ubuntu-latest, 10.x)
Details
@github-actions
Test (ubuntu-latest, 10.12.0)
Details
@github-actions
Test (windows-latest, 12.x)
Details
@github-actions
Test (macOS-latest, 12.x)
Details
@github-actions
Browser Test
Details
@eslint-github-bot
commit-message PR title follows commit message guidelines
Details
licence/cla Contributor License Agreement is signed.
Details
@eslint-github-bot
release-monitor No patch release is pending
Details
@bakkot bakkot deleted the bakkot:fallthrough-comment-in-switch-case branch Jun 18, 2021
@evelynhathaway
Copy link

@evelynhathaway evelynhathaway commented Jun 19, 2021

Oh yay! I submitted a PR for this a long time ago but it got closed. I think this also fixes #4652, #7889, #9080, and #9559

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants