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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bug]: @babel/parser@7.14.8 breaks IstanbulJS skip logic #13750
Comments
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." |
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. |
@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}
|
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:
We can now add that
As you can see, the comment doesn't "touch" the second
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:
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] === 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 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 |
@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. |
Behavior of I'm a little confused by what's happening with 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 I think the problem is similar where it's grouping the logical expression slightly differently, I can likely work around this if need be. |
if(/* istanbul ignore next */ (args[0] === 2 || args[0] === 3)) {} In this example the LogicalExpression starts with When 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. We could allow leading/trailing comments to span across parenthesis, but it will complicate current handling logic and kill performance. Although we have (/* 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 |
@JLHwung I played with 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. |
馃捇
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:
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
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.
The text was updated successfully, but these errors were encountered: