Fix href for aliased modules in search results

[panglesd]

Fix #1106

It is not ideal that calls to Url.from_identifier can result in 404 pages on aliased modules. This never happened before the introduction of search. This PR fixes this in an ad-hoc way for search.

I think it would be better to fix it in the identifier themselves: the ID of an aliased module would include the ID of its alias if it exists. Otherwise, generated URL can point to the definition point: the parent page with the appropriate anchor.

Before this is implemented, I think this fix is good enough. What do you think?

Ping @EmileTrotignon for the change in API of the search library.

[lpw25]

Sounds to me like there is some confusion internally about the difference between a module identifier and a signature identifier. A module identifier for Foo.Bar stands for the module Bar : ... binding within the page for Foo. A signature identifier for a module Foo.Bar stands for a page about Foo.Bar. The existence of a module identifier doesn't mean the corresponding signature identifier would be valid.

Since Url.from_identifier takes any identifier it cannot tell the difference between a signature identifier and a module identifier, so it should always assume it is a module identifier and give the url that definitely exists. If someone wants to link to the page for a module they should be made to provide a signature identifier.

[jonludlam]

Probably the simplest fix is to say 'stop_before=true' - this should cause links to everything that might have an expansion to go to the definition point instead of into the expansion. The current change definitely doesn't look right.

[panglesd]

Probably the simplest fix is to say 'stop_before=true'

The main problem is that standalone comments do not have an ID, and thus use the ID of their parent module/page. If we use stop_before=true in all cases, when clicking on a comment we will jump in the definition point of the module containing the comment, which is not ideal at all. That's why the fix is a bit more difficult than just stop_before=true...

Since Url.from_identifier takes any identifier it cannot tell the difference between a signature identifier and a module identifier,

I opened #1109 which also stems from this issue. Indeed fixing both #1109 and #1106 at the same time would be much better.

so it should always assume it is a module identifier and give the url that definitely exists.

That would turn all references of modules into links to the definition of the module. Currently those references link to the expansion. That can make sens but I'm always cautious when changing the current behaviour...
I'd like to try to change the odoc IDs to be able to distinguish between a module ID and a signature of module ID.

[lukemaurer]

That would turn all references of modules into links to the definition of the module.

In particular, it would make every [Core.List] link to the module List = List line in the odoc for core.mli, no?

[jonludlam]

Looking back on my comment I missed the vital bit of info: I meant just use stop_before=true for all modules (except compilation units) for search results. So any search for a module ends up at the place where the module is defined, as Leo suggested. I'm not convinced changing the behaviour if there's an expansion or not is worth it.

[panglesd]

I implemented your suggestion @jonludlam. Instead of using stop_before to true only for modules, I set it to false only for standalone docstrings. Indeed, other expansable items (such as module types) also suffer from the issue.

[jonludlam]

Could you please put a comment in the code here explaining why you're matching Doc? It's not very obvious!

[jonludlam]

Hmm.

I think my point was that the intent of this line of code is to prevent a link potentially going to a module/module type/class/etc that doesn't have an expansion. I was mostly expecting to see something more like match kind with | Module _ -> true | ModuleType _ -> true ... | _ -> false - where the wildcard is matching the things like Exception and so on. I don't think it's terribly obvious that stop_before has no impact on those things, so instead we can just match Doc and let stop_before do nothing for the other things.

[jonludlam]

That's much better, thanks!