Handle ambiguous labels #720
Conversation
|
To confirm, you're planning on moving the logic to the document IR layer? |
|
I'm working on it :) |
Julow
force-pushed
the
toc-ambiguous
branch
from
29bb373 to
db04e0e
Compare
|
I moved the disambiguation logic into the |
|
Noticed a problem with sub-subpages in my previous code, fixed now. This is ready for review. |
src/xref2/env.ml
Outdated
| @@ -68,7 +68,7 @@ let ident_of_element = function | |||
| | `ModuleType (id, _) -> (id :> Identifier.t) | |||
| | `Type (id, _) -> (id :> Identifier.t) | |||
| | `Value (id, _) -> (id :> Identifier.t) | |||
| | `Label id -> (id :> Identifier.t) | |||
| | `Label (id, _) -> (id :> Identifier.t) | |||
There was a problem hiding this comment.
Looks much more consistent :-)
| @@ -123,7 +133,8 @@ end = struct | |||
| let mark_shadow e = | |||
| if e.kind = kind then { e with shadowed = true } else e | |||
| in | |||
| if List.exists has_shadow tl then List.map mark_shadow tl else tl | |||
| if shadow && List.exists has_shadow tl then List.map mark_shadow tl | |||
| else tl | |||
There was a problem hiding this comment.
This seems a bit awkward. I wonder if it would be cleaner to handle the shadowing on the lookup path? It seems that the 'shadowed' bool is a bit redundant - we could recalculate it even if it wasn't being stored.
There was a problem hiding this comment.
The problem with recomputing it in find is that it depends on the kind, which might be too heavy for the fast path.
I've changed the code to remove the shadowing bit entirely. There's now two maps, one where shadowed elements are removed and an other for identifiers.
src/xref2/component.ml
Outdated
| @@ -455,6 +455,12 @@ and CComment : sig | |||
| end = | |||
| CComment | |||
|
|
|||
| and Label : sig | |||
| type t = | |||
| [ `Heading of Odoc_model.Comment.heading ] Odoc_model.Comment.with_location | |||
There was a problem hiding this comment.
I'm not convinced by this. In all of the other Components we use Ident.t rather than identifier (e.g. in the above definition of CComment). This is because we use these components for doing expansions and often end up placing them in different contexts than they were originally found - this usually works because we end up turning the local Ident.t back into a global Identifier.t at the right place, but if you're preserving the Identifier it'll be incorrect. I think what you actually want is to pick out the Heading from the CComment module above as a separate type and use that instead.
There was a problem hiding this comment.
This is fixed. I've also made Label.t a record to make sure we catch every places where the conversion is needed.
src/xref2/link.ml
Outdated
| (* Returns [0] if [heading] is the first occurence. The first occurence is at | ||
| the end of the list stored in [env]. Compare only the location as it is | ||
| unique to each heading and unlikely to be updated. *) | ||
| let rec dup_index = function |
There was a problem hiding this comment.
Given that a lot of this code is removed in a subsequent commit, can we squash them?
There was a problem hiding this comment.
I've squashed the related commits.
Keep the "titles" alongside the corresponding element. Remove a field from a record that is frequently updated, also slightly simplify the code.
Regular scoping rules don't apply to labels (for example, the first definition wins) and we'll need to check for ambiguity later (keep the duplicates into the env).
This will be used to disambiguate labels. Identifiers to duplicate labels are equal.
Labels need to be added to the env from top to bottom.
Suffix ambiguous labels with '_2', '_3', etc.. to make the TOC work as expected. Labels are unique across sub-pages and generated headings are covered. (eg. page title, functor parameters/sig)
The general ambiguity warning was bogus in case of ambiguous labels. Also update the API of 'Ref_tools' to highlight that it generates warnings.
Model.Comment is changed to keep track of whether the label is present or not in the source code.
To avoid unecessarily mangled names in subpages. This adds some boilerplate to the generators but each generator can choose the right strategy.
Make sure labels are converted to 'Ident.label' then converted back to identifiers.
Use two maps instead, one that maps names but don't keep shadowed elements and one that maps identifiers.
Julow
force-pushed
the
toc-ambiguous
branch
from
b31b574 to
0410de4
Compare
Looks like modules that only contain comments and not code get compiled into empty modules
CHANGES: Breaking changes - Remove odoc-parser into a separate repository (@jonludlam, ocaml/odoc#700) Additions - OCaml 4.13 support (@Octachron, ocaml/odoc#687, ocaml/odoc#689) - Better errors/warnings (@Julow, ocaml/odoc#692, ocaml/odoc#717, ocaml/odoc#720, ocaml/odoc#732) - ModuleType 'Alias' support (@jonludlam, ocaml/odoc#703) - Improved test suite (@lubega-simon, ocaml/odoc#697) - Improved documentation (@lubega-simon, @jonludlam, ocaml/odoc#702, ocaml/odoc#733) - Strengthen module types (@jonludlam, ocaml/odoc#731) Bugs fixed - `uwt` now can be documented (@jonludlam, ocaml/odoc#708) - Fix resolution involving deeply nested substitutions (@jonludlam, ocaml/odoc#727) - Fix off-by-one error in error reporting (@asavahista, ocaml/odoc#736)
Fixes #701
This PR ensures that heading anchors are unique internally to keep the TOC working. Also, the warning raised by references to ambiguous labels is improved.
If an explicit label is ambiguous, a warning is generated but no effort is made to avoid renaming the anchor or to allow references to it.
I think the suggested special case is not necessary because a warning is generated, which mean the input is considered wrong.