Resolve markdown crash for note references in labels#1712
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates RDoc’s Markdown note-reference handling so malformed note references raise RDoc::Markdown::ParseError instead of crashing with NoMethodError.
Changes:
- Adds a guard in
note_forfor invalid note-reference state. - Mirrors the parser change in the kpeg source and generated Ruby parser.
- Adds a regression test for the reported malformed note reference.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/rdoc/markdown.kpeg |
Adds ParseError handling in the Markdown parser source. |
lib/rdoc/markdown.rb |
Updates generated Markdown parser runtime behavior. |
test/rdoc/rdoc_markdown_test.rb |
Adds regression coverage for invalid note reference parsing. |
Comments suppressed due to low confidence (1)
lib/rdoc/markdown.kpeg:406
- The kpeg source and checked-in generated parser are out of sync here: this raises a generic
invalid note reference, whilelib/rdoc/markdown.rbraisesinvalid note reference: #{ref}. Since the repository documentsmarkdown.kpegas the source that generatesmarkdown.rb, regenerating the parser would silently change the runtime error message; keep the source and generated code equivalent.
raise ParseError, 'invalid note reference' unless @note_order
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # the note order list for proper display at the end of the document. | ||
|
|
||
| def note_for ref | ||
| raise ParseError, "invalid note reference: #{ref}" unless @note_order |
There was a problem hiding this comment.
Seems like a valid problem. But different?
I'm proposing a solution in a separate PR:
#1713
Issue originally reported here: ruby#654
fd37331 to
f7bb997
Compare
| # the note order list for proper display at the end of the document. | ||
|
|
||
| def note_for ref | ||
| raise ParseError, "invalid note reference: #{ref}" unless @note_order |
There was a problem hiding this comment.
I think the markdown below is valid, but currently fails. Valid markdown shouldn't raise ParseError.
[foo[^1]bar]
[^1]: footnoteIn def parse markdown, there's a source code comment:
# using note_order on the first pass would be a bug
@note_order = []The reported error occurs in the first pass (peg_parse 'References'), so referencing @note_order is already a bug. I think just returning nil is better in this case.
# No-op when called in the `References` pass
return unless @note_orderThere was a problem hiding this comment.
Great feedback!
I assume, this is fixed now.
Note references can be parsed while Markdown references are being collected, before footnote ordering has started. Treat that early pass as a no-op so malformed reference labels and valid labels containing footnotes do not crash the parser.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
lib/rdoc/markdown.rb:791
- This guard only covers
NoteReference;InlineNotestill reads@note_order.lengthduring the same reference-gathering pass before@note_orderis initialized. A reference label containing an inline note, such as[x^[note]]: ..., can still raiseNoMethodError, so the crash fix remains incomplete.
return unless @note_order
lib/rdoc/markdown.kpeg:406
- This guard only covers
NoteReference;InlineNotestill reads@note_order.lengthduring the same reference-gathering pass before@note_orderis initialized. A reference label containing an inline note, such as[x^[note]]: ..., can still raiseNoMethodError, so the crash fix remains incomplete.
return unless @note_order
| # the note order list for proper display at the end of the document. | ||
|
|
||
| def note_for ref | ||
| return unless @note_order |
| # the note order list for proper display at the end of the document. | ||
|
|
||
| def note_for ref | ||
| return unless @note_order |
Fixes #654.
Note references can be parsed during the reference-gathering pass, before footnote ordering is initialized. Previously this could crash with a NoMethodError for malformed reference labels.
RDoc::Markdown.parse("[[^0]\n")Before the fix, it raised:
Or:
RDoc::Markdown.parse("[foo[^1]bar]\n\n[^1]: footnote\n")