Identifier lookup fails to lookup functors and warns

[Leonidas-from-XIV]

Given a module like this:

module Xref_failure : sig
  module Make (T : sig end) : sig type included end
  module Named : module type of Make

  module Applicant : sig end

  module Applied : module type of Named(Applicant)
end

That is where the functor Make is not directly applied but goes through the redirection of Named the xref code fails to see this and can't link it up, creating a spurious warning:

Warning: Failed to lookup type identifier(root(Functor).Xref_failure.Named,false)(identifier(root(Functor).Xref_failure.Applicant,false)).included Parent_module: Parent_expr: Apply module is not a functor

So basically it is missing that Named is actually referring to a functor.

[jonludlam]

This is probably better off as a test case in test/xref2 - there's quite a lot more lightweight than the generator tests.

[Leonidas-from-XIV]

@jonludlam Ah yes, I wasn't aware of these. That's much better since now the output is captured in the test and not just printed in the terminal as a side-effect of generating the outputs.

[Leonidas-from-XIV]

@jonludlam I've extended handle_apply to deal with module type of. I am not 100% sure whether to use the path from the first argument of the TypeOf constructor or the second argument (@panglesd says it refers to the "original path", in many cases they seem to be the same anyway) but I believe the fix in the commit is a reasonable way to find potential functors. It certainly does fix the cram test warning :)

[jonludlam]

Nice! One minor comment though. I think while it's possible to do module type of <some functor>, I don't think it's possible to do module type of struct include <some Functor> end - which is what the StructInclude variant means. So we should probably make that an error rather than treating it the same way as ModPath.

[Leonidas-from-XIV]

The reason I found this issue is due to the fact that I am working on code that desugars into (thus excuse the autogenerated names):

module Make (T : sig type t end) : sig type included end
module INCLUDE_11 : module type of Make

module BODY_8 : sig type t end
include module type of BODY_8

module APPLY_35 : module type of struct include INCLUDE_11(BODY_8) end
include module type of APPLY_35

This seems to compile using both OCaml 5.4.1 and OxCaml hence I assume that the StructInclude is also correct. Am I wrong in this assumption?

[jonludlam]

I don't think it's needed there, no - in your example the module type of struct include ... end is including a regular module, not a functor.

[Leonidas-from-XIV]

Ah yes, you're right. We're talking about the module type of Make case not the later include. Out of curiosity I tried and module type of struct include Make end is indeed invalid ("This module is not a structure").

I've pushed a commit to exclude StructInclude from the lookup.

[jonludlam]

Hmm, it's occurred to me that we've already got the functionality of find_functor. I think it might be better to just use expansion_of_module and match on the result of that:

let rec handle_apply env func_path arg_path m =
  expansion_of_module env m
  |> map_error (fun e -> (e :> simple_module_type_expr_of_module_error))
  >>= function
  | Signature _ | Functor (Unit, _) -> Error `ApplyNotFunctor
  | Functor (Named arg, result) ->
      let new_module = { m with Component.Module.type_ = ModuleType result } in
      let substitution = `Substituted arg_path in
      let path = `Apply (func_path, arg_path) in
      let subst =
        Subst.add_module
          (arg.id :> Ident.module_)
          (`Resolved substitution) substitution Subst.identity
      in
      let subst = Subst.unresolve_opaque_paths subst in
      Ok (path, Subst.module_ subst new_module)

The return type of expansion_of_module is either a signature or a functor, so it has to handle the chasing of alias chains.

handle_apply is pretty old (it was introduced in the commit 86e7c67#diff-09ac494fff0dccc99a2e6ef5843291c432f7c6eecd54e6fcb676caf18ff68dd5R351 and predates the expansion_of_module function, so I think we just didn't have this functionality around back then.

In order to make this work I also ended up removing the unused error `UnresolvedOriginalPath so that the expansion_of_module_error could be cast into simple_module_type_expr_of_module_error. It's also slightly nicer in that we're not throwing away the error from resolve_module_type (as we were in the original code) and resolve_module (as we were in your PR).

[Leonidas-from-XIV]

I was discarding the return code because that was the precedent in the function but it is of course nicer not to.

I've applied your suggestions. That overall ended deleting more code than adding (bar the test), which is always neat!

[jonludlam]

Great! thanks!