6.17.2 release breaks /* istanbul ignore */ comments in some scenarios

[bcoe]

this change introduced in babylon@6.17.2 seems to break Istanbul's ignore next functionality in some situations. See the following code:

Input Code

'use strict'

const test = foo => {
  switch (foo) {
    case 'ok':
      return console.log('this is fine')

    /* istanbul ignore next */
    default:
      throw new Error('nope')
  }
}

test('ok')

Expected Behavior

the comment /* istanbul ignore next */ ignores the default statement (behavior in 6.17.1).

Current Behavior

the comment /* isanbul ignore next */ fails to ignore the default switch statement (behavior in 6.17.2).

Thoughts

I believe the issue is that the comment gets folded up onto the prior line, and then we fail to traverse to the default statement when deciding what to ignore.

CC: @aardito2

[hzoo]

Hey @bcoe! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

[loganfsmyth]

Have you considered using https://github.com/istanbuljs/babel-plugin-istanbul? Generally I don't think I'd recommend running tooling on the output from Babel.

[bcoe]

@loganfsmyth https://github.com/istanbuljs/babel-plugin-istanbul/blob/master/package.json#L13 this change landed in babylon@6.17.2 breaks babel-plugin-istanbul and istanbuljs -- or at least their ignore next handling.

Basically I think because the comment gets lumped up onto the prior line, we fail to recognize it as something that should be ignored; I'm digging into istanbul's instrumenter now to see if this is something that could be fixed upstream 😄

[bcoe]

It seems like the underlying issue might be that the comment will no longer be treated as a leadingComment as a result to moving changing its scope, which breaks this logic in istanbul-lib-instrument:

            node.leadingComments.forEach(function (c) {
                const v = (c.value || /* istanbul ignore next: paranoid check */ "").trim();
                const groups = v.match(COMMENT_RE);
                if (groups) {
                    hint = groups[1];
                }
            });

[loganfsmyth]

Ahh sorry, I misunderstood the issue then.

[aardito2]

For the example code given, adding a semicolon after the console.log in the first case results in the expected behavior:

'use strict'

const test = foo => {
  switch (foo) {
    case 'ok':
      return console.log('this is fine');

    /* istanbul ignore next */
    default:
      throw new Error('nope')
  }
}

test('ok')

I'm not sure what the expected output should be without the semicolon - it is ambiguous whether it should be treated as a leading comment on the next case or a trailing comment on the return.

Removing the final case shows the same behavior:

In:

switch (foo) {
  case 'ok':
    console.log('this is fine')
    /* istanbul ignore next */
}

Out:

'use strict';

switch (foo) {
  case 'ok':
    console.log('this is fine'
    /* istanbul ignore next */
    );}

In:

switch (foo) {
  case 'ok':
    console.log('this is fine');
    /* istanbul ignore next */
}

Out:

'use strict';

switch (foo) {
  case 'ok':
    console.log('this is fine');
  /* istanbul ignore next */
}

[bcoe]

@aardito2 it feels weird to me that the comment ends up scoped to inside the console.log's parenthesis -- this agrees with what I'm seeing in unit tests:

Just switching it to this:

  'use strict'

  const test = foo => {
    switch (foo) {
      // the bug discussed in #64, seems to only
      // crop up when a function is invoked before
      // a single line comment, hence Date.now().
      case 'ok':
        return Date.now

      /* istanbul ignore next */
      default:
        throw new Error('nope')
    }
  }

  test('ok')
  output = 'unknown'

also works; any thoughts about a workaround, that would solve both the bug you were addressing with your patch, and allow ignore to continue working?

[existentialism]

Fixed in #575