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

Allow shadowing of items coming from an include #1892

Merged
merged 4 commits into from Jul 25, 2018

Conversation

@trefis
Copy link
Contributor

commented Jul 10, 2018

This GPR implements the change described in https://blog.janestreet.com/plans-for-ocaml-408/#shadowing-of-items-from-include.

Most of the actual work happens in typemod.ml and revolves around check_name and simplify_signature.

Previously:

  • check_name was checking that all names in a signature were unique (excluding value names for which the check wasn't called)
  • simplify_signature was dropping shadowed value names from a signature.

Now:

  • check_name keeps track of what can (or not) be shadowed, and errors when one tries to shadow something that's not allowed to be
  • simplify_signature drops shadowed items from a signature, and calls nondep_* to remove reference to the shadowed items from the ones that remain. That last step is not guaranteed to succeed, and a error message is printed when it fails.

All the other changes qualify as plumbing (to get vaguely decent error messages, to not call nondep_* in a loop, etc.).

I'm not all that fond of the error message that is emitted when something cannot be shadowed, (see testsuite/tests/shadow_include/cannot_shadow_error.{ml,compilers.reference} for an example), so if someone has a better formulation, I'd be happy to integrate it.

The last commit ("check for repeated name after having typed the item") is not strictly necessary for this PR, but it makes the implementation a bit more regular.

A note on performances: for the moment we always call nondep_, even though there might be no ident to hide (more clearly: we should call nondep only when an ident which can appear in a type gets shadowed). I plan to add one last commit to that, but I think one can already start to look at this PR even without that.

@Drup

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2018

@gasche Can I get enough rights to assign myself?

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

I'm not the right person to ask, but it seems implausible to me; I cannot assign you either, it looks like the set of assignables is included in people with write access. I think the easiest route would be for you to get write access after contributing a bit more :-)

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2018

I added the announced commit to only call nondep_ when it's actually necessary, as well as another "cosmetic" one.

Thanks for volunteering @Drup!

@trefis trefis force-pushed the trefis:shadow-includes branch 2 times, most recently from 0d1d4a4 to 9d947a0 Jul 12, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2018

Note: check-typo is still failing because the error message emitted goes over 80c...
Again: I welcome any suggestion regarding that error message ( @gasche , @Octachron ?).

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

@trefis - in this case, it's OK to add the files to .gitattributes (see below pr7160.ml in the file)

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

It is possible that at some point we may make check-typo expect-test aware ... but at the moment I'd like to do some work which feels like it moves the actual compiler forwards! 😄

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2018

Thanks @dra27 but in this case I don't think it's OK :p
I'm really wishing for that error message to magically improve without my help!

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

In terms of its length, I think that from the compiler it goes through Format, and so is wrapped? I think it's only the tests which hit this, because the "screen" is assumed to be infinitely wide?

Or do you hope for a nicer wording which comes in under 80 chars?

| Cannot_hide_id (item_kind, id, kind, id', shadow_loc) ->
fprintf ppf
"The %s name %s coming from this include cannot be shadowed as it is \
used later on by %s %s@ %a"

This comment has been minimized.

Copy link
@Armael

Armael Jul 12, 2018

Member

I think this message should include break points, to avoid having an error spanning >80 columns as shown in the tests.

This comment has been minimized.

Copy link
@Armael

Armael Jul 12, 2018

Member

Oops well sorry, I just noticed it was already being discussed actively!

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

Looking at the error message

File "cannot_shadow_error.ml", line 22, characters 2-19:
Error: The type name t coming from this include cannot be shadowed as it is used later on by value print

File "cannot_shadow_error.ml", line 23, characters 2-36:
Shadowing of type name t

the order of the two message seems a bit strange: the primary message points to a statement that is only erroneous due to the the second error.

Would it make sense to reverse the two:

File "cannot_shadow_error.ml", line 23, characters 2-36:
Error: Shadowing of included type t \n
File "cannot_shadow_error.ml", line 22, characters 2-19:
Type t was included here and was then used in value print
Included types can only be shadowed if they are not used afterward

With this the error follows a schema

  • direct cause
  • root cause
  • explanation

that should contain all information and allow users to skip later parts of the message.

Also how hard would it be to give a location for the component depending on the shadowed component? With such a location, another error message could be

File "cannot_shadow_error.ml", line 23, characters 2-36:
Error: Shadowing of included type t
File "cannot_shadow_error.ml", line ??, characters ??-??:
The value print depends on this shadowed type t
Included types can only be shadowed if no components depend on them

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2018

The reordering makes a lot of sense, thanks!
Getting the location of the item that depends on something which was shadowed is also fairly easy, but I think I'd still like to keep the location of the item that gets shadowed in the error. Do you want to have a go at writing a decent error message containing 3 locations ? :)

Included types can only be shadowed if no components depend on them

That explanation is potentially confusing, consider the following valid examples of usage and shadowing:

# module M = struct type t1 type t2 = int end;;
module M : sig type t1 type t2 = int end
# module N = struct
    include M
    type u = t1
    let f (x : t2) = x + 3
    type t1 = float
    type t2
  end;;
module N : sig type u = M.t1 val f : int -> int type t1 = float type t2 end
# module O = struct
    include struct type t1 type t2 = int end
    type u = t1
    let f (x : t2) = x + 3
    type t1 = float
    type t2
  end;;
module O : sig type u val f : int -> int type t1 = float type t2 end

against:

# module P = struct
    include struct type t end
    let f (_ : t) = ()
    type t
  end;;
Error: The type name t coming from this include cannot be shadowed as it is used later on by value f

How about: "Included (types|modules|...) can only be shadowed if they aren't needed to typecheck other items" or something similar (but better)?

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 13, 2018

File "cannot_shadow_error.ml", line 23, characters 2-36:
Error: Illegal shadowing of included type t/123 by t/1234
File "cannot_shadow_error.ml", line 22, characters 2-19:
Type t/123 was included here
File "cannot_shadow_error.ml", line ??, characters ??-??:
The value print has no valid type if included t/123 is shadowed
@gasche

This comment has been minimized.

Copy link
Member

commented Jul 13, 2018

The blog post that you refer to gives a relevant example of why we would like the feature on include. But could you explain why it is not desirable to have the feature for other ways of defining a structure item in the module?

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2018

Thanks for the proposed error message @gasche, I like it!

The blog post that you refer to gives a relevant example of why we would like the feature on include. But could you explain why it is not desirable to have the feature for other ways of defining a structure item in the module?

Well, the difference between the two kind of items (the ones coming from an include, and the others) lies in the different usage, during their typing, of the type information that is propagated forward.

That is: during the typing of a module M, when one encounters a type declaration t (one could also consider a module or any other item), and one knows that M exports a t, then one can assume that it is that same t.
At the moment, that assumption is only used for typing recursive modules (and is necessary for doing so), but Leo mentioned offline that the information will also be needed for implicits.

However, that information is much less likely to be useful for items coming from includes, because one usually includes a module path, in which case there is no benefit in knowing the expected type.
And indeed, the assumption is currently not used for items coming from includes. So if one takes the example from Xavier's paper:

$ cat recmod.ml
module rec A : sig
  type t
  val x : unit
end = struct
  type t = C
  let x = B.f C
end
and B : sig
  val f : A.t -> unit
end = struct
  let f _ = ()
end
$ ocamlc -i recmod.ml
module rec A : sig type t val x : unit end
and B : sig val f : A.t -> unit end

and modify it a bit:

$ cat recmod.ml
module rec A : sig
  type t
  val x : unit
end = struct
  include struct type t = C end
  let x = B.f C
end
and B : sig
  val f : A.t -> unit
end = struct
  let f _ = ()
end
$ ocamlc -i recmod.ml
File "recmod.ml", line 6, characters 14-15:
Error: This expression has type t but an expression was expected of type A.t

That difference makes it very easy to shadow items coming from an include, whereas to allow shadowing of other items, one would have to know when trying to type them if they are shadowed later on or not (i.e. if they are the one in the signature or not), to decide to strengthen or not.
Incidentally, the next item in the blog post proposes to add private items, which would give us that information: if an item is private, it won't make it in the signature anyway, so we don't need to strengthen it, and we can shadow it. But that's a discussion for another day.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 13, 2018

Thanks for the explanation.

cc @Armael with whom I had discussion about error message formats.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2018

Note: I updated the error message earlier today and it’s actually hard to get something that looks good when printed from the compiler, the toplevel and expect_test (that last one being less important of course, but still).

@Drup

Drup approved these changes Jul 19, 2018

Copy link
Contributor

left a comment

I think the feature is very useful, so I'm fairly happy about this PR. The actual implementation looks good.

The changes can be divided into two parts:

  1. Various changes to nondep to give it multiple ids at the same time. This is mostly for efficiency purposes.
    This is very uniform, and I trust @trefis did the plumbing correctly.
  2. The changes to typemod that @trefis described above. This is quite a bit simpler than I expected and the code looks clean. I'm confident that the implementation does what @trefis say it does, and the reasoning for the changes look sound to me (in particular, I don't think other module weird features are going to explode in our faces).

So, most of my remarks are syntactic, regarding comments and coding style, but none of them are truly blocking. The tests look comprehensive enough.

One thing that I like is that the implementation is fairly flexible. Currently, the shadowing mechanism only triggers for includes, but everything is there if needed for other constructs (and will be used for the upcoming patch for private fields, I presume).

It also completely conflicts with my Eliom patches, but I'm the only caring about that. :p

end
| None ->
Tconstr(p, List.map (nondep_type_rec env ids) tl, ref Mnil)
end

This comment has been minimized.

Copy link
@Drup

Drup Jul 19, 2018

Contributor

Is that change simply a performance optimization to avoid changing the type if it's not one of the ids to remove?

This comment has been minimized.

Copy link
@trefis

trefis Jul 20, 2018

Author Contributor

Not sure which change you're referring too (the one above or the one below?), but neither of them is here to avoid modifying the type. It's just "plumbing", i.e. the behavior doesn't change from the previous version, it's just that instead of if Path.is_free we have match Path.find_free_opt

This comment has been minimized.

Copy link
@Drup

Drup Jul 20, 2018

Contributor

Ah, Indeed, sorry!

@@ -150,67 +150,72 @@ let scrape_for_type_of env mty =

This comment has been minimized.

Copy link
@Drup

Drup Jul 19, 2018

Contributor

You should change the comment above.
Although, honestly, I'm not even sure what the comment is really trying to say.

This comment has been minimized.

Copy link
@trefis

trefis Jul 20, 2018

Author Contributor

That comment is still valid AFAICT, I think the only modification that could be done is to change In nondep_supertype to In nondep_supertype and nondep_sig_item.

Raise [Not_found] if no such type exists. *)
val nondep_sig_item: Env.t -> Ident.t list -> signature_item -> signature_item
(* FIXME: doc *)

This comment has been minimized.

Copy link
@Drup

Drup Jul 19, 2018

Contributor

📃

| Papply(p1, p2) ->
match find_free_opt ids p1 with
| None -> find_free_opt ids p2
| Some _ as res -> res

This comment has been minimized.

Copy link
@Drup

Drup Jul 19, 2018

Contributor

Path.find_free_opt will only find one free identifier at the time, but due to functor application in path, there could be multiple one. This doesn't seem problematic for this particular patch since, as far as I can see, the returned ids are only used in error messages, but this doesn't seem like a very future-proof semantics. It might be better to return a list option.

This comment has been minimized.

Copy link
@trefis

trefis Jul 20, 2018

Author Contributor

I agree with you (though I would probably simply return a list and not a list option). But I don't think it's worth doing right now.
It's easy enough to update if the need ever arises.

match kind with
| Kind.Value
| Kind.Extension_constructor -> lst
| _ -> id :: lst

This comment has been minimized.

Copy link
@Drup

Drup Jul 19, 2018

Contributor

I think the predicate which says if the id associated with a declaration can appear in types or not should be promoted to a real predicate (maybe put in in the new Kind module), and spell out all the constructors, to avoid having a fragile pattern.

I would add classes and class types there, as the associated types are already split and it makes the intent clearer, but that's up to you.

| Sig_class (id, _, _) ->
check_class names loc id ~info:(Shadowable (id, loc))
| Sig_class_type (id, _, _) ->
check_class_type names loc id ~info:(Shadowable (id, loc))

(* Simplify multiple specifications of a value or an extension in a signature.
(Other signature components, e.g. types, modules, etc, are checked for
name uniqueness.) If multiple specifications with the same name,
keep only the last (rightmost) one. *)

This comment has been minimized.

Copy link
@Drup

Drup Jul 19, 2018

Contributor

Another comment to update.

@@ -651,64 +682,131 @@ let check_recmod_typedecls env sdecls decls =

(* Auxiliaries for checking uniqueness of names in signatures and structures *)

This comment has been minimized.

Copy link
@Drup

Drup Jul 19, 2018

Contributor

Another comment to update.

| Extension_constructor -> "extension constructor"
| Class -> "class"
| Class_type -> "class type"
end

This comment has been minimized.

Copy link
@Drup

Drup Jul 19, 2018

Contributor

Bikesheding time: I like the new type/module, but I don't like the name. Declaration_kind might be better. @gasche, propositions ? :)

| None -> ()
| Some (id, infos) ->
to_be_removed := Ident.Map.add id infos !to_be_removed
in

This comment has been minimized.

Copy link
@Drup

Drup Jul 19, 2018

Contributor

As far as I can tell, check_name (and its descendants) and rm_id are always used together. I would propose to just merge them and add an hashtbl argument to check_name that collects the identifiers to be removed.

val from : Foo.Bar.t -> t
module Bar : sig type t = Foo.Bar.t val int : int end
end
|}]

This comment has been minimized.

Copy link
@Drup

Drup Jul 19, 2018

Contributor

Here I would add a test for the associated module type that a user would put in the signature:

module type Extended_sig = sig
  include module type of struct include Foo end
  module Bar : sig
    include module type of Bar
    val int : int
  end
end

This comment has been minimized.

Copy link
@trefis

trefis Jul 20, 2018

Author Contributor

Indeed, done.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

I think that the design rationale is well-justified and the design choices convincing. I haven't reviewed the implementation, but I am inclined to trust @Drup's review, and approve and merge sometime next week, once the more minor remarks have been addressed and the patchset has converged to a state that both @trefis and @Drup are happy with.

cc @xavierleroy, @garrigue and @alainfrisch, in case they have concerns with this PR.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2018

Note: I plan to address Drup's remarks tomorrow. I also plan to do an ultimate pass on the error message, but I'm currently waiting to see if @Armael, who is apparently working on cleaning up the way errors are printed, can makes things easier for me in that area.

@trefis trefis force-pushed the trefis:shadow-includes branch from 4f6eae0 to dd4c603 Jul 20, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2018

@Drup: I updated the code according to your comments.

@Drup

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2018

The update looks good and I think all my remarks are addressed. I'll let other people discuss error messages!

@gasche

gasche approved these changes Jul 20, 2018

Copy link
Member

left a comment

Approving on @Drup's behalf.

@trefis: I'm not sure it is necessary to wait for Armaël -- we don't know how long he needs to converge, and he can always update your message structure after the fact. Do as you wish.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2018

Ack, thanks.
I'll wait a few more days, that way the people you've pinged can still comment if they wish.

If nothing happens, I'll rebase, cleanup the history, and merge some time next week.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2018

Also, thanks for the review Drup!

@trefis trefis force-pushed the trefis:shadow-includes branch from dd4c603 to ee0b20a Jul 25, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

I rebased and squashed most of the commits.
The error messages are now pretty in all the backends thanks to @Armael.

Unless someone objects, I will merge once travis and appveyor confirm everything is fine.

@trefis trefis merged commit 429f42d into ocaml:trunk Jul 25, 2018

2 checks passed

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

@trefis trefis deleted the trefis:shadow-includes branch Jul 25, 2018

@trefis trefis referenced this pull request Nov 6, 2018

ejgallego added a commit to ejgallego/dune that referenced this pull request Nov 21, 2018

[stdune] Support for OCaml 4.08 `List.t` type.
We piggyback on ocaml/ocaml#1892 "Allow shadowing of items coming from
an include" to support OCaml 4.08 and avoid a conflict with OCaml's
new `List.t`.

Thanks to @diml for pointing this out.

Note the comment from @diml regarding `Stdune.List`:

> Ideally, we shouldn't use include and manually import the function we use in dune. That would make the code more robust against upstream changes.

This is left to future work.

Signed-off-by: Emilio Jesus Gallego Arias <e+git@x80.org>

ejgallego added a commit to ejgallego/dune that referenced this pull request Nov 21, 2018

[stdune] Support for OCaml 4.08 `List.t` type.
We piggyback on ocaml/ocaml#1892 "Allow shadowing of items coming from
an include" to support OCaml 4.08 and avoid a conflict with OCaml's
new `List.t`.

Thanks to @diml for pointing this out.

Note the comment from @diml regarding `Stdune.List`:

> Ideally, we shouldn't use include and manually import the function we use in dune. That would make the code more robust against upstream changes.

This is left to future work.

Signed-off-by: Emilio Jesus Gallego Arias <e+git@x80.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.