Signature local bindings by trefis · Pull Request #2122 · ocaml/ocaml

trefis · 2018-10-26T15:38:53Z

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 · 2018-10-26T15:54:14Z

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 · 2018-10-26T17:21:32Z

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 · 2018-10-26T17:26:04Z

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 · 2018-10-26T17:33:51Z

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 · 2018-10-26T19:57:18Z

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 · 2018-10-29T10:47:39Z

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 · 2018-10-31T11:57:53Z

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 · 2018-10-31T13:40:32Z

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

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

lpw25 · 2018-11-06T15:23:26Z

parsing/parsetree.mli

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


Maybe pms_manifest to match the type declaration case?

lpw25 · 2018-11-06T15:23:43Z

parsing/parsetree.mli

         *)
  | Psig_type of rec_flag * type_declaration list
-        (* type t1 = ... and ... and tn = ... *)
+        (* type t1  = ... and ... and tn  = ... *)


Accidental change to whitespace.

lpw25 · 2018-11-06T15:53:28Z

parsing/parsetree.mli

  | 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 rec_flag should be removed because substitutions should all be "nonrec".

lpw25 · 2018-11-06T16:50:58Z

ocamldoc/odoc_sig.ml


-        | 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 *) ->


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

I will try to have a look.

lpw25 · 2018-11-06T16:55:52Z

typing/typemod.ml

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


Misspelling

lpw25 · 2018-11-06T16:57:16Z

typing/typemod.ml

  | 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 ]


No real need for a polymorphic variant here.

lpw25 · 2018-11-06T17:09:41Z

testsuite/tests/typing-sigsubst/sig_local_aliases_syntax_errors.compilers.reference

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


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

lpw25 · 2018-11-06T17:13:54Z

testsuite/tests/typing-sigsubst/sig_local_aliases.ml

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


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

lpw25 · 2018-11-06T17:36:56Z

typing/typemod.ml

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


What about private?

lpw25 · 2018-11-06T17:46:42Z

typing/typemod.ml

+              `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 should sometimes be an alias.

trefis · 2018-11-09T13:34:16Z

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").

type [params] id := type_expr { and [params] id := type_expr } module Uid := extended-module-path

lpw25

I think this is ready to merge now.

trefis mentioned this pull request Oct 26, 2018

Private items in structures and signatures #2016

Closed

garrigue mentioned this pull request Oct 27, 2018

Extending open to accept arbitrary module expression #1506

Closed

trefis force-pushed the sig-local-aliases branch from a8889c2 to fdc51ef Compare October 29, 2018 10:46

trefis force-pushed the sig-local-aliases branch from 12d72ea to 0db1c35 Compare October 30, 2018 16:18

lpw25 requested changes Nov 6, 2018

View reviewed changes

trefis force-pushed the sig-local-aliases branch from 093143e to 4f3638f Compare November 8, 2018 17:21

trefis added 4 commits November 9, 2018 16:43

introduce local substitutions in signatures

6805595

type [params] id := type_expr { and [params] id := type_expr } module Uid := extended-module-path

update testsuite

faa8e05

manual

9145aab

Changes

912b841

trefis force-pushed the sig-local-aliases branch from ceb5802 to 912b841 Compare November 9, 2018 16:46

lpw25 approved these changes Nov 9, 2018

View reviewed changes

Merge branch 'trunk' into sig-local-aliases

ebe5801

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

trefis deleted the sig-local-aliases branch November 12, 2018 10:15

yallop mentioned this pull request Dec 4, 2018

Provide let operators in the standard library #2170

Closed

2 tasks

jonludlam mentioned this pull request Feb 13, 2019

Odoc fails to compile with OCaml 4.08 ocaml/odoc#304

Closed

anmonteiro mentioned this pull request Nov 4, 2019

Upgrade Parser to 4.08 AST and add 4.08 features reasonml/reason#2487

Merged

yawaramin mentioned this pull request May 1, 2020

Support signature local bindings (4.08+) ocaml/odoc#433

Closed

yallop mentioned this pull request May 13, 2020

Type $t is not included in type $t #6634

Closed

314eter mentioned this pull request Aug 25, 2021

Support new features (OCaml 4.14) tree-sitter/tree-sitter-ocaml#54

Merged

Conversation

trefis commented Oct 26, 2018

Uh oh!

trefis commented Oct 26, 2018

Uh oh!

alainfrisch commented Oct 26, 2018

Uh oh!

alainfrisch commented Oct 26, 2018

Uh oh!

trefis commented Oct 26, 2018

Uh oh!

gasche commented Oct 26, 2018

Uh oh!

trefis commented Oct 29, 2018

Uh oh!

trefis commented Oct 31, 2018

Uh oh!

trefis commented Oct 31, 2018

Uh oh!

lpw25 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trefis commented Nov 9, 2018

Uh oh!

lpw25 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants