Skip to content
This repository was archived by the owner on Jun 15, 2023. It is now read-only.

Some((a, b)) formats to Some(a, b) #274

Closed
johnridesabike opened this issue Feb 24, 2021 · 11 comments
Closed

Some((a, b)) formats to Some(a, b) #274

johnridesabike opened this issue Feb 24, 2021 · 11 comments

Comments

@johnridesabike
Copy link

johnridesabike commented Feb 24, 2021

let x = Some((1, 2))

formats to:

let x = Some(1, 2)

Either one compiles, so maybe this isn’t a bug? The syntax is misleading, though, since the Some constructor only takes one value.

This happens with other variant constructors as well.

@IwanKaramazow
Copy link
Contributor

Where are you seeing this?

Just tested this on ReScript 9 and it seems to look ok?
image

@johnridesabike
Copy link
Author

johnridesabike commented Feb 25, 2021

I can't test it on a local install at the moment, but it's reproducible on the playground. Click "format" https://rescript-lang.org/try?code=DYUwLgBAHhC8EGUD2BbEAKdBGANBATAJSFA

Regardless of the formatter preference, I wonder if Some(1, 2) should even be valid?

@ryyppy
Copy link
Member

ryyppy commented Feb 26, 2021

this is a bug in the playground when formatting code. Will fix.

ryyppy added a commit to ryyppy/rescript-compiler that referenced this issue Feb 26, 2021
Configure the parser to pass forPrinter:true when parsing / printing ReScript.
This fixes issues where e.g. Some((1, 2)) get misprinted to Some(1,2) etc.

Relevant bug reports:
- rescript-lang/syntax#274
- rescript-lang/syntax#265
@ryyppy
Copy link
Member

ryyppy commented Feb 26, 2021

Should now be fixed on the playground:
Check example

@IwanKaramazow
Copy link
Contributor

Thanks for the work, this is great.

@johnridesabike
Copy link
Author

I'm still curious if both of these are supposed to compile the same:

let a = Some((1, 2))
let b = Some(1, 2)

Right now, either one compiles identically (and the formatter doesn't seem to have a preference), but b doesn't look like correct syntax to me.

Sorry if this is the intended behavior, and thanks for fixing the playground.

@IwanKaramazow
Copy link
Contributor

This is a technical consequence of ocaml's representation of your source code. We as humans think in terms of "multiple variant arguments", but internally in the compiler this is expressed as a tuple.

In ReScript variant argument are written between parens: Some(...arguments go here...). The compiler however requires a tuple to express multiple arguments, hence you get Some((1, 2)). This doesn't look super nice of course. So in this case we parse Some(1, 2) as Some((1, 2)) behind the scenes.

Does this make sense?

@anmonteiro
Copy link

anmonteiro commented Feb 26, 2021

I don't think that's true, and it's the reason for @implicit_arity to exist in Reason.

In OCaml there's a difference between Foo of 'a * 'b and Foo of ('a * 'b)

$ refmt -pa
Some(1,2)
[D
  structure_item ([1,0+0]..[1,0+9])
    Pstr_eval
    expression ([1,0+0]..[1,0+9])
      attribute "explicit_arity"
        []
      Pexp_construct "Some" ([1,0+0]..[1,0+4])
      Some
        expression ([1,0+4]..[1,0+9])
          Pexp_tuple
          [
            expression ([1,0+5]..[1,0+6])
              Pexp_constant PConst_int (1,None)
            expression ([1,0+7]..[1,0+8])
              Pexp_constant PConst_int (2,None)
          ]
]

$ refmt -pa
Some((1,2))

  structure_item ([1,0+0]..[1,0+11])
    Pstr_eval
    expression ([1,0+0]..[1,0+11])
      attribute "explicit_arity"
        []
      Pexp_construct "Some" ([1,0+0]..[1,0+4])
      Some
        expression ([1,0+4]..[1,0+11])
          Pexp_tuple
          [
            expression ([1,0+5]..[1,0+10])
              Pexp_tuple
              [
                expression ([1,0+6]..[1,0+7])
                  Pexp_constant PConst_int (1,None)
                expression ([1,0+8]..[1,0+9])
                  Pexp_constant PConst_int (2,None)
              ]
          ]
]

@IwanKaramazow
Copy link
Contributor

oh wait, yes, that has something to do with it. Bit fuzzy on the topic, but explicit_arity gets ppx'ed away right in Reason?
There's an invariant that a tuple should have 2 or more things: https://github.com/ocaml/ocaml/blob/4.06/parsing/parsetree.mli#L277

So the second snippet becomes the first one?

@chenglou
Copy link
Member

There's indeed a difference and historically it stemmed from ocaml's small syntactic ambiguity in that case, which then gets disambiguated later in the pipeline instead of at the syntax level.

ryyppy added a commit to ryyppy/rescript-compiler that referenced this issue Apr 18, 2021
Configure the parser to pass forPrinter:true when parsing / printing ReScript.
This fixes issues where e.g. Some((1, 2)) get misprinted to Some(1,2) etc.

Relevant bug reports:
- rescript-lang/syntax#274
- rescript-lang/syntax#265
ryyppy added a commit to ryyppy/rescript-compiler that referenced this issue Apr 18, 2021
Configure the parser to pass forPrinter:true when parsing / printing ReScript.
This fixes issues where e.g. Some((1, 2)) get misprinted to Some(1,2) etc.

Relevant bug reports:
- rescript-lang/syntax#274
- rescript-lang/syntax#265
ryyppy added a commit to ryyppy/rescript-compiler that referenced this issue May 7, 2022
Configure the parser to pass forPrinter:true when parsing / printing ReScript.
This fixes issues where e.g. Some((1, 2)) get misprinted to Some(1,2) etc.

Relevant bug reports:
- rescript-lang/syntax#274
- rescript-lang/syntax#265
ryyppy added a commit to rescript-lang/rescript that referenced this issue May 31, 2022
Configure the parser to pass forPrinter:true when parsing / printing ReScript.
This fixes issues where e.g. Some((1, 2)) get misprinted to Some(1,2) etc.

Relevant bug reports:
- rescript-lang/syntax#274
- rescript-lang/syntax#265
ryyppy added a commit to rescript-lang/rescript that referenced this issue Jun 1, 2022
Configure the parser to pass forPrinter:true when parsing / printing ReScript.
This fixes issues where e.g. Some((1, 2)) get misprinted to Some(1,2) etc.

Relevant bug reports:
- rescript-lang/syntax#274
- rescript-lang/syntax#265
cristianoc pushed a commit to rescript-lang/rescript that referenced this issue Jun 1, 2022
* Add all relevant changes from previous 2020 bundle API

For reference on the previous work in 2020, refer to #4518

* Add ninja rule for building jsoo_refmt_main

* Fix formatting edge-cases when formatting ReScript -> ReScript

Configure the parser to pass forPrinter:true when parsing / printing ReScript.
This fixes issues where e.g. Some((1, 2)) get misprinted to Some(1,2) etc.

Relevant bug reports:
- rescript-lang/syntax#274
- rescript-lang/syntax#265

* Introduce initial type hints in playground bundle results

* Adapt playground to newest internal syntax apis

* Make jsoo_refmt_main work again by removing refmt

* Rename jsoo_refmt_main to jsoo_playground_main

* Move jsoo_playground_main to jscomp/main

* Remove irrelevant code

* Remove Refmt_api

* Make repl.js more reliable with missing .mli files

* repl.js: Log target compiler file, set js_playground_main as default

* Update CONTRIBUTING to align with the renamed playground jsoo entrypoint

Co-authored-by: Patrick Stapfer <[email protected]>
mununki pushed a commit to mununki/rescript-compiler that referenced this issue Jul 15, 2022
* Add all relevant changes from previous 2020 bundle API

For reference on the previous work in 2020, refer to rescript-lang#4518

* Add ninja rule for building jsoo_refmt_main

* Fix formatting edge-cases when formatting ReScript -> ReScript

Configure the parser to pass forPrinter:true when parsing / printing ReScript.
This fixes issues where e.g. Some((1, 2)) get misprinted to Some(1,2) etc.

Relevant bug reports:
- rescript-lang/syntax#274
- rescript-lang/syntax#265

* Introduce initial type hints in playground bundle results

* Adapt playground to newest internal syntax apis

* Make jsoo_refmt_main work again by removing refmt

* Rename jsoo_refmt_main to jsoo_playground_main

* Move jsoo_playground_main to jscomp/main

* Remove irrelevant code

* Remove Refmt_api

* Make repl.js more reliable with missing .mli files

* repl.js: Log target compiler file, set js_playground_main as default

* Update CONTRIBUTING to align with the renamed playground jsoo entrypoint

Co-authored-by: Patrick Stapfer <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants