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

[RFC] Be more lax with semicolons #1887

Merged
merged 3 commits into from
May 2, 2018
Merged

[RFC] Be more lax with semicolons #1887

merged 3 commits into from
May 2, 2018

Conversation

let-def
Copy link
Contributor

@let-def let-def commented Mar 30, 2018

This patch allows to omit semi colons in front of other tokens (let, type, module, ...) when it is non-ambiguous.
One can now write:

module X = {
  type t = int
  let x : t = 5
}
let x = X.x

However, semicolons are still necessary in front of expressions (return position), as well as attributes and doc comments.

@IwanKaramazow
Copy link
Contributor

Why isn’t this part of the grammar?

@chenglou
Copy link
Member

chenglou commented Mar 30, 2018

^ Same question

I like that it's a parser change but not a printer change for now. A bit safer.

Edit:

it is easier to express at the token level than grammatical one
This isn’t entirely satisfying though

Does that mean we might make accidental changes in the future that produce ambiguous grammar, since menhir doesn't know about these semicolon changes?

@IwanKaramazow
Copy link
Contributor

Don't think it'll result in ambiguous grammar, since the semicolons get inserted at runtime.
Grammar gets checked at compile time. What's the performance impact?

@jordwalke
Copy link
Member

I'm also curious about the runtime impact (in the case when there are no syntax errors primarily).

The benefit of this approach is that the grammar remains clean, and is much easier to understand. You can see from my previous approach to this problem how much uglier it made the grammar.

The downside (not really) is that we cannot tell people that semicolons are optional. We are relaxed about semicolons as a developer time convenience in refmt.
It's very hard to guarantee that semicolons will always remain optional, and the cases where this approach relaxes are difficult to describe.

Still this approach is very helpful because it makes refmt much more usable and also makes autocompletion much more accurate in cases where you are editing text like this:

let y  = <cursorHere>
let x  = something;

Because it is actually a valid parse, and so there's much less error recovery needed.

@let-def
Copy link
Contributor Author

let-def commented Apr 9, 2018

The runtime impact should not be measurable (I didn't try so I might be wrong :D).
The new codepath is only exercised if there is an error.

Furthermore, there are some of low-hanging fruits that could improve performance more than this patch affects them. Once again, that doesn't mean these improvements would be perceptible (for compilation, the rest of the backend is still more expensive, so one can hope for a few percent improvements, for reformatting, the pretty-printer is significantly more expensive than the parser in my previous measurements).

@mrkaspa
Copy link

mrkaspa commented Apr 11, 2018

How will it look a function with multiple lines of side effects?

@let-def
Copy link
Contributor Author

let-def commented Apr 11, 2018

@mrkaspa

let f = (x) => {
  let x = foo
  let y = bar;
  x + y
}

Or

let f = (x) => {
  foo;
  bar;
  baz
}

Where foo, bar, baz are arbitrary expressions.

@mrkaspa
Copy link

mrkaspa commented Apr 11, 2018

@let-def looks nice more rust like, but how is this gonna behave with the formatter since it inserts ; even in the last line the expression to return

@chenglou
Copy link
Member

Alright, I guess this is good to go? cc @jordwalke

@chenglou chenglou merged commit 767a697 into reasonml:master May 2, 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.

6 participants