Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Omit dynamic imported module from require, import statement #6232

Merged
merged 9 commits into from
May 1, 2023
Merged

Omit dynamic imported module from require, import statement #6232

merged 9 commits into from
May 1, 2023

Conversation

mununki
Copy link
Member

@mununki mununki commented May 1, 2023 •
edited
Loading

Fixes #6223

This PR fixes the issue of dynamic import modules being added to require or import statements. The main changes are as follows.

  1. Lam: Lam.Lglobal_module representation and print
Lam.Lglobal_module ident
(* to *)
Lam.Lglobal_module ident * bool (* dynamic_import *)
  1. JS IR: J.module_id definition
and module_id = { id : ident; kind : Js_op.kind }
(* to *)
and module_id = { id : ident; kind : Js_op.kind ; dynamic_import : bool }
  1. If Lglobal_module has dynamic import=true, the module is considered to have side effects. Omit dynamic imported module from require, import statement #6232 (comment)

@mununki mununki changed the title add tests Omit dynamic imported module from require, import statement May 1, 2023
cristianoc
cristianoc reviewed May 1, 2023
View reviewed changes
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.
Left some comments.

jscomp/core/js_dump_program.ml Outdated
@@ -73,8 +73,12 @@ let node_program ~output_dir f (x : J.deps_program) =
P.newline f;
let cxt =
Js_dump_import_export.requires L.require Ext_pp_scope.empty f
(Ext_list.map x.modules (fun x ->
( x.id,
(* dynamic import should not print in require statement *)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not be emitted in require statements

Copy link
Member Author

@mununki mununki May 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh actually, it happened. Not only es6_program. I you correct my comment, haha. Got it.
It's a bit confusing, but do you mean that it's not generated in commonJS by default? In fact, we can use dynamic imports in commonJS as well. So, the changes in this PR would also apply to the node_program.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I just meant to suggest an alternative phrasing of the comment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. 3603852

jscomp/core/js_dump_program.ml Outdated
@@ -83,8 +87,12 @@ let node_program ~output_dir f (x : J.deps_program) =
let es6_program ~output_dir fmt f (x : J.deps_program) =
let cxt =
Js_dump_import_export.imports Ext_pp_scope.empty f
(Ext_list.map x.modules (fun x ->
( x.id,
(* dynamic import should not print in import statement *)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not be emitted in require statements

jscomp/core/lam_analysis.ml Outdated
@@ -32,7 +32,7 @@ let not_zero_constant (x : Lam_constant.t) =
let rec no_side_effects (lam : Lam.t) : bool =
match lam with
| Lvar _ | Lconst _ | Lfunction _ -> true
| Lglobal_module _ -> true
| Lglobal_module (_, dynamic_import) -> not dynamic_import
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the impact of this change in practice?
Do we have tests that show what would happen with and without this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Now that I look at it again, Lglobal_module always being no side effect might have a different context. I'll do some testing.

Copy link
Member Author

@mununki mununki May 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just found there are already side_effect.res and side_effect_free.res in tests. I've changed my tests to import_side_effect.res and import_side_effect_free.res. I confirmed that the compiler correctly detects side effects when dynamically importing a value with no side effects. So, it seems that the original Lglobal_module expression without side effects is indeed correct in the existing code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See b5d8366

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is indeed correct. https://github.com/rescript-lang/rescript-compiler/blob/master/jscomp/core/lam_analysis.ml#L35-L38

jscomp/test/import_unnecessary.res Outdated
@@ -0,0 +1,5 @@
// Import_lazy_component is not a pure module
// It is supposed not to be required by dynamic import
let a = Js.import(Import_lazy_component.a)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is tested in this example?

Copy link
Member Author

@mununki mununki May 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

// import_lazy_component.res
let a = Belt.Array.map([1], x => x)

import_lazy_component.res has side effect. So, import_unnecessary.res module's JS output also had unnecessary require("./import_lazy_component.js"); before this PR changes. See the test output before this PR changes affect: dface2b

cristianoc
cristianoc approved these changes May 1, 2023
View reviewed changes
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's ready to go? When the changelog is updated.

jscomp/core/lam_convert.ml Outdated
@@ -505,9 +505,13 @@ let convert (exports : Set_ident.t) (lam : Lambda.lambda) :
primitive %s"
s
in
let args = Ext_list.map args convert_aux in
let dynamic_import = match primitive with
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be simplified as primitive = Pimport.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Done 3603852

@mununki mununki force-pushed the fix-unnecessary-import branch from 45f2537 to 3603852 Compare May 1, 2023 17:29
@mununki
Copy link
Member Author

mununki commented May 1, 2023

Looks like it's ready to go? When the changelog is updated.

8cd19cf
Yes, ready to go.

@mununki mununki merged commit 1c0bbf1 into master May 1, 2023
@mununki mununki deleted the fix-unnecessary-import branch May 1, 2023 17:55
@fhammerschmidt fhammerschmidt mentioned this pull request Oct 2, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cristianoc cristianoc cristianoc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unused static import emitted in addition to dynamic import
2 participants
@mununki @cristianoc