expose Pprintast.longident by gasche · Pull Request #1996 · ocaml/ocaml

gasche · 2018-08-20T07:34:56Z

This is a continuation of #1918 (Longident.to_string) with a different implementation approach suggested by @Octachron and @jberdine.

(I haven't updated the Changes yet, but a review would still be welcome.)

Armael · 2018-08-20T09:29:44Z

parsing/ast_helper.ml


-type lid = Longident.t loc
-type str = string loc
+type 'a with_loc = 'a Location.loc


Does this suggest that 'a Location.loc would be better named 'a Location.with_loc? (but changing that now may be not worth the hassle..)

Octachron · 2018-08-20T10:39:28Z

The hardening against empty identifiers in 360b867 makes me wonder if this should be an AST invariant. Except this point, I think that this the right minimal change.
(p.s. I am not sure if the inclusion of #1903 changes is intentional?)

Octachron · 2018-08-20T10:42:43Z

parsing/pprintast.mli

 val structure: Format.formatter -> Parsetree.structure -> unit
 val string_of_structure: Parsetree.structure -> string
+
+val longident : Format.formatter -> Longident.t -> unit


I think that longident could be at the top rather than the bottom of the interface since it prints simpler items.

Will do -- the order in the other declarations is not the one you suggest, so I will reorder everything.

Octachron · 2018-08-20T10:43:56Z

parsing/longident.mli

+[@@@ocaml.doc{|
+To print a longident, see {!Pprintast.longident}, using
+{!Format.asprintf} to convert to a string.
+|}]


Was there a problem with a documentation comment here ((** ... *)) ?

I wasn't sure that I would get the right comment-by-itself behavior with just newlines, so I thought that this more explicit syntax would be better. Do you think I should change back?

I think so: I just realized that the right attribute was ocaml.text and not ocaml.doc. A newline should be fine (and checking with -dsource works well).

Good catch... I just changed it.

jberdine · 2018-08-20T10:18:18Z

parsing/parsetree.mli

           [`A of & T1 & .. & Tn] ( true,  [T1;...Tn] )

-          - The 2nd field is true if the tag contains a
+          - The 3rd field is true if the tag contains a


Now it is actually the 2nd field.

jberdine · 2018-08-20T10:40:06Z

parsing/ast_iterator.ml

    | Psig_extension (x, attrs) ->
-        sub.extension sub x; sub.attributes sub attrs
+        sub.attributes sub attrs;
+        sub.extension sub x


I'm not sure if there are consequences to this order change, but it is more consistent with other cases after this change.

The idea is to have .attributes come right after .location for all AST nodes, so that you can know when you process an attribute the location of its whole attached node (attribute included) by an ugly side-effect. See #1765. This is fragile of course, and would like a better long-term way to do it.

gasche · 2018-08-20T17:38:17Z

Sitting on top of #1903 is unintentional, sorry. @jberdine, I think that the bits you reviewed come from #1903. (Thanks! I will make the change there.)

gasche · 2018-08-21T16:32:29Z

(I rebased the PR.)

Octachron · 2018-08-21T16:40:32Z

testsuite/tests/compiler-libs/test_longident.ml

+Line 1, characters 25-33:
+  let last_ident = L.last (L.lident "foo")
+                           ^^^^^^^^
+Error: Unbound value L.lident


This error does not seem intentional?

Indeed, sorry. I just reworked the tests and rebased.

Octachron

The changes look good to me.

trefis · 2018-08-22T10:33:07Z

testsuite/tests/compiler-libs/test_longident.ml

+let parse_complex = L.parse "M.F(M.N).N.foo"
+[%%expect {|
+val parse_complex : L.t =
+  L.Ldot (L.Ldot (L.Ldot (L.Ldot (L.Lident "M", "F(M"), "N)"), "N"), "foo")


"F(M"), "N)" really? Yirk.

I missed that, but yes Longident.parse only parses the subset of Longident without application.

Can you (or @gasche) comment on what you think should be done then?
Should we change Longident.parse? Should we remove the test? Add a comment on the test?

Note that a few lines below we're converting back that broken Longident to a string, and everything looks "fine". I don't think we should do that.

Personally I think that Longident.parse should be fixed to be complete. In fact I hadn't really looked at the test output (otherwise I would have said something here); this continuing experience of surprises makes me half-glad half-sorry that I insisted on writing unit tests for compiler-libs feature.

Going down this rabit's hole, making Longident.parse complete also means udapting the call in makedepend.ml and typemod.ml to reject applications in -open arguments .

An intermediate fix would be to make Longident.parse fail upon seeing a parenthesis.

[edit] better yet, make it check that the components are valid identifers.

I think an option returning version might be better since in the two current use cases of Longident the error handling should be specialized:

In Typemod, the compiler should warns that -open F(X) is not valid (should the input be validated earlier?)

in toplevel/genprintval.ml, the extension printer should abort the search for extensions constructor to handle cases like

module F(X:sig end) = struct type t = .. type t+= A end module E = struct end let x : F(E).t = let module M = F(E) in M.A;;

val x : F(E).t = <extension>

( currently, the printer searches for a module Lident F(E) and fails)

In Typemod, the compiler should warns that -open F(X) is not valid

That won't stay true for very long: I'm about to send a refreshed version of the "extended open" PR which allows this.

I'm about to send a refreshed version of the "extended open" PR which allows this.

I think that's only true for .mli files, for .ml files you probably still need a warning.

In this case, would it not be simpler to expose the corresponding parser rule (module_expr?) from the parser and use constr_longident for the extension printer?

Armael reviewed Aug 20, 2018

View reviewed changes

Octachron reviewed Aug 20, 2018

View reviewed changes

jberdine reviewed Aug 20, 2018

View reviewed changes

gasche force-pushed the Pprintast.longident branch from 360b867 to 106b048 Compare August 20, 2018 17:46

gasche mentioned this pull request Aug 20, 2018

Longident.to string #1918

Closed

gasche force-pushed the Pprintast.longident branch from 106b048 to 256e6e3 Compare August 20, 2018 18:02

Octachron reviewed Aug 21, 2018

View reviewed changes

gasche force-pushed the Pprintast.longident branch from 256e6e3 to a7a0abf Compare August 21, 2018 16:53

Octachron approved these changes Aug 21, 2018

View reviewed changes

trefis reviewed Aug 22, 2018

View reviewed changes

gasche force-pushed the Pprintast.longident branch 2 times, most recently from f9c67fe to eef31c5 Compare August 23, 2018 19:02

gasche added 5 commits August 23, 2018 21:09

expose Pprintast.longident and mention it in Longident's mli

951575c

Pprintast: protect against empty strings in the AST

a62ac34

tests for Longident

e289a92

pprintast.mli: reorder from smaller to larger AST structures

eec37cf

(reorder Changes entries)

eef31c5

gasche merged commit 617930b into ocaml:trunk Aug 25, 2018

vicuna mentioned this pull request Mar 14, 2019

ocamldep generates circular dependencies which don't exist #4618

Closed

Conversation

gasche commented Aug 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Octachron commented Aug 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gasche commented Aug 20, 2018

Uh oh!

gasche commented Aug 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Octachron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damiendoligez Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Octachron Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

damiendoligez Nov 9, 2018 •

edited

Loading

Octachron Nov 9, 2018 •

edited

Loading