Path-references lookup to pages #1150

Julow · 2024-06-21T15:16:02Z

This implements the resolving of some path-references using the new Named_roots mechanism recently added.

This is work in progress. Currently, current-package and absolute references are resolved from pages. On top of #1142

Julow · 2024-07-03T14:27:21Z

This should be ready for review.

This is based on top of #1142 and #1151 and implements absolute, relative, current-package-relative path-references to pages.

panglesd · 2024-07-08T12:41:49Z

src/xref2/env.mli

  | Forward

+type path_query = [ `Page | `Unit ] * Reference.tag_page_path * string list


Curry curry, curry!

(this is not a comment asking for any change. I was just amused at seeing the input of the function being a tuple)

src/odoc/resolver.ml

panglesd · 2024-07-08T12:49:31Z

src/odoc/resolver.ml

+  (* Do not implement [lookup_path] in compile mode, as that might return
+     different results depending on the compilation order. *)


This is also true for environments for impl, and compile env for pages.

Yes but they are implemented in compile mode. Hopefully not used.

src/xref2/ref_tools.ml

These references are resolved using the 'Named_roots' mechanism introduced recently. This only handles current-package references from pages, as the current root is not detected yet when resolving from modules. The resolving takes place in Resolver, which takes a simpler 'path_query' type.

Note: By name lookups are not implemented yet.

panglesd

Very nice!

panglesd · 2024-07-11T07:52:15Z

src/odoc/resolver.ml

@@ -389,53 +418,125 @@ let add_unit_to_cache u =
  in
  Hashtbl.add unit_cache target_name [ u ]

+let lookup_path _ap ~pages ~libs:_ ~hierarchy (kind, tag, path) =
+  let module Env = Odoc_xref2.Env in
+  let ( >>= ) x f = match x with Some x' -> f x' | None -> None in


There now is an OptionMonad in the Odoc_utils library to open for that!

I just realized how ridiculous the interface is and how hard it makes error propagation. I'll revamp the interface.

panglesd · 2024-07-11T09:33:04Z

src/odoc/resolver.ml

+        let pages = Named_roots.create ~current_root:current_package page_roots
+        and libs = Named_roots.create ~current_root:current_lib lib_roots in
+        let hierarchy =
+          match Named_roots.find_by_path pages ~path:(Fpath.v ".") with


Thanks, by using a bug you uncovered its existence!

As I though it, find_by_path was supposed to only return files (and not directories).
I think it is fine for it to return directories, but in this case we need to check that the output is a file (using Sys.is_regular_file f or not (Sys.is_directory f))

Here we want a directory. This requires a path that will be used to resolve relative references later, it is not expected to be a page.

Elsewhere, the lookup function loads the page/unit files before returning a positive result so no need to check that at any place. The lookup can't succeed on a directory.

panglesd · 2024-07-11T09:37:15Z

src/odoc/resolver.ml

+        match Hierarchy.resolve_relative hierarchy path with
+        | Ok path -> load_page path
+        | Error `Escape_hierarchy ->
+            Env.Path_not_found (* TODO: propagate more information *))


Writing a comment here not to forget this TODO.

This will be part of improving error propagation in the resolver (cf comment #1150 (comment)) that I would prefer to think about later.

panglesd · 2024-07-11T09:40:09Z

src/odoc/resolver.ml

+    match hierarchy with
+    | Some hierarchy -> (
+        match Hierarchy.resolve_relative hierarchy path with
+        | Ok path -> load_page path


if Sys.is_directory path then Env.Path_directory else load_page path

(this check should probably be in load_page maybe)

Yes!

Though this would be detected while loading the page, but the error is not propagated and is instead printed as a warning outside of the usual warning mechanisms.

No! I think this should be reported by the load_page function. Sys.is_directory is just an other point where an exception can be raised and an other inconsistency.
That code have a TODO that I would leave for later.

I can't see a TODO yet on this specific error...

The problem is that if you have a directory with the specific name page-directory.odoc, then you might have an uncaught exception when resolving {!./page-directory}:

Error while unmarshalling "h/pkg/doc/page-directory.odoc": Sys_error("Is a directory")

I'm happy to leave a better error reporting for later, but I would really prefer to merge without the risk of uncaught exceptions when resolving references... (note that when we lookup for assets, the right of having a directory referenced will be much higher).

That's not an uncaught exception but a very badly reported expected error.

I agree this should be checked eventually but without a way to propagate the errors correctly, this will just slightly improve a very bad error message.
In any case, I don't want to spam the code with this kind of checks. This check must happen just before loading the file, not elsewhere.

I'm doing the required refactoring to make this possible.

Oh indeed, that's not an uncaught exception. It was obvious from the message and I missed the "catch all exceptions" in load_...

Thanks for doing the refactoring!

I changed the interface of the resolver completely in order to pass the errors. Which I leave for later.

src/xref2/ref_tools.ml

The lookup_path code is refactored into the OptionMonad style until better error propagation is implemented.

Remove the 'lookup_path' function and change 'lookup_page' and 'lookup_unit' to handle by-name and by-path lookups. This removes the dependent query and result type that are annoying to work with and allow to propagate errors from the resolver in a unified way.

panglesd

Once CI is happy, I think this is ready for merge!

Julow · 2024-07-12T12:50:37Z

CI was not happy.

Julow mentioned this pull request Jun 26, 2024

Add the '--current-package' option #1151

Merged

Julow changed the title ~~Path-references lookup~~ Path-references lookup to pages Jul 3, 2024

Julow marked this pull request as ready for review July 3, 2024 14:27

panglesd reviewed Jul 8, 2024

View reviewed changes

Julow force-pushed the path_references_lookup branch from 1a10a77 to f1aee06 Compare July 9, 2024 09:52

Julow mentioned this pull request Jul 9, 2024

Parsing of path-references to pages and modules #1142

Merged

Julow force-pushed the path_references_lookup branch from f1aee06 to bb51516 Compare July 10, 2024 14:39

Julow added 7 commits July 11, 2024 10:37

test: Path references lookups

2346067

Add Fs.File.of_segs

b92245f

Resolve absolute path references to pages

b675929

Update CHANGES

c6f5f5b

Enable current package lookups by passing --current-package

db1a7d0

Resolve relative references to pages

8493161

Julow force-pushed the path_references_lookup branch from bb51516 to 8493161 Compare July 11, 2024 08:44

Remove temporary rooted page lookups

4571ee9

Note: By name lookups are not implemented yet.

panglesd approved these changes Jul 11, 2024

View reviewed changes

Julow added 3 commits July 11, 2024 12:03

Code style

8c28197

Avoid passing empty list to Fs.File.of_segs

20e2b18

The lookup_path code is refactored into the OptionMonad style until better error propagation is implemented.

panglesd approved these changes Jul 12, 2024

View reviewed changes

Compat with 4.02

7d2c9d0

Julow force-pushed the path_references_lookup branch from 3c2eb3a to 7d2c9d0 Compare July 12, 2024 13:11

panglesd merged commit 13200ed into ocaml:master Jul 12, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Path-references lookup to pages #1150

Path-references lookup to pages #1150

Julow commented Jun 21, 2024

Julow commented Jul 3, 2024

panglesd Jul 8, 2024

panglesd Jul 8, 2024

panglesd Jul 8, 2024

Julow Jul 11, 2024

panglesd left a comment

panglesd Jul 11, 2024

Julow Jul 11, 2024

panglesd Jul 11, 2024

Julow Jul 11, 2024

panglesd Jul 11, 2024

Julow Jul 11, 2024

panglesd Jul 11, 2024

Julow Jul 11, 2024

Julow Jul 11, 2024

panglesd Jul 11, 2024

Julow Jul 11, 2024 •

edited

Loading

panglesd Jul 11, 2024

Julow Jul 11, 2024

panglesd left a comment

Julow commented Jul 12, 2024

		\| Forward

		type path_query = [ `Page \| `Unit ] * Reference.tag_page_path * string list

		(* Do not implement [lookup_path] in compile mode, as that might return
		different results depending on the compilation order. *)

Path-references lookup to pages #1150

Path-references lookup to pages #1150

Conversation

Julow commented Jun 21, 2024

Julow commented Jul 3, 2024

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

Julow Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panglesd left a comment

Choose a reason for hiding this comment

Julow commented Jul 12, 2024

Julow Jul 11, 2024 •

edited

Loading