pragdave / earmark

Markdown parser for Elixir
Other
859 stars 135 forks source link

Inaccurate spec of Earmark.Parser.as_ast/2 #505

Open phihos opened 1 month ago

phihos commented 1 month ago

Hi,

recently I upgraded Earmark from v1.4.46 to v1.4.47. Since then dialyzer is complaining about this code:

  # ...
  markdown_html =
    Earmark.as_ast!(text)
    |> style_markdown_ast()
    |> Earmark.Transform.transform()
    |> Phoenix.HTML.raw()
  # ...

  defp style_markdown_ast(ast) when is_list(ast) do
    # ...

with the following message:

...

The function call will not succeed.

MyModule.style_markdown_ast(binary())

will never return since the 1st arguments differ
from the success typing arguments:

(maybe_improper_list())
...
The guard test:

is_list(_ast :: binary())

can never succeed.

If I understand this correctly dialyzer expects the output of Earmark.as_ast! always to be binary(). Since the code still works I suspect this is not true. I dug around in the changes for v1.4.47 and found this commit. In it Earmark.Parser.as_ast/2 has the following spec:

 @spec as_ast([String.t()] | String.t(), Earmark.Options.options()) ::
          {:error, binary(), [any()]} | {:ok, binary(), [map()]}

And since Earmark.as_ast! extracts the middle part of {:ok, binary(), [map()]} dialyzer thinks the type is always binary(). I am not really sure what a proper fix looks like, but I expected the spec to be more as follows:

 @spec as_ast([String.t()] | String.t(), Earmark.Options.options()) ::
          {:error, binary(), [any()]} | {:ok, Earmark.ast_node(), [map()]}

Does that make sense?