Skip to content

Remove `Forward constructor - #1478

Open
Leonidas-from-XIV wants to merge 3 commits into
ocaml:masterfrom
Leonidas-from-XIV:remove-forward
Open

Remove `Forward constructor#1478
Leonidas-from-XIV wants to merge 3 commits into
ocaml:masterfrom
Leonidas-from-XIV:remove-forward

Conversation

@Leonidas-from-XIV

Copy link
Copy Markdown
Member

I was trying to understand what `Forward does. To do so figure out where a value of `Forward is constructed (from scratch, not as part of a map operation) and see what context this is useful. But it seems that such a value is never constructed. To test my hypothesis I removed the `Forward constructor from the codebase and it seems to compile fine and the tests also pass.

Thus my investigation seems to point to the fact that `Forward is unnecessary and can be removed. I'm opening a draft PR and invite maintainers to take a look - maybe I am missing some context in which `Forward is used, but if anything then at least it points at the fact that this functionality is not covered by any tests.

@Leonidas-from-XIV Leonidas-from-XIV changed the title Remove ` Forward`` constructor Remove `Forward constructor Aug 14, 2026

@art-w art-w left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it looks like pure dead code!

Probably dead code too (?):

odoc/src/xref2/tools.ml

Lines 956 to 958 in 1c3102c

| `Forward f ->
resolve_module env (`Root (ModuleName.make_std f))
|> map_error (fun e -> `Parent (`Parent_module e))

odoc/src/xref2/tools.mli

Lines 33 to 37 in 1c3102c

If the path is a 'Forward' path, that is, a path to a module that has not
yet been compiled, then it may not be possible to resolve the path if this
is being called during the 'compile' phase, in which case the function will
return an unresolved path with no component. Resolution should be attempted
again during the link phase.

(env.ml also have some Forward which might be removable, and the UnresolvedForwardPath error seems guarded by is_module_forward = const false)

Comment thread src/xref2/cpath.ml Outdated
@Leonidas-from-XIV

Copy link
Copy Markdown
Member Author

Good points, I removed some more unreachable code.

I hoped I could remove the UnresolvedForwardPath error, but it is emitted by

odoc/src/xref2/tools.ml

Lines 953 to 954 in 1c3102c

| Some Env.Forward ->
Error (`Parent (`Parent_sig `UnresolvedForwardPath))
. I looked up Forward and through some layers it seems this can only happen if
if important_digests then Ok Odoc_xref2.Env.Forward_reference
evaluates to a a Forward_reference which is dependent on the function called with important_digests = true. This seems to be set when the odoc binary is launched with the -r/--resolve-fwd-refs option. Unfortunately this path seems to be untested, so I am a bit cautious removing things that might have a user-visible effect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants