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

Simplify grammar & syntax (parens) of first class modules #1949

Merged
merged 6 commits into from
Jun 3, 2018

Conversation

IwanKaramazow
Copy link
Contributor

@IwanKaramazow IwanKaramazow commented May 23, 2018

Fixes #530
Fixes #1715

Goals

  1. Simplify menhir grammar by over-parsing
  2. Allow first-class modules with less parentheses

Result

  • Simpler constraints on let-bindings, uppercase automatically implies a Ptyp_package
* Less parentheses
```reason
/* before */
let f = ((module Add : S.Z), x) => Add.add(x);
module Modifier = (val (Db.Hashtbl.create (): (module Db.Sig with type t = Mods.t)));
let thing: (module Thing) = (module MyModule);

/* after */
let f = (module Add : S.Z, x) => Add.add(x);
module Modifier = (val Db.Hashtbl.create (): Db.Sig with type t = Mods.t);
let thing: module Thing = (module MyModule);
  • let module syntax will need to be deprecated, because of too much conflicts.
    Update: let module-syntax stays as is

@chenglou
Copy link
Member

let module is long deprecated and refmting to module Foo = .... So I think it's an ok time to get rid of it

@IwanKaramazow
Copy link
Contributor Author

IwanKaramazow commented Jun 1, 2018

@chenglou this is ready.
TLDR

  • simplify grammar where possible
  • less parens for first class modules (parsing + printing)
  • no deprecations, let module stays as is.

@chenglou
Copy link
Member

chenglou commented Jun 2, 2018

For let thing: Thing = (module MyModule), is it possible to still not accept it? Only let thing: module Thing = (module MyModule)?

I'm not too sure that we're at a stage where folks all use refmt yet

@IwanKaramazow
Copy link
Contributor Author

IwanKaramazow commented Jun 2, 2018

Sure, no problem, I'll restructure the grammar.

Update: done ✅

Iwan added 5 commits June 2, 2018 16:16
Might cause too much confusion for newcomers and people not using refmt.
@@ -1061,6 +1061,47 @@ let filter_raise_spread_syntax msg nodes =
raise Reason_syntax_util.(Error(dotdotdotLoc, (Syntax_error msg)))
| None -> node
) nodes

let err loc s =
raise Reason_syntax_util.(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not have a similar utility already? Genuine question. We should have a single one that throws a single type of exception

| pattern COLON core_type { mkpat(Ppat_constraint($1, $3)) }
| pattern COLON core_type
{ mkpat(Ppat_constraint($1, $3)) }
(* If we kill the `let module …` syntax, this can be placed inside pattern.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So apparently @jaredly still uses let module. Otherwise I think we can kill let module?

@jaredly
Copy link
Contributor

jaredly commented Jun 2, 2018

I super like let module because it's one less difference to memorize between statement-level syntax and expression-level syntax.
If we could make module X = Y work at the expression level, I'd be fine with removing let module everywhere

@hcarty
Copy link
Contributor

hcarty commented Jun 2, 2018

@jaredly Where does module X = Y not currently work in expressions? I thought that worked anywhere let module worked.

Edit: "not", not "it"

@jaredly
Copy link
Contributor

jaredly commented Jun 3, 2018 via email

@chenglou
Copy link
Member

chenglou commented Jun 3, 2018

We’ll kill it later. Keeping it for another version at least for now.

Thanks!

@chenglou chenglou merged commit fe79b90 into reasonml:master Jun 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants