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

Signature local bindings #2122

Merged
merged 5 commits into from Nov 12, 2018

Conversation

Projects
None yet
5 participants
@trefis
Copy link
Contributor

commented Oct 26, 2018

This is the revamped version of #2016: it allows some type and module aliases to be bound in a signature without being exported.
The chosen syntax is the one that @garrigue proposed, which is reminiscent of destructive substitution:

type t := int
module M := N

A few things to note:

  • support for module types (and class types?) wouldn't be absurd, but since the existing implementation of destructive substitution doesn't support them, I chose to omit them from this PR. I think we could add support (in both places) in a follow up PR.
  • the restriction to aliases is enforced in the parser. Unfortunately it means that if one tries to write type t := { x : int } the error message will be pretty bad.
    I could allow it in the parser and reject it later, e.g. from Ast_invariants, with a decent error message. But I'm hopeful that we could get good syntax error messages soon. Am I too optimistic?
  • I don't know how to name these new constructs. My current approximation is "signature local binding", but something with "substitution" / "substituted" / etc. in the name could be nice as well.
@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2018

Also: the code to remove these local bindings calls nondep, which isn't necessary. It was just convenient because it's already there and will be needed for the generalized open construction.

@lpw25 just voiced (offline) that he'd rather not use nondep in that case and simply use Subst.
I'll push a commit changing that in a couple of days.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

I could allow it in the parser and reject it later, e.g. from Ast_invariants, with a decent error message.

IMO, that would be better.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

Did you consider introducing new constructors in the AST (Parsetree and Typedtree) instead of adding a component to a tuple? That would reduce the impact on e.g. ppx rewriting, and should not make the code really more complex. Also, at the Typedtree level, you could use a more precise representation than the generic one (just a type expression, not a type declaration).

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2018

Did you consider introducing new constructors in the AST (Parsetree and Typedtree) instead of adding a component to a tuple? That would reduce the impact on e.g. ppx rewriting, and should not make the code really more complex. Also, at the Typedtree level, you could use a more precise representation than the generic one (just a type expression, not a type declaration).

I had a first implementation doing just that, but the churn was actually much bigger and I eventually gave up on it.
The current proposal is indeed a bit less precise, but I think the impact on ppxes should be roughly equivalent.
I could give it another go if you insist.

As for allowing the forbidden construction to parse and reject them later: I'll make the change; but I'd like to revisit that once we get menhir to generate nice syntax error messages.

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

But I'm hopeful that we could get good syntax error messages soon. Am I too optimistic?

Even with good support for error messages, LR messages will detect a situation of the sort type t := <not a type>: they will detect a failure on {. I don't think that we (as human error-message tuners) will have both the control and the courage to write different error message depending on whether the unexpected error message, at this point, is or not the prefix of a valid type declaration.

So you can reasonably hope to get "we were expecting a type (expression) here", but not "you are trying to write a type declaration, but only type synonyms are accepted", I think.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

I don't think that we (as human error-message tuners) will have both the control and the courage to write different error message depending on ...

I'm not sure we'll have the control, as for the courage: we have it right now to produce it from a different part of the compiler, so I don't think it would make much difference.
But the rest of your message makes sense.

I updated the code to emit a proper error message from typemod (doing it in Ast_invariant wouldn't have been good enough since that's only called when a rewriter has been applied).

@trefis trefis force-pushed the trefis:sig-local-aliases branch from 12d72ea to 0db1c35 Oct 30, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

I pushed two new commits:

  • the first one switches the use of nondep to Subst for sig-local bindings. It also cleans up the code around that area (removing one assert false!).
  • the second one adds support for module M := F(A), whereas the initial version of the code only allow module aliases. This is consistent with what destructive substitution supports.
    This commit also introduces specific constructors in the parsetree and typedtree: P/Tsig_modsubst. I will try again to do something similar for types and report back.
@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

OK I now also introduced a new constructor for type substitutions.
I think the thing that was annoying with my first experiment was that I tried to do the same thing that Alain suggested, i.e.

use a more precise representation than the generic one (just a type expression, not a type declaration)

Which was very painful. I'm currently not doing that, I still use a type declaration but that's precisely what destructive substitution uses.
Again, I think if we want to change that, we should change both together (and definitely not in the present PR).

This should now be ready for review: the history is a bit less clean than I'd have hoped, but the whole diff should be easy to read.

@lpw25
Copy link
Contributor

left a comment

There's a few things that need fixing, but other than those this looks good to merge.

and module_substitution =
{
pms_name: string loc;
pms_synonym: Longident.t loc;

This comment has been minimized.

Copy link
@lpw25

lpw25 Nov 6, 2018

Contributor

Maybe pms_manifest to match the type declaration case?

@@ -719,13 +719,18 @@ and signature_item_desc =
external x: T = "s1" ... "sn"
*)
| Psig_type of rec_flag * type_declaration list
(* type t1 = ... and ... and tn = ... *)
(* type t1 = ... and ... and tn = ... *)

This comment has been minimized.

Copy link
@lpw25

lpw25 Nov 6, 2018

Contributor

Accidental change to whitespace.

@@ -719,13 +719,18 @@ and signature_item_desc =
external x: T = "s1" ... "sn"
*)
| Psig_type of rec_flag * type_declaration list
(* type t1 = ... and ... and tn = ... *)
(* type t1 = ... and ... and tn = ... *)
| Psig_typesubst of rec_flag * type_declaration list

This comment has been minimized.

Copy link
@lpw25

lpw25 Nov 6, 2018

Contributor

This rec_flag should be removed because substitutions should all be "nonrec".

@@ -904,7 +908,8 @@ module Analyser =
let new_env = Odoc_env.add_extension env e.ex_name in
(maybe_more, new_env, [ Element_exception e ])

| Parsetree.Psig_type (rf, name_type_decl_list) ->
| Parsetree.Psig_type (rf, name_type_decl_list)
| Parsetree.Psig_typesubst (rf, name_type_decl_list) (* FIXME *) ->

This comment has been minimized.

Copy link
@lpw25

lpw25 Nov 6, 2018

Contributor

Maybe we can do better here. Perhaps @Octachron would like to think about how we want to display these in OCamldoc's output.

This comment has been minimized.

Copy link
@Octachron

Octachron Nov 9, 2018

Contributor

I will try to have a look.

user_kind: Sig_component_kind.t;
user_loc: Location.t;
}
type hidding_error =

This comment has been minimized.

Copy link
@lpw25

lpw25 Nov 6, 2018

Contributor

Misspelling

@@ -92,6 +93,7 @@ type error =
| Cannot_scrape_alias of Path.t
| Badly_formed_signature of string * Typedecl.error
| Cannot_hide_id of hidding_error
| Invalid_subst_rhs of [ `Module | `Type ]

This comment has been minimized.

Copy link
@lpw25

lpw25 Nov 6, 2018

Contributor

No real need for a polymorphic variant here.

Line 3, characters 15-18:
3 | module M1 := sig end
^^^
Error: Syntax error: 'end' expected

This comment has been minimized.

Copy link
@lpw25

lpw25 Nov 6, 2018

Contributor

I think you need an error production in the parser to give this a better error message

^^^^
Error: Unbound module N
|}]

This comment has been minimized.

Copy link
@lpw25

lpw25 Nov 6, 2018

Contributor

I suggest adding tests for type ... and and potentially recursive substitutions somewhere in this file.

Typedecl.transl_type_decl env rec_flag sdecls
in
List.iter (fun td ->
if td.typ_kind <> Ttype_abstract || td.typ_manifest = None

This comment has been minimized.

Copy link
@lpw25

lpw25 Nov 6, 2018

Contributor

What about private?

`Substituted_away (Subst.add_module id path Subst.identity)
in
Signature_names.check_module ~info names pms.pms_name.loc id;
let newenv = Env.enter_module_declaration id md env in

This comment has been minimized.

Copy link
@lpw25

lpw25 Nov 6, 2018

Contributor

This should sometimes be an alias.

@trefis trefis force-pushed the trefis:sig-local-aliases branch from 093143e to 4f3638f Nov 8, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2018

OK I pushed a few more commits addressing @lpw25's comments (there should be one commit per review comment).
I also updated the "language extensions" chapter of the manual to describe the new constructions (that I have named "local substitution[ declaration]s").

trefis added some commits Oct 26, 2018

introduce local substitutions in signatures
    type [params] id := type_expr { and [params] id := type_expr }
    module Uid := extended-module-path

@trefis trefis force-pushed the trefis:sig-local-aliases branch from ceb5802 to 912b841 Nov 9, 2018

@lpw25

lpw25 approved these changes Nov 9, 2018

Copy link
Contributor

left a comment

I think this is ready to merge now.

@trefis trefis merged commit 4333d9b into ocaml:trunk Nov 12, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@trefis trefis deleted the trefis:sig-local-aliases branch Nov 12, 2018

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.