From 5605fc23f73c9fbc38c4ca2daecee38fb36e735c Mon Sep 17 00:00:00 2001 From: Yunfei Date: Mon, 26 Feb 2024 20:37:14 +0800 Subject: [PATCH] fix: rewrite `AssignmentTargetProperty` --- .../finalizer/impl_visit_mut_for_finalizer.rs | 61 +++++++++++++++++-- crates/rolldown/src/finalizer/rename.rs | 39 ++++++++++++ .../preserve-for-of-iterable/artifacts.snap | 44 +++++++++++++ .../preserve-for-of-iterable/iterables.js | 20 ++++++ .../rollup/preserve-for-of-iterable/loops.js | 19 ++++++ .../rollup/preserve-for-of-iterable/main.js | 7 +++ .../preserve-for-of-iterable/test.config.json | 7 +++ .../artifacts.snap | 24 ++++++++ .../simplify-with-destructuring/main.js | 9 +++ .../test.config.json | 7 +++ packages/rollup-tests/src/status.json | 2 +- packages/rollup-tests/src/status.md | 2 +- 12 files changed, 235 insertions(+), 6 deletions(-) create mode 100644 crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/artifacts.snap create mode 100644 crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/iterables.js create mode 100644 crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/loops.js create mode 100644 crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/main.js create mode 100644 crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/test.config.json create mode 100644 crates/rolldown/tests/fixtures/rollup/simplify-with-destructuring/artifacts.snap create mode 100644 crates/rolldown/tests/fixtures/rollup/simplify-with-destructuring/main.js create mode 100644 crates/rolldown/tests/fixtures/rollup/simplify-with-destructuring/test.config.json diff --git a/crates/rolldown/src/finalizer/impl_visit_mut_for_finalizer.rs b/crates/rolldown/src/finalizer/impl_visit_mut_for_finalizer.rs index 701147e15e6..f2804c3c78f 100644 --- a/crates/rolldown/src/finalizer/impl_visit_mut_for_finalizer.rs +++ b/crates/rolldown/src/finalizer/impl_visit_mut_for_finalizer.rs @@ -4,6 +4,7 @@ use oxc::{ ast::{self, SimpleAssignmentTarget}, VisitMut, }, + span::Span, }; use rolldown_common::{ExportsKind, ModuleId, SymbolRef, WrapKind}; use rolldown_oxc::{Dummy, ExpressionExt, IntoIn, StatementExt, TakeIn}; @@ -278,13 +279,14 @@ impl<'ast, 'me: 'ast> VisitMut<'ast> for Finalizer<'me, 'ast> { if ident.name != canonical_name { ident.name = canonical_name.clone(); } - *ident.symbol_id.get_mut() = None; + ident.symbol_id.get_mut().take(); } else { // Some `BindingIdentifier`s constructed by bundler don't have `SymbolId` and we just ignore them. } } fn visit_identifier_reference(&mut self, ident: &mut ast::IdentifierReference) { + // This ensure all `IdentifierReference`s are processed debug_assert!( self.is_global_identifier_reference(ident) || ident.reference_id.get().is_none(), "{} doesn't get processed in {}", @@ -341,11 +343,13 @@ impl<'ast, 'me: 'ast> VisitMut<'ast> for Finalizer<'me, 'ast> { fn visit_object_property(&mut self, prop: &mut ast::ObjectProperty<'ast>) { // Ensure `{ a }` would be rewritten to `{ a: a$1 }` instead of `{ a$1 }` - match prop.value { - ast::Expression::Identifier(ref id_ref) if prop.shorthand => { - if let Some(expr) = self.generate_finalized_expr_for_reference(id_ref, true) { + match &mut prop.value { + ast::Expression::Identifier(id_ref) if prop.shorthand => { + if let Some(expr) = self.generate_finalized_expr_for_reference(id_ref, false) { prop.value = expr; prop.shorthand = false; + } else { + id_ref.reference_id.get_mut().take(); } } _ => {} @@ -371,6 +375,7 @@ impl<'ast, 'me: 'ast> VisitMut<'ast> for Finalizer<'me, 'ast> { ident.name = canonical_name.clone(); prop.shorthand = false; } + ident.symbol_id.get_mut().take(); } } // Ensure `const { a = 1 } = ...;` will be rewritten to `const { a: a$1 = 1 } = ...` instead of `const { a$1 = 1 } = ...` @@ -388,6 +393,7 @@ impl<'ast, 'me: 'ast> VisitMut<'ast> for Finalizer<'me, 'ast> { ident.name = canonical_name.clone(); prop.shorthand = false; } + ident.symbol_id.get_mut().take(); } } _ => { @@ -438,6 +444,53 @@ impl<'ast, 'me: 'ast> VisitMut<'ast> for Finalizer<'me, 'ast> { } } + fn visit_assignment_target_property( + &mut self, + property: &mut ast::AssignmentTargetProperty<'ast>, + ) { + if let ast::AssignmentTargetProperty::AssignmentTargetPropertyIdentifier(prop) = property { + if let Some(target) = + self.generate_finalized_simple_assignment_target_for_reference(&prop.binding) + { + *property = ast::AssignmentTargetProperty::AssignmentTargetPropertyProperty( + ast::AssignmentTargetPropertyProperty { + name: ast::PropertyKey::Identifier( + self.snippet.id_name(prop.binding.name.clone()).into_in(self.alloc), + ), + binding: if let Some(init) = prop.init.take() { + ast::AssignmentTargetMaybeDefault::AssignmentTargetWithDefault( + ast::AssignmentTargetWithDefault { + binding: ast::AssignmentTarget::SimpleAssignmentTarget(target), + init, + span: Span::default(), + } + .into_in(self.alloc), + ) + } else { + ast::AssignmentTargetMaybeDefault::AssignmentTarget( + ast::AssignmentTarget::SimpleAssignmentTarget(target), + ) + }, + span: Span::default(), + } + .into_in(self.alloc), + ); + } else { + prop.binding.reference_id.get_mut().take(); + } + } + + // visit children + match property { + ast::AssignmentTargetProperty::AssignmentTargetPropertyIdentifier(ident) => { + self.visit_assignment_target_property_identifier(ident); + } + ast::AssignmentTargetProperty::AssignmentTargetPropertyProperty(prop) => { + self.visit_assignment_target_property_property(prop); + } + } + } + fn visit_simple_assignment_target(&mut self, target: &mut SimpleAssignmentTarget<'ast>) { self.rewrite_simple_assignment_target(target); diff --git a/crates/rolldown/src/finalizer/rename.rs b/crates/rolldown/src/finalizer/rename.rs index e82ad70d749..d0fa8b97cce 100644 --- a/crates/rolldown/src/finalizer/rename.rs +++ b/crates/rolldown/src/finalizer/rename.rs @@ -53,6 +53,45 @@ where None } + /// return `None` if + /// - the reference is for a global variable/the reference doesn't have a `SymbolId` + /// - the reference doesn't have a `ReferenceId` + /// - the canonical name is the same as the original name + pub fn generate_finalized_simple_assignment_target_for_reference( + &self, + id_ref: &IdentifierReference, + ) -> Option> { + // Some `IdentifierReference`s constructed by bundler don't have `ReferenceId` and we just ignore them. + let reference_id = id_ref.reference_id.get()?; + + // we will hit this branch if the reference points to a global variable + let symbol_id = self.scope.symbol_id_for(reference_id)?; + + let symbol_ref: SymbolRef = (self.ctx.id, symbol_id).into(); + let canonical_ref = self.ctx.symbols.par_canonical_ref_for(symbol_ref); + let symbol = self.ctx.symbols.get(canonical_ref); + + if let Some(ns_alias) = &symbol.namespace_alias { + let canonical_ns_name = self.canonical_name_for(ns_alias.namespace_ref); + let prop_name = &ns_alias.property_name; + let access_expr = + self.snippet.literal_prop_access_member_expr(canonical_ns_name.clone(), prop_name.clone()); + + return Some(ast::SimpleAssignmentTarget::MemberAssignmentTarget( + access_expr.into_in(self.alloc), + )); + } + + let canonical_name = self.canonical_name_for(canonical_ref); + if id_ref.name != canonical_name { + return Some(ast::SimpleAssignmentTarget::AssignmentTargetIdentifier( + self.snippet.id_ref(canonical_name.clone()).into_in(self.alloc), + )); + } + + None + } + pub fn try_rewrite_identifier_reference_expr( &mut self, expr: &mut ast::Expression<'ast>, diff --git a/crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/artifacts.snap b/crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/artifacts.snap new file mode 100644 index 00000000000..13827953ffd --- /dev/null +++ b/crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/artifacts.snap @@ -0,0 +1,44 @@ +--- +source: crates/rolldown/tests/common/case.rs +expression: content +input_file: crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable +--- +# Assets + +## main.mjs + +```js +import { default as assert } from "assert"; + +// iterables.js +let dirty; +const zeroToFive = { + [Symbol.iterator](){ + return { + current:0, + last:5, + next(){ + const ret = this.current < this.last ? { + done:false, + value:this.current++ + } : { + done:true + }; + dirty = this.current; + return ret; + } + }; + } +}; + +// loops.js +const iterate = iterable => { + for (const value of iterable) { + } +}; + +// main.js +assert.equal(dirty, undefined); +iterate(zeroToFive); +assert.equal(dirty, 5); +``` diff --git a/crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/iterables.js b/crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/iterables.js new file mode 100644 index 00000000000..a2afb6cd272 --- /dev/null +++ b/crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/iterables.js @@ -0,0 +1,20 @@ +export let dirty; + +export const zeroToFive = { + [Symbol.iterator]() { + return { + current: 0, + last: 5, + next() { + const ret = this.current < this.last + ? { done: false, value: this.current++ } + : { done: true }; + + // assert later + dirty = this.current; + + return ret; + } + }; + } +}; diff --git a/crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/loops.js b/crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/loops.js new file mode 100644 index 00000000000..58031387846 --- /dev/null +++ b/crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/loops.js @@ -0,0 +1,19 @@ +export const awaitable = async (iterable) => { + for await (const value of iterable) { + } +} + +// This is equivalent to the above 'awaitable' function. +export const equivalent = async (iterable) => { + const iterator = iterable[Symbol.asyncIterator]() + let { done } = await iterator.next() + while (!done) { + ({ done } = await iterator.next()) + } + +} + +export const iterate = iterable => { + for (const value of iterable) { + } +} diff --git a/crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/main.js b/crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/main.js new file mode 100644 index 00000000000..f5ec0015249 --- /dev/null +++ b/crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/main.js @@ -0,0 +1,7 @@ +import assert from 'assert'; +import { zeroToFive, dirty } from './iterables'; +import { iterate } from './loops'; + +assert.equal(dirty, undefined); +iterate(zeroToFive); +assert.equal(dirty, 5); diff --git a/crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/test.config.json b/crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/test.config.json new file mode 100644 index 00000000000..0e36e4fe62d --- /dev/null +++ b/crates/rolldown/tests/fixtures/rollup/preserve-for-of-iterable/test.config.json @@ -0,0 +1,7 @@ +{ + "input": { + "external": [ + "assert" + ] + } +} \ No newline at end of file diff --git a/crates/rolldown/tests/fixtures/rollup/simplify-with-destructuring/artifacts.snap b/crates/rolldown/tests/fixtures/rollup/simplify-with-destructuring/artifacts.snap new file mode 100644 index 00000000000..22b6b958a54 --- /dev/null +++ b/crates/rolldown/tests/fixtures/rollup/simplify-with-destructuring/artifacts.snap @@ -0,0 +1,24 @@ +--- +source: crates/rolldown/tests/common/case.rs +expression: content +input_file: crates/rolldown/tests/fixtures/rollup/simplify-with-destructuring +--- +# Assets + +## main.mjs + +```js +import { default as assert } from "assert"; + +// main.js +let foo, unused; +null,{foo} = { + foo:'bar' +}; +assert.strictEqual(foo, 'bar'); +const assign = () => unused = {foo} = { + foo:'baz' +}; +assign(); +assert.strictEqual(foo, 'baz'); +``` diff --git a/crates/rolldown/tests/fixtures/rollup/simplify-with-destructuring/main.js b/crates/rolldown/tests/fixtures/rollup/simplify-with-destructuring/main.js new file mode 100644 index 00000000000..c09471d5027 --- /dev/null +++ b/crates/rolldown/tests/fixtures/rollup/simplify-with-destructuring/main.js @@ -0,0 +1,9 @@ +import assert from 'assert'; + +let foo, unused; +null, { foo } = { foo: 'bar' }; +assert.strictEqual(foo, 'bar'); + +const assign = () => unused = { foo } = { foo: 'baz' }; +assign(); +assert.strictEqual(foo, 'baz'); diff --git a/crates/rolldown/tests/fixtures/rollup/simplify-with-destructuring/test.config.json b/crates/rolldown/tests/fixtures/rollup/simplify-with-destructuring/test.config.json new file mode 100644 index 00000000000..0e36e4fe62d --- /dev/null +++ b/crates/rolldown/tests/fixtures/rollup/simplify-with-destructuring/test.config.json @@ -0,0 +1,7 @@ +{ + "input": { + "external": [ + "assert" + ] + } +} \ No newline at end of file diff --git a/packages/rollup-tests/src/status.json b/packages/rollup-tests/src/status.json index e0243bcf2c0..c583a853892 100644 --- a/packages/rollup-tests/src/status.json +++ b/packages/rollup-tests/src/status.json @@ -1,6 +1,6 @@ { "failed": 0, - "skipFailed": 655, + "skipFailed": 657, "skipped": 0, "passed": 235 } \ No newline at end of file diff --git a/packages/rollup-tests/src/status.md b/packages/rollup-tests/src/status.md index 45668b5c678..1f10f45b79d 100644 --- a/packages/rollup-tests/src/status.md +++ b/packages/rollup-tests/src/status.md @@ -1,6 +1,6 @@ | | number | |----| ---- | | failed | 0| -| skipFailed | 655| +| skipFailed | 657| | skipped | 0| | passed | 235|