From 5be35d0dda9edbec38c9a5915ae0832fd3196b42 Mon Sep 17 00:00:00 2001 From: Jon Ludlam Date: Fri, 8 May 2026 11:33:22 +0100 Subject: [PATCH 1/3] Add test for issue 1431 --- test/xref2/dune | 1 + test/xref2/github_issue_1431.t/legacy.mli | 13 +++++++++++ test/xref2/github_issue_1431.t/run.t | 28 +++++++++++++++++++++++ test/xref2/github_issue_1431.t/top.mli | 1 + 4 files changed, 43 insertions(+) create mode 100644 test/xref2/github_issue_1431.t/legacy.mli create mode 100644 test/xref2/github_issue_1431.t/run.t create mode 100644 test/xref2/github_issue_1431.t/top.mli diff --git a/test/xref2/dune b/test/xref2/dune index 4134c6b961..7265ca085a 100644 --- a/test/xref2/dune +++ b/test/xref2/dune @@ -43,6 +43,7 @@ js_stack_overflow expansion github_issue_1066 + github_issue_1431 include_module_type_of_preamble) (enabled_if (>= %{ocaml_version} 4.08.0))) diff --git a/test/xref2/github_issue_1431.t/legacy.mli b/test/xref2/github_issue_1431.t/legacy.mli new file mode 100644 index 0000000000..5da2bb59d3 --- /dev/null +++ b/test/xref2/github_issue_1431.t/legacy.mli @@ -0,0 +1,13 @@ +(** A signature with a sibling module declaration and a destructive module + substitution whose manifest references that sibling. + + When this signature is loaded as the expansion of [include module type of + Legacy] elsewhere, [Of_Lang] rewrites internal cross-references to local + idents — so the manifest of [Original_components] becomes a + [`Resolved (`Local _)] path. *) + +module Components : sig + type t +end + +module Original_components := Components diff --git a/test/xref2/github_issue_1431.t/run.t b/test/xref2/github_issue_1431.t/run.t new file mode 100644 index 0000000000..efb67e030c --- /dev/null +++ b/test/xref2/github_issue_1431.t/run.t @@ -0,0 +1,28 @@ +Repro of #1431. We shouldn't see any errors or warnings from odoc. + + $ ocamlc -bin-annot -c legacy.mli + $ ocamlc -bin-annot -c top.mli + $ odoc compile legacy.cmti + $ odoc compile -I . top.cmti + odoc: internal error, uncaught exception: + Failure("error") + Raised at Stdlib.failwith in file "stdlib.ml", line 29, characters 17-33 + Called from Odoc_xref2__Tools.apply_inner_substs.inner in file "src/xref2/tools.ml", line 2414, characters 24-34 + Called from Odoc_xref2__Tools.apply_inner_substs in file "src/xref2/tools.ml", line 2417, characters 20-34 + Called from Odoc_xref2__Compile.include_.get_expansion in file "src/xref2/compile.ml", line 430, characters 19-51 + Called from Odoc_xref2__Compile.signature_items.loop in file "src/xref2/compile.ml", line 348, characters 27-41 + Called from Odoc_xref2__Compile.signature in file "src/xref2/compile.ml", line 365, characters 19-49 + Called from Odoc_xref2__Compile.content.(fun) in file "src/xref2/compile.ml", line 100, characters 15-54 + Called from Odoc_xref2__Compile.unit in file "src/xref2/compile.ml", line 68, characters 21-47 + Called from Odoc_xref2__Lookup_failures.with_ref in file "src/xref2/lookup_failures.ml", line 13, characters 10-14 + Called from Odoc_xref2__Lookup_failures.catch_failures in file "src/xref2/lookup_failures.ml", line 60, characters 20-37 + Called from Odoc_odoc__Compile.resolve_and_substitute in file "src/odoc/compile.ml", line 154, characters 4-49 + Called from Odoc_model__Error.catch in file "src/model/error.ml", line 52, characters 21-27 + Called from Odoc_model__Error.catch_warnings.(fun) in file "src/model/error.ml", line 87, characters 18-22 + Called from Odoc_model__Error.with_ref in file "src/model/error.ml", line 65, characters 12-16 + Re-raised at Odoc_model__Error.with_ref in file "src/model/error.ml", line 70, characters 4-11 + Called from Odoc_odoc__Compile.compile.(fun) in file "src/odoc/compile.ml", line 407, characters 6-206 + Called from Cmdliner_term.app.(fun) in file "cmdliner_term.ml", line 22, characters 19-24 + Called from Cmdliner_term.app.(fun) in file "cmdliner_term.ml", line 20, characters 12-19 + Called from Cmdliner_eval.run_parser in file "cmdliner_eval.ml", line 41, characters 7-16 + [2] diff --git a/test/xref2/github_issue_1431.t/top.mli b/test/xref2/github_issue_1431.t/top.mli new file mode 100644 index 0000000000..0d8083cc1a --- /dev/null +++ b/test/xref2/github_issue_1431.t/top.mli @@ -0,0 +1 @@ +include module type of Legacy From e3bd74bb6eb21c1c7dc49261ed153c17f28913d2 Mon Sep 17 00:00:00 2001 From: Jon Ludlam Date: Fri, 8 May 2026 12:17:10 +0100 Subject: [PATCH 2/3] Fix 1431 The fix for #930 introduced a problem where a local module substitution fails to resolve, which led to an unrecoverable exception being raised. This commit detects the problem and essentially leaves the signature as it was found. This means that the fix for #930 is _partial_ (and always was). The behaviour that this commit introduces is better now, but not perfect. The original #930 problem is due to a clash of identifier caused by an include with a module substitution. The fix was to turn the module substitution into a `sig ... end with module Foo :=` expression, calculate the result and use that as the body of the signature, hence no clash. This was applied universally, so any time an include had a substitution in it, we used this code. However, if the right-hand side of the module substitution was a local module, as in the test in this PR, odoc cannot resolve this, and hence raises an error. This commit detects that problem and reverts the signature to its original form. Thus the new behaviour means that there's still potential for #930, but now the condition for a problem is that there's a module substitution being included into a signature that has a module with the same name (the #930 problem), _and_ that the substitution was for a local module (the cause of #1431) --- src/xref2/tools.ml | 47 +++++++++++++++++++--------- test/xref2/github_issue_1431.t/run.t | 27 +++------------- 2 files changed, 36 insertions(+), 38 deletions(-) diff --git a/src/xref2/tools.ml b/src/xref2/tools.ml index 9e25ac5299..b0e33e2405 100644 --- a/src/xref2/tools.ml +++ b/src/xref2/tools.ml @@ -2356,25 +2356,32 @@ let apply_inner_substs env (sg : Component.Signature.t) : Component.Signature.t let rec inner (items : Component.Signature.item list) : Component.Signature.item list = match items with - | Component.Signature.TypeSubstitution (id, typedecl) :: rest -> ( + | (Component.Signature.TypeSubstitution (id, typedecl) as orig) :: rest -> ( let subst = Component.ModuleType.TypeSubst (`Dot (`Root, Ident.Name.type_ id), typedecl.equation) in - let rest = + let rest' = inner rest in + let inlined = Component.Signature.Type (id, Ordinary, Component.Delayed.put (fun () -> typedecl)) - :: inner rest + :: rest' in - match fragmap env subst { sg with items = rest } with + match fragmap env subst { sg with items = inlined } with | Ok sg' -> sg'.items - | Error _ -> failwith "error") - | Component.Signature.ModuleSubstitution (id, modsubst) :: rest -> ( + | Error e -> + Lookup_failures.report_internal + "apply_inner_substs: fragmap failed: %a" Errors.Tools_error.pp + (e :> Errors.Tools_error.any); + orig :: rest') + | (Component.Signature.ModuleSubstitution (id, modsubst) as orig) :: rest + -> ( let subst = Component.ModuleType.ModuleSubst (`Dot (`Root, Ident.Name.module_ id), modsubst.manifest) in - let rest = + let rest' = inner rest in + let inlined = Component.Signature.Module ( id, Ordinary, @@ -2386,17 +2393,23 @@ let apply_inner_substs env (sg : Component.Signature.t) : Component.Signature.t canonical = None; hidden = false; }) ) - :: inner rest + :: rest' in - match fragmap env subst { sg with items = rest } with + match fragmap env subst { sg with items = inlined } with | Ok sg' -> sg'.items - | Error _ -> failwith "error") - | Component.Signature.ModuleTypeSubstitution (id, modtypesubst) :: rest -> ( + | Error e -> + Lookup_failures.report_internal + "apply_inner_substs: fragmap failed: %a" Errors.Tools_error.pp + (e :> Errors.Tools_error.any); + orig :: rest') + | (Component.Signature.ModuleTypeSubstitution (id, modtypesubst) as orig) + :: rest -> ( let subst = Component.ModuleType.ModuleTypeSubst (`Dot (`Root, Ident.Name.module_type id), modtypesubst.manifest) in - let rest = + let rest' = inner rest in + let inlined = Component.Signature.ModuleType ( id, Component.Delayed.put (fun () -> @@ -2406,11 +2419,15 @@ let apply_inner_substs env (sg : Component.Signature.t) : Component.Signature.t expr = Some modtypesubst.manifest; canonical = None; }) ) - :: inner rest + :: rest' in - match fragmap env subst { sg with items = rest } with + match fragmap env subst { sg with items = inlined } with | Ok sg' -> sg'.items - | Error _ -> failwith "error") + | Error e -> + Lookup_failures.report_internal + "apply_inner_substs: fragmap failed: %a" Errors.Tools_error.pp + (e :> Errors.Tools_error.any); + orig :: rest') | x :: rest -> x :: inner rest | [] -> [] in diff --git a/test/xref2/github_issue_1431.t/run.t b/test/xref2/github_issue_1431.t/run.t index efb67e030c..66ff8ea862 100644 --- a/test/xref2/github_issue_1431.t/run.t +++ b/test/xref2/github_issue_1431.t/run.t @@ -1,28 +1,9 @@ -Repro of #1431. We shouldn't see any errors or warnings from odoc. +Repro of #1431. The unresolvable substitution is logged as a non-fatal +internal warning rather than triggering an assertion failure. $ ocamlc -bin-annot -c legacy.mli $ ocamlc -bin-annot -c top.mli $ odoc compile legacy.cmti $ odoc compile -I . top.cmti - odoc: internal error, uncaught exception: - Failure("error") - Raised at Stdlib.failwith in file "stdlib.ml", line 29, characters 17-33 - Called from Odoc_xref2__Tools.apply_inner_substs.inner in file "src/xref2/tools.ml", line 2414, characters 24-34 - Called from Odoc_xref2__Tools.apply_inner_substs in file "src/xref2/tools.ml", line 2417, characters 20-34 - Called from Odoc_xref2__Compile.include_.get_expansion in file "src/xref2/compile.ml", line 430, characters 19-51 - Called from Odoc_xref2__Compile.signature_items.loop in file "src/xref2/compile.ml", line 348, characters 27-41 - Called from Odoc_xref2__Compile.signature in file "src/xref2/compile.ml", line 365, characters 19-49 - Called from Odoc_xref2__Compile.content.(fun) in file "src/xref2/compile.ml", line 100, characters 15-54 - Called from Odoc_xref2__Compile.unit in file "src/xref2/compile.ml", line 68, characters 21-47 - Called from Odoc_xref2__Lookup_failures.with_ref in file "src/xref2/lookup_failures.ml", line 13, characters 10-14 - Called from Odoc_xref2__Lookup_failures.catch_failures in file "src/xref2/lookup_failures.ml", line 60, characters 20-37 - Called from Odoc_odoc__Compile.resolve_and_substitute in file "src/odoc/compile.ml", line 154, characters 4-49 - Called from Odoc_model__Error.catch in file "src/model/error.ml", line 52, characters 21-27 - Called from Odoc_model__Error.catch_warnings.(fun) in file "src/model/error.ml", line 87, characters 18-22 - Called from Odoc_model__Error.with_ref in file "src/model/error.ml", line 65, characters 12-16 - Re-raised at Odoc_model__Error.with_ref in file "src/model/error.ml", line 70, characters 4-11 - Called from Odoc_odoc__Compile.compile.(fun) in file "src/odoc/compile.ml", line 407, characters 6-206 - Called from Cmdliner_term.app.(fun) in file "cmdliner_term.ml", line 22, characters 19-24 - Called from Cmdliner_term.app.(fun) in file "cmdliner_term.ml", line 20, characters 12-19 - Called from Cmdliner_eval.run_parser in file "cmdliner_eval.ml", line 41, characters 7-16 - [2] + File "top.cmti": + Warning: apply_inner_substs: fragmap failed: Unresolved module path resolved(Components/2) (Local id found: Components/2) From fa31cf285c12b726f543e0a786e3d1f5017edf60 Mon Sep 17 00:00:00 2001 From: Jon Ludlam Date: Mon, 11 May 2026 14:59:25 +0100 Subject: [PATCH 3/3] Update CHANGES.md --- CHANGES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES.md b/CHANGES.md index a2602e4f12..9b291c5ac2 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -5,6 +5,7 @@ (@jonludlam, #1427) - Fix #1429, which broke docs in packages including `merlin-lib` (@jonludlam, #1430) +- Fix #1431, a regression introduced in v3.2.0 (#1433, @jonludlam) # 3.2.0