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

does not print "let open" correctly in "if" #7196

Closed
vicuna opened this issue Mar 26, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link

commented Mar 26, 2016

Original bug ID: 7196
Reporter: craff
Assigned to: @gasche
Status: closed (set by @xavierleroy on 2017-09-24T15:32:16Z)
Resolution: fixed
Priority: normal
Severity: major
Platform: linux
OS: Debian
OS Version: stable
Version: 4.02.3
Fixed in version: 4.03.0+dev / +beta1
Category: ~DO NOT USE (was: OCaml general)
Monitored by: runhang @hcarty

Bug description

Consider the following file

let _ =
if true then (
let open List in
();
());
()

The pretty printing of ast is wrong:

$ ocamlc -dsource bug.ml
let _ = if true then let open List in ((); ()); ()

it should be

let _ = if true then (let open List in ((); ())); ()

Otherwise, the last unit is parser inside the then branch.

Steps to reproduce

Given above

Additional information

4.03.0 is affected too, I did not check 4.01.0

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 27, 2016

Comment author: craff

I wrote major, because this can break bootstraping of -pp or -ppx extensions
that use ascii pretty printing as a simple way to boostrap (by producing
.ml/.mli file that do not use the extension)

Actually this broke decap bootstrap at some point and forced me not to use the "let open" syntax inside conditions

Moreover, this should be an easy fix ...

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 27, 2016

Comment author: @gasche

Indeed, this was very simple to fix:
c676334
Thanks for the report!

Despite a few rewrites, the pretty-printing machinery is still baroque and I wouldn't trust it. It would be nice to overhaul it to exactly match the precedence levels and nonterminals used in the parser, to get verifiably correct minimal parentheses insertion. I'd rather get the parser converted to Menhir first to work from a nicer grammar.

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 27, 2016

Comment author: craff

I use extensively the pretty printing to bootstrap decap and this is the first time I have a problem ...

What would be a real gain is to change the grammar itself, the priority of
if then else should not change by adding a let / fun / etc ...

This is a nightmare if you do not use the yacc way of giving precedence,
which is the case when pretty printing.

The nice things, is that changing the grammar at this place actually breaks little code (I tested), and can be easily detected by parsing both with the old and the new parser.

I would suggest a few deep revision of the syntax (in places that hurt students):

  • if then else priority
  • optional endif, endfun, endmatch endfunction
  • "in" replaced by ";" (YES)
  • let replaced by val in structure (syntax error sooner in the code)

If this change are not mandatory, a smooth transition is possible

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 27, 2016

Comment author: craff

And thanks for the fix !!!

@vicuna vicuna closed this Sep 24, 2017

@vicuna vicuna added the bug label Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.