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.

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

MPR#7663: reword unsafe recursive modules message #1694

Merged
merged 4 commits into from Apr 4, 2018

Conversation

Projects
None yet
3 participants
@Octachron
Copy link
Contributor

Octachron commented Apr 3, 2018

This PR proposes to reword the error message raised when a recursive module cycle cannot be safely evaluated and add few examples in the corresponding part of the manual. Currently, trying to compile this example

module rec A: sig val x: int end = struct let x = B.y end
and B:sig val x: int val y:int end = struct let x = C.x let y = 0 end
and C:sig val x: int end = struct let x = B.y end ;;

yields

Error: Cannot safely evaluate the definition
of the recursively-defined module B

As reported in MPR#7633, an user then would have to guess that this unsafe evaluation error relates to the safe modules described inside the recursive module section of the manual. The aim of this PR is to make this connection more evident, by rewording the previous error message as

Error: Cannot safely evaluate the definition of the following cycle
of recursively-defined modules: B -> C -> B.
There are no safe modules in this cycle (see manual section 8.4)

In this reworded message, the notion of safe module appears explicitly, even if their descriptions is relegated to the manual ( using the fact that there is now a tool for testing the consistency of such references). Moreover, the error is assigned to the cycle rather than a single module.

To complement this new error message, the recursive module section of the manual has been extended with one example of unsafe cycle and another example of unfounded recursion raising an Undefined_recursive_module exception at runtime.

And since this error message was not exercised by the test suite, I added these two examples to the basic-modules tests.

Printtyp.ident id
"@[Cannot safely evaluate the definition of the following cycle@ \
of recursively-defined modules:@ %a.@ \
There is no safe modules in this cycle@ (see manual section %d.%d)@]"

This comment has been minimized.

@hcarty

hcarty Apr 3, 2018

Contributor

I think this should be "There are no safe modules" or "There is no safe module".

This comment has been minimized.

@Octachron

Octachron Apr 3, 2018

Author Contributor

You are right. Fixed, thanks!

@Octachron Octachron force-pushed the Octachron:manual_reference_for_recursive_module_errors branch from c2744cc to 8713974 Apr 3, 2018

@@ List.init num_bindings (fun j -> i::l, j ) in
explore (children @ stack) end
in
explore [[], cycle_start]

This comment has been minimized.

@gasche

gasche Apr 4, 2018

Member

I believe that the implementation is correct, but that a conceptually simpler approach (redoing less of the main loop's computations) would be possible: have Inprogress of int option keep the parent of the node (in the end, parents depend on their children), and change emit_binding i to emit_binding parent i (the initial call is emit_binding None i). At any point in the computation, following parents traces back the unique path any Inprogress node to the root of the DFS traversal (the only node to have None parent). If emit_binding parent i reaches an Inprogress _ node, you can construct the cycle by just recursively following parent's path until you hit i again.

This comment has been minimized.

@Octachron

Octachron Apr 4, 2018

Author Contributor

This works too. I have updated the code since it was indeed simpler (and I expanded the corresponding test to also test the cycle orientation).

@gasche
Copy link
Member

gasche left a comment

This is indeed simpler, thanks! See minor comments.

| Inprogress ->
let cycle = extract_unsafe_cycle id fv i in
| Inprogress _ ->
status.(i) <- Inprogress parent;

This comment has been minimized.

@gasche

gasche Apr 4, 2018

Member

I'm not fond of the write here; my intuition would be that you could do without it if you changed the stopping condition in the extraction code: check i = stop before looking at status.(i) (so with the un-shadowed i variable).

This comment has been minimized.

@Octachron

Octachron Apr 4, 2018

Author Contributor

I would prefer to keep this write here since it closes the dependency cycle and makes the extract_unsafe_cycle works starting from any point in the cycle.

This comment has been minimized.

@gasche

gasche Apr 4, 2018

Member

But it also means that you erase/lose information on the way (for example, you cannot recover which initial module provoked the cycle detection). But if you like it this way, ok, you are entitled to your (dubious) preference :-)

| Inprogress of int option (** parent node *)
| Defined

let extract_unsafe_cycle id status cycle_start =

This comment has been minimized.

@gasche

gasche Apr 4, 2018

Member

You could have a comment on the fact that cycle_start will occur both at start and the end of the result list. This is a desirable property in the end-user printing, but it is not immediate (to me) that it is the natural specification of extract_unsafe_cycle, and someone reading this code without looking at the test could think the duplication is a mistake.

(Or a version of this function without the repetition would be fine (not clearly better); I would then expect the currently-repeated element to be present only at the end of the list, to make it easy to add the duplicate back afterwards with just a cons.)

This comment has been minimized.

@Octachron

Octachron Apr 4, 2018

Author Contributor

I added a comment on this duplication, since indeed this not a completely natural representation of a cycle.

@Octachron Octachron force-pushed the Octachron:manual_reference_for_recursive_module_errors branch from 1c7e9c0 to 1a74d65 Apr 4, 2018

@gasche

gasche approved these changes Apr 4, 2018

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Apr 4, 2018

Approved. I'll let you merge whenever you feel like it.

@Octachron Octachron merged commit 6e927dc into ocaml:trunk Apr 4, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vicuna vicuna referenced this pull request Mar 14, 2019

Closed

suboptimal error message #7663

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.