Merge pull request #310 from commonmark/issue-292

Fix LinkReferenceDefinition having null SourceSpan when title is present

Author: robinst

commonmark/src/main/java/org/commonmark/internal/LinkReferenceDefinitionParser.java

@@ -100,6 +100,10 @@ State getState() {
     }
 
     private boolean startDefinition(Scanner scanner) {
+        // Finish any outstanding references now. We don't do this earlier because we need addSourceSpan to have been
+        // called before we do it.
+        finishReference();
+
         scanner.whitespace();
         if (!scanner.next('[')) {
             return false;
@@ -205,7 +209,6 @@ private boolean startTitle(Scanner scanner) {
                 title.append('\n');
             }
         } else {
-            finishReference();
             // There might be another reference instead, try that for the same character.
             state = State.START_DEFINITION;
         }
@@ -235,7 +238,6 @@ private boolean title(Scanner scanner) {
             return false;
         }
         referenceValid = true;
-        finishReference();
         paragraphLines.clear();
 
         // See if there's another definition.

commonmark/src/main/java/org/commonmark/parser/block/BlockParser.java

@@ -34,6 +34,10 @@ public interface BlockParser {
 
     BlockContinue tryContinue(ParserState parserState);
 
+    /**
+     * Add the part of a line that belongs to this block parser to parse (i.e. without any container block markers).
+     * Note that the line will only include a {@link SourceLine#getSourceSpan()} if source spans are enabled for inlines.
+     */
     void addLine(SourceLine line);
 
     /**

commonmark/src/test/java/org/commonmark/test/ListBlockParserTest.java

@@ -60,7 +60,6 @@ public void testOrderedListIndents() {
     private void assertListItemIndents(String input, int expectedMarkerIndent, int expectedContentIndent) {
         Node doc = PARSER.parse(input);
         ListItem listItem = Nodes.find(doc, ListItem.class);
-        assertNotNull(listItem);
         assertEquals(expectedMarkerIndent, listItem.getMarkerIndent());
         assertEquals(expectedContentIndent, listItem.getContentIndent());
     }

commonmark/src/test/java/org/commonmark/test/Nodes.java

@@ -21,20 +21,32 @@ public static List<Node> getChildren(Node parent) {
      * @param parent    The node to get children from (node itself will not be checked)
      * @param nodeClass The type of node to find
      */
-    public static <T> T find(Node parent, Class<T> nodeClass) {
+    public static <T> T tryFind(Node parent, Class<T> nodeClass) {
         Node node = parent.getFirstChild();
         while (node != null) {
             Node next = node.getNext();
             if (nodeClass.isInstance(node)) {
                 //noinspection unchecked
                 return (T) node;
             }
-            T result = find(node, nodeClass);
+            T result = tryFind(node, nodeClass);
             if (result != null) {
                 return result;
             }
             node = next;
         }
         return null;
     }
+
+    /**
+     * Recursively try to find a node with the given type within the children of the specified node. Throw if node
+     * could not be found.
+     */
+    public static <T> T find(Node parent, Class<T> nodeClass) {
+        var node = tryFind(parent, nodeClass);
+        if (node == null) {
+            throw new IllegalArgumentException("Could not find a " + nodeClass.getSimpleName() + " node in " + parent);
+        }
+        return node;
+    }
 }

commonmark/src/test/java/org/commonmark/test/SourceSpansTest.java

@@ -8,6 +8,7 @@
 import java.util.ArrayDeque;
 import java.util.Arrays;
 import java.util.Deque;
+import java.util.List;
 
 import static org.junit.Assert.assertEquals;
 
@@ -164,6 +165,24 @@ public void linkReferenceDefinition() {
         assertEquals(Arrays.asList(SourceSpan.of(1, 0, 4)), paragraph.getSourceSpans());
     }
 
+    @Test
+    public void linkReferenceDefinitionMultiple() {
+        var doc = PARSER.parse("[foo]: /foo\n[bar]: /bar\n");
+        var def1 = (LinkReferenceDefinition) doc.getFirstChild();
+        var def2 = (LinkReferenceDefinition) doc.getLastChild();
+        assertEquals(List.of(SourceSpan.of(0, 0, 11)), def1.getSourceSpans());
+        assertEquals(List.of(SourceSpan.of(1, 0, 11)), def2.getSourceSpans());
+    }
+
+    @Test
+    public void linkReferenceDefinitionWithTitle() {
+        var doc = PARSER.parse("[1]: #not-code \"Text\"\n[foo]: /foo\n");
+        var def1 = (LinkReferenceDefinition) doc.getFirstChild();
+        var def2 = (LinkReferenceDefinition) doc.getLastChild();
+        assertEquals(List.of(SourceSpan.of(0, 0, 21)), def1.getSourceSpans());
+        assertEquals(List.of(SourceSpan.of(1, 0, 11)), def2.getSourceSpans());
+    }
+
     @Test
     public void linkReferenceDefinitionHeading() {
         // This is probably the trickiest because we have a link reference definition at the start of a paragraph

commonmark/src/test/java/org/commonmark/test/ThematicBreakParserTest.java

@@ -2,8 +2,6 @@
 
 import org.commonmark.node.ThematicBreak;
 import org.commonmark.parser.Parser;
-import org.commonmark.renderer.html.HtmlRenderer;
-import org.commonmark.testutil.RenderingTestCase;
 import org.junit.Test;
 
 import static org.junit.Assert.assertEquals;
@@ -23,7 +21,6 @@ public void testLiteral() {
 
     private static void assertLiteral(String expected, String input) {
         var tb = Nodes.find(PARSER.parse(input), ThematicBreak.class);
-        assertNotNull(tb);
         assertEquals(expected, tb.getLiteral());
     }
 }