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

Pprintast: add parentheses around `fun (type t) -> ...` and in `(lazy ...)` patterns #2190

Merged
merged 4 commits into from Apr 10, 2019

Conversation

@nojb
Copy link
Contributor

commented Dec 8, 2018

No description provided.

@nojb nojb changed the title Pprintast: add parenthesis when printing Pexp_newtype Pprintast: add parentheses around `fun (type t) -> ...` and `(lazy ...)` patterns Dec 8, 2018
@nojb nojb force-pushed the nojb:ast_mapper branch from 60b7ff5 to e6b6514 Dec 8, 2018
@nojb nojb changed the title Pprintast: add parentheses around `fun (type t) -> ...` and `(lazy ...)` patterns Pprintast: add parentheses around `fun (type t) -> ...` and in `(lazy ...)` patterns Dec 8, 2018
@gasche

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

I suppose you have examples that show that those parentheses are necessary for correct reparsing?

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2018

Yes: f (fun (type t) -> ...) will be pretty printed as f fun (type t) -> ... and (lazy (A ...)) is printed as (lazy A ...). Do you think a test is in order? (not sure adding individual counterexamples is the most efficient way of testing Pprintast.)

@gasche

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

If I remember correctly, some people ( @Drup ? ) have tested the fact that adding a printing-reparsing step to the compiler preserves compilability of the compiler distribution. In that spirit, having these patterns occur anywhere in the codebase or testsuite should be enough to (eventually, as in "eventual consistency") detect the issue -- typically in exotic_syntax. But yes, I would think that individual failures warrant individual regressions, which can catch issues later in a refactoring -- or at least increase confidence in a result.

I suppose that there is some logic to detect Pexp_newtype adjacent to other Pexp_newtype or Pexp_fun, and print them as a n-ary abstraction fun x (type t) ..?

I think that it is a bug to have placed Pexp_newtype in simple_expr, which should be reserved to fully-enclosed forms. Instead of enforcing parentheses, it should probably be handled like Pexp_fun: at the expression level, with the weird when ctxt.semi test as well. Would you mind trying this?

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

Yes, I will give it a try.

@nojb nojb force-pushed the nojb:ast_mapper branch from e6b6514 to c79d413 Apr 10, 2019
@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@gasche Moved the treatment of Pexp_newtype to be the same as Pexp_fun and added a test.

@gasche
gasche approved these changes Apr 10, 2019
Copy link
Member

left a comment

This looks good to me, thanks. See a minor comment inline, and please feel free to merge once the CI agrees.

@@ -7364,3 +7364,11 @@ let x = ` (* wait for it *) Bar
type (+' a, -' a', ' a'b', 'ab', ' abcd', ' (* ! *) x) t =
' a * ' a' * ' a'b' * 'ab' * ' abcd' * ' (* !! *) x
as ' a'

(* GPR#2190 *)

This comment has been minimized.

Copy link
@gasche

gasche Apr 10, 2019

Member

You could drop the GPR prefix now.

This comment has been minimized.

Copy link
@nojb

nojb Apr 10, 2019

Author Contributor

Yes, done.

@nojb nojb force-pushed the nojb:ast_mapper branch from 8fb77bb to 4fa8e3a Apr 10, 2019
@nojb nojb merged commit def1fee into ocaml:trunk Apr 10, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nojb nojb deleted the nojb:ast_mapper branch Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.