Serialize MarkdownAST nodes via Markdown.jl when getting hash for admonitions

[asinghvi17]

This was an online only PR so it might fail CI :D

But the idea here is to do a best effort render to Markdown.jl, assuming all the crazy magic has already been expanded by ExpandTemplates. This might potentially help with issues hashing admonition nodes as reported in https://github.com/JuliaDocs/Documenter.jl/pull/2676#discussion_r2082477911.

Still needs a changelog and tests to make sure this doesn't happen again, but let's see if this approach is viable first

[vtjnash]

It probably would work, but internally MarkdownAST.copy_tree is implemented with deepcopy, so that function crashes if there's anything in the tree that isn't simple Markdown nodes. We'd first have to fix that bug in MarkdownAST.copy_tree, or reimplement a non-broken version of that function here :/

[asinghvi17]

Oops - will look into that at some point. An MWE is probably not too hard to create on the MarkdownAST end from the tree you posted.

[vtjnash]

Turns out to be not too bad to avoid the bad methods, just undocumented. This version of the function fixes the build of Base docs for me:

diff --git a/src/html/HTMLWriter.jl b/src/html/HTMLWriter.jl
index 21cb8f6d1..05abd6c0b 100644
--- a/src/html/HTMLWriter.jl
+++ b/src/html/HTMLWriter.jl
@@ -2369,6 +2369,20 @@ function domify(dctx::DCtx, node::Node, f::MarkdownAST.FootnoteDefinition)
     return DOM.Node[]
 end
 
+# This function inspired by Michael Goerz in https://github.com/JuliaDocs/MarkdownAST.jl/issues/18
+function _markdownast_to_str(node::MarkdownAST.Node)
+    md = MarkdownAST._convert_element(node)
+    text = Markdown.plain(md)
+    return strip(text)
+end
+# uncomment the following to debug mystery hangs:
+#MarkdownAST._convert_element(::MarkdownAST.Node, e::MarkdownAST.AbstractElement) =
+#   error("Unable to convert element of type $(typeof(e))")
+MarkdownAST._convert_element(::MarkdownAST.Node, e::Documenter.AbstractDocumenterBlock) = "TODO"
+MarkdownAST._convert_element(::MarkdownAST.Node, e::Documenter.PageLink) = "TODO"
+MarkdownAST._convert_element(::MarkdownAST.Node, e::Documenter.LocalLink) = "TODO"
+MarkdownAST._convert_element(::MarkdownAST.Node, e::Documenter.LocalImage) = "TODO"
+
 function domify(dctx::DCtx, node::Node, a::MarkdownAST.Admonition)
     @tags header div details summary
     colorclass =

[vtjnash]

from the tree you posted

Note that I only posted a fraction of the tree. The dump of the actual tree was 15k lines long, when printed even just with a limited depth

[vtjnash]

FWIW, that also points to the source of the failure also: there is no show method for PageLink, and so it falls back to printing everything reachable from that page instead. This would usually not run into too many issues (other than getting a incorrectly computed hash here), except that MarkdownAST declares that all subtypes are expected to be mutable struct, but here there are custom AST elements created, and those are declared just struct, and show ends up not liking that they are just declared struct when MarkdownAST expected them to be mutable.

[mortenpi]

that MarkdownAST declares that all subtypes are expected to be mutable struct, but here there are custom AST elements created, and those are declared just struct, and show ends up not liking that they are just declared struct when MarkdownAST expected them to be mutable.

@vtjnash I'm confused by this. AbstractElements do not have to be mutable and MarkdownAST makes no such declaration (most of the "native" elements in MarkdownAST.jl are immutable).

---

As for the bug itself -- I think the correct solution here is to add the necessary show methods for the custom types that are problematic? That seems simpler and probably faster than jumping through the mdflatten/Markdown.jl hoop (where you also have to define convert methods for all custom elements).

[vtjnash]

I have the impression now that mdflatten is the expected way this would be implemented, based upon the impression that is the way we usually create other id's. Defining show seems like a good bugfix anyways too.

[vtjnash]

most of ... native elements are immutable

I was looking at the source before making that claim: all of the native elements have all their fields declared mutable: https://github.com/JuliaDocs/MarkdownAST.jl/blob/main/src/markdown.jl

except for the non-native element struct JuliaValue

But you're right: I thought the API said you could mutate elements in place, but it may have meant the field, not the values. And I thought you'd need to implement show_circular to prevent this from hanging, but that should be handled automatically, so this may just be endlessly slow, not actually infinitely hanging.

[mortenpi]

all of the native elements have all their fields declared mutable

That's fair actually. But yea, the intent there is two fold: (1) you should be able to change the .element field of a Node object, to change the element, and (2) you should also be able to edit the .element objects themselves (i.e. their fields) so that you could update them in place (e.g. if you want to change the code in a code block, or a URL of a link). The AST tree as a whole is meant to be mutable.

But there is no hard requirement for the .elements to be mutable per se. It's just something that you probably want in most cases. But there might be cases where you want immutable structs, or partially mutable (i.e. mutable, but const fields).

I have the impression now that mdflatten is the expected way this would be implemented

Hmm. Actually, maybe, yes, it would make sense (and be safer?). And potentially produce more stable hashes (it wouldn't be dependent on internal Julia representation etc). But it shouldn't be strictly necessary for the bugfix, so let's leave that as a change for the next minor release.

[fingolfin]

This now uses mdflatten. Other than a missing changelog entry, is it OK to merge this now, @mortenpi @vtjnash ?

I guess the important question is: does this fix the issue? I'll try to use it to build the Julia manual now.

[fingolfin]

So I grabbed Julia master, did cd doc, then made sure to dev Documenter.jl from my branch (I double checked that it really used the right Documenter version by inserting @show pkgdir(Documenter) into Julia's doc/make.jl).

Turns out the manual builds fine (well, at least make html) even with Documenter.jl master, so without this PR?

[asinghvi17]

It was probably https://github.com/JuliaDocs/Documenter.jl/pull/2714 that fixed it. This is probably not a bad thing to add though.

[fingolfin]

@asinghvi17 Looks good to me now, but could you please add a changelog entry?