-
Notifications
You must be signed in to change notification settings - Fork 455
feat(oxcaml): parameterised inline_tests #12707
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -264,7 +264,23 @@ include Sub_system.Register_end_point (struct | |||||||||||
| Resolve.Memo.List.concat_map backends ~f:(fun (backend : Backend.t) -> | ||||||||||||
| backend.runner_libraries) | ||||||||||||
| in | ||||||||||||
| let* lib = Lib.DB.resolve lib_db (loc, Library.best_name lib) in | ||||||||||||
| let* arguments = | ||||||||||||
| Resolve.Memo.lift_memo | ||||||||||||
| @@ Memo.List.map info.arguments ~f:(fun (loc, dep) -> | ||||||||||||
| let open Memo.O in | ||||||||||||
| let+ dep = Lib.DB.resolve lib_db (loc, dep) in | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason to not also use |
||||||||||||
| loc, dep) | ||||||||||||
|
Comment on lines
+270
to
+272
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the one hand, I like that the local open of My conclusion is that the extra local open here actually makes the code more confusing, given the pre-existing context. My suggestion is to stick with the surrounding convention for now
Suggested change
and maybe do a followup to make the binding context more narrowly scoped throughout this file in a followup, if you think it is worth it. |
||||||||||||
| in | ||||||||||||
| let* lib = | ||||||||||||
| let open Memo.O in | ||||||||||||
| let+ lib = Lib.DB.resolve lib_db (loc, Library.best_name lib) in | ||||||||||||
| Lib.Parameterised.instantiate | ||||||||||||
| ~from:`inline_tests | ||||||||||||
| ~loc | ||||||||||||
| lib | ||||||||||||
| arguments | ||||||||||||
| ~parent_parameters:[] | ||||||||||||
| in | ||||||||||||
| let* more_libs = | ||||||||||||
| Resolve.Memo.List.map info.libraries ~f:(Lib.DB.resolve lib_db) | ||||||||||||
| in | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -168,24 +168,25 @@ let lib_hidden_deps ~sctx ~kind lib requires = | |
| let obj_dir = Lib_info.obj_dir lib_info in | ||
| let+ modules = | ||
| match Lib_info.modules lib_info with | ||
| | External None -> | ||
| Code_error.raise "dependency has no modules" [ "lib", Lib.to_dyn dep ] | ||
| | External (Some modules) -> Action_builder.return modules | ||
| | External opt_modules -> Action_builder.return opt_modules | ||
| | Local -> | ||
| let local_lib = Lib.Local.of_lib_exn lib in | ||
| let+ modules = | ||
| Action_builder.of_memo (Dir_contents.modules_of_local_lib sctx local_lib) | ||
| in | ||
| Modules.With_vlib.modules modules | ||
| Some (Modules.With_vlib.modules modules) | ||
| in | ||
| Modules.With_vlib.fold_no_vlib_with_aliases | ||
| modules | ||
| ~init:[] | ||
| ~normal:(fun module_ acc -> | ||
| match Obj_dir.Module.cm_file obj_dir module_ ~kind:(Ocaml Cmi) with | ||
| | None -> acc | ||
| | Some cmi -> cmi :: acc) | ||
| ~alias:(fun _group acc -> acc))) | ||
| (match modules with | ||
| | None -> [] | ||
| | Some modules -> | ||
| Modules.With_vlib.fold_no_vlib_with_aliases | ||
| modules | ||
| ~init:[] | ||
| ~normal:(fun module_ acc -> | ||
| match Obj_dir.Module.cm_file obj_dir module_ ~kind:(Ocaml Cmi) with | ||
| | None -> acc | ||
| | Some cmi -> cmi :: acc) | ||
| ~alias:(fun _group acc -> acc)))) | ||
| >>| Dep.Set.of_files | ||
| ;; | ||
|
|
||
|
|
@@ -256,12 +257,19 @@ let build_modules ~sctx ~obj_dir ~modules_obj_dir ~dep_graph ~mode ~requires ~li | |
| Module_name.Map.add_exn acc (Module.name module_) instance) | ||
| ;; | ||
|
|
||
| let dep_graph ~obj_dir ~modules impl_only = | ||
| let dep_graph ~ocaml_version ~preprocess ~obj_dir ~modules impl_only = | ||
| let pp_map = | ||
| Staged.unstage | ||
| @@ Pp_spec.pped_modules_map | ||
| (Dune_lang.Preprocess.Per_module.without_instrumentation preprocess) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this change just an unrelated bug fix? |
||
| ocaml_version | ||
| in | ||
| let per_module = | ||
| List.fold_left impl_only ~init:Module_name.Unique.Map.empty ~f:(fun acc module_ -> | ||
| let module_name_unique = Module.obj_name module_ in | ||
| let deps = | ||
| let open Action_builder.O in | ||
| let module_ = pp_map module_ in | ||
| let+ deps = | ||
| Dep_rules.read_immediate_deps_of module_ ~modules ~obj_dir ~ml_kind:Impl | ||
| in | ||
|
|
@@ -284,10 +292,8 @@ let obj_dir_for_dep_rules dir = | |
| let instantiate ~sctx lib = | ||
| let ctx = Super_context.context sctx in | ||
| let build_dir = Context.build_dir ctx in | ||
| let* { Lib_config.ext_lib; _ } = | ||
| let+ ocaml = ctx |> Context.ocaml in | ||
| ocaml.lib_config | ||
| in | ||
| let* ocaml = Context.ocaml ctx in | ||
| let ext_lib = ocaml.lib_config.ext_lib in | ||
| let lib_info = Lib.info lib in | ||
| let modules_obj_dir = Lib_info.obj_dir lib_info in | ||
| let* deps_obj_dir, modules = | ||
|
|
@@ -303,7 +309,14 @@ let instantiate ~sctx lib = | |
| modules_obj_dir, Modules.With_vlib.modules modules | ||
| in | ||
| let impl_only = Modules.With_vlib.impl_only modules in | ||
| let dep_graph = dep_graph ~obj_dir:deps_obj_dir ~modules impl_only in | ||
| let dep_graph = | ||
| dep_graph | ||
| ~ocaml_version:ocaml.version | ||
| ~preprocess:(Lib_info.preprocess lib_info) | ||
| ~obj_dir:deps_obj_dir | ||
| ~modules | ||
| impl_only | ||
| in | ||
| let* requires = | ||
| Lib.closure ~linking:true [ lib ] | ||
| |> Resolve.Memo.map | ||
|
|
@@ -337,6 +350,7 @@ let resolve_instantiation scope str = | |
| | Some lib -> | ||
| let args = List.map args ~f:(fun arg -> Loc.none, arg) in | ||
| Lib.Parameterised.instantiate | ||
| ~from:`depends | ||
| ~loc:Loc.none | ||
| (Resolve.return lib) | ||
| args | ||
|
|
||
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.
Just a suggstion, feel free to disregard:
I think it can help with readability to accompany examples with descriptions how how to map the example to the general concept:
IMO, this can leave the reader with less to puzzle out, tho it does come at the cost of redundancy.