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

manual: ellipses in examples #1765

Merged
merged 7 commits into from May 7, 2018

Conversation

Projects
None yet
2 participants
@Octachron
Copy link
Contributor

Octachron commented May 6, 2018

As suggested in #1209, this PR proposes to add the ability to elide some of the content in the caml_example code examples. More precisely, term annotated by [@ellipsis] or between [@ellipsis.start] and [ellipsis.stop] are replaced by \ldots (i.e. ):

module M = struct
  [@@@ellipsis.start]
  type t = T
  let x = 0
  [@@@ellipsis.stop]
end
let f = fun (type t) (foo : t list) -> assert false[@ellipsis]

becomes

module M = structend
let f = fun (type t) (foo : t list) -> …

in the manual text.

As an illustration, I translated the locally abstract type section of the extension chapter to caml_examples and one of the verbatim examples in the section on first-class modules.

This PR also adds a notion of textual transformation to caml_tex2 and checks that no transformation applies to intersecting text fragment. Consequently, it is not possible to elide a part of an example which raises a warning or an error.

@gasche
Copy link
Member

gasche left a comment

Thanks!

The code looks very reasonable to me.

Do I correctly understand that the current handling of "transform intersection" forbids having a warning/error location that contains an ellipsis? Isn't that something that we could want to have in the future? If yes, and you see a way to enable it that is not too much work, maybe that could be nice.

(I regret not trying to merge #1209 earlier because we are giving ourselves more rebase work by having one giant PR instead of merging whatever is ready when it's ready.)

\begin{verbatim}
module type DEVICE = sig ... end
\begin{caml_example*}{verbatim}
module type DEVICE = sig [@@@ellipsis.start][@@@ellipsis.stop] end

This comment has been minimized.

@gasche

gasche May 6, 2018

Member

Couldn't just [@@@ellipsis] work here?

This comment has been minimized.

@Octachron

Octachron May 6, 2018

Author Contributor

Yes, I was too focused on having tests for [@ellipsis.start]...[@ellipsis.end].

let f (type t) () =
let module M = struct exception E of t end in
(fun x -> M.E x), (function M.E x -> Some x | _ -> None)
\end{verbatim}
\end{caml_example*}

This comment has been minimized.

@gasche

gasche May 6, 2018

Member

This environment change and the one below look like they are likely to conflict with #1209 for no very good reason.

This comment has been minimized.

@Octachron

Octachron May 6, 2018

Author Contributor

True, I will remove them. (I could also remove all changes to exten.etex since they are more here as an example than anything else.).

| "ellipsis" ->
transforms :=
{ Text_transform.kind=Ellipsis; start; stop= max attr_stop stop }
:: !transforms

This comment has been minimized.

@gasche

gasche May 6, 2018

Member

Shouldn't you check that left_mark is not active to guarantee the absence of nested ellipses?

This comment has been minimized.

@Octachron

Octachron May 6, 2018

Author Contributor

Possibly. However, this case is already caught later by check_partition. I will think a bit about this independence problem.

This comment has been minimized.

@Octachron

Octachron May 7, 2018

Author Contributor

I have added a test in order to catch nested ellipsis early in all cases (which makes the the test for unmatched stop/start simpler).

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented May 6, 2018

Isn't that something that we could want to have in the future? If yes, and you see a way to enable it that is not too much work, maybe that could be nice.

I added the possibility of eliding strict subset of underlined interval. The complextity increase seems moderate.

@gasche

gasche approved these changes May 7, 2018

Copy link
Member

gasche left a comment

Good to go, but I think that a Changes entry could be nice. I plan to cherry-pick into 4.07 as well, to be able to use it in the 4.07 manual.

Octachron added some commits May 7, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented May 7, 2018

I added the change entry in the 4.07 section.

@gasche gasche merged commit 7398fd2 into ocaml:trunk May 7, 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 7, 2018

Merge pull request #1765 from Octachron/manual_ellipsis_in_examples
manual: ellipses in examples
(cherry picked from commit 7398fd2)
let left_mark = ref None (* stored position of [@@@ellipsis.start]*) in
let location _this loc =
(* we rely on the fact that the default iterator call first
the location subiterator, then the attribute subiterator *)

This comment has been minimized.

@gasche

gasche May 7, 2018

Member

On reviewing I didn't think much of this assertion, but upon usage I see bugs caused by the fact that it is wrong. For example the following does not behave correctly:

module type DEVICE = sig val draw : picture -> unit[@@ellipsis] end

The ellipsis should elide the whole signature item val draw ..., but it only elides the unit part, because location iterators are called in the wrong order.

The mismatch can be found in the sources of ast_iterator.ml:

    module_type_declaration =
      (fun this {pmtd_name; pmtd_type; pmtd_attributes; pmtd_loc} ->
         iter_loc this pmtd_name;
         iter_opt (this.module_type this) pmtd_type;
         this.attributes this pmtd_attributes;
         this.location this pmtd_loc
);

(My first idea on how to fix it turned out not to work, so I'm not sure of what's the best fix. It may be to special-case the situations in which we support the ellipsis attribute as we can reliably extract locations, and fail on the others, for now.)

This comment has been minimized.

@Octachron

Octachron May 7, 2018

Author Contributor

I should have indeed checked more carefully this assertion. If you find the fix too time consuming, please do not hesitate to ping me.

This comment has been minimized.

@gasche

gasche May 7, 2018

Member

In fact I am currently under the impression that there is no nice fix that does not touch ast_iterator (which would be the condition to go into 4.07). I started writing the following

let located_attributed_iterator located_attribute =
  let open Ast_iterator in
  let super = default_iterator in
  let rec this = {
    super with
    structure_item = (fun this ({pstr_loc = loc; pstr_desc = desc} as si) ->
      begin match desc with
        | Pstr_eval (_, attrs) -> located_attributes loc attrs
        | Pstr_extension (_, attrs) -> located_attributes loc attrs
      end;
      super.structure_item this si;
      );

but the idea of having to extend with definition with all the cases under the sun is killing me, and I don't think this is the right approach.

I would be tempted to revert the ellipsis stuff from 4.07, because I don't like the idea of releasing something that we know is broken. Then for trunk we can discuss either:

  • fixing the Ast_iterator control flow (but then I realized that some things have attributes but no locations, so we would probably need to change Parsetree as well if we want to enforce the invariant that all attributable things are located. This is the right thing to do, but it is bound to be long, painful, and break user code.)
  • and/or using extension points instead of attributes (that was actually my very first interface idea).

This comment has been minimized.

@gasche

gasche May 7, 2018

Member

@Octachron I'll wait until you confirm that you agree there is no simple/easy fix here before reverting.

This comment has been minimized.

@Octachron

Octachron May 7, 2018

Author Contributor

Reading more carefully ast_iterator, I found only 7 exception to the location; attribute pattern:

  • iter_type_extension
  • iter_structure_item
  • iter_signature_item
  • value_description
  • module_declaration
  • module_type_declaration
  • module_binding
    My biased opinion is that writing a custom iterator rewriting those part would be ugly but acceptable.

This comment has been minimized.

@Octachron

Octachron May 7, 2018

Author Contributor

See this commit for some ugliness quantification.

Concerning your other points, extension nodes were my second idea after pseudo-latex macro; but I found that attribute were the right interface at the user level since there is nothing special going at the code level, and this is really a meta-information. I think it is worth it to try to support this interface.
In term of maintenability, would it be a bad idea to move (on trunk) the tuned iterator to parsing/ast_iterator.ml as another variant of ast iterator?
For the attributable items that need a more precise location, I noted attribute locations polymorphic variant and object tag, exceptions. This seems like a small set which could be fixed at an ulterior date.

At least, this my opinion on the spot, I would rethink the issue tomorrow.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented May 10, 2018

I still think that using extension points would be better here. Even with we fix a handful of cases of the iterator, we still have the problem that some attributes are not located, so the technology cannot be correct without parsetree changes, and I don't really like that. (Although we can fix the attribute locability in trunk later.)

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented May 10, 2018

Ok, I will write a PR that switch to extension nodes.

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented May 11, 2018

Another point to put in the balance, that I forgot before starting the extension node implementation, is that some extension nodes do not have a corresponding payload: typically node extension can replace class expression, class field or module types; but payloads are either structure, signature or patterns.
For instance,

class c = object [%ellipsis method m = ( ) ] end

is invalid due to the payload, and the closest valid option would be

class c = object [%ellipsis object method m = ( ) end ] end
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.