Stable anchors in links to implementation. #993

panglesd · 2023-08-28T19:29:36Z

Instead of relying on internals of the compiler to generate anchors in source code rendering, this PR makes "semantic" anchors; thus improving stability between OCaml versions/changes in the implementation.

It is done in the following way:

For local identifiers (unqualified identifiers):
- For definition, we generate an id from the name and the position, since names might not be unique. Example of such id: local_elt_7165
- we build a table from Ident.t to id,
- that we use for occurrences (since occurrences don't have access to the location of the definition, an information needed to generate the anchor)
For global identifiers (that have a Uid.t)
- When traversing the typedtree, we record the parent structure to generate semantic id from the names. Example of such id: module-Reference.value-render_resolved.
- We build a table loc_to_id that we combine with the uid_to_loc table to have a uid_to_id table, and store this table in .odoc files.
- After resolving shapes, we use the corresponding table to get the id and the link right for occurrences.

Note: This PR is extracted from https://github.com/jonludlam/odoc/tree/source-locations-3, leaving the "Global jump" part aside. It contains some refactoring.

test/sources/source.t/run.t

src/loader/implementation.ml

src/syntax_highlighter/syntax_highlighter.ml

src/loader/ident_env.cppo.ml

panglesd · 2023-08-31T06:12:36Z

Thanks @Julow for the review!

I've added one "nontrivial" commit: it seems that functor parameter do have a Uid in the uid_to_loc table, so I added that to the traverse. But I had to change a bit the environment so that ids for functor parameter are only created once.

Julow

I've left a few remarks but otherwise this looks ready to be merged :)

src/loader/shape_.mli

Julow · 2023-09-04T15:11:16Z

src/loader/implementation.ml

+  in
+  defs @ poses
+
+let string_of_full_name_ty : Odoc_model.Paths.Identifier.full_name_ty -> string


We already have strings for the different kinds and they are different: https://github.com/ocaml/odoc/blob/master/src/document/url.ml#L109, https://github.com/ocaml/odoc/blob/master/src/xref2/ref_tools.ml#L60 and https://github.com/ocaml/odoc/blob/master/src/model/reference.ml#L33.

It would be weird to see different strings for the same things in the generated doc and in future uses of the new full_name function.

I addressed that in 7804bc0 if you want to have a look!
Basically, I removed full_name (in any case, it did not included enough information to generate unique anchors), and reused the Odoc_document.Url.Anchor.kind.

src/model/paths_types.ml

src/loader/shape_.mli

panglesd · 2023-09-05T14:46:38Z

I added support for having anchors to constructors, but they will only get one when ocaml/ocaml#12508 is merged (more specifically, ocaml/ocaml@6c1fa73).

jonludlam · 2023-09-13T16:26:01Z

I know I suggested the foo_<location> syntax for anchors for local identifiers, but I'm now wondering whether this is actually stable in the presence of line-ending normalization on windows? @dra27?

jonludlam · 2023-09-13T16:32:43Z

src/loader/implementation.ml

+  | DefJmp of Shape.Uid.t
+
+module Analysis = struct
+  let ( @ ) = List.rev_append


I don't like the redefinition of @ to be semantically different from the usual @! It's confusing.

Also I'm not sure that we care too much about the ordering of things do we? There are a bunch of List.revs below that can be removed if we don't.

I guess we can turn the "lift" into a "fold" and carry an accumulator around, that would be the best option.

Yes, the order does not matter.

Turned the traverse into a fold with an accumulator in 9361f0a. Hopefully it's better like that, with less list concatenations!

These should only be referred to by elements on that same source page. This is in contrast with the `SourceLocation identifier, which uses an anchor found using the Shapes API, and can refer across source pages. This commit introduces the constructor but it is currently unused.

This fixes the problem of having anchors that are too dependent on compiler internals by naming the anchor after the position in the source of the particular value.

This is a general function that is intended to be used for naming specific identifiers uniquely.

Signed-off-by: Paul-Elliot <[email protected]> Co-authored-by: Paul-Elliot <[email protected]>

"Implementation loading" and "shape resolution" go to their own module. Cleaned up some test Signed-off-by: Paul-Elliot <[email protected]>

Signed-off-by: Paul-Elliot <[email protected]>

They get an entry in the `uid_to_loc` table, so we need to add them to the `loc_to_ident` table as well for semantic anchor to work. Signed-off-by: Paul-Elliot <[email protected]>

Signed-off-by: Paul-Elliot <[email protected]>

Co-authored-by: Jules Aguillon <[email protected]> Signed-off-by: Paul-Elliot <[email protected]>

Signed-off-by: Paul-Elliot <[email protected]>

The anchors are different, since source code anchors needs the full path, but at least the qualification is similar. Replaces `full_name` which did not included enough information to disambiguate functor parameters. Signed-off-by: Paul-Elliot <[email protected]>

This will allow to have correct anchors to constructors, as soon as they are added to the `uid_to_loc` table. Signed-off-by: Paul-Elliot <[email protected]>

Signed-off-by: Paul-Elliot <[email protected]>

for more self-explanatoriness Signed-off-by: Paul-Elliot <[email protected]>

Now that anchors are semantic, they should not depend on the OCaml version.

Also simplify the interfaces of the loader a bit.

panglesd · 2023-09-15T07:19:29Z

src/odoc/odoc_link.ml

      |> handle_warnings ~input_warnings ~warnings_options
      >>= fun (m, warnings) ->
-      Odoc_file.save_unit output ~warnings (m, shape);
+      let m = {m with Odoc_model.Lang.Compilation_unit.shape = None} in


Doesn't it make more sense to modify the compilation unit record only in the link_unit function?
When a record is modified in many place, I find it difficult to have an idea of which field will go into the file...

Moreover, it would be nice to add a small comment, just in case it is not clear later (shapes are not used after the link phase).

I was aiming for 'as close as possible to the save function' - the comment is a good idea.

panglesd · 2023-09-15T09:02:19Z

I know I suggested the foo_ syntax for anchors for local identifiers, but I'm now wondering whether this is actually stable in the presence of line-ending normalization on windows?

I tested and yes, the "same" file with windows and unix ending won't have the same locations. (One could argue that it's not really the same file...)

We cannot use filename/line number/pos_bol since the anchor might not be unique, due to line directives.
Maybe the best option is to use our own counter for those anchors!

Signed-off-by: Paul-Elliot <[email protected]>

So that adding some content at the end of the file does not modify anchors that are above. Signed-off-by: Paul-Elliot <[email protected]>

Julow

A lot has changed in a small amount of time making the review difficult.
But I think this is ready to merge!

Julow · 2023-09-15T13:48:58Z

src/odoc/odoc_link.ml

+         the compiler and load it in odoc x.y compiled with a different
+         version of the compiler. This is an important use case for
+         voodoo. *)
+      let m = { m with Odoc_model.Lang.Compilation_unit.shape = None } in


And uid_to_id as well ?

Ooh yes, I had originally thought that wouldn't be needed but you're right, we do need to sort that.

Hrm, I'm rather more nervous about putting an empty map in there than a None. Might need some more options...

OK, fixed, I think!

Looks nice :)

Julow · 2023-09-15T13:50:46Z

src/odoc/odoc_link.ml

+         save an odocl file with odoc x.y compiled with one version of
+         the compiler and load it in odoc x.y compiled with a different
+         version of the compiler. This is an important use case for
+         voodoo. *)


I don't think these assumptions are true. Perhaps just say that we avoid adding more constraint.

I've reworded it.

Julow · 2023-09-15T13:56:20Z

src/loader/odoc_loader.ml

@@ -45,24 +42,44 @@ exception Not_an_interface

 exception Make_root_error of string

+#if OCAML_VERSION >= (4, 14, 0)


I think this cppo contamination is not needed, this function could be moved to Implementation.

I think this is a nice idea, yes.

dra27 · 2023-09-17T12:32:37Z

I know I suggested the foo_ syntax for anchors for local identifiers, but I'm now wondering whether this is actually stable in the presence of line-ending normalization on windows?

I tested and yes, the "same" file with windows and unix ending won't have the same locations. (One could argue that it's not really the same file...)

We cannot use filename/line number/pos_bol since the anchor might not be unique, due to line directives. Maybe the best option is to use our own counter for those anchors!

Does the compiler keep actual locations as well? Not looked, but presumably Merlin uses actual locations (insensitive to #line directives)?

In general, the key thing for being LF/CRLF compatible is to ensure that the No measurement can ever span a new line. So pos_bol is unusable but pos_cnum is fine. Similarly, for spans you need to start lnum+cnum and end lnum+cnum rather than just end offset.

jonludlam · 2023-09-18T11:13:25Z

OK, I think any other issues with this are minor and can be fixed up in follow-up PRs. In it goes!

panglesd commented Aug 28, 2023

View reviewed changes

test/sources/source.t/run.t Outdated Show resolved Hide resolved

panglesd commented Aug 28, 2023

View reviewed changes

test/sources/source.t/run.t Outdated Show resolved Hide resolved

panglesd force-pushed the semantic-anchors branch from 8073c59 to 4c022d1 Compare August 30, 2023 14:31

Julow reviewed Aug 30, 2023

View reviewed changes

src/loader/implementation.ml Outdated Show resolved Hide resolved

src/syntax_highlighter/syntax_highlighter.ml Outdated Show resolved Hide resolved

src/loader/ident_env.cppo.ml Outdated Show resolved Hide resolved

src/loader/ident_env.cppo.ml Show resolved Hide resolved

panglesd force-pushed the semantic-anchors branch 2 times, most recently from 4766011 to 37bc8de Compare August 31, 2023 07:21

Julow approved these changes Sep 4, 2023

View reviewed changes

panglesd force-pushed the semantic-anchors branch from f5fd9ae to 7804bc0 Compare September 5, 2023 13:32

panglesd force-pushed the semantic-anchors branch 2 times, most recently from fdb99d9 to 23ec596 Compare September 6, 2023 08:43

jonludlam reviewed Sep 13, 2023

View reviewed changes

jonludlam and others added 16 commits September 13, 2023 22:00

Use char offsets in source for local source links/anchors

65aace9

This fixes the problem of having anchors that are too dependent on compiler internals by naming the anchor after the position in the source of the particular value.

Separate syntax from other source annotations

e75fa97

Add full_name to Paths.Identifier

1997858

This is a general function that is intended to be used for naming specific identifiers uniquely.

Semantic anchors for source locations

a9e404c

Signed-off-by: Paul-Elliot <[email protected]> Co-authored-by: Paul-Elliot <[email protected]>

Refactoring and compatibility

8efefd9

"Implementation loading" and "shape resolution" go to their own module. Cleaned up some test Signed-off-by: Paul-Elliot <[email protected]>

Add exception to [uid_to_loc] table

19dff47

Signed-off-by: Paul-Elliot <[email protected]>

Add local jump to definition for constructors

c412072

Signed-off-by: Paul-Elliot <[email protected]>

optimize list concatenation

ce0334a

Signed-off-by: Paul-Elliot <[email protected]>

Never add links from ghost locations

44fa491

Signed-off-by: Paul-Elliot <[email protected]>

Add semantic anchors for extension constructors

4637797

They get an entry in the `uid_to_loc` table, so we need to add them to the `loc_to_ident` table as well for semantic anchor to work. Signed-off-by: Paul-Elliot <[email protected]>

Fix module with constraints not being folded over

45f63a1

Signed-off-by: Paul-Elliot <[email protected]>

fix wrong location taken on types

090d0fb

Signed-off-by: Paul-Elliot <[email protected]>

Use compiler-independent anchor for functor argument

d463927

Signed-off-by: Paul-Elliot <[email protected]>

Do not add anchors to ghost locations from uid_to_loc table

ae41cb8

Signed-off-by: Paul-Elliot <[email protected]>

ocamlformat

4256697

Signed-off-by: Paul-Elliot <[email protected]>

panglesd and others added 11 commits September 13, 2023 22:08

Style-related review comments

b86bd15

Co-authored-by: Jules Aguillon <[email protected]> Signed-off-by: Paul-Elliot <[email protected]>

Compatibility

4505b82

Signed-off-by: Paul-Elliot <[email protected]>

Loader: Add constructors to the environment

e7f7278

This will allow to have correct anchors to constructors, as soon as they are added to the `uid_to_loc` table. Signed-off-by: Paul-Elliot <[email protected]>

Loader: Rename shape_.ml into lookup_def.ml

6a109ce

Signed-off-by: Paul-Elliot <[email protected]>

Rename SourceLocationInt to SourceLocationInternal

dc4b564

for more self-explanatoriness Signed-off-by: Paul-Elliot <[email protected]>

Remove constraint on source tests

b15f452

Now that anchors are semantic, they should not depend on the OCaml version.

Move the shape lookup code into xref2, where other resolution happens

8088e83

Also simplify the interfaces of the loader a bit.

Update mdx tests

de7641f

Update driver to take cmt argument

4500c04

Formatting and backwards compat

0121576

jonludlam force-pushed the semantic-anchors branch from 23ec596 to 0121576 Compare September 14, 2023 15:09

ocamlformat ignore updates

105c500

jonludlam mentioned this pull request Sep 14, 2023

Source locations #981

Closed

panglesd commented Sep 15, 2023

View reviewed changes

jonludlam and others added 4 commits September 15, 2023 14:24

Remove shape from odocl files

f2bafc2

ocamlformating loader/implementation.ml files

c848d55

Signed-off-by: Paul-Elliot <[email protected]>

Source loading: turned traverse from "lift" to "fold".

11c1bd0

Signed-off-by: Paul-Elliot <[email protected]>

Source rendering: anchors do not depend on locations

652931c

Signed-off-by: Paul-Elliot <[email protected]>

panglesd force-pushed the semantic-anchors branch from 6a2ba13 to 652931c Compare September 15, 2023 12:24

panglesd and others added 2 commits September 15, 2023 15:08

Source rendering: Anchor counter should increase

ae5bc8b

So that adding some content at the end of the file does not modify anchors that are above. Signed-off-by: Paul-Elliot <[email protected]>

Add explanatory comment

6e51617

Julow approved these changes Sep 15, 2023

View reviewed changes

jonludlam added 3 commits September 15, 2023 15:46

Move version-dependent code from odoc_loader to implementation

f6cbd64

Combine shape and uid_to_id to one field in Lang

3cd4f2f

Update tests

008efb3

jonludlam merged commit 86f33b1 into ocaml:master Sep 18, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stable anchors in links to implementation. #993

Stable anchors in links to implementation. #993

panglesd commented Aug 28, 2023

panglesd commented Aug 31, 2023

Julow left a comment

Julow Sep 4, 2023

panglesd Sep 5, 2023 •

edited

Loading

panglesd commented Sep 5, 2023

jonludlam commented Sep 13, 2023

jonludlam Sep 13, 2023

panglesd Sep 14, 2023

panglesd Sep 15, 2023

panglesd Sep 15, 2023

jonludlam Sep 15, 2023

panglesd commented Sep 15, 2023

Julow left a comment

Julow Sep 15, 2023

jonludlam Sep 15, 2023

jonludlam Sep 15, 2023

jonludlam Sep 15, 2023

Julow Sep 18, 2023

Julow Sep 15, 2023

jonludlam Sep 15, 2023

Julow Sep 15, 2023

jonludlam Sep 15, 2023

jonludlam Sep 15, 2023

dra27 commented Sep 17, 2023 •

edited

Loading

jonludlam commented Sep 18, 2023

		@@ -45,24 +42,44 @@ exception Not_an_interface

		exception Make_root_error of string

		#if OCAML_VERSION >= (4, 14, 0)

Stable anchors in links to implementation. #993

Stable anchors in links to implementation. #993

Conversation

panglesd commented Aug 28, 2023

panglesd commented Aug 31, 2023

Julow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panglesd Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

panglesd commented Sep 5, 2023

jonludlam commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panglesd commented Sep 15, 2023

Julow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dra27 commented Sep 17, 2023 • edited Loading

jonludlam commented Sep 18, 2023

panglesd Sep 5, 2023 •

edited

Loading

dra27 commented Sep 17, 2023 •

edited

Loading