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

Uniform parenthesized syntax for abstraction and application #1299

Merged
merged 108 commits into from Aug 18, 2017

Conversation

Projects
None yet
@let-def
Contributor

let-def commented Jun 14, 2017

This PR pushes #1236 a deeper.
It is not yet ready for merging but some feedback could help further development

Expressions

/* Abstraction */
() => ...                      /* function taking unit */
(a) => ...                     /* function of one argument */
(a : int, b) => ...            /* function of two arguments, first being of type `int` */
((a,b)) => ...                 /* function of one argument that is a tuple */
fun (a,b) : t => ...           /* function of two arguments returning a `t` */
(:x, :y) => ...                /* function with two labelled arguments `x` and `y` */
(:x : int, :y : string) => ... /* function with two labelled arguments `x` and `y` of types `int` and `string` */
(:x = 5, :y = ?, :z : string = ?, ()) => ... /* function with three optional arguments and a unit, first defaulting to the integer `5`, second has no default, third has no default but should be a string */

/* Application */
f()                    /* applying f to `()` */
f(x)                   /* applying f to `x` */
f(x,y)                 /* applying f to `x` and `y` */
f((x,y))               /* applying f to the tuple made of `x` and `y` */
f(:x)                  /* applying labelled argument `x` */
f(:x?, ())             /* applying optional argument `x` and `()` */
f(:x 5, :z? f(x), ())  /* applying with explicit values */

Arrow type

(a) => b      /* functions from a to b */
(a, b) => c   /* functions from a to (b to c), equivalent to (a) => (b) => c */
((a, b)) => c /* functions from a tuple (a, b) to c */
(:x int, :y string) => c /* functions with two labelled arguments to c */
(:x a?) => b  /* functions with an optional argument x of type a to b */

Type constructors

/* Abstraction */
type t = ...           /* Ground type */
type t('a) = ...       /* One parameter, a */
type t('a, 'b) = ...   /* Two parameters */
class type t = ...     /* Same with class types */
class type t('a) = ... 
class type t('a, 'b) = ... 

/* Application */
t        /* constant type */
t(a)     /* t with actual parameter a */
t(a,b)   /* ... */

Value constructors

/* Definition */
type t =
  | A                 /* no arguments */
  | B(int)            /* one argument of type int */
  | C(int, int)       /* two arguments that are of type int */
  | D((int, int))     /* one argument that is a tuple of int */
  | E{x: int, y: int} /* two named arguments (inline record) */

/* Construction */
A
B(1)
C(1, 2)
D((1, 2))
E{x: 1, y: 2}

/* Elimination */
let f = fun
  | A => ...
  | B(x) => ...
  | C(x, y) => ...
  | D((x, y)) => ...
  | D(tuple) => ...
  | E {x, y} => ...

/* Wildcard */
let f = fun
  | A(_) | B(_) | C(_) => ...
  | E(_) => ...
  | E {x, _} => ... 

Modules

/* Application */
F1(Int)
F2(Int,String)
Effectful()
Effectful2(Int,())
Effectful2(Int)()

/* Abstraction */
module F1 (A : ASig) = { ... };
module F2 (A : ASig, B : BSig) () = { ... };
module Effectful () = { ... }
module Effectful2 (A : ASig, ()) = { ... }

/* Module types */
() => ...          /* One "unit" argument */
(A : ASig) => ...  /* One dependent argument */
(ASig) => ...      /* One non dependent argument */
(A : ASig, B : BSig) => ... /* Two arguments */

Bindings

Previously, = was used for binding most values but the sugar for binding functions used =>.
Now = is always allowed and { ... } can be used for function bodies.

let add(a,b) = a + b;

let add(a,b) {
  a + b
};

let immediate_object(a,b) = {
  pub a = a;
  pub b { b };
};

Classes

class a(x) {
  /* one value parameter x */
  pub a = 0;
  pub a() = 0;
  pub a(x,y) = x + y;
  pub a(x,y) {
    let result = x + y;
    print_endline(" x + y = " ++ string_of_int(x) ++ " + " ++ string_of_int(y) ++ " = " ++ string_of_int(result));
    result
  };
} 

class container('a)(x) {
  /* one type parameter 'a, one value parameter x */
} 

let-def and others added some commits Apr 27, 2017

replace mapper chain by a mapper composition
GC stat after
==============

parsing:
        allocated_words: 37657442
        minor_words: 15582165
        promoted_words: 2166069
        major_words: 24241346
        minor_collections: 103
        major_collections: 24
        heap_words: 7015424
        heap_chunks: 20
        top_heap_words: 7015424
        compactions: 0

reformatting:
        allocated_words: 153497667
        minor_words: 131135436
        promoted_words: 36777601
        major_words: 59139832
        minor_collections: 553
        major_collections: 41
        heap_words: 9278464
        heap_chunks: 22
        top_heap_words: 9278464
        compactions: 0

GC stat before
==============

parsing:
        allocated_words: 41243812
        minor_words: 19168535
        promoted_words: 3911746
        major_words: 25987023
        minor_collections: 117
        major_collections: 25
        heap_words: 7015424
        heap_chunks: 20
        top_heap_words: 7015424
        compactions: 0

reformatting:
        allocated_words: 159464070
        minor_words: 137101839
        promoted_words: 39600932
        major_words: 61963163
        minor_collections: 576
        major_collections: 43
        heap_words: 8068096
        heap_chunks: 21
        top_heap_words: 8068096
        compactions: 0
optimise replace_string
GC stat after
=============

parsing:
        allocated_words: 35870858
        minor_words: 13795581
        promoted_words: 2141052
        major_words: 24216329
        minor_collections: 96
        major_collections: 24
        heap_words: 7015424
        heap_chunks: 20
        top_heap_words: 7015424
        compactions: 0

reformatting:
        allocated_words: 149597673
        minor_words: 127235442
        promoted_words: 36784443
        major_words: 59146674
        minor_collections: 539
        major_collections: 42
        heap_words: 395776
        heap_chunks: 1
        top_heap_words: 9278464
        compactions: 2
refactor lexbuf copy
Replace a global buffer by a buffer local to parsing code.

This fixes a bug when processing multiple inputs. For instance with
refmt a.re b.re, b.re comments will (incorrectly) be read from a buffer
that begins with content from a.
@chenglou

This comment has been minimized.

Show comment
Hide comment
@chenglou

chenglou Aug 2, 2017

Contributor

@let-def semicolon is still the biggest hurdle I've seen from working with newcomers. Btw I'm very, very interested in seeing #1235 in the future. I'd say for now, special-casing.

Contributor

chenglou commented Aug 2, 2017

@let-def semicolon is still the biggest hurdle I've seen from working with newcomers. Btw I'm very, very interested in seeing #1235 in the future. I'd say for now, special-casing.

@chenglou

This comment has been minimized.

Show comment
Hide comment
@chenglou

chenglou Aug 5, 2017

Contributor

@let-def I'm getting the following:

File "src/reason_toolchain.ml", line 349, characters 51-60:
Error: This expression has type
         string Migrate_parsetree.Ast_404.Location.loc *
         Migrate_parsetree.Ast_404.Parsetree.payload
       but an expression was expected of type
         Parsetree.attribute = string Asttypes.loc * Parsetree.payload
       Type
         Migrate_parsetree.Ast_404.Parsetree.payload =
           Ast_404.Parsetree.payload
       is not compatible with type Parsetree.payload 

Freshly updated OPAM, OCaml 4.03.0

Edit: works with 4.04

Contributor

chenglou commented Aug 5, 2017

@let-def I'm getting the following:

File "src/reason_toolchain.ml", line 349, characters 51-60:
Error: This expression has type
         string Migrate_parsetree.Ast_404.Location.loc *
         Migrate_parsetree.Ast_404.Parsetree.payload
       but an expression was expected of type
         Parsetree.attribute = string Asttypes.loc * Parsetree.payload
       Type
         Migrate_parsetree.Ast_404.Parsetree.payload =
           Ast_404.Parsetree.payload
       is not compatible with type Parsetree.payload 

Freshly updated OPAM, OCaml 4.03.0

Edit: works with 4.04

@chenglou

This comment has been minimized.

Show comment
Hide comment
@chenglou

chenglou Aug 5, 2017

Contributor

Putting this here in case we forget:

  • We should special-case switch ((a, b)) to switch (a, b)
  • Does this mean array access with foo[0] can be unambiguous now?
  • let f(a) = a is gonna re-trip up lots of folks. Apparently let f(a) => a is hard; maybe we should reconsider the sugar and set for the uniform let f = (a) => a
Contributor

chenglou commented Aug 5, 2017

Putting this here in case we forget:

  • We should special-case switch ((a, b)) to switch (a, b)
  • Does this mean array access with foo[0] can be unambiguous now?
  • let f(a) = a is gonna re-trip up lots of folks. Apparently let f(a) => a is hard; maybe we should reconsider the sugar and set for the uniform let f = (a) => a
@let-def

This comment has been minimized.

Show comment
Hide comment
@let-def

let-def Aug 5, 2017

Contributor

Does this mean array access with foo[0] can be unambiguous now?

Ahah yes, though for now I chose to allow f[0] and f{x,y} as shortcuts for f([0]) and f({x,y}). However f[0] for array-access is probably a better use of syntax estate.

Contributor

let-def commented Aug 5, 2017

Does this mean array access with foo[0] can be unambiguous now?

Ahah yes, though for now I chose to allow f[0] and f{x,y} as shortcuts for f([0]) and f({x,y}). However f[0] for array-access is probably a better use of syntax estate.

@hcarty hcarty referenced this pull request Aug 10, 2017

Closed

Implement decorators #1112

@let-def

This comment has been minimized.

Show comment
Hide comment
@let-def

let-def Aug 12, 2017

Contributor

Ok, I implemented a[x] for array access.
This has two side-effects:

  • f[x] is no longer a sugar for f([x]) (obviously)
  • in JSX lists, we have to choose whether <tag>a[x] </tag> means "ident a then list [x]" or "index x of array a".

Right now the second interpretation is chosen, which means that a list in a JSX list has to be wrapped between parentheses. I am not familiar enough with JSX to know what is more appropriate.

Contributor

let-def commented Aug 12, 2017

Ok, I implemented a[x] for array access.
This has two side-effects:

  • f[x] is no longer a sugar for f([x]) (obviously)
  • in JSX lists, we have to choose whether <tag>a[x] </tag> means "ident a then list [x]" or "index x of array a".

Right now the second interpretation is chosen, which means that a list in a JSX list has to be wrapped between parentheses. I am not familiar enough with JSX to know what is more appropriate.

@chenglou

This comment has been minimized.

Show comment
Hide comment
@chenglou

chenglou Aug 12, 2017

Contributor

Latter is fine for now; I don't think I've seen this pattern very often

Contributor

chenglou commented Aug 12, 2017

Latter is fine for now; I don't think I've seen this pattern very often

@hcarty

This comment has been minimized.

Show comment
Hide comment
@hcarty

hcarty Aug 14, 2017

Contributor

Could a{i, j, k} be used for bigarray access?

Contributor

hcarty commented Aug 14, 2017

Could a{i, j, k} be used for bigarray access?

@chenglou

This comment has been minimized.

Show comment
Hide comment
@chenglou

chenglou Aug 15, 2017

Contributor

@let-def this is still weird:

make(:foo bar)[||]

The array should be on the inside.

Contributor

chenglou commented Aug 15, 2017

@let-def this is still weird:

make(:foo bar)[||]

The array should be on the inside.

@rickyvetter

This comment has been minimized.

Show comment
Hide comment
@rickyvetter

rickyvetter Aug 16, 2017

Member

I'm concerned about the overloading of colon for types and named arguments. I think it will make things confusing coming from typescript/flow. Since named arguments are foreign to JS, would it make sense to keep the OCaml syntax and use ~ to denote a named arg?

Member

rickyvetter commented Aug 16, 2017

I'm concerned about the overloading of colon for types and named arguments. I think it will make things confusing coming from typescript/flow. Since named arguments are foreign to JS, would it make sense to keep the OCaml syntax and use ~ to denote a named arg?

@chenglou

This comment has been minimized.

Show comment
Hide comment
@chenglou

chenglou Aug 18, 2017

Contributor

Ok folks, time to merge this.

We've cut a release before this PR that includes the pipe formatting PR (a sore refmt bug before, thanks again @IwanKaramazow). It's now live on OPAM. Don't forget, 1.13.7, not 2.0.0. We've decided on a final release before landing this PR because we didn't want it to look like we're conning you into using this syntax just for the pipe formatting =P.

If you were/are against this function syntax change, hopefully you can see that we try our best to make sure folks are happy with most of our changes! And thanks for the discussion so far. We've let it drag on a year because we understand this is a big topic. If we've ignored any opinion, it's accidental.

Additionally, as @let-def pointed out, the change does have some essence beyond pure bikeshedding. If the syntax ends up unpopular, then we can always revisit this decision. But give us some time to iterate on it.

Speaking of iteration: we will be giving you a smooth upgrade path when this is ready. The PR's the first step; the outcome printer, some tests and the error messages need to be fixed. After that, we'll consider changing some others small things, like @rickyvetter's suggestion. I'll then go test on our internal codebase and let my teammates suffer a bit =). Finally, we'll release the upgrade util if things go well.

Thanks again for all the input! And remember: most of you here are Reason "veteran" now. Whether you like this change or not, this PR isn't for you; it's for the future folks you'll be helping.

Contributor

chenglou commented Aug 18, 2017

Ok folks, time to merge this.

We've cut a release before this PR that includes the pipe formatting PR (a sore refmt bug before, thanks again @IwanKaramazow). It's now live on OPAM. Don't forget, 1.13.7, not 2.0.0. We've decided on a final release before landing this PR because we didn't want it to look like we're conning you into using this syntax just for the pipe formatting =P.

If you were/are against this function syntax change, hopefully you can see that we try our best to make sure folks are happy with most of our changes! And thanks for the discussion so far. We've let it drag on a year because we understand this is a big topic. If we've ignored any opinion, it's accidental.

Additionally, as @let-def pointed out, the change does have some essence beyond pure bikeshedding. If the syntax ends up unpopular, then we can always revisit this decision. But give us some time to iterate on it.

Speaking of iteration: we will be giving you a smooth upgrade path when this is ready. The PR's the first step; the outcome printer, some tests and the error messages need to be fixed. After that, we'll consider changing some others small things, like @rickyvetter's suggestion. I'll then go test on our internal codebase and let my teammates suffer a bit =). Finally, we'll release the upgrade util if things go well.

Thanks again for all the input! And remember: most of you here are Reason "veteran" now. Whether you like this change or not, this PR isn't for you; it's for the future folks you'll be helping.

@chenglou chenglou merged commit 2027482 into facebook:master Aug 18, 2017

0 of 2 checks passed

ci/circleci Your tests failed on CircleCI
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details

@chenglou chenglou referenced this pull request Aug 23, 2017

Closed

ES6 style function #1236

ChristianHersevoort added a commit to ChristianHersevoort/reason that referenced this pull request Jun 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment