[Bug]: @babel/parser@7.14.8 breaks IstanbulJS skip logic

[bcoe]

💻

• Would you like to work on a fix?

How are you using Babel?

Programmatic API (babel.transform, babel.parse)

Input code

There are unit tests in Istanbul that test the following syntax, and ensure that the else statement is removed from coverage tracking when an ignore comment is added:

  if (args[0] === 1) {
    output = '1';
  } /* istanbul ignore else */ else if (args[0] === 2) {
    output = '2';
  } else if (args[0] === 3) {
    output = '3';
  } else {
    output = 'other';
  }

Configuration file name

No response

Configuration

No response

Current and expected behavior

Current behavior

IstanbulJS has stopped parsing ignore comments for else blocks.

Expected behavior

IstanbulJS continues handling ignore comments.

Environment

• npm i @babel/parser@>=7.14.8

Possible solution

No response

Additional context

My hunch is that the refactor here changed how comments attach to else nodes, resulting in Istanbul's visitor no longer working.

The code that visits comments and skips the subsequent node can be found here.

I'm betting the visitor needs to be updated slightly to match @JLHwung's overhaul, but could use some help getting on the right track.

[babel-bot]

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."

[nicolo-ribaudo]

Technically there isn't a "correct" way to attach comments, but I'm marking this as a regression for now since IstanbulJS a big Babel consumer.

[bcoe]

@nicolo-ribaudo I updated the tests to pass with the new behavior, with the old behavior in comments.

Reading the prior behavior, it seems that it was expected that coverage would stop being counted from the else statement forward (the old behavior seems slightly weird to me too, I don't understand why it would be expected that line 3 and 4 are in the coverage table):

name: ignore chained else
code: |
  if (args[0] === 1) {
    output = '1';
  } /* istanbul ignore else */ else if (args[0] === 2) {
    output = '2';
  } else if (args[0] === 3) {
    output = '3';
  } else {
    output = 'other';
  }
tests:
  - name: '1'
    args: [1]
    out: '1'
    # Refs: https://github.com/babel/babel/issues/13750
    # lines: {'1': 1, '2': 1, '3': 0, '4': 0}
    lines: {'1': 1, '2': 1, '3': 0, '4': 0, '5': 0, '6': 0, '8': 0}
    # Refs: https://github.com/babel/babel/issues/13750
    # branches: {'0': [1, 0], 1: [0]}
    branches: {'0': [1, 0], 1: [0, 0], 2: [0, 0]}
    # Refs: https://github.com/babel/babel/issues/13750
    # statements: {'0': 1, '1': 1, '2': 0, '3': 0}
    statements: {'0': 1, '1': 1, '2': 0, '3': 0, '4': 0, '5': 0, '6': 0}
  - name: '2'
    args: [2]
    out: '2'
    # Refs: https://github.com/babel/babel/issues/13750
    # lines: {'1': 1, '2': 0, '3': 1, '4': 1}
    lines: {'1': 1, '2': 0, '3': 1, '4': 1, '5': 0, '6': 0, '8': 0}
    # Refs: https://github.com/babel/babel/issues/13750
    # branches: {'0': [0, 1], 1: [1]}
    branches: {'0': [0, 1], 1: [1, 0], 2: [0, 0]}
    # Refs: https://github.com/babel/babel/issues/13750
    # statements: {'0': 1, '1': 0, '2': 1, '3': 1}
    statements: {'0': 1, '1': 0, '2': 1, '3': 1, '4': 0, '5': 0, '6': 0}
  - name: '3'
    args: [3]
    out: '3'
    # Refs: https://github.com/babel/babel/issues/13750
    # lines: {'1': 1, '2': 0, '3': 1, '4': 0}
    lines: {'1': 1, '2': 0, '3': 1, '4': 0, '5': 1, '6': 1, '8': 0}
    # Refs: https://github.com/babel/babel/issues/13750
    # branches: {'0': [0, 1], 1: [0]}
    branches: {'0': [0, 1], 1: [0, 1], 2: [1, 0]}
    # Refs: https://github.com/babel/babel/issues/13750
    # statements: {'0': 1, '1': 0, '2': 1, '3': 0}
    statements: {'0': 1, '1': 0, '2': 1, '3': 0, '4': 1, '5': 1, '6': 0}

and:

name: ignore in complex logical expression
code: >
  if (args[0] === 1  || /* istanbul ignore next */ (args[0] === 2  || args[0] === 3)) {
     output = args[0] + 10;
  } else {
     output = 20;
  }
tests:
  - args: [1]
    out: 11
    lines: {'1': 1, '2': 1, '4': 0}
    # Refs: https://github.com/babel/babel/issues/13750
    # branches: {'0': [1, 0], '1': [1]}
    branches: {'0': [1, 0], '1': [1, 0, 0]}
    statements: {'0': 1, '1': 1, '2': 0}

• lines: representing lines in the source.
• branches: representing distinct logical paths in if, ||, etc.
• statements: e.g., output = '1';, they are similar to lines but will have a start and end position associated with them.

[nicolo-ribaudo]

I'm inclined to consider this as a bugfix and not as a regression.

Consider this input code:

if (A) { B; } else if (C) { D; } else { E; }

We can mark some AST nodes relevant to this bug:

if (A) { B;     } else if (C) { D; } else { E; }
       ^^^^^^^^^^      ^^^^^^^^^^^^^^^^^^^^^^^^^
       First if's       Second if statement, or
       consequent        first if's alternate

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                First if statement

We can now add that /* istanbul ignore else */ comment to our code:

if (A) { B;     } /* comment */ else if (C) { D; } else { E; }
       ^^^^^^^^^^                    ^^^^^^^^^^^^^^^^^^^^^^^^^
       First if's                     Second if statement, or
       consequent                      first if's alternate

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                First if statement

As you can see, the comment doesn't "touch" the second if statement. We could attach it to the AST in two places:

• as a trailing comment of First if's consequent
• as an inner comment of First if statement

We give precedence to trailing/leading comments, since it's a more specific indication than a generic "inner comment". You can verify on ASTExplorer that in your example the comment is attached as a trailing comment of that block statement.

For some reason, pre-7.14.8 Babel was attaching that comment as a leading comment of Second if statement, as if you had this input code:

if (A) { B;     } else /* comment */ if (C) { D; } else { E; }
       ^^^^^^^^^^                    ^^^^^^^^^^^^^^^^^^^^^^^^^
       First if's                     Second if statement, or
       consequent                      first if's alternate

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                First if statement

or

if (args[0] === 1) {
  output = '1';
} else /* istanbul ignore else */ if (args[0] === 2) {
  output = '2';
} else if (args[0] === 3) {
  output = '3';
} else {
  output = 'other';
}

I admit that your fixtures are too cryptic for me to understand (or maybe I could, but it's 11pm here and I'm tired 😬), but looking ad the AST and at your visitor I think that pre-7.14.8 the ignored lines were only these:

if (args[0] === 1) {
  output = '1';
} else if (args[0] === 2) {
  output = '2';
} else if (args[0] === 3) { // <-- ignored
  output = '3';             // <-- ignored
} else {                    // <-- ignored
  output = 'other';         // <-- ignored
}                           // <-- ignored

if it's true (if it's not, please correct me), as an InstanbulJS user I would find the old behavior surprising: why isn't it also also ignoring the if (args[0] === 2) branch? The fixed comment attachment forces me to write either

if (args[0] === 1) {
  output = '1';
} else /* istanbul ignore else */ if (args[0] === 2) {
  output = '2';
} else if (args[0] === 3) {
  output = '3';
} else {
  output = 'other';
}

or

/* istanbul ignore else */
if (args[0] === 1) {
  output = '1';
} else if (args[0] === 2) {
  output = '2';
} else if (args[0] === 3) {
  output = '3';
} else {
  output = 'other';
}

making it more clear (imo) which if statement's else branch should be ignored.

---

I'm now looking at the "ignore in complex logical expression" test. It looks more like a bug (the comment is attached as an inner comment of the first || expression, rather than as a leading comment of the second one), but @JLHwung might explain if it's expected or not. I suspect that it has to do with parens, but I still find it surprising. As a workaround to restore the old behavior, you can enable the createParenthesizedExpressions parser option.

[bcoe]

@nicolo-ribaudo you're right, testing this behavior things to work as you imply:

name: ignore chained else
code: |
  /* istanbul ignore else */
  if (args[0] === 1) {
    output = '1';
  } else if (args[0] === 2) {
    output = '2';
  } else if (args[0] === 3) {
    output = '3';
  } else {
    output = 'other';
  }
tests:
  - name: '1'
    args: [1]
    out: '1'
    lines: {'2': 1, '3': 1}
    branches: {'0': [1]}
    statements: {'0': 1, '1': 1}

Also, to get the old behavior you can just refactor to this:

  if (args[0] === 1) {
    output = '1';
  } else /* istanbul ignore else */ if (args[0] === 2) {
    output = '2';
  } else if (args[0] === 3) {
    output = '3';
  } else {
    output = 'other';
  }

Edit: this seems quite reasonable to me, thanks for the help debugging.

[bcoe]

Behavior of if/else seems reasonable 👍

I'm a little confused by what's happening with ignore next for a logical expression:

name: ignore in complex logical expression
code: >
  if (args[0] === 1  || /* istanbul ignore next */ (args[0] === 2  || args[0] === 3)) {
     output = args[0] + 10;
  } else {
     output = 20;
  }
tests:
  - args: [1]
    out: 11
    lines: {'1': 1, '2': 1, '4': 0}
    # Refs: https://github.com/babel/babel/issues/13750
    # branches: {'0': [1, 0], '1': [1]}
    branches: {'0': [1, 0], '1': [1, 0, 0]}
    statements: {'0': 1, '1': 1, '2': 0}

In older versions of Istanbul I believe /* istanbul ignore next */ (args[0] === 2 || args[0] === 3)) appropriately ignored the logical expression in the parenthesis.

I think the problem is similar where it's grouping the logical expression slightly differently, I can likely work around this if need be.

[JLHwung]

if(/* istanbul ignore next */ (args[0] === 2 || args[0] === 3)) {}

In this example the LogicalExpression starts with arg, the left parenthesis separates the comment from LogicalExpression, so the comment does not attach to the LogicalExpression, and because no AST node starts with (, it is then classified as an inner comment of upper AST structure, in this case, the IfStatement.

When createParenthesizedExpressions is enabled, a ParethesizedExpression starting with ( is created, the comment now attaches to the ParethesizedExpression as a leading comment.

If you want the comment attaching to the LogicalExpression, the comment should immediately come before the start of LogicalExpression. Also note that when multiple AST node follow a comment, it will be attached to the uppermost node:

if((/* istanbul ignore next */ args[0] === 2 || args[0] === 3)) {} // ignore LogicalExpression

if((/* istanbul ignore next */ args[0] === 2) || args[0] === 3) {} // ignore the first BinaryExpression

Before 7.14.8, a non-whitespace token (i.e. () can appear between a leading comment and the attached AST node. After 7.14.8, only comment whitespace can exist between a leading comment and the attached AST node, with exception of trailing commas (a bit messy to track)

We could allow leading/trailing comments to span across parenthesis, but it will complicate current handling logic and kill performance. Although we have extra.parenStart, it is not sufficient to support comments attaching spanning because extra.parenStart only records the outermost parenthesis:

(/* 1 */(/* 2 */(/* 3* /a = 1)))
// Currently only `/* 3 */` attached to `a = 1`
// the other two comments are innerComments of ExpressionStatement

I lean to use the createParenthesizedExpressions option if you demand the comment should be attached to a parenthesized expression. After all, the comment attaching relies on the AST layout: The more granular the AST structure is, the more likely a non-whitespace token can be represented by an AST node, to which a comment can attach without being an inner comment of upper AST node.

[bcoe]

@JLHwung I played with createParenthesizedExpressions but it doesn't quite give the behavior I'd expect (I'd likely need to fiddle with the logic for applying comments to the AST). However, I'm a bit worried that adding these additional branches to the AST could hurt performance.

I played more, and I think your suggestion of moving the comment to inside the parenthesis is reasonable:

name: ignore in complex logical expression
code: >
  if (args[0] === 1 || (/* istanbul ignore next */ args[0] === 2  || args[0] === 3)) {
     output = args[0] + 10;
  } else {
     output = 20;
  }
tests:
  - args: [1]
    out: 11
    lines: {'1': 1, '2': 1, '4': 0}
    branches: {'0': [1, 0], '1': [1]}
    statements: {'0': 1, '1': 1, '2': 0}

☝️ it's not too much trouble to have people move their comment as demonstrated above, and it solves problem of this new behavior without changing the parse too much.

Thanks for your help investigating.