-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #1786: use _interopRequireWildcard
for import when required by an export
#1829
Conversation
I have fixed the merge and the test; everything is passing now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Thanks, I should be able to push the requested changes soon.
Regarding merging the visitors, it seemed very inconvenient to constantly
borrow and dereference from the `RC<RefCell<>>`. This is why I opted for
using two visitors.
Is there a good way to avoid this? Can I change `chain()` to accept a
smaller visitor factory, and then teach chain to invoke the factory and use
the value it returns? This would avoid the messy borrowing refactor and
also avoid creating new visitors.
…On Fri, Jun 18, 2021, 10:42 PM 강동윤 ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Thanks!
------------------------------
In src/builder.rs
<#1829 (comment)>:
> @@ -177,18 +180,20 @@ impl<'a, 'b, P: swc_ecma_visit::Fold> PassBuilder<'a, 'b, P> {
))
};
+ let scope_instance: Scope = Default::default();
Can you changed this to Scope::default()?
------------------------------
In ecmascript/transforms/module/tests/common_js.rs
<#1829 (comment)>:
> @@ -4782,6 +4787,60 @@ test!(
console.log('aFunc: ', (0, _testlibrary).aFunc(1, 2));
"
);
+test!(
+ syntax(),
+ |_| {
+ let scope_instance: Scope = Default::default();
Please change this to Scope::default()
------------------------------
In ecmascript/transforms/module/src/util.rs
<#1829 (comment)>:
> @@ -78,7 +78,7 @@ impl Default for Lazy {
}
#[derive(Clone, Default)]
-pub(super) struct Scope {
If possible, please hide all fields with pub(crate) (on fields)
------------------------------
In ecmascript/transforms/module/src/umd.rs
<#1829 (comment)>:
> + let mut scope_ref_mut = self.scope.borrow_mut();
+ let scope = scope_ref_mut.deref_mut();
+ let mut umd = UmdWorker {
+ cm: &mut self.cm,
+ root_mark: self.root_mark,
+ in_top_level: self.in_top_level,
+ config: &mut self.config,
+ scope,
+ exports: &mut self.exports,
+ };
+ umd.fold_module(module)
+ }
+}
+
+struct UmdWorker<'a> {
+ cm: &'a mut Lrc<SourceMap>,
&mut is not necessary in &mut Arc<T> / &mut Rc<T>
------------------------------
In ecmascript/transforms/module/src/amd.rs
<#1829 (comment)>:
> exports: Exports,
}
+impl Fold for Amd {
Can you merge Amd and AmdWorker?
Creating a new visitor is really bad for binary size.
------------------------------
In ecmascript/transforms/module/src/common_js.rs
<#1829 (comment)>:
> in_top_level: bool,
}
impl Fold for CommonJs {
noop_fold_type!();
+ fn fold_module(&mut self, module: Module) -> Module {
Can you merge CommonJs and CommonJsWorker?
Creating a new visitor is really bad for binary size.
------------------------------
In ecmascript/transforms/module/src/import_analysis.rs
<#1829 (comment)>:
> }
-impl Fold for ImportAnalyzer {
+impl Fold for ImportAnalyzerWorker<'_> {
noop_fold_type!();
fn fold_module(&mut self, module: Module) -> Module {
Can you merge visitors?
------------------------------
In ecmascript/transforms/module/src/umd.rs
<#1829 (comment)>:
> exports: Exports,
}
impl Fold for Umd {
noop_fold_type!();
+ fn fold_module(&mut self, module: Module) -> Module {
Can you merge visitors?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1829 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAC35OF5NMT5EIVS2KXCIWLTTP7YHANCNFSM4633C44A>
.
|
No. As far as I know, runtime-borrows are cumbersome because they should be (in rust).
I think you should create an alternative maco like But this is a complex problem. You want to pass down This is because you can't pass down |
Yes I should be able to resolve these issues this weekend, today or
tomorrow.
…On Sat, Jun 19, 2021 at 11:09 AM 강동윤 ***@***.***> wrote:
I think this will conflict with #1834
<#1834>
I'll postpone #1834 <#1834> if you
are going to resolve issues soon.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1829 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAC35OFUEHGTNZK6TBY7NGDTTSXMLANCNFSM4633C44A>
.
|
Great, thanks! |
I was so obsessed with learning Rust, I attempted something slightly different than merging the visitors. I implemented a This allows This change adds I noticed that the sourcemap tests require node 12 or older. Node 14 and 16 changed their stack trace output. Should I make a PR adding this requirement to CONTRIBUTING.md? |
Sorry, I forgot to change the other |
I'm not sure about such change. |
I looked at the code of amd pass, and it's two visitor and it's not related to |
I profiled the size on mac os. master:
It's 21045 KB pr:
It's 57182 KB More than triple. I also found that this pr makes compilation much slower (14m -> 27m) |
Yeah I accidentally forgot to change some of the visitors, I'm still working on that. |
.entry(($i.sym.clone(), $i.span.ctxt())) | ||
}; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This macro began raising an error that I did not understand, and I could not figure out how to fix it unless I stopped using the macro. I think the error was caused by folder.scope_mut()
returning a RefMut<Scope>
.
@@ -20,8 +21,8 @@ use swc_ecma_visit::{Fold, FoldWith, VisitWith}; | |||
|
|||
pub(super) trait ModulePass: Fold { | |||
fn config(&self) -> &Config; | |||
fn scope(&self) -> &Scope; | |||
fn scope_mut(&mut self) -> &mut Scope; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, this change is necessary because we cannot return a &Scope
without also passing ownership of the Ref
that it comes from.
I've removed all the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGMT. Thanks a lot!
}); | ||
drop(scope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without it I get this error:
error[E0502]: cannot borrow `*self` as mutable because it is also borrowed as immutable
--> ecmascript/transforms/module/src/amd.rs:361:54
|
308 | let mut scope_ref_mut = self.scope.borrow_mut();
| ---------- immutable borrow occurs here
...
361 | None => Box::new(Expr::Ident(orig.clone()).fold_with(self)),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
...
409 | }
| - immutable borrow might be used here, when `scope_ref_mut` is dropped and runs the destructor for type `RefMut<'_, Scope>`
As far as I understand, scope
and scope_ref_mut
constitute an immutable borrow of self
which will not be dropped till near the bottom of this function. This conflicts with mutable borrows made by fold_with(self)
. But if I drop scope
and scope_ref_mut
earlier, there is no conflict.
.import_types | ||
.entry(src.value.clone()) | ||
.or_insert(false); | ||
} | ||
} | ||
|
||
drop(scope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
} | ||
_ => {} | ||
} | ||
|
||
drop(scope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
}); | ||
|
||
stmts.reserve(export.specifiers.len()); | ||
|
||
drop(scope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
Based on advice in #1826
Fixes #1786
Might also fix #1790
The
ImportAnalyzer
andCommonJs
folders need to share aScope
. To facilitate this, I refactored each module folder into twoFold
structs, for example,CommonJs
andCommonJsWorker
. The worker is created infold_module
, given a&mut Scope
to do its work, and then dropped afterward. This let me avoid addingborrow()
andderef()
calls all over the place.