Module support: handle structs, enums and scope calls across modules #497

AltGr · 2023-08-15T22:13:31Z

Current state:

Works: modules compiled with catala module --build foo.catala_en then used
through catala interpret --use foo.catala_en bar.catala_en
(this is the use-case for running the interpreter on tests that will actually
do a native-code computation)
Usage from native code ~~doesn't work yet (but it's a matter of bugfixing)~~ works as well (use catala module --link for now)
Desambiguation doesn't yet work cross-modules yet (but no technical
difficulty, just needs handling ; can be left for a later PR)
Code needs cleanup (debugs remaining here and there...)
On second thought storing the paths as part of the program_ctx isn't the
best approach: I'll revert that part and add the paths to an expanded version
of Uid.t. The information will be closer to where it's needed, it will avoid
limitation in printers, and this just needs the current path to be passed along
during name resolution, when initialising the (Q)Uids.
Done now, code is much simpler but one needs to remember to lookup the correct module context when querying scope or def info.
~~web_api and french_law friends are broken and temporarily disabled~~ Now fixed, at least all tests and benches pass now
no support yet for other backends (but that will be left for later PRs)

This works for the `--reset` option, but I have some doubts about it.

AltGr · 2023-08-15T22:15:41Z

(and obviously, the git history needs a rebase ;) )

denismerigoux

Thanks @AltGr for another massive PR! It still needs some work though, but that'll be after your holidays :)

build_system/clerk_runtest.mli

compiler/dcalc/from_scopelang.ml

denismerigoux · 2023-08-17T12:38:21Z

compiler/dcalc/from_scopelang.ml

+    (* Find a witness of a mark in the definitions *)
+    match sigma.scope_decl_rules with
+    | [] ->
+      (* Todo: are we sure this can't happen in normal code ? E.g. is calling a


Yeah ideally a scope with only input variables should compile, you can maybe return Pos.no_pos at this point.

It's a bit more involved because of the GADT for annotations, we would need to take a type witness as parameter of the function. I'll see what I can do ^^

compiler/dcalc/from_scopelang.ml

examples/NSW_community_gaming/tests/test_nsw_social_housie.catala_en

denismerigoux · 2023-08-17T13:40:23Z

examples/aides_logement/dune

+;; (rule
+;;  (targets aides_logement.ml aides_logement_api_web.ml)
+;;  (deps
+;;   (source_tree ..)


denismerigoux · 2023-08-17T13:41:04Z

examples/dune

@@ -0,0 +1 @@
+(dirs :standard \ aides_logement)


Ah this is temporary and will be removed from the finalised PR, see 0a98f63

denismerigoux · 2023-08-17T13:41:20Z

french_law/dune

@@ -0,0 +1,3 @@
+(dirs catala_legifrance)
+
+;; ocaml js python


(same as above, this disables most of french_law)

tests/test_scope/good/scope_call4.catala_en

AltGr · 2023-08-31T15:18:21Z

Should now be regression-free and ready for final review;
I should still do some cleanup on the branch git history, but without impact on the result.

AltGr · 2023-08-31T15:32:00Z

cf. ocaml/opam-repository#24342 on CI failure

... and add a custom printer Since this is a very common bug, this patch should gain us a lot of time when debugging uncaught Not_found errors, because the element not found can now be printed straight away without the need for further debugging. The small cost is that one should remember to catch the correct specialised `Foo.Map.Not_found _` exception rather than the standard `Not_found` (which would type-check but not catch the exception). Using `find_opt` should be preferred anyway. Note that the other functions from the module `Map` that raise `Not_found` are not affected ; these functions are `choose`, `min/max_binding`, `find_first/last` which either take a predicate or fail on the empty map, so it wouldn't make sense for them (and we probably don't use them much).

(they use UIDs so this doesn't jeopardize proper name resolution ; and it allows resolution of subscope calls without further code modifications)

This makes sure `catala module` finds the local runtime when run from the catala source tree; and fixes lookup of the catala exec on custom uses of `clerk runtest`.

AltGr · 2023-08-31T16:13:17Z

Done cleaning up the git history a bit

Half-baked because clerk doesn't know to order the tests properly at the moment, and we spit compiled files into the source test directory.

rather than scattered in structures The context is still hierarchical for defs though, so one needs to retrieve the path to lookup in the correct context for info. Exceptions are enums and struct defs, which are re-exposed at toplevel.

denismerigoux

OK to merge! While I'm thinking about it : the invariants checking phase that @adelaett made is only triggered by the Invariants CLI target, but shouldn't they be checked more often and at least in the CI ?

denismerigoux · 2023-09-01T07:56:41Z

compiler/desugared/linting.ml

@@ -136,7 +136,8 @@ let detect_unused_struct_fields (p : program) : unit =
  in
  StructName.Map.iter
    (fun s_name fields ->
-      if
+      if StructName.path s_name <> [] then ()


Ah so you don't check structs from other modules during linting. I guess that makes sense but that would be better with a little comment no?

In time we may also want to fine-tune the warnings when compiling modules: in some cases it makes sense that stuff is unused here.

denismerigoux · 2023-09-01T07:58:22Z

compiler/driver.ml

-    let prg, _ = surface options in
-    let prg = load_module_interfaces prg options link_modules in
+    let prg, _ = surface options ~link_modules in
+    Message.emit_debug "- DESUGARED -";


This breaks style with the other debug messages right?

Yes: I see them as section titles, it makes it easy to see what stage you were in when there is a crash.
Example:

[DEBUG] - INIT - [DEBUG] Could not read plugin directory /home/lg/inria/catala/_build/default/compiler/../lib/catala/plugins [DEBUG] Loading plugins from _build/default/compiler/plugins [DEBUG] Plugin "_build/default/compiler/plugins/api_web.cmxs" loaded [DEBUG] Plugin "_build/default/compiler/plugins/python.cmxs" loaded [DEBUG] Plugin "_build/default/compiler/plugins/modules.cmxs" loaded [DEBUG] Plugin "_build/default/compiler/plugins/explain.cmxs" loaded [DEBUG] Plugin "_build/default/compiler/plugins/lazy_interpreter.cmxs" loaded [DEBUG] Plugin "_build/default/compiler/plugins/json_schema.cmxs" loaded [DEBUG] - SURFACE - [DEBUG] Parsing sources/impot_revenu.catala_fr [DEBUG] Parsing sources/prologue.catala_fr [DEBUG] Parsing sources/vérifications.catala_fr [DEBUG] Parsing sources/cgi_revenus.catala_fr [DEBUG] Parsing sources/lfr_2022.catala_fr [DEBUG] Parsing sources/loi_2022-1158.catala_fr [DEBUG] - DESUGARED - [DEBUG] Name resolution... [DEBUG] Desugaring... [DEBUG] Disambiguating... [DEBUG] Linting... [DEBUG] - SCOPELANG - [DEBUG] - DCALC - [DEBUG] Typechecking... [DEBUG] Translating to default calculus... [DEBUG] Typechecking again... [DEBUG] - LCALC - [DEBUG] Catala runtime libraries found at /home/lg/.opam/latest/lib/catala/runtime. [DEBUG] Compiling OCaml shared object file sources/impot_revenu.cmxs...

May add in some color as well 🤔

Okay, looks good to me. Yep color would be perfect :)

denismerigoux · 2023-09-01T07:59:38Z

compiler/lcalc/to_ocaml.ml

@@ -245,11 +223,15 @@ let format_var (fmt : Format.formatter) (v : 'm Var.t) : unit =
  let lowercase_name = String.to_ascii lowercase_name in
  if
    List.mem lowercase_name ["handle_default"; "handle_default_opt"]
-    || String.begins_with_uppercase (Bindlib.name_of v)
+    (* O_O *)


Hm I don't exactly remember the mood I was in when I wrote this 🤣
But certainly there should be a better way of special-casing without matching the final ident strings.

denismerigoux · 2023-09-01T08:00:01Z

compiler/lcalc/to_ocaml.ml

@@ -529,7 +530,7 @@ let format_ctx
      format_to_module_name (`Ename enum_name)
      (Format.pp_print_list
         ~pp_sep:(fun fmt () -> Format.fprintf fmt "@\n")
-         (fun _fmt (enum_cons, enum_cons_type) ->
+         (fun fmt (enum_cons, enum_cons_type) ->


Well caught!

denismerigoux · 2023-09-01T08:03:26Z

compiler/shared_ast/typing.ml

+  let rec dump ppf env =
+    let pp_sep = Format.pp_print_space in
+    Format.pp_open_vbox ppf 0;
+    (* Format.fprintf ppf "structs: @[<hov>%a@]@,"


Well technically this function is dead code, it shouldn't be used outside of debugging sessions — but pretty convenient nevertheless and we shouldn't need to rewrite it every time. The same applies to the commented code: while you generally wouldn't want all the typedecls listed here, I thought that having this available to be uncommented here could be helpful in debugging sessions where you might actually have use for it.

But well, since it's normally unused we may indeed prefer to uncomment to make sure the extended function continues to typecheck...

AltGr · 2023-09-01T08:21:44Z

the invariants checking phase that @adelaett made is only triggered by the Invariants CLI target, but shouldn't they be checked more often and at least in the CI ?

It may even make sense to enable it in tests by default, when applicable 🤔

AltGr added 4 commits August 4, 2023 12:08

Implement reversing the dependencies for tests

378669b

This works for the `--reset` option, but I have some doubts about it.

Clerk: split code for running tests from the main (Ninja) engine

fa33d37

Disable recursive tests reset for now; remove rel paths in test out

edd6dce

modules plugin: fix target directory of dlls

4a06faa

AltGr marked this pull request as draft August 15, 2023 22:13

denismerigoux approved these changes Aug 17, 2023

View reviewed changes

AltGr changed the title ~~[DRAFT] Module support: handle structs, enums and scope calls across modules~~ Module support: handle structs, enums and scope calls across modules Aug 31, 2023

AltGr marked this pull request as ready for review August 31, 2023 15:41

AltGr added 6 commits August 31, 2023 17:54

Implement module lookups for scopes, structs, and enums

9bac045

Gather the maps for subscope vars to the top level

c58e76f

(they use UIDs so this doesn't jeopardize proper name resolution ; and it allows resolution of subscope calls without further code modifications)

Reformat

72882f8

Some fixes & cleanup after early review

17172cf

Some fixes to relative file lookups in modules and tests

a71b4e7

This makes sure `catala module` finds the local runtime when run from the catala source tree; and fixes lookup of the catala exec on custom uses of `clerk runtest`.

AltGr force-pushed the modules+subscopes branch from 28fb39c to 20eea70 Compare August 31, 2023 16:12

AltGr added 10 commits August 31, 2023 18:31

Add (half-baked) test on module usage

b5baa91

Half-baked because clerk doesn't know to order the tests properly at the moment, and we spit compiled files into the source test directory.

Simplification: store paths in Uids

7db63e5

rather than scattered in structures The context is still hierarchical for defs though, so one needs to retrieve the path to lookup in the correct context for info. Exceptions are enums and struct defs, which are re-exposed at toplevel.

Fix OCaml backend for cross-module refs

6bccd89

Reformat

8e33355

Cleanup tmp debug code

e75ea3d

Improve module error messages

ff60aee

Make the passes appear more clearly in the driver debug

8278bb8

Fixes related to environments and lookups

544e18e

Update tests

ce97b55

CI: fix docker image name escaping

4e9f738

AltGr force-pushed the modules+subscopes branch from 20eea70 to 4e9f738 Compare August 31, 2023 16:32

denismerigoux approved these changes Sep 1, 2023

View reviewed changes

AltGr merged commit dcb057b into CatalaLang:master Sep 1, 2023
1 check passed

AltGr mentioned this pull request Sep 5, 2023

Implement reversing the dependencies for tests #491

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Module support: handle structs, enums and scope calls across modules #497

Module support: handle structs, enums and scope calls across modules #497

AltGr commented Aug 15, 2023 •

edited

Loading

AltGr commented Aug 15, 2023

denismerigoux left a comment

denismerigoux Aug 17, 2023

AltGr Aug 25, 2023

denismerigoux Aug 17, 2023

denismerigoux Aug 17, 2023

AltGr Aug 25, 2023

denismerigoux Aug 17, 2023

AltGr Aug 25, 2023

AltGr commented Aug 31, 2023

AltGr commented Aug 31, 2023

AltGr commented Aug 31, 2023

denismerigoux left a comment

denismerigoux Sep 1, 2023

AltGr Sep 1, 2023

denismerigoux Sep 1, 2023

AltGr Sep 1, 2023

denismerigoux Sep 1, 2023

denismerigoux Sep 1, 2023

AltGr Sep 1, 2023

denismerigoux Sep 1, 2023

denismerigoux Sep 1, 2023

AltGr Sep 1, 2023

AltGr commented Sep 1, 2023

Module support: handle structs, enums and scope calls across modules #497

Module support: handle structs, enums and scope calls across modules #497

Conversation

AltGr commented Aug 15, 2023 • edited Loading

AltGr commented Aug 15, 2023

denismerigoux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AltGr commented Aug 31, 2023

AltGr commented Aug 31, 2023

AltGr commented Aug 31, 2023

denismerigoux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AltGr commented Sep 1, 2023

AltGr commented Aug 15, 2023 •

edited

Loading