Migrate to `Code.fetch_docs/1` #7828

lackac · 2018-07-04T00:43:04Z

Related to #7198.

lackac · 2018-07-04T00:55:12Z

It seems to me that while the approach of parsing the docs chunk always on the caller side does the job, it leads to a lot of repetition. There are currently two implementations that look up specific kind of docs, two or three variations on how to interpret doc values (:none, :hidden, %{"en" => doc}), etc. For another example do a search for docs_v1 in the PR and consider what would need to change when we need to upgrade to docs_v2.

I think that we should either add a Code.Doc module to deal with these or provide some minimal helpers in Code. I'm leaning towards the former since it would follow the pattern of Code.Identifier and Code.Typespec. Code.fetch_docs/1 would also move to Code.Doc.fetch_docs/1.

@josevalim thoughts?

josevalim · 2018-07-04T09:51:49Z

lib/iex/lib/iex/introspection.ex

+  defp translate_doc(nil), do: nil
+  defp translate_doc(:none), do: nil
+  defp translate_doc(:hidden), do: nil
+  defp translate_doc(binary) when is_binary(binary), do: binary


When can we hit this clause?

From line 379

Should we make it be %{"en" => message} on line 379 then? Or too much hassle?

Similarly, if we can avoid nil and use :none instead... but I am not sure how feasible is that.

This is exactly the symptom. Currently the code is in two minds. Do we keep the atoms used by the chunk and let clients deal with that or translate them early on into idiomatic Elixir? I'm not sure which I would prefer, but certainly not both. :)

The current implementation was mostly a mechanical replacement of Code.get_docs/1. I think with spending more time on it we could refactor the IEx related bits more. Moving some things like count_defaults to the chunk while sharing some code between IEx.Autocomplete and IEx.Introspection.

josevalim · 2018-07-04T09:55:00Z

It seems to me that while the approach of parsing the docs chunk always on the caller side does the job, it leads to a lot of repetition.

Good point.

I have seen some repetitions:

There is some repetition on Behaviour and Kernel.Typespec, but those are deprecated, so I would leave them as is.
There is some repetition such as raising exceptions when the chunk is not available, such as in the doctest. For those we could add Code.fetch_docs!(...).
Finally there is some repetition in terms of ignoring the {:error, _} case, but I don't think there is much to do about it.

Did I miss some cases?

lackac · 2018-07-04T10:26:54Z

There is some repetition on Behaviour and Kernel.Typespec, but those are deprecated, so I would leave them as is.

I agree, if it were only these I wouldn't mind the verbosity.

Other repetitions:

interpreting the doc value – this involves usually turning :none into :nil, :hidden into false and %{"en" => doc} into doc. The last one deserves special mention. If at some point we allow i18n of docs we'll need a more maintainable way of fetching the language matching some locale configuration.
the get_docs/2 and count_defaults/1 private functions in IEx.Autocomplete and IEx.Introspection are the same. The former follows in the footsteps of Code.get_docs/2 and I think is a necessity since many times you're not interested in all the docs, but only some kind of docs. The latter I think we could replace by adding something like defaults: 2 or defaults: [:year, :options] to the metadata in elixir_erl.
the has_content?/1 private in IEx.Introspection is a variation of the hidden_fun?/2 private from IEx.Autocomplete

I'm mostly concerned about scattering docs_v1 all over the place, but I'm not sure we can do anything about it without adding a layer between Code.fetch_docs/1 and the callers that extracts a specific part.

lackac · 2018-07-04T10:27:55Z

While looking for ways to remove repetition I also noticed that we could handle two more cases in Code.fetch_docs/1:

case :beam_lib.chunks(bin_or_path, [@docs_chunk]) do
  {:ok, {_module, [{@docs_chunk, bin}]}} ->
    case :erlang.binary_to_term(bin) do
      {:docs_v1, _, _, _, _, _, _} = chunk ->
        chunk

      tuple when is_tuple(tuple) and is_atom(elem(tuple, 0)) ->
        {:error, {:unsupported_version, elem(tuple, 0)}}

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

  {:error, :beam_lib, {:missing_chunk, _, @docs_chunk}} ->
    {:error, :docs_chunk_not_found}
end

This would also ensure that we're adhering to the spec we promised.

josevalim · 2018-07-04T11:11:12Z

I think we should move to :none and :hidden because they are clearer and more idiomatic than nil and false. 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. -- *José Valimwww.plataformatec.com.br <http://www.plataformatec.com.br/>Founder and Director of R&D*

josevalim · 2018-07-04T11:12:07Z

Also let’s go ahead with the fetch_docs change. :) -- *José Valimwww.plataformatec.com.br <http://www.plataformatec.com.br/>Founder and Director of R&D*

lackac · 2018-07-04T14:32:14Z

@josevalim ok, let's live with the duplication for now. I completed the outstanding changes. This frees us to move forward and deal with the removal of ExDc, related code and prepare the generation of the docs chunk for the inclusion of more metadata.

fertapric · 2018-07-04T14:34:19Z

lib/ex_unit/lib/ex_unit/doc_test.ex


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


Typo “modoule”

fertapric · 2018-07-04T15:19:00Z

lib/iex/lib/iex/introspection.ex

+    doc =
+      Enum.find(docs, &match?({_, ^fun, ^arity}, elem(&1, 0))) ||
+        find_doc_defaults(docs, fun, arity)
+
    if doc != nil and has_content?(doc), do: doc
  end

  defp find_doc_defaults(docs, function, min) do


In order to get rid of that elem(doc, 2) what do you think about?

defp find_doc_defaults(docs, function, min) do Enum.find(docs, fn {{_, ^function, arity}, _, signature, _, _} when arity > min -> defaults = count_defaults(signature) arity <= min + defaults _ -> false end) end

josevalim · 2018-07-04T16:50:49Z

lib/elixir/lib/code.ex

+          {:docs_v1, _, _, _, _, _, _} = chunk ->
+            chunk
+
+          tuple when is_tuple(tuple) and is_atom(elem(tuple, 0)) ->


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.

@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}.

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.

josevalim · 2018-07-04T16:52:53Z

lib/elixir/lib/code.ex

+          tuple when is_tuple(tuple) and is_atom(elem(tuple, 0)) ->
+            {:error, {:unsupported_version, elem(tuple, 0)}}
+
+          unknown ->


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). :)

josevalim · 2018-07-04T16:55:19Z

lib/iex/lib/iex/info.ex

-      else
-        ""
+      case Code.fetch_docs(mod) do
+        {:docs_v1, _, _, _, %{}, _, _} -> "Use h(#{inspect(mod)}) to access its documentation.\n"


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.

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

Oh, I see, thank you.

josevalim · 2018-07-04T16:58:21Z

lib/mix/lib/mix/task.ex

@@ -124,9 +124,10 @@ defmodule Mix.Task do
  """
  @spec moduledoc(task_module) :: String.t() | nil


According to the typespec we always return nil. Maybe we could get away with returning nil for false as well?

Good catch, I should've looked at the typespec.

josevalim · 2018-07-04T16:58:33Z

@lackac I have dropped some final comments. :)

michalmuskala · 2018-07-04T19:53:26Z

lib/elixir/lib/kernel/typespec.ex

@@ -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")


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

michalmuskala · 2018-07-04T19:57:33Z

lib/ex_unit/lib/ex_unit/doc_test.ex

@@ -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} ->


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?

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.

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

michalmuskala · 2018-07-04T22:19:53Z

lib/iex/lib/iex/autocomplete.ex

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


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\" \\\\ \"))"

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

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.

josevalim · 2018-07-04T22:38:33Z

lib/mix/lib/mix/task.ex

@@ -122,11 +122,12 @@ defmodule Mix.Task do

  Returns the moduledoc or `nil`.
  """
-  @spec moduledoc(task_module) :: String.t() | nil
+  @spec moduledoc(task_module) :: String.t() | nil | false


Apparently the spec was wrong, hence the build failure. I Fixed it and updated the spec. If the build is green tomorrow morning I will go ahead and merge this. :)

Thanks, I was about to push the same thing. :)

josevalim · 2018-07-04T22:45:07Z

❤️ 💚 💙 💛 💜

Migrate to Code.fetch_docs/1

a59baf2

josevalim reviewed Jul 4, 2018

View reviewed changes

lackac added 3 commits July 4, 2018 16:05

Assure that the tuple returned by fetch_docs adheres to the spec

9048197

Remove the need to interpret nil and binary in translate_doc

efd10a3

Handle all Code.fetch_docs/1 errors in doc tests

be53c4c

fertapric reviewed Jul 4, 2018

View reviewed changes

Fix typo

ade1fdf

fertapric reviewed Jul 4, 2018

View reviewed changes

josevalim reviewed Jul 4, 2018

View reviewed changes

michalmuskala reviewed Jul 4, 2018

View reviewed changes

Final changes

90034d8

michalmuskala reviewed Jul 4, 2018

View reviewed changes

Update task.ex

266285f

josevalim reviewed Jul 4, 2018

View reviewed changes

josevalim merged commit caa90b5 into elixir-lang:master Jul 4, 2018

lackac deleted the migrate-to-fetch-docs branch July 5, 2018 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to `Code.fetch_docs/1` #7828

Migrate to `Code.fetch_docs/1` #7828

lackac commented Jul 4, 2018

lackac commented Jul 4, 2018

josevalim Jul 4, 2018

lackac Jul 4, 2018

josevalim Jul 4, 2018

josevalim Jul 4, 2018

lackac Jul 4, 2018

josevalim commented Jul 4, 2018

lackac commented Jul 4, 2018

lackac commented Jul 4, 2018

josevalim commented Jul 4, 2018 via email

josevalim commented Jul 4, 2018 via email

lackac commented Jul 4, 2018

fertapric Jul 4, 2018

fertapric Jul 4, 2018

josevalim Jul 4, 2018

lackac Jul 4, 2018

josevalim Jul 4, 2018

josevalim Jul 4, 2018

josevalim Jul 4, 2018

lackac Jul 4, 2018

josevalim Jul 4, 2018

josevalim Jul 4, 2018

lackac Jul 4, 2018

josevalim commented Jul 4, 2018

michalmuskala Jul 4, 2018

michalmuskala Jul 4, 2018

lackac Jul 4, 2018

josevalim Jul 4, 2018

michalmuskala Jul 4, 2018 •

edited

josevalim Jul 4, 2018

lackac Jul 4, 2018

josevalim Jul 4, 2018

lackac Jul 4, 2018

josevalim commented Jul 4, 2018

		@@ -124,9 +124,10 @@ defmodule Mix.Task do
		"""
		@spec moduledoc(task_module) :: String.t() \| nil

Migrate to Code.fetch_docs/1 #7828

Migrate to Code.fetch_docs/1 #7828

Conversation

lackac commented Jul 4, 2018

lackac commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Jul 4, 2018

lackac commented Jul 4, 2018

lackac commented Jul 4, 2018

josevalim commented Jul 4, 2018 via email

josevalim commented Jul 4, 2018 via email

lackac commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalmuskala Jul 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Jul 4, 2018

Migrate to `Code.fetch_docs/1` #7828

Migrate to `Code.fetch_docs/1` #7828

michalmuskala Jul 4, 2018 •

edited