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

Improve error reporting for ill-typed applicative functor type #1491

Merged
merged 6 commits into from Dec 13, 2017

Conversation

Projects
None yet
5 participants
@sliquister
Copy link
Contributor

sliquister commented Nov 24, 2017

Right now, type errors in applicative functor types are annoying to fix since they contain very little information. Here is an example of what I change:

type t = Set.Make(M).t
 [%%expect{|
 Line _, characters 9-22:
-Error: Ill-typed functor application Set.Make(M)
+Error: The type of M does not match Set.Make's parameter
+       Modules do not match:
+         sig type t val equal : 'a -> 'a -> bool end
+       is not included in
+         Set.OrderedType
+       The value `compare' is required but not provided
+       File "set.mli", line 52, characters 4-31: Expected declaration
 |} ]

The change introduces a dependency from typetexp.ml to includemod.ml, I am not sure if that's undesirable.

While I was there, I also fixed MPR#7611 and changed the wording of related error messages.

@Drup
Copy link
Contributor

Drup left a comment

The change is very useful and looks correct to me (It simply doesn't throw away information that was already there).
Also, the patch only changes code that is used to investigate a type error, so it's fairly safe. The commits are weirdly organized and clearly not self-contained. I think you could just squash them.

- GPR#1370: Fix code duplication in Cmmgen
(Vincent Laviron, with help from Pierre Chambart,
reviews by Gabriel Scherer and Luc Maranget)

This comment has been minimized.

@Drup

Drup Nov 26, 2017

Contributor

That shouldn't be here.

This comment has been minimized.

@sliquister

sliquister Nov 26, 2017

Author Contributor

I don't see why not. It's about the same as fixing a typo in a comment.

This comment has been minimized.

@trefis

trefis Nov 27, 2017

Contributor

I agree with @sliquister .
I had already noticed the issue here but it's apparently not worth anyone's time to create a separate PR. So fixing it here sounds good.

This comment has been minimized.

@Drup

Drup Dec 2, 2017

Contributor

Ah, I though it was a change that was included by mistake. Not sure why it's included in a commit that has literally nothing to do with it, but fair enough. :)

type t = Generative(M).t
[%%expect{|
type t = Generative(M).t
|}]

This comment has been minimized.

@Drup

Drup Nov 26, 2017

Contributor

That probably shouldn't be a test, or at least not like that.

This comment has been minimized.

@sliquister

sliquister Nov 26, 2017

Author Contributor

I think it makes sense, while writing tests for applicative functor application, to add related tests.
This test particularly test stays broken, but I don't see why that's an issue, as long as it's clearly indicated.

| mty_arg ->
let details =
match mty_param_opt with
| None -> None

This comment has been minimized.

@Drup

Drup Nov 26, 2017

Contributor

IIRC, this case only appears when the functor is generative and the argument should be (). So, current bug non-withstanding, we could also give a good error message here.

This comment has been minimized.

@sliquister

sliquister Nov 26, 2017

Author Contributor

It's not the only case. What can happen, and would have no details, is this:

module F(X : sig module type S module F : S end) = struct
  type t = X.F(Parsing).t
end

because we go through the _ -> None above, since the type of F is Mty_ident "X.S" or something.

Otherwise, yeah, we could reject functors that take (), anticipating a better behavior from the rest of the typer. In fact, fixing the typer sounds trivial.

We might be able to remove all the cases with no details.

@sliquister

This comment has been minimized.

Copy link
Contributor Author

sliquister commented Nov 26, 2017

Thanks for reviewing. I don't know what you don't like about the commits. Clearly, I don't want the make depend stuff mixed in with the rest, as it has nothing to do with my pull request but I am forced to run it. Adding tests, then fixing them is a standard way of showing the change of behavior in the diff.

In any case, I also fixed error reporting involving abstract signatures, as well as fixed MPR#7611 by rejecting F(M).t where F is generative. So technically, this feature doesn't only change the reporting of type errors, but the change to the typer proper is very simple (and it's extremely unlikely to break existing code).

Though now, the pull request could use some squashing, though I'll wait until it's accepted because it's much harder to split changes than to squash them.

@trefis

This comment has been minimized.

Copy link
Contributor

trefis commented Nov 27, 2017

Thanks for reviewing. I don't know what you don't like about the commits. Clearly, I don't want the make depend stuff mixed in with the rest, as it has nothing to do with my pull request but I am forced to run it. Adding tests, then fixing them is a standard way of showing the change of behavior in the diff.

I see changes to .depend in two commits, one clearly identified (the first commit, called "make depend") and another one in "Improve error reporting for ill-typed ap...".
The second one seems to be because, as you said, you had a dependency from typetexp to includemod.
Are you saying that the first "make depend" commit is unrelated to your PR and is due to people forgetting to run it in the past?

Also, I think Drup is confused because github's interface is bad and orders commits by date and not by dependency.

@sliquister

This comment has been minimized.

Copy link
Contributor Author

sliquister commented Nov 27, 2017

Are you saying that the first "make depend" commit is unrelated to your PR and is due to people forgetting to run it in the past?

Yes. It fact, while trying to see if I could write a CI check to reject changes that don't update .depend files, I learnt that there is such a thing as make alldepend that updates other .depend files like the ones in otherlibs, and it's even more out of date.

@trefis

This comment has been minimized.

Copy link
Contributor

trefis commented Nov 27, 2017

trying to see if I could write a CI check to reject changes that don't update .depend files

That'd be lovely.

| Use_generative_functor_as_applicative flid ->
fprintf ppf "@[%a is a generative functor,@ and@ so@ cannot@ be@ applied@ \
in@ type@ expressions@]"
longident flid

This comment has been minimized.

@Drup

Drup Dec 2, 2017

Contributor

That error message sounds wrong. Shouldn't be it _ is a generative functor but was applied to _. It should be applied to () ?

@Drup

This comment has been minimized.

Copy link
Contributor

Drup commented Dec 2, 2017

If people find the commit log fine, who am I to complain ? :p

The new commits look ok, but I don't find the error messages very good. I think the code factorization you did at the end does more harm than good: You should just follow the exact same wording than is used for functions: "This module has type _. It is not a functor, it cannot be applied". Also, something more explicit for generative functors that says it should be applied to () (most people don't know what generative and applicative functors are). I'll let error messages experts give their opinion (@Octachron, @gasche ? :)

@Octachron
Copy link
Contributor

Octachron left a comment

I like the changes, and I think that the improved error messages would be very useful to a large spectrum of users. However, I think it may be possible to do better by making the error messages less dry. I made few remarks in this direction.

| Ill_typed_functor_application lid ->
fprintf ppf "Ill-typed functor application %a" longident lid
| Ill_typed_functor_application (lid, details) ->
fprintf ppf "@[Ill-typed functor application %a%t@]"

This comment has been minimized.

@Octachron

Octachron Dec 2, 2017

Contributor

I find this error message too unprecise. For instance, in

Ill-typed functor application F(M)
Module types do not match
$1
is not included in
$2

I fear that it is not clear for an unattentive reader that $1 is the module type of M and $2is the expected type of the first argument of F . I would rather use a far less generic error message:

Ill-typed functor application F(M)
The functor F cannot be applied to M (* maybe redundant ?*)
The actual module type of the argument
$1
is not included in its expected module type
$2

This comment has been minimized.

@sliquister

sliquister Dec 3, 2017

Author Contributor

Yeah, the message sucks a bit for the reason you say, but at the same time, but I would expect people to understand what's wrong from $1 and $2 regardless.
I am not fond of overly verbose messages.

My message:

File "a.ml", line 2, characters 16-29:
Error: Ill-typed functor application Map.Make(M)
       Modules do not match: sig  end is not included in Map.OrderedType
       The value `compare' is required but not provided
       File "map.mli", line 51, characters 4-31: Expected declaration
       The type `t' is required but not provided
       File "map.mli", line 48, characters 4-10: Expected declaration

The same message at the module level:

File "a.ml", line 2, characters 22-23:
Error: Signature mismatch:
       Modules do not match: sig  end is not included in Map.OrderedType
       The value `compare' is required but not provided
       File "map.mli", line 51, characters 4-31: Expected declaration
       The type `t' is required but not provided
       File "map.mli", line 48, characters 4-10: Expected declaration

It's not any better and no one seems to be unhappy about it. Well the position is more narrow, I suppose.

This comment has been minimized.

@Octachron

Octachron Dec 3, 2017

Contributor

It's not any better and no one seems to be unhappy about it.

People may not hate these error messages enough to make a PR, but I have heard complaints about the fact that it is not clear which module is the interface and which one is the implementation.

I am not fond of overly verbose messages.

It is certainly better to strike a balance between clarity and verbosity. In this case, maybe a shorter version would be better

File "a.ml", line 2, characters 16-29:
Error: Ill-typed functor application Map.Make(M)
       The argument module type sig  end is not included in Map.OrderedType
       The value `compare' is required but not provided
       File "map.mli", line 51, characters 4-31: Expected declaration
       The type `t' is required but not provided
       File "map.mli", line 48, characters 4-10: Expected declaration

This gives us the same number of words, and only 8 more letters.

Notwithstanding my nitpickings, I agree than the current message is no worse than the existing inclusion errror message.

This comment has been minimized.

@sliquister

sliquister Dec 3, 2017

Author Contributor

I certainly agree with the complaint that it's unclear which module type is which.
In any case, I did change the message and I think it should mostly address your concern. I think the scope of this pull request has grown enough, I'd rather avoid doing any more.

This comment has been minimized.

@Octachron

Octachron Dec 4, 2017

Contributor

The reworded error message is nice. Thank you for humoring my nitpickings!

| `Generative_used_as_applicative ->
"a generative functor", "an applicative functor"
in
fprintf ppf "@[The module %a is %s,@ not %s@]" longident lid is but_used_as

This comment has been minimized.

@Octachron

Octachron Dec 2, 2017

Contributor

I share the opinion of @Drup, these error messages might end un being unscrutable for people not familiar with the module system since they ends un just classifying the type of the error rather than detailing the issue at hand.

I would replace

The module M is a structure, not a functor
The module M is abstract, not a functor
The module M is a functor, not a structure
The module M is abstract, not a structure
The module M is a generative functor, not an applicative functor

by

The module M cannot be applied, it is a structure not a functor
The abstract module M cannot be applied
As a functor, the module M does not have any components that can be accessed
As an abstract module, the module M does not have any components that can be accessed
The generative functor M can only be applied to ( )

@sliquister sliquister force-pushed the sliquister:ill-typed-functor branch 3 times, most recently from 2309ec1 to 747fe8c Dec 3, 2017

@sliquister

This comment has been minimized.

Copy link
Contributor Author

sliquister commented Dec 3, 2017

I changed a few error messages, maybe it's slighty better now.
I didn't change the error about generative functor to what you were both suggesting though, because my message is correct: in Generative(M).t, the error shouldn't say "you need to write Generative().t", because that makes no sense and in fact doesn't even parse.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 13, 2017

@Octachron, do you approve the change?

(There now are conflicts that need to be fixed in any case.)

@sliquister sliquister force-pushed the sliquister:ill-typed-functor branch from 747fe8c to cf2dd9b Dec 13, 2017

@Octachron
Copy link
Contributor

Octachron left a comment

I approve this helpful change, the code looks good and the error message are nice (and translatable).

@@ -64,10 +64,15 @@ type error =
| Unbound_class of Longident.t
| Unbound_modtype of Longident.t
| Unbound_cltype of Longident.t
| Ill_typed_functor_application of Longident.t
| Ill_typed_functor_application
of Longident.t * Longident.t * Includemod.error list option

This comment has been minimized.

@Octachron

Octachron Dec 13, 2017

Contributor

I have a small question here: is there a meaningful difference between Some [] and None for the include error detail?

This comment has been minimized.

@sliquister

sliquister Dec 14, 2017

Author Contributor

I think neither is possible, so well, it's hard to say what counts as different.

@Drup

Drup approved these changes Dec 13, 2017

Copy link
Contributor

Drup left a comment

I also quite like the new error messages, lgtm. :)

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 13, 2017

Before we merge, could you use a *-bullet for the changes entry for f03aaad, which is unrelated to error messages themselves and may break some (dubious but existing) code? You might want to write a couple extra line to show the example of codes that were previously accepted and are now rejected, so that users have an idea what to look for.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 13, 2017

Thinking about this again, I am uncomfortable with the mixing of separate concerns in this PR. @sliquister, could you please remove f03aaad before we merge, and submit it as a separate PR to be reviewed separately?

(I think that there is a mild risk that this change would reject more programs than expected, whereas change to the error-message machinery are less risky.)

@Octachron, @Drup, have you reviewed this one commit carefully, and do you vouch for its correctness?

@trefis

This comment has been minimized.

Copy link
Contributor

trefis commented Dec 13, 2017

@gasche I find your last message confusing. What is it that you are worried about?
When you say:

there is a mild risk that we would reject more programs than expected

what does "expected" mean?

Do you mean:

  • "there might be more programs than we thought that are using this feature"

or:

  • "the commit might contain a bug causing us to reject other programs than the ones using generative functors as applicatives constructors in type paths"

?

If it's the first option, then asking if the commit was reviewed carefully doesn't make much sense (well, I guess I just answered my own question right there). But I also think that we shouldn't be worried about the number of people using this "bug".
Everyone seems to agree it is a bug, let's just fix it.

If it's the second option, I have just reread the commit you pointed out and it looks correct to me.
(Some parts of typetexp looked a bit meh, but they have been cleaned up in later commits, so I'm happy with it)

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 13, 2017

Yes, I meant the second point.

@Octachron

This comment has been minimized.

Copy link
Contributor

Octachron commented Dec 13, 2017

@gasche , I vouch for the correctness of this commit: generative functor application should only happen inside module expression, and the parser takes care to not extend generative application (F()) to type expression (or module types or class types). For instance,

module module G() = struct type t end;;
type t = G().t;;

raises a syntax error.

This commit f03aaad merely closes the hole that was left open by this syntactic restriction on generative application by making sure that a lookup for the result of a generative function in the current environment never successes.

In fact, I would argue that with this commit, we are in the paradoxical situation that the rare conjunction of two mistakes (applying a generative functor inside a type expression/module type/class type expression and applying it to the wrong kind of argument) yields a better error message than the one for the root error.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 13, 2017

Ok, it seems there is consensus that keeping the commit in the present PR is fine. (I still don't like this mixing of concerns in a single PR.)

@gasche gasche merged commit 4b30d29 into ocaml:trunk Dec 13, 2017

2 checks passed

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

@sliquister sliquister deleted the sliquister:ill-typed-functor branch Dec 14, 2017

@sliquister

This comment has been minimized.

Copy link
Contributor Author

sliquister commented Dec 14, 2017

Alright, thanks everyone!

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.