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

expose Pprintast.longident #1996

Merged
merged 5 commits into from Aug 25, 2018

Conversation

Projects
None yet
7 participants
@gasche
Copy link
Member

commented Aug 20, 2018

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.)

@@ -19,9 +19,11 @@ open Asttypes
open Parsetree
open Docstrings

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

This comment has been minimized.

Copy link
@Armael

Armael Aug 20, 2018

Member

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..)

This comment has been minimized.

Copy link
@gasche

gasche Aug 20, 2018

Author Member

Yes?

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

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?)

@@ -24,3 +24,5 @@ val pattern: Format.formatter -> Parsetree.pattern -> unit
val signature: Format.formatter -> Parsetree.signature -> unit
val structure: Format.formatter -> Parsetree.structure -> unit
val string_of_structure: Parsetree.structure -> string

val longident : Format.formatter -> Longident.t -> unit

This comment has been minimized.

Copy link
@Octachron

Octachron Aug 20, 2018

Contributor

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

This comment has been minimized.

Copy link
@gasche

gasche Aug 20, 2018

Author Member

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

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

This comment has been minimized.

Copy link
@Octachron

Octachron Aug 20, 2018

Contributor

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

This comment has been minimized.

Copy link
@gasche

gasche Aug 20, 2018

Author Member

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?

This comment has been minimized.

Copy link
@Octachron

Octachron Aug 20, 2018

Contributor

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).

This comment has been minimized.

Copy link
@gasche

gasche Aug 20, 2018

Author Member

Good catch... I just changed it.

(* [`A] ( true, [] )
[`A of T] ( false, [T] )
[`A of T1 & .. & Tn] ( false, [T1;...Tn] )
[`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

This comment has been minimized.

Copy link
@jberdine

jberdine Aug 20, 2018

Now it is actually the 2nd field.

@@ -259,7 +277,8 @@ module MT = struct
| Psig_class_type l ->
List.iter (sub.class_type_declaration sub) l
| Psig_extension (x, attrs) ->
sub.extension sub x; sub.attributes sub attrs
sub.attributes sub attrs;
sub.extension sub x

This comment has been minimized.

Copy link
@jberdine

jberdine Aug 20, 2018

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

This comment has been minimized.

Copy link
@gasche

gasche Aug 20, 2018

Author Member

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

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2018

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 gasche force-pushed the gasche:Pprintast.longident branch from 360b867 to 106b048 Aug 20, 2018

@gasche gasche referenced this pull request Aug 20, 2018

Closed

Longident.to string #1918

@gasche gasche force-pushed the gasche:Pprintast.longident branch from 106b048 to 256e6e3 Aug 20, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2018

(I rebased the PR.)

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

This comment has been minimized.

Copy link
@Octachron

Octachron Aug 21, 2018

Contributor

This error does not seem intentional?

This comment has been minimized.

Copy link
@gasche

gasche Aug 21, 2018

Author Member

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

@gasche gasche force-pushed the gasche:Pprintast.longident branch from 256e6e3 to a7a0abf Aug 21, 2018

@Octachron
Copy link
Contributor

left a comment

The changes look good to me.

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")

This comment has been minimized.

Copy link
@trefis

trefis Aug 22, 2018

Contributor

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

This comment has been minimized.

Copy link
@Octachron

Octachron Aug 22, 2018

Contributor

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

This comment has been minimized.

Copy link
@trefis

trefis Aug 22, 2018

Contributor

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.

This comment has been minimized.

Copy link
@gasche

gasche Aug 22, 2018

Author Member

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.

This comment has been minimized.

Copy link
@Octachron

Octachron Aug 22, 2018

Contributor

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 .

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Nov 9, 2018

Member

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.

This comment has been minimized.

Copy link
@Octachron

Octachron Nov 9, 2018

Contributor

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)

This comment has been minimized.

Copy link
@trefis

trefis Nov 9, 2018

Contributor

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.

This comment has been minimized.

Copy link
@lpw25

lpw25 Nov 9, 2018

Contributor

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.

This comment has been minimized.

Copy link
@Octachron

Octachron Nov 9, 2018

Contributor

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?

@gasche gasche force-pushed the gasche:Pprintast.longident branch 2 times, most recently from f9c67fe to eef31c5 Aug 22, 2018

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.