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.

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

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

Open
phihos opened this issue Aug 12, 2024 · 1 comment
Open

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

phihos opened this issue Aug 12, 2024 · 1 comment

Comments

@phihos
Copy link

phihos commented Aug 12, 2024

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?

@flurin
Copy link

flurin commented Oct 3, 2024

This blocking an upgrade for us as well. Shall I make a PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants