-
Notifications
You must be signed in to change notification settings - Fork 77
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
Clerk and testing improvements, rework resolution of module elements #535
Conversation
- Add a `-I` option that allows defined modules to be available from other directories - Add reporting of the number of successful / failed tests - Locate the project root, and always run the commands from there
This changes the `decl_ctx` to be toplevel only, with flattened references to uids for most elements. The module hierarchy, which is still useful in a few places, is kept separately. Module names are also changed to UIDs early on, and support for module aliases has been added (needs testing). This resolves some issues with lookup, and should be much more robust, as well as more convenient for most lookups. The `decl_ctx` was also extended for string ident lookups, which avoids having to keep the desugared resolution structure available throughout the compilation chain.
1a20c10
to
3649f92
Compare
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.
Thank you @AltGr for this very clear and thorough improvement!
compiler/driver.ml
Outdated
(ModuleName.of_string mname, intf), using | ||
in | ||
let rec aux req_chain acc modules = | ||
(* modulename * program * (id -> modulename) *) |
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.
you can turn that into a proper type annotation no?
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.
Takes 5x as much space but fine ;)
compiler/driver.ml
Outdated
| _ -> | ||
let get_scope_uid (ctx : decl_ctx) (scope : string) : ScopeName.t = | ||
if String.contains scope '.' then | ||
Message.raise_error "Only references to the top-level module are allowed"; |
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.
"... are allowed in scope reference %a ..."
scdef.D.scope_vars ctx) | ||
modul.D.module_scopes ctx | ||
in | ||
(* Todo: since we rename all scope vars at this point, it would be better to |
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.
I agree.
@@ -819,6 +819,15 @@ let line_dir_arg_re = | |||
eol | |||
]) | |||
|
|||
let line_dir_arg_upcase_re = |
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.
What does this catch?
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.
Yeah here we are using a low-cost parser on the directive lines, but the trouble is that e.g. the french translation uses many words (> Usage de MyModule en tant que M
) so recovering the actual parameters can't be done with simple word indexing (the previous version would only return the last M
here). Rather than enriching the line-lexer for sub-tokens on these directives and then a more serious parser, the cheaper solution here was to just rely on capitalisation of the module names 😅
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.
Woupsy that's kind of sketchy, this deserves at least a comment :)
```catala-test-inline | ||
$ catala typecheck | ||
[ERROR] | ||
A file that declares a module cannot be used through the raw '> Include' directive. You should use it as a module with '> Use This_is_not_the_file_name' instead. |
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.
I love this error
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.
It's an arbitrary limitation tho ;)
```catala-test-inline | ||
$ catala typecheck | ||
[ERROR] | ||
Module declared as This_is_not_the_file_name, which does not match the file name "tests/test_modules/bad/mod_badname.catala_en" |
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.
"... To match the filename, the module should be named XXX"
@@ -1,10 +1,10 @@ | |||
> Using Mod_middle | |||
> Using Mod_middle as M |
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.
Yes!
Clerk improvements
Add a
-I
option that allows defined modules to be available from otherdirectories
Add reporting of the number of successful / failed tests
Locate the project root, and always run the commands from there
Module elements
This changes the
decl_ctx
to be toplevel only, with flattened referencesto uids for most elements. The module hierarchy, which is still useful in a
few places, is kept separately.
Module names are also changed to UIDs early on, and support for module
aliases has been added (needs testing).
This resolves some issues with lookup, and should be much more robust, as
well as more convenient for most lookups.
The
decl_ctx
was also extended for string ident lookups, which avoidshaving to keep the desugared resolution structure available throughout the
compilation chain.