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

Fix printing of functor types #1687

Merged
merged 1 commit into from May 29, 2018

Conversation

Projects
None yet
4 participants
@314eter
Copy link
Contributor

314eter commented Mar 30, 2018

The module type (S -> S) -> S is currently printed as S -> S -> S, which is equal to S -> (S -> S).

and module_type1 ctxt f x =
match x.pmty_desc, x.pmty_attributes with
| Pmty_functor _, [] -> paren true (module_type ctxt) f x
| _ -> module_type ctxt f x

This comment has been minimized.

@trefis

trefis Mar 30, 2018

Contributor

Can you explain why you add parentheses around a functor only when there are no attributes, aren't they also necessary if there are some attributes?

This comment has been minimized.

@gasche

gasche Mar 30, 2018

Member

This is standard in foo1 functions (see for example core_type1, to defer to the toplevel foo function in case of attributes, because it prints the attributes with a lot of parentheses for safety -- see attribute printing in module_type.

On the other hand, the structure of module_type1 is non-standard: these higher-level printers should handle the constructions that they know do not need parethesizing, and defer the others to higher-level printers or add parentheses to go back to the toplevel printer (module_type here). This is not what this function does, and I am not sure it is correct (for example, it would be wrong if given (S1 -> S2) with eqn as argument; but this construction is not type-correct).

I would expect the following two levels:

  • simple_module_type which prints all constructions that are fully parenthesized (sig..end) or atomic (identifiers), for others it calls module_type and wraps parentheses around it
  • module_type which prints the rest, and in particular the Pmty_functor construction; in the S1 -> S2 case, it calls simple_module_type on the left-hand-side.
    Maybe it would be more correct

This comment has been minimized.

@trefis

trefis Mar 30, 2018

Contributor

Thanks, I had missed that.

This comment has been minimized.

@314eter

314eter Mar 30, 2018

Author Contributor

I'm using the standard structure for module_type and module_type1 now. I think the previous approach was correct too, but maybe this is more future proof.

and module_type1 ctxt f x =
match x.pmty_desc, x.pmty_attributes with
| Pmty_functor _, [] -> paren true (module_type ctxt) f x
| _ -> module_type ctxt f x

This comment has been minimized.

@gasche

gasche Mar 30, 2018

Member

This is standard in foo1 functions (see for example core_type1, to defer to the toplevel foo function in case of attributes, because it prints the attributes with a lot of parentheses for safety -- see attribute printing in module_type.

On the other hand, the structure of module_type1 is non-standard: these higher-level printers should handle the constructions that they know do not need parethesizing, and defer the others to higher-level printers or add parentheses to go back to the toplevel printer (module_type here). This is not what this function does, and I am not sure it is correct (for example, it would be wrong if given (S1 -> S2) with eqn as argument; but this construction is not type-correct).

I would expect the following two levels:

  • simple_module_type which prints all constructions that are fully parenthesized (sig..end) or atomic (identifiers), for others it calls module_type and wraps parentheses around it
  • module_type which prints the rest, and in particular the Pmty_functor construction; in the S1 -> S2 case, it calls simple_module_type on the left-hand-side.
    Maybe it would be more correct
@gasche
Copy link
Member

gasche left a comment

I like the new structure better, thanks.

| Pmty_signature (s) ->
pp f "@[<hv0>@[<hv2>sig@ %a@]@ end@]" (* "@[<hov>sig@ %a@ end@]" *)
(list (signature_item ctxt)) s (* FIXME wrong indentation*)
| Pmty_with (mt, []) -> module_type1 ctxt f mt

This comment has been minimized.

@gasche

gasche Mar 30, 2018

Member

It is strange that you match the same construction in both functions, maybe remove this Pmty_with (mt, []) clause? (It is not wrong, and in theory it could save parentheses in some cases (although I don't think they can actually happen), but I like sticking to the standard pattern.)

This comment has been minimized.

@314eter

314eter Mar 30, 2018

Author Contributor

I did it to save parentheses, but Pmty_with (mt, []) can only occur in artificial parsetrees, so I'll remove it.

@@ -1074,11 +1075,11 @@ and signature_item ctxt f x : unit =
| pmd :: tl ->
if not first then
pp f "@ @[<hov2>and@ %s:@ %a@]%a" pmd.pmd_name.txt
(module_type ctxt) pmd.pmd_type
(module_type1 ctxt) pmd.pmd_type

This comment has been minimized.

@gasche

gasche Mar 30, 2018

Member

Why are you using the inner-level here and below? (And not in Psig_module.)

This comment has been minimized.

@314eter

314eter Mar 30, 2018

Author Contributor

Otherwise you would get

module rec A : S with type t = t and B : S with type t = t

which can't be parsed (the and B : S is considered part of the constraint). In principle, it's not really necessary below, but I did it for symmetry.

In Psig_module there is no and possible. Is it better to be consequent and add parentheses there too?

module type S = (functor[@foo] (M : S) -> S) -> S

module type S = sig
module rec A : (S with type t = t)

This comment has been minimized.

@gasche

gasche Mar 30, 2018

Member

The parentheses here are a consequence of using type1 above, but why are they desirable/useful?

@gasche

gasche approved these changes Mar 30, 2018

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Mar 30, 2018

(I made a minor mistake in "approving" the changes, I just wanted to dismiss my previous change request which has been addressed. There are minor issues still to discuss, and I think you should have a Changes entry.)

@damiendoligez

This comment has been minimized.

Copy link
Member

damiendoligez commented May 18, 2018

@gasche you could dismiss your review, right?

@gasche

This comment has been minimized.

Copy link
Member

gasche commented May 18, 2018

Well yes, but actually @314eter responded to my questions and I am now happy to fully approve it. I may merge soon unless someone objects.

@damiendoligez

This comment has been minimized.

Copy link
Member

damiendoligez commented May 29, 2018

ping @gasche

@gasche

This comment has been minimized.

Copy link
Member

gasche commented May 29, 2018

I just rebased the branch on top of the current trunk, and will push here when I'm done testing locally, then merge if the CI passes.

@gasche gasche force-pushed the 314eter:pprint-functor-type branch from 0150ab1 to 5633c84 May 29, 2018

@gasche

This comment has been minimized.

Copy link
Member

gasche commented May 29, 2018

(This is now rebased and should be ready to merge. Feel free to ping me if you come here and the CI has finished. I have also rebased on 4.07 and plan to push both at once.)

@gasche gasche merged commit d159aa2 into ocaml:trunk May 29, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

gasche added a commit that referenced this pull request May 29, 2018

Merge pull request #1687 from 314eter/pprint-functor-type
Fix printing of functor types

(cherry picked from commit d159aa2)
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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.