fix: Ensure correct scope references after traversal

[leebyron]

(Replaces https://github.com/istanbuljs/istanbuljs/pull/283)

This fixes a bug that can be seen when istanbul coverage is enabled for a babel@7 project which uses reference replacement, such as babel-plugin-lodash orbabel-plugin-ramda.

The root issue is that plugins like these assume that scope references will always be Identifiers but Istanbul may replace an identifier with a SequenceExpression. In babel@7 this replacement overwrites the existing scope reference and later plugin scope reference replacement is at best unsafe and at worst may result in an invalid AST.

This seems like a pretty unfortunate situation, and ideally Babel automatically maintains these scope references during transforms or the many packages using scope would account for this situation. The pattern I see in other first-class babel plugins prefer to re-crawl scope after a transform which can affect identifiers (example: https://github.com/babel/minify/blob/master/packages/babel-plugin-minify-dead-code-elimination/src/index.js#L969)

To follow that example, I'm proposing this fix that crawls the scope after the path traversal - ensuring any subsequent references to scope get correct references.

I tested this in a project that used istanbul via jest along with babel-plugin-lodash that suffered from this issue and found that with this patch the issue was resolved and the inspected compiled output looked correct.

I'm also including two tests in this repo to illustrate the issue directly. One test illustrates it directly with the Babel plugin API and another with a popular babel plugin which uses this API and suffers from the issue. Both tests fail in master and pass with the fix applied.

[coreyfarrell]

@leebyron thank you for submitting the failing test followed by the fix. So I'm a little unclear, are you suggesting that the fix should go into babel-plugin-istanbul only? This would assume that istanbul-lib-instrument would not be directly used when combining with other babel plugins - which does make sense to me.

[coveralls]

Coverage increased (+0.08%) to 97.143% when pulling 57f63b0f9e784d667fb13051d2b10d63acaacd56 on leebyron:test-scope into de543208fa6c2ba503f7a88f6e6ec8d7b2577fe5 on istanbuljs:master.

[coveralls]

Coverage increased (+0.08%) to 97.143% when pulling 57f63b0f9e784d667fb13051d2b10d63acaacd56 on leebyron:test-scope into de543208fa6c2ba503f7a88f6e6ec8d7b2577fe5 on istanbuljs:master.

[coveralls]

Coverage increased (+0.08%) to 97.143% when pulling 57f63b0f9e784d667fb13051d2b10d63acaacd56 on leebyron:test-scope into de543208fa6c2ba503f7a88f6e6ec8d7b2577fe5 on istanbuljs:master.

[coveralls]

Coverage increased (+0.08%) to 97.143% when pulling 57f63b0f9e784d667fb13051d2b10d63acaacd56 on leebyron:test-scope into de543208fa6c2ba503f7a88f6e6ec8d7b2577fe5 on istanbuljs:master.

[coveralls]

Coverage increased (+0.08%) to 97.143% when pulling 57f63b0f9e784d667fb13051d2b10d63acaacd56 on leebyron:test-scope into de543208fa6c2ba503f7a88f6e6ec8d7b2577fe5 on istanbuljs:master.

[coveralls]

Coverage increased (+0.08%) to 97.143% when pulling 57f63b0f9e784d667fb13051d2b10d63acaacd56 on leebyron:test-scope into de543208fa6c2ba503f7a88f6e6ec8d7b2577fe5 on istanbuljs:master.

[leebyron]

Hey @coreyfarrell you're fast on the comment :) I just closed the istanbul-lib-instrument PR as soon as I saw the tests passing here. On your suggestion to include more complete tests I realized that not only was this repo the appropriate place for those tests, it's probably also a more appropriate place for the fix since the problem is really using the istanbul transform in tandem with other babel transforms, which is likely to only be the case when using it as a babel plugin.

It also is a minor performance win in that it only recrawls the scope once, while the previous PR may have crawled multiple times as it encountered different untransformed subtrees.

[coreyfarrell]

Thanks for quickly providing the failing test-case for this. Unfortunately I won't be able to review this immediately. Obviously not much to review for the implementation but I want to review the tests, dig a little deeper.

[leebyron]

Understandable, no immediate rush. Some notes from my dig in:

I spent more time than I'd like to admit digging through Babel's scope and traversal implementation trying to gain a better understanding of the issue. My understanding is that Babel builds a scope tree once during parse which then tracks references to NodePaths. Since Babel transforms apply via mutation, these references are mostly maintained as transforms are applied in and around them, however the issue occurs when the reference itself is mutated, which Istanbul may do when replacing an expression (which could be a reference Identifier) with a SequenceExpression to add coverage tracking. I think that ideally Babel should handle this automatically, removing and replacing the reference or at least determining for itself that the Scope is corrupt and re-crawling, but I found no back references from those nodes, so it doesn't seem possible (without major surgery) to have Babel do this.

I stumbled onto this suggested fix by digging into what other Babel plugins do to solve this same issue, and came across a few just by grepping for other use of the API where it's solving a similar problem.

babel-plugin-minify-dead-code-elimination babel-plugin-transform-react-remove-prop-types babel-plugin-minify-mangle-names/lib/index.js @babel/plugin-transform-parameters/lib/index.js

[coreyfarrell]

Thanks for troubleshooting and contributing this.

[coreyfarrell]

@leebyron sorry for the delay, this is now released to npm under the next tag. This will be promoted to latest once we're done testing the new version of nyc.

[pi0]

Hey. This PR is introducing a regression described in #201.