Skip to content
Prev Previous commit
Next Next commit
add comments, using labeled argument
  • Loading branch information
mununki committed Apr 23, 2023
commit 18abaf8d5d24cc6b21b628bea4809490f5d474f2
2 changes: 1 addition & 1 deletion jscomp/core/js_implementation.ml
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ let after_parsing_impl ppf outputprefix (ast : Parsetree.structure) =
let lambda, exports =
Translmod.transl_implementation modulename typedtree_coercion
in
let js_program module_system =
let js_program ~module_system =
print_if_pipe ppf Clflags.dump_rawlambda Printlambda.lambda lambda
|> Lam_compile_main.compile outputprefix module_system exports
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the root cause breaking the tests

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems delaying the evaluation of js_program causing the test failure.

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 how it goes #6191

Copy link
Collaborator

Choose a reason for hiding this comment

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

Bingo.

in
Expand Down
158 changes: 79 additions & 79 deletions jscomp/core/lam_compile.ml

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions jscomp/core/lam_compile.mli
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,6 @@
(** Compile single lambda IR to JS IR *)

val compile_recursive_lets :
string -> Js_packages_info.module_system -> Lam_compile_context.t -> (Ident.t * Lam.t) list -> Js_output.t
output_prefix:string -> Js_packages_info.module_system -> Lam_compile_context.t -> (Ident.t * Lam.t) list -> Js_output.t
Copy link
Collaborator

@cristianoc cristianoc Apr 23, 2023

Choose a reason for hiding this comment

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

About invasive:

This and the function below are the only 2 entry points where the 2 new parameters are passed in.
No need to thread them though each recursive call if they never change and are passed from the outside.

Copy link
Collaborator

@cristianoc cristianoc Apr 23, 2023

Choose a reason for hiding this comment

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

Once the number of changes is reduced, it should be possible to just isolate what change is responsible.
E.g. perhaps first remove Pimport, then remove gradually every other change in turn, locally, until the tests stops failing and one finds out the root cause.


val compile_lambda : string -> Js_packages_info.module_system -> Lam_compile_context.t -> Lam.t -> Js_output.t
val compile_lambda : output_prefix:string -> Js_packages_info.module_system -> Lam_compile_context.t -> Lam.t -> Js_output.t
12 changes: 6 additions & 6 deletions jscomp/core/lam_compile_main.ml
Original file line number Diff line number Diff line change
Expand Up @@ -60,20 +60,20 @@ let compile_group output_prefix module_system (meta : Lam_stats.t)
(* let lam = Optimizer.simplify_lets [] lam in *)
(* can not apply again, it's wrong USE it with care*)
(* ([Js_stmt_make.comment (Gen_of_env.query_type id env )], None) ++ *)
Lam_compile.compile_lambda output_prefix module_system { continuation = Declare (kind, id);
Lam_compile.compile_lambda ~output_prefix module_system { continuation = Declare (kind, id);
jmp_table = Lam_compile_context.empty_handler_map;
meta
} lam

| Recursive id_lams ->
Lam_compile.compile_recursive_lets output_prefix module_system
Lam_compile.compile_recursive_lets ~output_prefix module_system
{ continuation = EffectCall Not_tail;
jmp_table = Lam_compile_context.empty_handler_map;
meta
}
id_lams
| Nop lam -> (* TODO: Side effect callls, log and see statistics *)
Lam_compile.compile_lambda output_prefix module_system {continuation = EffectCall Not_tail;
Lam_compile.compile_lambda ~output_prefix module_system {continuation = EffectCall Not_tail;
jmp_table = Lam_compile_context.empty_handler_map;
meta
} lam
Expand Down Expand Up @@ -288,18 +288,18 @@ js
let (//) = Filename.concat

let lambda_as_module
(lambda_output : Js_packages_info.module_system -> J.deps_program)
(lambda_output : module_system: Js_packages_info.module_system -> J.deps_program)
(output_prefix : string)
: unit =
let package_info = Js_packages_state.get_packages_info () in
if Js_packages_info.is_empty package_info && !Js_config.js_stdout then begin
Js_dump_program.dump_deps_program ~output_prefix NodeJS (lambda_output NodeJS) stdout
Js_dump_program.dump_deps_program ~output_prefix NodeJS (lambda_output ~module_system: NodeJS) stdout
end else
Js_packages_info.iter package_info (fun {module_system; path; suffix} ->
let output_chan chan =
Js_dump_program.dump_deps_program ~output_prefix
module_system
(lambda_output module_system)
(lambda_output ~module_system)
chan in
let basename =
Ext_namespace.change_ext_ns_suffix
Expand Down
2 changes: 1 addition & 1 deletion jscomp/core/lam_compile_main.mli
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,4 @@ val compile : string -> Js_packages_info.module_system -> Ident.t list -> Lambda
{!Env.get_unit_name ()}
*)

val lambda_as_module : (Js_packages_info.module_system -> J.deps_program) -> string -> unit
val lambda_as_module : (module_system: Js_packages_info.module_system -> J.deps_program) -> string -> unit
10 changes: 7 additions & 3 deletions jscomp/core/lam_compile_primitive.ml
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ let translate output_prefix module_system loc (cxt : Lam_compile_context.t)
| _ -> E.runtime_call Js_runtime_modules.option "nullable_to_opt" args
)
| _ -> assert false)
(* Compile #import: The module argument for dynamic import is represented as a path,
and the module value is expressed through wrapping it with promise.then *)
| Pimport -> (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add comment describing what the transformation does.

match args with
| [ e ] -> (
Expand All @@ -111,8 +113,8 @@ let translate output_prefix module_system loc (cxt : Lam_compile_context.t)
let module_id, module_value =
match module_of_expression e.expression_desc with
| [ module_ ] -> module_
| _ -> assert false
(* TODO: graceful error message here *)
| _ -> Location.raise_errorf ~loc
"Invalid argument: Dynamic import requires a module or a module value as its argument. Passing a value or local module is not allowed."
Copy link
Collaborator

Choose a reason for hiding this comment

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

The location seems to be wrong:

FAILED: test/Import.cmi

We've found a bug for you!
test/Import.res

Invalid argument: Dynamic import requires a module or a module value as its argument. Passing a value or local module is not allowed.

in

let path =
Expand All @@ -123,7 +125,9 @@ let translate output_prefix module_system loc (cxt : Lam_compile_context.t)
match module_value with
| Some value -> wrap_then (import_of_path path) value
| None -> import_of_path path)
| _ -> assert false)
| [] | _ ->
Location.raise_errorf ~loc
"Invalid argument: Dynamic import must take a single module or module value as its argument.")
| Pjs_function_length -> E.function_length (Ext_list.singleton_exn args)
| Pcaml_obj_length -> E.obj_length (Ext_list.singleton_exn args)
| Pis_null -> E.is_null (Ext_list.singleton_exn args)
Expand Down
1 change: 1 addition & 0 deletions jscomp/frontend/ast_await.ml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ let create_await_expression (e : Parsetree.expression) =
in
Ast_helper.Exp.apply ~loc unsafe_await [(Nolabel, e)]

(* Transform `@res.await M` to unpack(@res.await Js.import(module(M: __M0__))) *)
let create_await_module_expression ~module_type_name (e : Parsetree.module_expr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a comment explaining what this transformation does.

=
let open Ast_helper in
Expand Down
4 changes: 3 additions & 1 deletion jscomp/frontend/bs_builtin_ppx.ml
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,8 @@ let local_module_name =
incr v;
"local_" ^ string_of_int !v

(* Unpack requires core_type package for type inference;
use module type bindings and a function to create safe local names instead. *)
let local_module_type_name =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add comment about what this does.

let v = ref 0 in
fun ({txt} : Longident.t Location.loc) ->
Expand Down Expand Up @@ -505,10 +507,10 @@ let rec structure_mapper (self : mapper) (stru : Ast_structure.t) =
| _ -> expand_reverse acc (structure_mapper self rest)
in
aux [] stru
(* Dynamic import of module transformation: module M = @res.await Belt.List *)
| Pstr_module
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a comment explaining what this transformation does.

({pmb_expr = {pmod_desc = Pmod_ident {txt; loc}; pmod_attributes} as me}
as mb)
(* module M = @res.await Belt.List *)
when Res_parsetree_viewer.hasAwaitAttribute pmod_attributes ->
let item = self.structure_item self item in
let safe_module_type_name = local_module_type_name {txt; loc} in
Expand Down