Fix footnote ordering

[aeskildsen]

Footnotes are now displayed in the order they are referenced in the document text, not in the order they are defined.

• Set up a separate list to keep track of footnote reference order
• Reorder footnote dict before rendering the list of footnotes
• Update docstrings to reflect chagnes

Tracking footnote references is something the logically fits within inline processing. But it must happen before tree processing, because that is where the list of footnotes is rendered to html. For that reason, it is done within the block processor.

Fixes #1367

[waylan]

The test failures for the py314 test can be ignored as they are unrelated. I have opened #1547 to address them.

[waylan]

Tracking footnote references is something the logically fits within inline processing. But it must happen before tree processing, because that is where the list of footnotes is rendered to html. For that reason, it is done within the block processor.

While I understand that it may be easier to implement this way, I am unconvinced that it is imposable to implement in the inline processor. My concern is that it may match things it shouldn't. For example, what if a footnote is included in a code span? Some tests might convince me.

In fact, this needs some new tests added to demonstrate that it is working as advertised.

[aeskildsen]

Thank you for the quick feedback. The code span example is definitely a valid concern.

I can add some tests to demonstrate the functionality + a footnote reference inside a code span.

I'll think about how to implement this with the inline processor. The problem is not detecting footnote references in the inline processor, it's the timing, since the tree processor which renders the footnotes runs before that.

[aeskildsen]

I've added a bunch of tests, including footnote references within code spans. See in particular TestFootnotes.testFootnoteOrder.

Concerning the block vs inline discussion: One consideration I would to add is that my implementation does not remove or modify any of the footnote references/labels during block processing. It just registers the ids of any footnote references it finds in the blocks. I have not touched the parts of the code that remove substrings or render html elements. I merely reorder the dict that holds the ids and footnote content just before the footnotes are rendered.

[waylan]

Sorry that I didn't mention this previously, but all new tests should be under the appropriate subdir of /tests/test_syntax (/tests/test_syntax/extensions.test_footnotes.py in this case) and make use of markdown.test_tools.TestCase when it makes sense to do so.

[aeskildsen]

Ah, I see. No worries, I've moved and adapted the new tests.

[aeskildsen]

Sorry, don't review or merge this just yet. I've come up with a test case that messes up the order (when a footnote reference id inside e.g. a code span is identical to a proper footnote reference id elsewhere). It is an edge case, but I'd rather get this corrected before review.

I'll provide a fix early next week.

[aeskildsen]

I've gone down the rabbithole with this and can report a few things:

While I understand that it may be easier to implement this way, I am unconvinced that it is imposable to implement in the inline processor.

It is indeed possible (trivial, even) to implement tracking the order of footnote references in an Inlineprocessor. What is not possible, however, is to then generate the list of footnotes in the order we have tracked, because the list of footnotes is produced by a Treeprocessor. To my knowledge, that is simply how the architecture is designed; Treeprocessors always run before Inlineprocessors.

So instead of trying to generate the footnotes div in the correct order, I have explored different ways of reordering it in a Postprocessor. They all lead to a fair amount of reparsing that seems counterintuitive and excessive at this stage in the pipeline. I've had some succes with a regex-based reordering method that works on the generated footnotes div. But it fails tests.test_legacy.TestExtensionsExtra.test_raw_html due to what I think is a state management issue between footnotes and md_in_html. I don't currently have the resources to dig deeper into the extras extension to figure out what that issue is.

So. Based on my work thus far, I will suggest that for this PR we go back to 0c2e4bf and proceed with review.

This entails ignoring the edge case that ce45459 tests. It is a pretty remote possibility, but even if it happens, it will not produce any errors. It will simply result in an illogical order of footnotes. This is, in my view, much preferable to the current behavior where the order of footnote definitions dictates the order of footnotes.

[waylan]

It is indeed possible (trivial, even) to implement tracking the order of footnote references in an Inlineprocessor. What is not possible, however, is to then generate the list of footnotes in the order we have tracked, because the list of footnotes is produced by a Treeprocessor. To my knowledge, that is simply how the architecture is designed; Treeprocessors always run before Inlineprocessors.

Actually, Inlineprocessors are wrapped in and run from markdown.treeprocessors.InlineProcessor, which is the first treeprocessor by default (as listed here). Therefore, it is possible to run a Treeprocessors either before or after inline processors by setting the priority either higher or lower than markdown.treeprocessors.InlineProcessor.

[aeskildsen]

Actually, Inlineprocessors are wrapped in and run from markdown.treeprocessors.InlineProcessor, which is the first treeprocessor by default (as listed here). Therefore, it is possible to run a Treeprocessors either before or after inline processors by setting the priority either higher or lower than markdown.treeprocessors.InlineProcessor.

Ah, that makes a lot of sense. Thank you for explaining that.

5d7f8ca adds an implementation that uses a treeprocessor, which is the cleanest solution of the ones I've experimented with.

It does fail one test, however: test_legacy.TestExtensionsExtra.test_raw_html. I don't fully understand why that particular test gets the order wrong, but I think it has to do with how state gets jumbled when md_in_html calls markdown multiple times. It's the same problem I encountered with a regex-based approach. As far as I can tell, the footnotes extension works correctly with .reset(), so that shouldn't be the issue.

[facelessuser]

As far as I can tell, the footnotes extension works correctly with .reset(), so that shouldn't be the issue.

The tests should be cleaning up after themselves. I would probably need to understand more about what the differences are and why they are different.

[aeskildsen]

The specific input file that fails to parse correctly is tests/extensions/extra/raw-html.txt

But let me see if I can show this in a simpler way. The problem occurs when we use the extra extension:

import markdown

MD = """
<div markdown="1">
Reference to [^first]

[^first]: First footnote definition
</div>

<div markdown="1">
Reference to [^second]

[^second]: Second footnote definition
</div>
"""

result = markdown.markdown(MD, extensions=["extra"], extension_configs={})
print(result)

Prints this:

<div>
<p>Reference to <sup id="fnref:first"><a class="footnote-ref" href="#fn:first">2</a></sup></p>
</div>
<div>
<p>Reference to <sup id="fnref:second"><a class="footnote-ref" href="#fn:second">1</a></sup></p>
</div>
<div class="footnote">
<hr />
<ol>
<li id="fn:second">
<p>Second footnote definition&#160;<a class="footnote-backref" href="#fnref:second" title="Jump back to footnote 1 in the text">&#8617;</a></p>
</li>
<li id="fn:first">
<p>First footnote definition&#160;<a class="footnote-backref" href="#fnref:first" title="Jump back to footnote 2 in the text">&#8617;</a></p>
</li>
</ol>
</div>

Which should be this:

<div>
<p>Reference to <sup id="fnref:first"><a class="footnote-ref" href="#fn:first">1</a></sup></p>
</div>
<div>
<p>Reference to <sup id="fnref:second"><a class="footnote-ref" href="#fn:second">2</a></sup></p>
</div>
<div class="footnote">
<hr />
<ol>
<li id="fn:first">
<p>First footnote definition&#160;<a class="footnote-backref" href="#fnref:first" title="Jump back to footnote 1 in the text">&#8617;</a></p>
</li>
<li id="fn:second">
<p>Second footnote definition&#160;<a class="footnote-backref" href="#fnref:second" title="Jump back to footnote 2 in the text">&#8617;</a></p>
</li>
</ol>
</div>

Why?
I'm fairly certain that the footnotes extension is working as expected, as it passes all other tests. The md_in_html extension appears to be involved here, but I don't know why or how. Perhaps the FootnoteInlineProcessor gets called in an improper order? Its job is to register the order of the footnote references and construct the sup element that contains the footnote reference.

[facelessuser]

md_in_html is a fairly complicated piece of code. Worst case, this illustrates that it may be impossible for this approach to guarantee order, but maybe there is something we can figure out.

I will state that plugins can do almost anything, and can cause some content to be processed out of the expected order, so there may be other 3rd party plugins that could potentially manifest something similar.

If it turns out we cannot guarantee order, I do wonder if all this effort is worth it. I do imagine it is possible to reasonably ensure order, but it may not be through the current approach. Just to be clear, I have not explored the current approach, so I don't know yet if what you are doing can simply be tweaked or not. I'll have to find some time to explore further.

[waylan]

I will note that the failing test passes without any of the changes here. In other words, the original order is preserved. So, the changes here seem to be changing the order.

[waylan]

With the changes here, this is what I am getting:

>>> src = """
... <div markdown="1">
... Reference to [^first]
...
... [^first]: First footnote definition
... </div>
...
... <div markdown="1">
... Reference to [^second]
...
... [^second]: Second footnote definition
... </div>
... """
>>> md = markdown.Markdown(extensions=['footnotes', 'md_in_html'])
>>> print(md.convert(src))
<div>
<p>Reference to <sup id="fnref:first"><a class="footnote-ref" href="#fn:first">2</a></sup></p>
</div>
<div>
<p>Reference to <sup id="fnref:second"><a class="footnote-ref" href="#fn:second">1</a></sup></p>
</div>
<div class="footnote">
<hr />
<ol>
<li id="fn:second">
<p>Second footnote definition&#160;<a class="footnote-backref" href="#fnref:second" title="Jump back to footnote 1 in the text">&#8617;</a></p>
</li>
<li id="fn:first">
<p>First footnote definition&#160;<a class="footnote-backref" href="#fnref:first" title="Jump back to footnote 2 in the text">&#8617;</a></p>
</li>
</ol>
</div>

That matches your result. But let's dig a little deeper...

>>> md.inlinePatterns[2]
<markdown.extensions.footnotes.FootnoteInlineProcessor object at 0x000001A3C5160B00>
>>> md.inlinePatterns[2].footnotes.footnote_order
['second', 'first']

Here we can see that the footnote_order is set to ['second', 'first'], which explains the output. Now to work out how they order was set that way. Let's add a debugging print statement...

diff --git a/markdown/extensions/footnotes.py b/markdown/extensions/footnotes.py
index 0d08dc7..7f29d06 100644
--- a/markdown/extensions/footnotes.py
+++ b/markdown/extensions/footnotes.py
@@ -163,6 +163,7 @@ class FootnoteExtension(Extension):
     def addFootnoteRef(self, id: str) -> None:
         """ Store a footnote reference id in order of appearance. """
         if id not in self.footnote_order:
+            print(f'Saving footnote ID: {id}')
             self.footnote_order.append(id)

     def get_separator(self) -> str:

and check the output...

>>> markdown.markdown(src, extensions=['footnotes', 'md_in_html'])
Saving footnote ID: second
Saving footnote ID: first
'<div>\n<p>Reference to <sup id="fnref:first"><a class="footnote-ref" href="#fn:first">2</a></sup></p>\n</div>\n<div>\n<p>Reference to <sup id="fnref:second"><a class="footnote-ref" href="#fn:second">1</a></sup></p>\n</div>\n<div class="footnote">\n<hr />\n<ol>\n<li id="fn:second">\n<p>Second footnote definition&#160;<a class="footnote-backref" href="#fnref:second" title="Jump back to footnote 1 in the text">&#8617;</a></p>\n</li>\n<li id="fn:first">\n<p>First footnote definition&#160;<a class="footnote-backref" href="#fnref:first" title="Jump back to footnote 2 in the text">&#8617;</a></p>\n</li>\n</ol>\n</div>'

Sure enough, the second ID is being processed/saved before the first. Presumably, the md_in_html extension is messing with the order that items are processed. I don't recall all the ins-and-outs of that extension, but even if we were able to change that extension so that it processed things in order, there is no guarantee that all third-party extensions would do so. And that brings us to the point that @facelssuser was making: It may not make sense to even attempt to support this.

[aeskildsen]

EDIT: I wrote this before reading your latest reply, @waylan .

I will note that the failing test passes without any of the changes here. In other words, the original order is preserved. So, the changes here seem to be changing the order.

Correct. I am fairly certain that is because the order is dictated by footnote definitions, which are detected during block processing. As opposed to my recent changes which detect the order of footnote references during inline processing. My most recent implementation assumes that inline matches are processed in document order, depth first. I suspect that is not the case in this particular test, due to md_in_html. I completely understand if my assumption is wrong and that processing order may not be guaranteed, as @facelessuser mentioned.

Though I would also suspect that other plugins which rely upon inline matches to occur in document order would be susceptible to similar issues.

[aeskildsen]

I will also say that the test we are talking about passes in my earlier implementation 0c2e4bf. That implementation relies upon block processing to detect the order.

I think we can safely disregard the edge case I mentioned afterwards and tested in 5d7f8ca. Worst case scenario is: A user uses the same footnote id in a code example as in a real footnote in the same document. That footnote may get a different place in the order of footnotes than expected, but it will do so silently and not produce an error.

For me, that would be preferable to the current behavior, where improper order can be achieved much easier than in the edge case; one just has to follow the instructions in the documentation and put definitions "anywhere in the document", as stated in the PHP Markdown Extra docs.

[waylan]

I have done some spelunking with the Python debugger and md_in_html is not processing anything out of order. It behaves exactly as you would think. In fact, after the blockparser has run, but before any treeprocessors have run, the tree contains the following structure:

<div>
<p>Reference to [^first]</p>
</div>
<div>
<p>Reference to [^second]</p>
</div>

The issue would seem to be with treeprocessors or inlineprocessors. What would be causing them to process things out of order I don't know.

[facelessuser]

Oh, that's interesting. I may dig a little if/when I get some time.

[aeskildsen]

I'll await for now then, as I don't know much about the internals of the processors. Do let me know if I can do anything to assist.

[waylan]

I resolved the issue with one minor change in 1c160ef. list.pop() removes and returns the last item from a list. Therefore, Elements in the stack were being processed in reverse order. However, only Elements with child Elements ever appear in the stack. Therefore, simple Markdown documents did not usually encounter this issue. However, deeply nested documents could. Once I understood the issue, it was easy to create other examples which failed without raw HTML. For example, prior to my change...

>>> src = '''
... * Reference to [^first]
...
... * Reference to [^second]
...
... [^first]: First footnote definition
... [^second]: Second footnote definition
... '''
>>> print(markdown.markdown(src, extensions=['footnotes']))
<ul>
<li>
<p>Reference to <sup id="fnref:first"><a class="footnote-ref" href="#fn:first">2</a></sup></p>
</li>
<li>
<p>Reference to <sup id="fnref:second"><a class="footnote-ref" href="#fn:second">1</a></sup></p>
</li>
</ul>
<div class="footnote">
<hr />
<ol>
<li id="fn:second">
<p>Second footnote definition&#160;<a class="footnote-backref" href="#fnref:second" title="Jump back to footnote 1 in the text">&#8617;</a></p>
</li>
<li id="fn:first">
<p>First footnote definition&#160;<a class="footnote-backref" href="#fnref:first" title="Jump back to footnote 2 in the text">&#8617;</a></p>
</li>
</ol>
</div>

Note that that example uses a "loose list" (there is a blank line between the list items), which results in each list item being wrapped in a <p> element. This creates the same scenario as the md_in_html example and fails in the same way. Simply by ensuring that we always pop the first element (with stack.pop(0)) the issue is resolved. I have added that as a test in 62b12b7.

Fortunately, none of the tests seem to rely on this weird behavior, so I think it should be safe to move forward. Hopefully no third-party extensions are relying on this and break from the change. ¯\_(ツ)_/¯

[waylan]

I'm wondering if perhaps 1c160ef should be in a separate PR by itself and this PR should be rebased on top of that. At a minimum, we need to note this change in the changelog. @facelessuser any thoughts?

[facelessuser]

I'm not sure I have any strong opinions on the change being separate. If we want to specifically track when this change occured, then yeah, maybe submit it as a separate change. This footnote change exposed potential issues with this, and seems like a fine place to fix it as well.

I would definitely note the change in the changelog though, if there are concerns, it might cause issues.

[waylan]

Looks good. Thank you for all your work on this.

[aeskildsen]

Looks good. Thank you for all your work on this.

I'm happy to help. Thank you for a nice process.

One small thing that keeps lingering at the back of my mind: I wonder if the change to stack.pop(0) could have a slight impact on performance. Would it be worth benchmarking?

[waylan]

One small thing that keeps lingering at the back of my mind: I wonder if the change to stack.pop(0) could have a slight impact on performance. Would it be worth benchmarking?

Perhaps. I briefly thought about that as well, but what could we do to address any potential performance issue? Would using collections.deque for the stack make sense here? If you are so inclined, feel free to benchmark and report back.

[aeskildsen]

One small thing that keeps lingering at the back of my mind: I wonder if the change to stack.pop(0) could have a slight impact on performance. Would it be worth benchmarking?

Perhaps. I briefly thought about that as well, but what could we do to address any potential performance issue? Would using collections.deque for the stack make sense here? If you are so inclined, feel free to benchmark and report back.

That's what I was thinking. I should be able to do some benchmarking in the next day or two.

[aeskildsen]

So I managed to do a bit of benchmarking.

I benchmarked the processing of two markdown documents using pyperf:

• sample_doc.md: A document attempting to simulate a big chunk of realistic README-like markdown. Contains all the markdown from the top level of Python-Markdown's docs directory.
• nested_doc.md: A document with 22 examples of nested structures which requires the 'extra' extension for footnotes, md_in_html, etc. It contains the loose list example @waylan provided, the contents of the aforementioned raw-html.txt test file, a bunch of AI-generated markdown with nested structures, and a single line containing 2000 footnote references (adapted from the original test file footnote_many_footnotes.txt), a few of which are nested within ** and __.

Let me know if you want to see the contents of these files.

The stack.pop(0) fix seems to have a very negligible impact on performance. I'm no statistician, but I would guess the difference is within the margin of error for the benchmarks I ran. collections.deque seems to show a very minor performance improvement in the case of the nested document, but again, likely not far from the margin of error.

I am no expert on how the inline processor works, but I think the reason we are not seeing big differences here is that inline nesting mostly does not go very deep. I believe this is what @waylan mentioned earlier. As I understand it, list.pop(0) is O(n) and deque.popleft() is O(1), but when the list is short the difference is probably negligible.

Data

sample_doc.md, default pyperf timeit benchmark

v3.8.2 with no changes: Mean +- std dev: 50.6 ms +- 0.9 ms
commit d8ecae6 (stack.pop(0) fix): Mean +- std dev: 51.4 ms +- 1.9 ms
commit d8ecae6 + collections.deque instead of list: Mean +- std dev: 51.9 ms +- 2.3 ms

nested_doc.md, default pyperf timeit benchmark

Here I used extensions=['extra'].

v3.8.2 with no changes: Mean +- std dev: 156 ms +- 3 ms
commit d8ecae6 (with stack.pop(0) fix): Mean +- std dev: 160 ms +- 5 ms
commit d8ecae6 + collections.deque instead of list: Mean +- std dev: 158 ms +- 3 ms

nested_doc.md, rigorous pyperf timeit benchmark

Here I used extensions=['extra'].

v3.8.2 with no changes: Mean +- std dev: 160 ms +- 9 ms
commit d8ecae6 (with stack.pop(0) fix): Mean +- std dev: 164 ms +- 11 ms
commit d8ecae6 + collections.deque instead of list: Mean +- std dev: 160 ms +- 7 ms

TLDR

There does not appear to be a noteworthy performance impact from the stack.pop(0) fix. I think it can be merged without major concerns about performance.

It is possible to use collections.deque as a drop-in replacement for the list, which could offer a slight improvement in documents with very complex nested inline structures. We could consider testing how it performs on a wider range of documents first, and so perhaps a different PR would be more appropriate for that.

[waylan]

Excellent! This looks good to me. Thanks for doing the work on this.