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

Improved error message for illegal permutations #8546

Merged
merged 3 commits into from Mar 27, 2019

Conversation

@Octachron
Copy link
Contributor

commented Mar 25, 2019

This PR supplements the error message for illegal permutations of runtime components in module types

module M: sig
  module type x = sig val x: int class y : object end end
end = struct
  module type x = sig class y: object end val x : int end
end

At position module type x =
Illegal permutation of structure fields

with an example of two swapped runtime components:

Illegal permutation of structure fields:
the class "y" and the value "x" are swapped.

This illustrative transposition is chosen to increase the number of fixed points in the coercing permutation (following the traditional decomposition of permutation). Thus applying the suggested swap may yield a new error message, but this chain of error messages should be finite.
(The alternative of displaying the whole cycle decomposition seemed a bit too noisy and hard to decipher for users.)

This PR comes with few new tests for illegal permutations.

Closes #3819

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

If undoing the swap is not enough to fix the error, this should be hinted at by the error message formulation. What about:

Illegal permutation of structure fields. For example,
the class "y" and the value "x" are not in the same order.

("not in the same order" is nicer than "swapped" because it does not suggest that the actual change that broke the ordering was a swap -- it could be, but we don't know that it was.)

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

Adding a for example or an at least is nicer indeed. I would add more context to the same order hint. Maybe:

  For example, at position functor (X) -> <here>,
  the class "two" and the value "one" are not in the same order
  between the expected and actual module types.

?
It could also help to rephrase the opening sentence to:

Illegal permutation of runtime components in a module type.
  For example, at position functor (X) -> <here>,
  the class "two" and the value "one" are not in the same order
  between the expected and actual module types.
@gasche

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

I find the use of "between" confusing here: "the same order between X and Y" reads to me like "the relative order of X and Y". What about just having "in" instead?

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

I am fine with in.

@Octachron Octachron force-pushed the Octachron:illegal_permutation branch from f8be7db to ce17e3f Mar 26, 2019
@gasche
gasche approved these changes Mar 26, 2019
Copy link
Member

left a comment

I read through the code and I like it. (From a distance it looks like too much code, but this is nice code.) Feel free to take inspiration of my nitpicking comments or not, and merge whenever you like.

typing/includemod.ml Outdated Show resolved Hide resolved
typing/includemod.ml Outdated Show resolved Hide resolved
typing/includemod.ml Show resolved Hide resolved
typing/includemod.ml Outdated Show resolved Hide resolved
typing/includemod.ml Show resolved Hide resolved
typing/includemod.ml Outdated Show resolved Hide resolved
@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

I went ahead with most suggestions, and I will merge tomorrow.

@Octachron Octachron force-pushed the Octachron:illegal_permutation branch from b1b83b6 to c238090 Mar 26, 2019
@Octachron Octachron merged commit 8e5e3c0 into ocaml:trunk Mar 27, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.