Skip to content

Fix resolution involving deeply nested substitutions #727

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 14, 2021
Merged

Fix resolution involving deeply nested substitutions #727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/document/generator.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1430,14 +1430,22 @@ module Make (Syntax : SYNTAX) = struct
++ Link.from_path (m :> Paths.Path.t)
++ O.txt " " ++ O.keyword "end"

and is_elidable_with_u : Odoc_model.Lang.ModuleType.U.expr -> bool = function
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not render nested substitutions ? It is rendered for T:

# Module Test
module type S = sig ... end
module type T = S with type M.t := int

but the documentation for T doesn't mention any substitution at all:

# Module type Test.T
module M : sig ... end
type t = int

Showing the substitution is not that bad:

# Module type Test.T
module M : M.S with type t := int
type t = int

It would be even more useful in case of non-destructive substitution because the definition of t would be in the same documentation page:

# Module type Test.T
module M : M.S with type t = int
type t = M.t

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, it was certainly not my intention to remove the 'with type' always, just the construct sig ... end with type ...

I'll double check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I misinterpreted what you mean - though I'm still not quite sure! To be clear, we've got:

module type S = sig
  module M: sig
    type t
  end
  type t = M.t
end
module type T = S with type M.t := int

You wrote that you thought this would be better:

# Module type Test.T
module M : M.S with type t := int
type t = int

I don't think you mean M.S - I think you might have meant S.M - but we can't do this because we can't have paths to elements within module types. Is this correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

By M.S, I meant S.M or even Test.S.M, sorry for the confusion.

Hmm, it was certainly not my intention to remove the 'with type' always, just the construct sig ... end with type ...

sig ... end with type ... is turned into just sig ... end but I'd expect Test.S.M with type ... even if M is not a module type.

but we can't do this because we can't have paths to elements within module types.

On the definition of T, we have the path to S and the substitution, why can't nested substitution be represented the same way ?
Perhaps we can represent it like (module type of Test.S.M) with type ... internally ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem isn't M, it's S - S is a module type so we can't do S.M.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That wouldn't be valid OCaml but it would be a valid reference (so can work internally). Do we require that every rendered paths should be valid OCaml paths ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Even internally it's a Path not a Reference, and in any case I strongly prefer sticking to valid OCaml than to deviate. If the language supports it some day we can do it - for example I'm keeping an eye on the transparent ascription PR as that will help with some tricker parts of handling module type of.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright :) Sorry for the digression.

| Path _ -> false
| Signature _ -> true
| With (_, expr) -> is_elidable_with_u expr
| TypeOf _ -> false

and umty : Odoc_model.Lang.ModuleType.U.expr -> text =
fun m ->
match m with
| Path p -> Link.from_path (p :> Paths.Path.t)
| Signature _ ->
Syntax.Mod.open_tag ++ O.txt " ... " ++ Syntax.Mod.close_tag
| With (_, expr) when is_elidable_with_u expr ->
Syntax.Mod.open_tag ++ O.txt " ... " ++ Syntax.Mod.close_tag
| With (subs, expr) -> mty_with subs expr
| TypeOf { t_desc; _ } -> mty_typeof t_desc
| Signature _ ->
Syntax.Mod.open_tag ++ O.txt " ... " ++ Syntax.Mod.close_tag

and mty : Odoc_model.Lang.ModuleType.expr -> text =
fun m ->
Expand Down Expand Up @@ -1470,6 +1478,8 @@ module Make (Syntax : SYNTAX) = struct
++ mty arg_expr ++ O.txt ")" ++ O.txt " " ++ Syntax.Type.arrow
)
++ O.txt " " ++ mty expr
| With { w_expr ; _ } when is_elidable_with_u w_expr ->
Syntax.Mod.open_tag ++ O.txt " ... " ++ Syntax.Mod.close_tag
| With { w_substitutions; w_expr; _ } -> mty_with w_substitutions w_expr
| TypeOf { t_desc; _ } -> mty_typeof t_desc
| Signature _ ->
Expand Down
31 changes: 6 additions & 25 deletions src/xref2/compile.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,20 +580,7 @@ and u_module_type_expr :
| None -> subs
in
let result : ModuleType.U.expr = With (subs', expr') in
match expr' with
| Signature _ -> (
(* Explicitly handle 'sig ... end with ...' - replace with a plain signature.
See equivalent in [module_type_expr] *)
let cu =
Component.Of_Lang.(u_module_type_expr empty (With (subs, expr')))
in
let expansion =
Expand_tools.aux_expansion_of_u_module_type_expr env cu
in
match expansion with
| Ok sg -> Signature Lang_of.(signature id empty sg)
| _ -> result)
| _ -> result)
result)
| TypeOf { t_desc; t_expansion } ->
let t_desc =
match t_desc with
Expand Down Expand Up @@ -634,17 +621,11 @@ and module_type_expr :
| With { w_substitutions; w_expansion; w_expr } as e -> (
let w_expansion = get_expansion w_expansion e in
let w_expr = u_module_type_expr env id w_expr in
match (w_expr, w_expansion) with
| Signature _, Some (Signature sg) ->
(* Explicitly handle 'sig ... end with ...' - replace with a plain signature.
See equivalent in [u_module_type_expr] *)
Signature sg
| _, _ -> (
let cexpr = Component.Of_Lang.(u_module_type_expr empty w_expr) in
let subs' = module_type_map_subs env id cexpr w_substitutions in
match subs' with
| None -> With { w_substitutions; w_expansion; w_expr }
| Some s -> With { w_substitutions = s; w_expansion; w_expr }))
let cexpr = Component.Of_Lang.(u_module_type_expr empty w_expr) in
let subs' = module_type_map_subs env id cexpr w_substitutions in
match subs' with
| None -> With { w_substitutions; w_expansion; w_expr }
| Some s -> With { w_substitutions = s; w_expansion; w_expr })
| Functor (param, res) ->
let param' = functor_parameter env param in
let env' = Env.add_functor_parameter param env in
Expand Down
8 changes: 4 additions & 4 deletions src/xref2/test.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,7 +739,7 @@ now we can ask for the signature of this module:
# let sg = get_ok @@ Tools.signature_of_module env (Component.Delayed.get m);;
val sg : Component.Signature.t =
{Odoc_xref2.Component.Signature.items =
[Odoc_xref2.Component.Signature.Module (`LModule (M, 39),
[Odoc_xref2.Component.Signature.Module (`LModule (M, 45),
Odoc_model.Lang.Signature.Ordinary,
{Odoc_xref2.Component.Delayed.v =
Some
Expand All @@ -751,7 +751,7 @@ val sg : Component.Signature.t =
None);
canonical = None; hidden = false};
get = None});
Odoc_xref2.Component.Signature.Module (`LModule (N, 40),
Odoc_xref2.Component.Signature.Module (`LModule (N, 46),
Odoc_model.Lang.Signature.Ordinary,
{Odoc_xref2.Component.Delayed.v =
Some
Expand All @@ -760,7 +760,7 @@ val sg : Component.Signature.t =
Odoc_xref2.Component.Module.ModuleType
(Odoc_xref2.Component.ModuleType.Path
{Odoc_xref2.Component.ModuleType.p_expansion = None;
p_path = `Dot (`Local (`LModule (M, 39), false), "S")});
p_path = `Dot (`Local (`LModule (M, 45), false), "S")});
canonical = None; hidden = false};
get = None})];
compiled = false; removed = []; doc = []}
Expand Down Expand Up @@ -793,7 +793,7 @@ val m : Component.Module.t Component.Delayed.t =
# get_ok @@ Tools.signature_of_module env (Component.Delayed.get m);;
- : Component.Signature.t =
{Odoc_xref2.Component.Signature.items =
[Odoc_xref2.Component.Signature.Type (`LType (t, 47),
[Odoc_xref2.Component.Signature.Type (`LType (t, 53),
Odoc_model.Lang.Signature.Ordinary,
{Odoc_xref2.Component.Delayed.v =
Some
Expand Down
6 changes: 5 additions & 1 deletion src/xref2/tools.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1233,8 +1233,12 @@ and signature_of_module_type_expr :
| Ok (_, mt) -> signature_of_module_type env mt
| Error e -> Error (`UnresolvedPath (`ModuleType (p_path, e))))
| Component.ModuleType.Signature s -> Ok s
| Component.ModuleType.With { w_expansion = Some e; _ } ->
(* | Component.ModuleType.With { w_expansion = Some e; _ } ->
Ok (signature_of_simple_expansion e)

Recalculate 'With' expressions always, as we need to know which
items have been removed
*)
| Component.ModuleType.With { w_substitutions; w_expr; _ } ->
signature_of_u_module_type_expr ~mark_substituted env w_expr >>= fun sg ->
handle_signature_with_subs ~mark_substituted env sg w_substitutions
Expand Down
8 changes: 8 additions & 0 deletions test/xref2/deep_substitution.t/m.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
module type S = sig
module M: sig
type t
end
type t = M.t
end
module type T = S with type M.t := int

46 changes: 46 additions & 0 deletions test/xref2/deep_substitution.t/run.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
Test to check that when doing a deep substitution inside a signature,
that items that reference that element are still correctly resolved
even if they're in a different path.

$ cat m.mli
module type S = sig
module M: sig
type t
end
type t = M.t
end
module type T = S with type M.t := int


In this, we want to check that the type `t` in module type `T` has
its RHS correctly replaced with an `int`

$ ocamlc -c -bin-annot m.mli
$ odoc compile m.cmti
$ odoc link m.odoc
$ odoc html-generate m.odocl --indent -o .
Comment on lines +18 to +21
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a helper script for this:

Suggested change
$ ocamlc -c -bin-annot m.mli
$ odoc compile m.cmti
$ odoc link m.odoc
$ odoc html-generate m.odocl --indent -o .
$ compile m.mli

The output is slightly different because it sets the parent. If that's annoying, perhaps we can change the script and update every other tests ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We do have a helper script, true - but this way the test is a bit more self contained, and I often find this useful when I'm working on them because I find building them out of tree to be essential when developing (and debugging) these tests.

$ odoc_print m.odocl -r T.t
{
"id": {
"`Type": [
{ "`ModuleType": [ { "`Root": [ "None", "M" ] }, "T" ] },
"t"
]
},
"doc": [],
"equation": {
"params": [],
"private_": "false",
"manifest": {
"Some": {
"Constr": [
{ "`Resolved": { "`Identifier": { "`CoreType": "int" } } },
[]
]
}
},
"constraints": []
},
"representation": "None"
}

4 changes: 2 additions & 2 deletions test/xref2/with.t/run.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ Let's check which module type `.content.Module.items[0].ModuleType` refers to:
]
}

And it ought to be a simple Signature after compiling and linking.
And it ought to still be a With after compiling and linking.

$ odoc_print test.odocl | jq '.content.Module.items[0].ModuleType.expr.Some | keys'
[
"Signature"
"With"
]