switch generates unneeded breaks

[edemaine]

This is a feature request that surfaces as a bug when trying to use eslint with standard rules, specifically the no-unreachable rule.

Input Code

convertToBoolean = (string) ->
  switch string
    when 'yes', 'Yes'
      if string.startsWith 'Y'
        'True'
      else
        'true'
    when 'no'
      'false'
    else
      null

try link

Expected Behavior

var convertToBoolean;

convertToBoolean = function(string) {
  switch (string) {
    case 'yes':
    case 'Yes':
      if (string.startsWith('Y')) {
        return 'True';
      } else {
        return 'true';
      }
    case 'no':
      return 'false';
    default:
      return null;
  }
};

Current Behavior

Currently, CoffeeScript generates dead/inaccessible break in the first case (because of the if expression) but it's smart enough not to in the second case.

var convertToBoolean;

convertToBoolean = function(string) {
  switch (string) {
    case 'yes':
    case 'Yes':
      if (string.startsWith('Y')) {
        return 'True';
      } else {
        return 'true';
      }
      break;
    case 'no':
      return 'false';
    default:
      return null;
  }
};

Possible Solution

Ideally, the existing mechanism for not generating break lines in switch statements can apply to complex implicit return expressions (such as if expressions), not just simple expressions.

Context

One could argue that the extra break statements are fine to leave in. Unfortunately, this 2016 eslint issue disagrees with this argument, so there's no easy way to allow these break statements in eslint without allowing all dead code (i.e. disabling the no-unreachable).

Environment

Live demo on try website

[helixbass]

I'd assume that when using eslint-plugin-coffee this isn't an issue because it's not operating against transpiled JS? So I'm not sure how strong of an argument there is that transpiled JS should pass any particular linting rules?

That being said, it's interesting if that isn't too hard to achieve. I just took a quick look at the associated PR. My instinct is it wouldn't be worth introducing the additional logic just for the sake of what could be considered an aesthetic improvement to the transpiled JS

[edemaine]

Indeed, now that I have eslint-plugin-coffee working in my setting (with many thanks to @helixbass), this is much less important. PR #5348 is indeed very complicated, so unless we see other need for dead-code elimination, I'm fine with closing both this issue and that PR. (On the plus side, writing #5348 taught me a lot about hacking the CoffeeScript compiler!)

[GeoffreyBooth]

I mean, it would be nice to not output unnecessary breaks, but aside from slightly less code is there any other benefit to #5348? In general I feel like additional complexity should add something worthwhile to justify its maintenance cost and any potential bugs it might introduce.

[edemaine]

I agree. Looking at the PR again, it's not much added complexity, but the only real improvement is nicer looking code, which probably isn't worthwhile (unless there's another reason we want to detect unreachable code). I originally wrote this issue before I knew of eslint-plugin-coffee; now that I know about it, I think we can close this.

#5348 does have some improvements to comments which I'll extract into another PR.

[GeoffreyBooth]

Part of CoffeeScript’s pitch on our improvements to switch is “hey, you never need to remember to type break anymore!” so it kind of makes sense that we always output break, even if it’s occasionally redundant. This isn’t an area where I’d be eager to optimize our output.

Aside from #5366, there’s only one area that I’d consider investing time in improving our JS output, and that’s to stop outputting var and instead output const and let at the appropriate places. I did a branch years ago that simply replaced var with let in our output, and everything still worked, but we didn’t merge it because it was a bit misleading since the variable declarations were still at the function scope level rather than the block scope level (and let is block scoped whereas var is function scoped). Tracking variable declarations at the block scope instead of the function scope would probably be easier, and could lead to a simplification of our codebase, in addition to being more idiomatic with modern ES6+ JavaScript. I would maybe do it in two passes, first replacing all the function-scoped var declarations with block-scoped let declarations, and second trying to use const when we know that a variable never gets reassigned.

[edemaine]

When you say "block-scoped let declarations", I assume you mean finding the smallest block that contains all uses of the given variable (sometimes top of the function, sometimes a contained block)?

This seems subtle. For example, the following code behaves differently if let x is put outside or inside the loop:

for i in [1..2]
  x = 5 unless x?
  console.log x++

(Outside, it prints 5, 6; inside, it prints 5, 5.)

And it's not enough to just pull it out of the containing loop. You need to pull it out of all loops. For example:

for j in [1..2]
  for i in [1..2]
    x = 5 unless x?
    console.log x++

This should print 5, 6, 7, 8. If let x goes inside one loop, it will print 5, 6, 5, 6. If let x goes in both loops, it will print 5, 5, 5, 5.

What's a good example where let can be put inside a block? I guess for something like

... code not using y ...
y = 7

...it might be nicer to write let y = 7; i.e. put the declaration with the assignment. But it's going to be tricky to detect when.

Distinguishing let vs. const seems more interesting, and presumably not too hard...

[GeoffreyBooth]

When you say “block-scoped let declarations”, I assume you mean finding the smallest block that contains all uses of the given variable (sometimes top of the function, sometimes a contained block)?

Yes, basically. Although the key here is to not cause any breaking changes. So when in doubt, keep the current “define at the top of the function scope” behavior.

A declaration/first assignment within a loop is a great example. In this case, a let x within a loop means that x is declared multiple times (once for each iteration of the loop) which as your examples show, might be a breaking change for existing code. So in general, any cases where variables declared/first assigned within loops should keep the existing behavior. There will probably be lots of cases like this.

If you look at this example:

if new Date().getHours() < 9
  breakfast = if new Date().getDay() is 6 then 'donuts' else 'coffee'
  alert "Time to make the #{breakfast}!"
else
  alert 'Time to get some work done.'

breakfast is only used within that if block, but it’s currently getting a var breakfast; line at the top of the output. The output instead could be this:

if (new Date().getHours() < 9) {
  let breakfast;

  breakfast = new Date().getDay() === 6 ? 'donuts' : 'coffee';
  alert(`Time to make the ${breakfast}!`);
} else {
  alert('Time to get some work done.');
}

And then phase 2 would see that breakfast is never reassigned and never referenced before its assignment (read about the let/const “temporal dead zone”) and use const instead:

if (new Date().getHours() < 9) {
  const breakfast = new Date().getDay() === 6 ? 'donuts' : 'coffee';
  alert(`Time to make the ${breakfast}!`);
} else {
  alert('Time to get some work done.');
}

[phil294]

breakfast is only used within that if block, but it’s currently getting a var breakfast; line at the top of the output. The output instead could be this:

This would certainly be helpful! Having variable declarations as far down block-wise as backwards compatibility allows could, by itself, already greatly aid in type checking.

fyi my lsp extension even already partly tries to apply phase 1 where possible and it helps detecting unexpected variable reassignments.

[edemaine]

Discussion moved to #5377. Thanks @GeoffreyBooth -- I think this is a cool thing to pursue.