Skip to content
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

Jump to bottom

Migrate to Code.fetch_docs/1 #7828

Merged
merged 7 commits into from Jul 4, 2018
Merged

Migrate to Code.fetch_docs/1 #7828

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a59baf2
Migrate to `Code.fetch_docs/1`
lackac Jul 4, 2018
9048197
Assure that the tuple returned by fetch_docs adheres to the spec
lackac Jul 4, 2018
efd10a3
Remove the need to interpret `nil` and `binary` in translate_doc
lackac Jul 4, 2018
be53c4c
Handle all `Code.fetch_docs/1` errors in doc tests
lackac Jul 4, 2018
ade1fdf
Fix typo
lackac Jul 4, 2018
90034d8
Final changes
lackac Jul 4, 2018
266285f
Update task.ex
josevalim Jul 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 9 additions & 3 deletions lib/elixir/lib/behaviour.ex
View file
Open in desktop
Expand Up @@ -97,14 +97,20 @@ defmodule Behaviour do
end

def __behaviour__(:docs) do
for {tuple, line, kind, docs} <- Code.get_docs(__MODULE__, :callback_docs) do
{:docs_v1, _, :elixir, _, _, _, docs} = Code.fetch_docs(__MODULE__)

for {{kind, name, arity}, line, _, doc, _} <- docs, kind in [:callback, :macrocallback] do
case kind do
:callback -> {tuple, line, :def, docs}
:macrocallback -> {tuple, line, :defmacro, docs}
:callback -> {{name, arity}, line, :def, __behaviour__doc_value(doc)}
:macrocallback -> {{name, arity}, line, :defmacro, __behaviour__doc_value(doc)}
end
end
end

defp __behaviour__doc_value(:none), do: nil
defp __behaviour__doc_value(:hidden), do: false
defp __behaviour__doc_value(%{"en" => doc}), do: doc

import unquote(__MODULE__)
end
end
Expand Down
17 changes: 15 additions & 2 deletions lib/elixir/lib/code.ex
View file
Open in desktop
Expand Up @@ -1069,7 +1069,11 @@ defmodule Code do
@spec fetch_docs(module) ::
{:docs_v1, anno, beam_language, format, module_doc :: doc, metadata,
docs :: [{{kind, name, arity}, anno, signature, doc, metadata}]}
| {:error, :module_not_found | :docs_chunk_not_found}
| {:error,
:module_not_found
| :docs_chunk_not_found
| {:unsupported_version, atom}
| {:unrecognized_chunk, term}}
when anno: :erl_anno.anno(),
beam_language: atom,
format: binary,
Expand All @@ -1095,7 +1099,16 @@ defmodule Code do
defp do_fetch_docs(bin_or_path) do
case :beam_lib.chunks(bin_or_path, [@docs_chunk]) do
{:ok, {_module, [{@docs_chunk, bin}]}} ->
:erlang.binary_to_term(bin)
case :erlang.binary_to_term(bin) do
{:docs_v1, _, _, _, _, _, _} = chunk ->
chunk

tuple when is_tuple(tuple) and is_atom(elem(tuple, 0)) ->
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, now looking at the code, I don't think we should return this. The idea of a version being supported or not should be up to the caller and not up to us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@josevalim I see, you want to make fetch_docs transparent and put the burden of knowing what's in the chunks entirely on the caller.

With this change and the other about rescuing binary_to_term I'm thinking that we should also revisit the typespec for Code.fetch_docs/1. We can't really promise to return the current spec unless we match on the result of binary_to_term. What should be the approach for the spec in this case? I would like to make the spec more useful than simply @spec fetch_docs(module) :: term | {:error, term}.

Copy link
Member

Choose a reason for hiding this comment

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

We can try:

@spec fetch_docs(module) :: {:docs_v1, ...} | {:error, term} | future_formats when future_formats: term

But this may have negative implications in dialyzer. We can give it a try though.

{:error, {:unsupported_version, elem(tuple, 0)}}

unknown ->
Copy link
Member

Choose a reason for hiding this comment

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

Since the binary_to_term could fail altogether, maybe we should do this:

try do
  :erlang.binary_to_term(binary)
rescue
  _ -> {:error, {:invalid_chunk, binary}}
end

Maybe somebody could put invalid data into the chunk but, if they do that, they deserve the errors they will get (which is true for any other chunk). :)

{:error, {:unrecognized_chunk, unknown}}
end

{:error, :beam_lib, {:missing_chunk, _, @docs_chunk}} ->
{:error, :docs_chunk_not_found}
Expand Down
16 changes: 13 additions & 3 deletions lib/elixir/lib/kernel/typespec.ex
View file
Open in desktop
Expand Up @@ -24,10 +24,20 @@ defmodule Kernel.Typespec do

@doc false
def beam_typedocs(module) when is_atom(module) or is_binary(module) do
IO.warn("Kernel.Typespec.beam_typedocs/1 is deprecated, please use Code.get_docs/2 instead")
IO.warn("Kernel.Typespec.beam_typedocs/1 is deprecated, please use Code.fetch_docs/1 instead")
Copy link
Member

Choose a reason for hiding this comment

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

Could we migrate to the @deprecated attribute here while we're at it?


case Code.fetch_docs(module) do
{:docs_v1, _, _, _, _, _, docs} ->
for {{:type, name, arity}, _, _, doc, _} <- docs do
case doc do
:none -> {{name, arity}, nil}
:hidden -> {{name, arity}, false}
%{"en" => doc_string} -> {{name, arity}, doc_string}
end
end

if docs = Code.get_docs(module, :type_docs) do
for {tuple, _, _, doc} <- docs, do: {tuple, doc}
{:error, _} ->
nil
end
end

Expand Down
4 changes: 2 additions & 2 deletions lib/elixir/pages/Writing Documentation.md
View file
Open in desktop
Expand Up @@ -121,6 +121,6 @@ Elixir warns if a private function has a `@doc` attribute and discards its conte

Private functions may still need internal documentation for maintainers, though. That can be accomplished with code comments.

## Code.get_docs/2
## Code.fetch_docs/1

Elixir stores documentation inside pre-defined chunks in the bytecode. It can be accessed from Elixir by using the `Code.get_docs/2` function. This also means documentation is only accessed when required and not when modules are loaded by the Virtual Machine. The only downside is that modules defined in-memory, like the ones defined in IEx, cannot have their documentation accessed as they do not have their bytecode written to disk.
Elixir stores documentation inside pre-defined chunks in the bytecode. It can be accessed from Elixir by using the `Code.fetch_docs/1` function. This also means documentation is only accessed when required and not when modules are loaded by the Virtual Machine. The only downside is that modules defined in-memory, like the ones defined in IEx, cannot have their documentation accessed as they do not have their bytecode written to disk.
49 changes: 30 additions & 19 deletions lib/ex_unit/lib/ex_unit/doc_test.ex
View file
Open in desktop
Expand Up @@ -427,39 +427,50 @@ defmodule ExUnit.DocTest do
## Extraction of the tests

defp extract(module) do
all_docs = Code.get_docs(module, :all)
case Code.fetch_docs(module) do
{:docs_v1, line, _, _, moduledoc, _, docs} ->
Copy link
Member

Choose a reason for hiding this comment

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

Is line always going to be a line number? The spec of Code.get_docs defines it as :erl_anno.anno() which might have more data in it. Should we use :erl_anno.line(anno) to get the line here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would certainly be safer. However, in doc tests we know that it's just a line since it was generated by the Elixir compiler where we don't include more information. But since that could change I think it's best to go with your advice.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, lets go with :erl_anno.line/1.

extract_from_moduledoc({line, moduledoc}, module) ++ extract_from_docs(docs, module)

unless all_docs do
raise Error,
module: module,
message:
"could not retrieve the documentation for module #{inspect(module)}. " <>
"The module was not compiled with documentation or its BEAM file cannot be accessed"
{:error, reason} ->
raise Error,
module: module,
message:
"could not retrieve the documentation for module #{inspect(module)}. " <>
explain_docs_error(reason)
end
end

moduledocs = extract_from_moduledoc(all_docs[:moduledoc], module)
defp explain_docs_error(:module_not_found),
do: "The BEAM file of the module cannot be accessed"

docs =
for doc <- all_docs[:docs],
doc <- extract_from_doc(doc, module),
do: doc
defp explain_docs_error(:docs_chunk_not_found),
do: "The module was not compiled with documentation"

moduledocs ++ docs
end
defp explain_docs_error({:unsupported_version, version}),
do: "The documentation version is not supported: #{version}"

defp explain_docs_error({:unrecognized_chunk, _}),
do: "The documentation chunk in the module cannot be recognized"

defp extract_from_moduledoc({_, doc}, _module) when doc in [false, nil], do: []
defp extract_from_moduledoc({_, doc}, _module) when doc in [:none, :hidden], do: []

defp extract_from_moduledoc({line, doc}, module) do
defp extract_from_moduledoc({line, %{"en" => doc}}, module) do
for test <- extract_tests(line, doc, module) do
normalize_test(test, :moduledoc)
end
end

defp extract_from_doc({_, _, _, _, doc}, _module) when doc in [false, nil], do: []
defp extract_from_docs(docs, module) do
for doc <- docs, doc <- extract_from_doc(doc, module), do: doc
end

defp extract_from_doc({{kind, _, _}, _, _, doc, _}, _module)
when kind not in [:function, :macro] or doc in [:none, :hidden],
do: []

defp extract_from_doc({fa, line, _, _, doc}, module) do
defp extract_from_doc({{_, name, arity}, line, _, %{"en" => doc}, _}, module) do
for test <- extract_tests(line, doc, module) do
normalize_test(test, fa)
normalize_test(test, {name, arity})
end
end

Expand Down
53 changes: 30 additions & 23 deletions lib/iex/lib/iex/autocomplete.ex
View file
Open in desktop
Expand Up @@ -409,7 +409,7 @@ defmodule IEx.Autocomplete do
not ensure_loaded?(mod) ->
[]

docs = Code.get_docs(mod, :docs) ->
docs = get_docs(mod, [:function, :macro]) ->
exports(mod)
|> Kernel.--(default_arg_functions_with_doc_false(docs))
|> Enum.reject(&hidden_fun?(&1, docs))
Expand All @@ -424,8 +424,8 @@ defmodule IEx.Autocomplete do
not ensure_loaded?(mod) ->
[]

docs = Code.get_docs(mod, :type_docs) ->
Enum.map(docs, &elem(&1, 0))
docs = get_docs(mod, [:type]) ->
Enum.map(docs, &extract_name_and_arity/1)

true ->
exports(mod)
Expand All @@ -437,43 +437,50 @@ defmodule IEx.Autocomplete do
not ensure_loaded?(mod) ->
[]

docs = Code.get_docs(mod, :callback_docs) ->
Enum.map(docs, &elem(&1, 0))
docs = get_docs(mod, [:callback, :macrocallback]) ->
Enum.map(docs, &extract_name_and_arity/1)

true ->
exports(mod)
end
end

defp get_docs(mod, kinds) do
case Code.fetch_docs(mod) do
{:docs_v1, _, _, _, _, _, docs} ->
for {{kind, _, _}, _, _, _, _} = doc <- docs, kind in kinds, do: doc

{:error, _} ->
nil
end
end

defp extract_name_and_arity({{_, name, arity}, _, _, _, _}), do: {name, arity}

defp default_arg_functions_with_doc_false(docs) do
for {{fun_name, arity}, _, _, args, false} <- docs,
count = count_defaults(args),
for {{_, fun_name, arity}, _, signature, :hidden, _} <- docs,
count = count_defaults(signature),
count > 0,
new_arity <- (arity - count)..arity,
do: {fun_name, new_arity}
end

defp count_defaults(args) do
Enum.count(args, &match?({:\\, _, _}, &1))
defp count_defaults(signature) do
signature
|> Stream.flat_map(&Regex.scan(~r/ \\\\ /, &1))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that's reliable - I could imagine code like this that would miscalculate here:

iex(3)> quote(do: def foo(bar \\ ~S| \\ |)) |> Macro.to_string
"def(foo(bar \\\\ ~S\" \\\\ \"))"

Copy link
Member

Choose a reason for hiding this comment

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

This is just temporary until we move the number of defaults to metadata. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right. We discussed this earlier:

I wrote:

I think we could replace [count_defaults] by adding something like defaults: 2 or defaults: [:year, :options] to the metadata in elixir_erl

@josevalim:

Regarding the defaults, the plan was to indeed at them to the metadata. So
I would leave the duplication for now and when we make it easier to define
the metadata from the Elixir side we can revisit it.

I would look at that after we remove ExDc and we have a chance to refactor the chunk generation and how and what we store about docs in ETS during compilation. I think we can live with this edge case in autocompletion until then.

|> Enum.count()
end

defp hidden_fun?(fun, docs) do
case List.keyfind(docs, fun, 0) do
nil ->
underscored_fun?(fun)

{_, _, _, _, false} ->
true

{fun, _, _, _, nil} ->
underscored_fun?(fun)

{_, _, _, _, _} ->
false
defp hidden_fun?({name, arity}, docs) do
case Enum.find(docs, &match?({{_, ^name, ^arity}, _, _, _, _}, &1)) do
nil -> underscored_fun?(name)
{_, _, _, :hidden, _} -> true
{_, _, _, :none, _} -> underscored_fun?(name)
{_, _, _, _, _} -> false
end
end

defp underscored_fun?({name, _}), do: hd(Atom.to_charlist(name)) == ?_
defp underscored_fun?(name), do: hd(Atom.to_charlist(name)) == ?_

defp ensure_loaded?(Elixir), do: false
defp ensure_loaded?(mod), do: Code.ensure_loaded?(mod)
Expand Down
7 changes: 3 additions & 4 deletions lib/iex/lib/iex/info.ex
View file
Open in desktop
Expand Up @@ -45,10 +45,9 @@ defimpl IEx.Info, for: Atom do

defp info_module(mod) do
extra =
if Code.get_docs(mod, :moduledoc) do
"Use h(#{inspect(mod)}) to access its documentation.\n"
else
""
case Code.fetch_docs(mod) do
{:docs_v1, _, _, _, %{}, _, _} -> "Use h(#{inspect(mod)}) to access its documentation.\n"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe here the opposite would be better?

{:error, _} -> ""
_ -> "Use h(#{inspect(mod)}) to access its documentation.\n"

This way if we add new versions in the future we won't need to change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The module doc elem can also be :none or :hidden in which case h/1 won't be able to return any documentation.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see, thank you.

_ -> ""
end

mod_info = mod.module_info()
Expand Down