-
Notifications
You must be signed in to change notification settings - Fork 351
Copy callback typespecs #815
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.
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
Conversation
end | ||
|
||
defp copy_callback_specs([], name, arity, {:ok, behaviour}) do | ||
callbacks = Kernel.Typespec.beam_callbacks(behaviour) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is private API :( any other options?
Ebert has finished reviewing this Pull Request and has found:
But beware that this branch is 1 commit behind the You can see more details about this review at https://ebertapp.io/github/elixir-lang/ex_doc/pulls/815. |
end | ||
end | ||
|
||
defp get_specs(module_info, type, function, name, arity, impl) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function takes too many parameters (arity is 6, max is 5).
defp copy_callback_specs([], name, arity, {:ok, behaviour}) do | ||
callbacks = Kernel.Typespec.beam_callbacks(behaviour) | ||
{_, specs} = List.keyfind(callbacks, {name, arity}, 0) | ||
Enum.map(specs, &Typespec.spec_to_ast(name, &1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in similar code in IEx [1] we're doing Macro.prewalk(&drop_macro_env/1)
, wasn't sure if we need it so I left it out.
[1] https://github.com/elixir-lang/elixir/blob/v1.5.3/lib/iex/lib/iex/introspection.ex#L434
there's one thing left which is a bit subtle: If the callback spec uses local type defined in behaviour module and we copy the spec over verbatim then that local type won't autolink - we'd need to update AST from local type to remote. ugh 😅 |
@wojtekmach I have just realized that one of the issues with this approach is that local types defined in the behaviour won't be available in the implementation. In the best scenario they will be out of context (non-existing) but in the worst scenario we will have conflicting versions. I am starting to think that maybe our best alternative here is to make sure we link for the callback and that then they resort to the callback for more information. |
@josevalim yes, exactly, see message above :D Again, I think one way would be to manually rewrite AST of the typespec, so: defmodule Behaviour do
@type t() :: term()
@callback foo(t()) :: t()
end would be "copied" as: foo(Behaviour.t()) :: Behaviour.t() However, I just realised that private types are problematic: defmodule Behaviour do
@typep p() :: term()
@callback foo(p()) :: p()
end I don't think we want to "copy" it as: foo(Behaviour.p()) :: Behaviour.p() as we don't expose private types in neither ExDoc nor IEx (elixir-lang/elixir#2612) |
Yeah, with the caveats above I think it's ok to drop this, and instead relying on linking to the callback module. Which we're already doing with #641 - do you think it's enough as is, or do you have some ideas to extend it? |
#641 seems enough to me. :) So I think we can close this? |
Fixes #806