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.

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Jun 14, 2021
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion 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 Jun 14, 2021
@mdjermanovic
Copy link
Member

@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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@bakkot bakkot deleted the fallthrough-comment-in-switch-case branch June 18, 2021 20:20
@evelynhathaway
Copy link

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

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 16, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no-fallthrough should allow the comment inside of a block when the case is a single block
5 participants