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

Full explanation for unsafe cycles in recursive modules #2058

Merged
merged 2 commits into from Oct 1, 2018

Conversation

@Octachron
Copy link
Contributor

commented Sep 23, 2018

This PR proposes to expand the error message for unsafe cycles in recursive module definition by providing an example of unsafe components for each unsafe module in the cycle.

For instance, the error message for

module rec A: sig
   module F: functor(X:sig end) -> sig end
   val f: unit -> unit
end = struct
  module F(X:sig end) = struct end
  let f () = B.value
end
and B: sig val value: unit end = struct let value = A.f () end

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

is expanded with two new lines:

Line 2, characters 3-42: Module A defines an unsafe functor, F .
Line 8, characters 11-26: Module B defines an unsafe value, value .

In term of implementation, this PR merely keep track of the unsafe components detected during the
recursive module analysis and expose them to users.

Hopefully, pointing the unsafe components would make it easier for users to fix the problematic cycle, (particularly when the cycle contains exotic unsafe components).

This improvement over #1694 was suggested by @ivg .

@pmetzger

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

A pure aside: it might be nice if error messages slowly started to follow the Gnu/FSF standard so that generic tools that parse compiler output would need less customization. See: https://www.gnu.org/prep/standards/html_node/Errors.html for details. That said, it is not necessary that this PR do that, I'm just reminded of it now.

@gasche

gasche approved these changes Sep 23, 2018

Copy link
Member

left a comment

This looks nice to me -- see minor comments.

type init_result =
| Ok of lambda * lambda
| Fail of unsafe_info
(* The Error constructor has been stolen earlier, we cannot use result *)

This comment has been minimized.

Copy link
@gasche

gasche Sep 23, 2018

Member

You can use Result.Error to resolve the ambiguity, right? I'm not against defining new types if we think it is best/clearest, but if you really want result I think you should use it.

This comment has been minimized.

Copy link
@Octachron

Octachron Sep 23, 2018

Author Contributor

Is it fine to use a new standard library module here? If it is the case, I think result would be better.

This comment has been minimized.

Copy link
@Octachron

Octachron Oct 1, 2018

Author Contributor

I went ahead and used result finally.

print_cycle cycle chapter section
| Conflicting_inline_attributes ->
fprintf ppf
"@[Conflicting ``inline'' attributes@]"
Location.errorf "@[Conflicting ``inline'' attributes@]"

This comment has been minimized.

Copy link
@gasche

gasche Sep 23, 2018

Member

Unrelated, but is there a reason to use backquotes and double-singlequotes here instead of just quotes?

This comment has been minimized.

Copy link
@pmetzger

pmetzger Sep 23, 2018

Member

It feels very TeX. :)

This comment has been minimized.

Copy link
@Octachron

Octachron Sep 23, 2018

Author Contributor

I don't know. @mshinwell , it looks like you are the one that wrote this error message in cd8a4a9, do you remember any particular reason?

This comment has been minimized.

Copy link
@mshinwell

mshinwell Sep 24, 2018

Contributor

There was no particular reason. They should probably be singles.

This comment has been minimized.

Copy link
@Octachron

Octachron Oct 1, 2018

Author Contributor

Done: I replaced ``inline'' by 'inline'.

@ivg

ivg approved these changes Sep 23, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 1, 2018

Thanks! Good to go when CI passes.

@Octachron Octachron merged commit 6dbf415 into ocaml:trunk Oct 1, 2018

2 checks passed

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

mshinwell added a commit to mshinwell/ocaml that referenced this pull request Oct 3, 2018

Full explanation for unsafe cycles in recursive modules (ocaml#2058)
* Full explanation for unsafe cycles in rec modules

damiendoligez added a commit to damiendoligez/ocaml that referenced this pull request Nov 5, 2018

Full explanation for unsafe cycles in recursive modules (ocaml#2058)
* Full explanation for unsafe cycles in rec modules
@trefis trefis referenced this pull request Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.