Add side effect detection visitor #9202
Conversation
Benchmark ResultsKitchen Sink ✅
Timings
Cold Bundles
Cached BundlesNo bundle changes detected. React HackerNews ✅
Timings
Cold BundlesNo bundle changes detected. Cached Bundles
AtlasKit Editor ✅
Timings
Cold Bundles
Cached Bundles
Three.js ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. |
| import './some-file'; | ||
| "#; | ||
|
|
||
| assert_eq!(check_side_effects(code, vec![]), true); |
There was a problem hiding this comment.
I wasn't sure about this scenario. Should this be considered a side effect or not?
There was a problem hiding this comment.
Unless I'm missing something, I think you can't tell unless you know what happens in ./some-file? Or does it not matter because import foo from './some-file' would cause ./some-file to execute just the same?
There was a problem hiding this comment.
What does Webpack do (and does it traverse imports in the analysis)?
I wouldn't expect there to be a difference regarding side-effects between import "some-file" and import "foo" from "some-file"; export function bar(){ return foo; } because they both might have side-effects in the imports (but again, maybe Webpack has run into this before)
(At least so far with explicit side-effects declarations, the meaning was "If you don't use any values from an imported side-effect-free asset, then the asset and its whole subgraph can be excluded". So this is a very valid concern)
There was a problem hiding this comment.
Webpack doesn't mark this as a side effect. I agree that there shouldn't be a difference between the two. My only concern was writing an import in this way is like the user declaring there intent that they want this import loaded now.
There was a problem hiding this comment.
I can't talk to the correctness of the Rust code (except where I commented on how to structure inline tests), but the concept and test coverage seems good to me - though obviously there could be other scenarios I haven't thought of.
I assume you looked at the test suite for the Webpack code to see if they had any fun edge cases that you might not have covered?
| @@ -3098,7 +3098,8 @@ describe('cache', function () { | |||
| async setup() { | |||
| await overlayFS.writeFile( | |||
| path.join(inputDir, 'src/index.js'), | |||
| `import "foo";`, | |||
| `import foo from "foo"; | |||
There was a problem hiding this comment.
I guess this change is because import "foo" is considered that it might have side-effects which breaks some of the other assertions in the tests?
There was a problem hiding this comment.
The foo asset was being stripped from the output completely as it was never used and the new detection was marking it as safe to be tree shaken. Simply logging it was enough to mark it as used.
There was a problem hiding this comment.
Might be easier to make foo itself have a side effect? Right now foo.js just has module.exports = 2 but if you add some side effect in there you won't need to update any of the tests. Btw, there is a sideEffectNoop global function provided by the test utils that you can call instead of console.log as well. That would clean up the test output if we actually run the resulting bundle.
| if let Some(expr) = node.as_expr() { | ||
| match expr { | ||
| Expr::Member(member_expr) => { | ||
| if match_member_expr(&member_expr, vec!["module", "exports"], &self.decls) |
There was a problem hiding this comment.
Is this basically saying that top level assignments to module.<exports|hot|require> are safe?
.. and the below is that only access of previously declared variables or imports are safe?
There was a problem hiding this comment.
Is this basically saying that top level assignments to module.<exports|hot|require> are safe?
Yes
and the below is that only access of previously declared variables or imports are safe?
Not access but assignment. Also I only allow assignment of locals declared in the file. I don't allow assignment to imports as that would mean the current asset is mutating an import in the top level scope which is a side effect.
| import './some-file'; | ||
| "#; | ||
|
|
||
| assert_eq!(check_side_effects(code, vec![]), true); |
There was a problem hiding this comment.
Unless I'm missing something, I think you can't tell unless you know what happens in ./some-file? Or does it not matter because import foo from './some-file' would cause ./some-file to execute just the same?
|
@marcins On the webpack tests, I actually couldn't find any tests for their logic. 🤷 |
…cel into side-effect-detection
Co-authored-by: Jasper De Moor <jasperdemoor@gmail.com>
|
@mattcompiles We discussed with @mischnic in the meeting today. Sounds like we will need a new value/mode for |
↪️ Pull Request
This is a first pass on a side effect detecting visitor that can upgrade assets in an unknown side effect state to
sideEffects: falsein some cases.Running it on a very large code base, it detected 33% of assets as side effect free. I haven't found any false positives but feedback and more test coverage should give more confidence.
Major changes in this PR
hasResolvedSideEffects) to Assets to be able to distinguish betweensideEffects: trueandsideEffects: undefined💻 Examples
🚨 Test instructions
✔️ PR Todo