Fix canonical #820
Fix canonical #820
Conversation
Given a fully qualified canonical path: `X.Y.Z`, we now resolve `X.Y.Z` and check it works. If it does, we then resolve `Y.Z` and `Z`, and pick the shortest one for which the identifier is the same as the fully qualified path. Additionally, we now also expand modules that are 'self canonical' even if the module they're an alias of is not hidden. This has led to the new expansion of Ocamlary.Aliases.P2.Z in the generator tests.
| } | ||
| } | ||
|
|
||
| This should probably not resolve at all, but that's a problem for another day. currently it resolves as Test.Wrapper3.X |
There was a problem hiding this comment.
There should be a warning at location of the canonical tag.
There was a problem hiding this comment.
A warning should be emitted, yes. I think we should address this in a separate PR though.
src/xref2/tools.ml
Outdated
| and [possibilities] is a function that, given the fully qualified | ||
| unresolved path, returns a list of all possible unresolved paths | ||
| (including the longest one) *) |
There was a problem hiding this comment.
possibilities should be sorted, shortest first ?
There was a problem hiding this comment.
Yes, I'll mention that.
| let resolved = filter_map resolve (possibilities env p2) in | ||
| let find_fn (r, _) = get_identifier r = fallback_id in | ||
| try Some (List.find find_fn resolved) with _ -> None) |
There was a problem hiding this comment.
filter_map can be avoided by doing resolve and find_fn directly during the List.find.
There was a problem hiding this comment.
But then I'd have to call resolve again. I could write a bit more of a specialised function but I opted for legibility here.
| If a module doesn't know it's canonical, it will fail the self-canonical check, and | ||
| therefore not necessarily be expanded. If this happens, we call [process_module_path] | ||
| to stick the [`Alias] constructor back on so we'll link to the correct place. *) |
There was a problem hiding this comment.
Shouldn't this be done by the caller ? Because like this, it'll still be wrapped in a `Canonical and that makes this function part of the complicated recursive functions.
There was a problem hiding this comment.
I'd prefer to leave it here so we don't spread the logic between two functions. It'd be better if we have one function to test for self-canonical modules that we can call from here and also from link.ml, but there's a bit of refactoring required for that.
There was a problem hiding this comment.
I mean the resolving could fail and the function that handles the `Canonical case can start again by calling reresolve_module and at the same time removing the `Canonical constructor.
There was a problem hiding this comment.
Ah, no, the canonical is still valid, it's just that it's going to point to the aliased module rather than the module itself, as the module itself won't be expanded because it doesn't know it's canonical :-)
CHANGES: Additions - New subcommand to resolve references (@panglesd, @lubega-simon, ocaml/odoc#812) - Improved rendering of long signatures (@panglesd, ocaml/odoc#782) - Handle comments attached to open statement as floating comment, instead of dropping them (@panglesd, ocaml/odoc#797) - Empty includes (containing entirely shadowed entries) are now hidden (@panglesd, ocaml/odoc#798) Bugs fixed - Fix a missing Result constructor during compile. This will cause some functor arguments to have different filenames (@jonludlam, ocaml/odoc#795) - Better memory/disk space usage when handling module alias chains (@jonludlam, ocaml/odoc#799) - Resolving class-type paths (ie., `val x : #c`) (@jonludlam, ocaml/odoc#809) - Skip top-level attributes while extracting the top comment. Fix top-comment extraction with PPX preprocessing (@jorisgio, ocaml/odoc#819) - Better handling of @canonical tags (@jonludlam, ocaml/odoc#820) - css: improved layout (@jonludlam, @Julow, ocaml/odoc#822)
Fixes #724.
I'd still like to verify that paths containing canonical modules are correct, but we weren't doing that before, either. See the new test for an example of this not working.