bugfix prevent no function clause in MapSet.union/2

herminiotorres commented 2 years ago

The MapSet.union/2 expected two MapSet data type, but receiving an List data type. To prevent this error, passing both data to MapSet.new/1.

Elixir 1.13.2 (compiled with Erlang/OTP 24)
Erlang 24.2

The issue:

I was working with Surface.Catalogue, and I was creating some Surface.Components, when I open the catalogue on my browser, I received this error here:

** (exit) an exception was raised:
    ** (FunctionClauseError) no function clause matching in MapSet.union/2
        (elixir 1.13.2) lib/map_set.ex:372: MapSet.union([], [])
        (earmark_parser 1.4.19) lib/earmark_parser/context.ex:66: EarmarkParser.Context._merge_messages/2
        (earmark_parser 1.4.19) lib/earmark_parser/context.ex:55: EarmarkParser.Context._merge_contexts/2
        (earmark_parser 1.4.19) lib/earmark_parser/context.ex:47: EarmarkParser.Context.prepend/3
        (earmark_parser 1.4.19) lib/earmark_parser/ast_renderer.ex:23: EarmarkParser.AstRenderer._render/3
        (earmark_parser 1.4.19) lib/earmark_parser.ex:494: EarmarkParser.as_ast/2
        (earmark 1.4.19) lib/earmark/internal.ex:42: Earmark.Internal.as_html/2
        (surface_catalogue 0.3.0) lib/surface/catalogue/markdown.ex:20: Surface.Catalogue.Markdown.to_html/2

I put the IO.inspect/2 inside the EarmarkParser.Context._merge_messages/2 to see the context and the message:

context: %EarmarkParser.Context{
  footnotes: %{},
  links: %{},
  options: %EarmarkParser.Options{
    annotations: nil,
    breaks: false,
    code_class_prefix: "language-",
    file: nil,
    footnote_offset: 1,
    footnotes: false,
    gfm: true,
    gfm_tables: false,
    line: 1,
    messages: [],
    parse_inline: true,
    pedantic: false,
    pure_links: true,
    renderer: EarmarkParser.HtmlRenderer,
    smartypants: false,
    timeout: nil,
    wikilinks: false
  },
  referenced_footnote_ids: #MapSet<[]>,
  rules: %{
    br: ~r/^ {2,}\n(?!\s*$)/,
    escape: ~r/^\\([\\`*\{}\[\]()\#+\-.!_>~|])/,
    footnote: ~r/\z\A/,
    strikethrough: ~r/^~~(?=\S)([\s\S]*?\S)~~/,
    text: ~r/^[\s\S]+?(?=[\\<!\[_*`~]|https?:\/\/| \{2,}\n|$)/,
    url: ~r/^(https?:\/\/[^\s<]+[^<.,:;\"\')\]\s])/
  },
  value: []
}
messages: []

To prevent this error, when I try to pass both data as a List, it was necessarily too passing into MapSet.new/1 first, before passing to MapSet.union/2.

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 5ec4581af0b77964c4f324aeb9a724fff5ce1ab8-PR-85

4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.6%) to 95.903%

Totals
Change from base Build 8a6e51cd27ef0f51b0ae3636439fd0adaecc87b6:	0.6%
Covered Lines:	749
Relevant Lines:	781

💛 - Coveralls

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 5ec4581af0b77964c4f324aeb9a724fff5ce1ab8-PR-85

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+1.2%) to 96.518%

Totals
Change from base Build 8a6e51cd27ef0f51b0ae3636439fd0adaecc87b6:	1.2%
Covered Lines:	887
Relevant Lines:	919

💛 - Coveralls

RobertDober commented 2 years ago

Muito obrigado o Herminio

actually you discovered a design flaw here :clap:

However the fix is too late IMHO the message field of the struct should not be exposed at all in the as_ast and we really want to contain this fields always a MapSet

Would you care to modify your PR in that way or shall I take care of it?

Thanx again

Até logo

RobertDober commented 2 years ago

to be more specific

when an option with anything else than an empty MapSet is passed into as_ast let us return {:error, ...}

or do you have a use case against it?

Options.normalize might be the correct place

herminiotorres commented 2 years ago

Hey @RobertDober , sorry to take too long to give you back an answer, I took a while to read more code and what is happening.

Yesterday I have been working on my personal project, and I use Surface and SurfaceCatalogue.

And the SurfaceCatalogue has Earmark and EarmarkParser as dependencies. The SurfaceCatalogue called:

Earmark.as_html(markdown, smartypants: false, code_class_prefix: "language-")

The markdown could be many things, like io_list, or a binary.
set these options: smartypants: false, code_class_prefix: "language-"

The Earmark.as_html/2 is delegating to Earmark.Internal.as_html/2.

Because it was passing a list with options, it will call Options.make_options(options), and the Earmark has its own implementation of the Options struct and for at list the messages key the struct is set as a list and not a MapSet, look: Earmark.Options.struct().

After building the Option struct it will call again, and for the second time, it will pass here.

So, check the chunk of code for this Earmark.Transform.postprocessed_ast/2.

If I pass my lines as a binary, it will split into a list, otherwise, I pass as an io_list. And then it calls Earmark.EarmarkParserProxy.as_ast/2](https://github.com/pragdave/earmark/blob/master/lib/earmark/earmark_parser_proxy.ex#L17-L23), it was just a wrapper to called EarmarkParser.as_ast/2.

And then, only then, we got this error, when trying to merge messages and either is a MapSet.

This PR https://github.com/RobertDober/earmark_parser/pull/76 is where you change the Options Struct and the message key pass to be a MapSet, and the Context use MapSet.union to combine them.

Also, I set this version on my personal project, and works well.

{:earmark, "1.4.19"},
{:earmark_parser, "1.4.17", override: true},

What do you have in mind? I like to contribute with.

RobertDober commented 2 years ago

Hi @herminiotorres no worries, your input is appreciated, I will have a look and study what you wrote and then come back to you...

RobertDober commented 2 years ago

Oh you need to update to Earmark 1.4.20 it seems, I will make a PR in surface, and then I will release a version of EarmarkParser that does not crash to avoid this

EarmarkParser.Options
iex(3)> as_ast("- a", %Options{messages: []})
** (FunctionClauseError) no function clause matching in MapSet.union/2    

    The following arguments were given to MapSet.union/2:

        # 1
        []

        # 2
        []

    Attempted function clauses (showing 2 out of 2):

        def union(%MapSet{map: map1, version: version} = map_set, %MapSet{map: map2, version: version})
        def union(%MapSet{map: map1}, %MapSet{map: map2})

    (elixir 1.13.0) lib/map_set.ex:372: MapSet.union/2
    (earmark_parser 1.5.0) lib/earmark_parser/context.ex:64: EarmarkParser.Context._merge_messages/2
    (earmark_parser 1.5.0) lib/earmark_parser/context.ex:55: EarmarkParser.Context._merge_contexts/2
    (earmark_parser 1.5.0) lib/earmark_parser/context.ex:36: EarmarkParser.Context.prepend/3
    (earmark_parser 1.5.0) lib/earmark_parser/ast_renderer.ex:24: EarmarkParser.AstRenderer._render/3
    (earmark_parser 1.5.0) lib/earmark_parser/ast_renderer.ex:189: EarmarkParser.AstRenderer.render_block/3
    (earmark_parser 1.5.0) lib/earmark_parser/ast_renderer.ex:23: EarmarkParser.AstRenderer._render/3
    (earmark_parser 1.5.0) lib/earmark_parser/ast_renderer.ex:167: EarmarkParser.AstRenderer.render_block/3
iex(3)>

but rather as described in #86 and #87

Is this ok for you @herminiotorres ?

herminiotorres commented 2 years ago

Well, IMHO, I think you have broken the contract in your public API for each project using the EarmarkParser as their dependency. Before you change your private implementation on Context, to prepend messages when was passing as a List, and now it is MapSet. Even if I update to use the Earmark 1.4.20, it keeps continuing throwing the same error, because Earmark, has an Options struct implementation and there are messages which is a List data type, look here, and when Earmark send lines and options arguments to EarmarkParser it will throw an error. When you change the private implementation to change List for MapSet, IMHO, the EarmarkParser should take care of it and pipe through the data was received as options to the Options struct and for lines/markdown as the first argument, called the MapSet.new.

You can keep your public API contract for who is calling yet, and you should sanitize/parser for the proper data type internally. Also, I think MapSet, even coming built-in data type with Elixir Language, is a complex data type, and I think you shouldn't be expected in your public API this MapSet datatype.

If you want, I can move around my quick fix, and treat to parsing those on the as_ast function for MapSet for lines/markdown and options. And write tests for Context.prepend, and for as_ast function.

WDYT? It is looking good to you? @RobertDober

herminiotorres commented 2 years ago

I test locally the surface_site which those versions you open the PR on Surface, and for many markdowns, codes were are using the Earmark and EarmarkParser, they are failure.

RobertDober commented 2 years ago

@herminiotorres ok sorry about that I just could not reproduce the error but anyway I will fix it with #86 or you if you adapt your PR. Just to be sure that we are on the same wavelength

EarmarkParser.as_ast assures that options.messages is converted to a MapSet and issues a deprecation warning if anything was passed in.
Can you please reproduce the failing call to Earmark.as_html in the console and copy paste it here

Muito obrigado, que pena que temos fallar ingles ;)

RobertDober commented 2 years ago

@herminiotorres just to explain

Well, IMHO, I think you have broken the contract in your public API for each project using the EarmarkParser as their dependency.

Yes because it never should have been in the public API, it leaked out, so sorry about that

RobertDober commented 2 years ago

Please use Earmark v1.4.20 it uses EarmarkParser 1.4.18 which does not use the MapSet yet

I cannot imagine how you got this error? Did you update your hex packages?

RobertDober commented 2 years ago

Fixed with #86, however I cannot release in the 1.4.* branch anymore so if the usage of Earmark 1.4.20 does not solve your issue you need to wait for Earmark 1.5.0 (coming soon I hope)

RobertDober / earmark_parser