Repository: kivikakk/comrak (rust)

Issue:
Unexpected sourcepos.
Hi. "sourcepos" is exactly what I was looking for.
What do you think of this result?

markdown

```md
[AB
CD](/)
```

my expected

```html
<p data-sourcepos="1:1-2:6"><a data-sourcepos="1:1-2:6" href="/">AB
CD</a></p>
```

comrak v0.18.0

```html
<p data-sourcepos="1:1-2:6"><a data-sourcepos="2:1-2:6" href="/">AB
CD</a></p>
```

- - -

Similar pattern.

markdown
```md
- A
[![B](/B.png)](/B)
```

comrak
```html
<ul data-sourcepos="1:1-2:18">
<li data-sourcepos="1:1-2:18">A
<a data-sourcepos="2:3-2:20" href="/B"><img data-sourcepos="2:4-2:15" src="/B.png" alt="B" /></a></li>
```

cmark
```html
<ul data-sourcepos="1:1-2:18">
<li data-sourcepos="1:1-2:18">A
<a href="/B"><img src="/B.png" alt="B" /></a></li>
</ul>
```


Discussion:
That is a bit surprising! It's actually _almost_ per upstream. Here's [cmark-gfm](https://github.com/github/cmark-gfm):

```console
$ cat o
[AB
CD](/)

$ build/src/cmark-gfm o -t xml --sourcepos
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE document SYSTEM "CommonMark.dtd">
<document sourcepos="1:1-2:6" xmlns="http://commonmark.org/xml/1.0">
  <paragraph sourcepos="1:1-2:6">
    <link sourcepos="2:1-2:6" destination="/" title="">
      <text sourcepos="1:2-1:3" xml:space="preserve">AB</text>
      <softbreak />
      <text sourcepos="2:1-2:2" xml:space="preserve">CD</text>
    </link>
  </paragraph>
</document>

$ build/src/cmark-gfm o --sourcepos
<p data-sourcepos="1:1-2:6"><a href="/">AB
CD</a></p>
```

So, in terms of the AST, comrak is doing the same thing as upstream, as shown in `<link sourcepos="2:1-2:6"`; the difference is that upstream doesn't actually put sourcepos on `<a>` in output HTML at all.

I'd be happy with fixing this in either direction; either fix how we record sourcepos on the link node, or stop outputting in it HTML. (It's possible that "fixing" the former might have unintended consequences.)
I see. I prefer the latter(stop outputting).
I don't think writing logic that has nothing to do with the upstream is a good idea since it will make things complex.
Me too! PRs happily accepted; I don't have the time for this right now myself.
Wait...the idea is to _not_ have `sourcepos` on the `<a>`?  Are you wanting to remove from other non-block html?  Please don't...

I tried running it against `markdown-it.rs`, I get what was expected

```
<p data-sourcepos="1:1-2:6"><a data-sourcepos="1:1-2:6" href="/">AB
CD</a></p>
```
In general, I'd rather "matches the upstream we explicitly follow" than not. Right now we have "doesn't match upstream _and_ is incorrect". I'd accept a solution either way.
But it does match the upstream, doesn't it?  Block-level sourcepos works correctly I think.

Inline sourcepos is a great enhancement. I'd certainly rather see a fix, but if that's not feasible at the moment, then how about an option to enable it? Those that want it can enable it, and work on a fix.  `comrak` is already going beyond what upstream offers (thankfully) while trying to maintain backward compatibility.  So I think ripping it out should be last resort.

Patch:
diff --git a/src/html.rs b/src/html.rs
--- a/src/html.rs
+++ b/src/html.rs
@@ -725,33 +725,31 @@ impl<'o, 'c: 'o> HtmlFormatter<'o, 'c> {
                 }
             }
             NodeValue::Text(ref literal) => {
+                // No sourcepos.
                 if entering {
                     self.escape(literal.as_bytes())?;
                 }
             }
             NodeValue::LineBreak => {
+                // No sourcepos.
                 if entering {
-                    self.output.write_all(b"<br")?;
-                    self.render_sourcepos(node)?;
-                    self.output.write_all(b" />\n")?;
+                    self.output.write_all(b"<br />\n")?;
                 }
             }
             NodeValue::SoftBreak => {
+                // No sourcepos.
                 if entering {
                     if self.options.render.hardbreaks {
-                        self.output.write_all(b"<br")?;
-                        self.render_sourcepos(node)?;
-                        self.output.write_all(b" />\n")?;
+                        self.output.write_all(b"<br />\n")?;
                     } else {
                         self.output.write_all(b"\n")?;
                     }
                 }
             }
             NodeValue::Code(NodeCode { ref literal, .. }) => {
+                // No sourcepos.
                 if entering {
-                    self.output.write_all(b"<code")?;
-                    self.render_sourcepos(node)?;
-                    self.output.write_all(b">")?;
+                    self.output.write_all(b"<code>")?;
                     self.escape(literal.as_bytes())?;
                     self.output.write_all(b"</code>")?;
                 }
@@ -773,52 +771,47 @@ impl<'o, 'c: 'o> HtmlFormatter<'o, 'c> {
                 }
             }
             NodeValue::Strong => {
+                // No sourcepos.
                 let parent_node = node.parent();
                 if !self.options.render.gfm_quirks
                     || (parent_node.is_none()
                         || !matches!(parent_node.unwrap().data.borrow().value, NodeValue::Strong))
                 {
                     if entering {
-                        self.output.write_all(b"<strong")?;
-                        self.render_sourcepos(node)?;
-                        self.output.write_all(b">")?;
+                        self.output.write_all(b"<strong>")?;
                     } else {
                         self.output.write_all(b"</strong>")?;
                     }
                 }
             }
             NodeValue::Emph => {
+                // No sourcepos.
                 if entering {
-                    self.output.write_all(b"<em")?;
-                    self.render_sourcepos(node)?;
-                    self.output.write_all(b">")?;
+                    self.output.write_all(b"<em>")?;
                 } else {
                     self.output.write_all(b"</em>")?;
                 }
             }
             NodeValue::Strikethrough => {
+                // No sourcepos.
                 if entering {
-                    self.output.write_all(b"<del")?;
-                    self.render_sourcepos(node)?;
-                    self.output.write_all(b">")?;
+                    self.output.write_all(b"<del>")?;
                 } else {
                     self.output.write_all(b"</del>")?;
                 }
             }
             NodeValue::Superscript => {
+                // No sourcepos.
                 if entering {
-                    self.output.write_all(b"<sup")?;
-                    self.render_sourcepos(node)?;
-                    self.output.write_all(b">")?;
+                    self.output.write_all(b"<sup>")?;
                 } else {
                     self.output.write_all(b"</sup>")?;
                 }
             }
             NodeValue::Link(ref nl) => {
+                // No sourcepos.
                 if entering {
-                    self.output.write_all(b"<a")?;
-                    self.render_sourcepos(node)?;
-                    self.output.write_all(b" href=\"")?;
+                    self.output.write_all(b"<a href=\"")?;
                     let url = nl.url.as_bytes();
                     if self.options.render.unsafe_ || !dangerous_url(url) {
                         self.escape_href(url)?;
@@ -833,10 +826,9 @@ impl<'o, 'c: 'o> HtmlFormatter<'o, 'c> {
                 }
             }
             NodeValue::Image(ref nl) => {
+                // No sourcepos.
                 if entering {
-                    self.output.write_all(b"<img")?;
-                    self.render_sourcepos(node)?;
-                    self.output.write_all(b" src=\"")?;
+                    self.output.write_all(b"<img src=\"")?;
                     let url = nl.url.as_bytes();
                     if self.options.render.unsafe_ || !dangerous_url(url) {
                         self.escape_href(url)?;
@@ -949,17 +941,14 @@ impl<'o, 'c: 'o> HtmlFormatter<'o, 'c> {
                 }
             }
             NodeValue::FootnoteDefinition(ref nfd) => {
+                // No sourcepos.
                 if entering {
                     if self.footnote_ix == 0 {
-                        self.output.write_all(b"<section")?;
-                        self.render_sourcepos(node)?;
                         self.output
-                            .write_all(b" class=\"footnotes\" data-footnotes>\n<ol>\n")?;
+                            .write_all(b"<section class=\"footnotes\" data-footnotes>\n<ol>\n")?;
                     }
                     self.footnote_ix += 1;
-                    self.output.write_all(b"<li")?;
-                    self.render_sourcepos(node)?;
-                    self.output.write_all(b" id=\"fn-")?;
+                    self.output.write_all(b"<li id=\"fn-")?;
                     self.escape_href(nfd.name.as_bytes())?;
                     self.output.write_all(b"\">")?;
                 } else {
@@ -970,18 +959,14 @@ impl<'o, 'c: 'o> HtmlFormatter<'o, 'c> {
                 }
             }
             NodeValue::FootnoteReference(ref nfr) => {
+                // No sourcepos.
                 if entering {
                     let mut ref_id = format!("fnref-{}", nfr.name);
-
-                    self.output.write_all(b"<sup")?;
-                    self.render_sourcepos(node)?;
-
                     if nfr.ref_num > 1 {
                         ref_id = format!("{}-{}", ref_id, nfr.ref_num);
                     }
-
                     self.output
-                        .write_all(b" class=\"footnote-ref\"><a href=\"#fn-")?;
+                        .write_all(b"<sup class=\"footnote-ref\"><a href=\"#fn-")?;
                     self.escape_href(nfr.name.as_bytes())?;
                     self.output.write_all(b"\" id=\"")?;
                     self.escape_href(ref_id.as_bytes())?;
@@ -1019,11 +1004,10 @@ impl<'o, 'c: 'o> HtmlFormatter<'o, 'c> {
                 }
             }
             NodeValue::Escaped => {
+                // No sourcepos.
                 if self.options.render.escaped_char_spans {
                     if entering {
-                        self.output.write_all(b"<span data-escaped-char")?;
-                        self.render_sourcepos(node)?;
-                        self.output.write_all(b">")?;
+                        self.output.write_all(b"<span data-escaped-char>")?;
                     } else {
                         self.output.write_all(b"</span>")?;
                     }
@@ -1040,10 +1024,9 @@ impl<'o, 'c: 'o> HtmlFormatter<'o, 'c> {
                 }
             }
             NodeValue::WikiLink(ref nl) => {
+                // No sourcepos.
                 if entering {
-                    self.output.write_all(b"<a")?;
-                    self.render_sourcepos(node)?;
-                    self.output.write_all(b" href=\"")?;
+                    self.output.write_all(b"<a href=\"")?;
                     let url = nl.url.as_bytes();
                     if self.options.render.unsafe_ || !dangerous_url(url) {
                         self.escape_href(url)?;
@@ -1055,6 +1038,7 @@ impl<'o, 'c: 'o> HtmlFormatter<'o, 'c> {
                 }
             }
             NodeValue::Underline => {
+                // No sourcepos.
                 if entering {
                     self.output.write_all(b"<u>")?;
                 } else {
@@ -1062,6 +1046,7 @@ impl<'o, 'c: 'o> HtmlFormatter<'o, 'c> {
                 }
             }
             NodeValue::SpoileredText => {
+                // No sourcepos.
                 if entering {
                     self.output.write_all(b"<span class=\"spoiler\">")?;
                 } else {
@@ -1069,6 +1054,7 @@ impl<'o, 'c: 'o> HtmlFormatter<'o, 'c> {
                 }
             }
             NodeValue::EscapedTag(ref net) => {
+                // No sourcepos.
                 self.output.write_all(net.as_bytes())?;
             }
         }
diff --git a/src/parser/inlines.rs b/src/parser/inlines.rs
--- a/src/parser/inlines.rs
+++ b/src/parser/inlines.rs
@@ -1648,13 +1648,12 @@ impl<'a, 'r, 'o, 'c, 'd, 'i> Subject<'a, 'r, 'o, 'c, 'd, 'i> {
             self.pos,
             self.pos,
         );
-        inl.data.borrow_mut().sourcepos.start.column = self.brackets[brackets_len - 1]
+        inl.data.borrow_mut().sourcepos.start = self.brackets[brackets_len - 1]
             .inl_text
             .data
             .borrow()
             .sourcepos
-            .start
-            .column;
+            .start;
         inl.data.borrow_mut().sourcepos.end.column =
             usize::try_from(self.pos as isize + self.column_offset + self.block_offset as isize)
                 .unwrap();
diff --git a/src/parser/mod.rs b/src/parser/mod.rs
--- a/src/parser/mod.rs
+++ b/src/parser/mod.rs
@@ -755,9 +755,14 @@ pub struct RenderOptions {
     /// ```
     pub list_style: ListStyleType,
 
-    /// Include source position attributes in XML output.
+    /// Include source position attributes in HTML and XML output.
     ///
-    /// Not yet compatible with extension.description_lists.
+    /// Sourcepos information is reliable for all core block items, and most
+    /// extensions. The description lists extension still has issues; see
+    /// <https://github.com/kivikakk/comrak/blob/3bb6d4ce/src/tests/description_lists.rs#L60-L125>.
+    ///
+    /// Sourcepos information is **not** reliable for inlines. See
+    /// <https://github.com/kivikakk/comrak/pull/439> for a discussion.
     ///
     /// ```rust
     /// # use comrak::{markdown_to_commonmark_xml, Options};


Test patch:
diff --git a/src/tests/core.rs b/src/tests/core.rs
--- a/src/tests/core.rs
+++ b/src/tests/core.rs
@@ -495,3 +495,109 @@ fn case_insensitive_safety() {
         "<p><a href=\"\">a</a> <a href=\"\">b</a> <a href=\"\">c</a> <a href=\"\">d</a> <a href=\"\">e</a> <a href=\"\">f</a> <a href=\"\">g</a></p>\n",
     );
 }
+
+#[test]
+fn link_sourcepos_baseline() {
+    assert_ast_match!(
+        [],
+        "[ABCD](/)\n",
+        (document (1:1-1:9) [
+            (paragraph (1:1-1:9) [
+                (link (1:1-1:9) [
+                    (text (1:2-1:5) "ABCD")
+                ])
+            ])
+        ])
+    );
+}
+
+// https://github.com/kivikakk/comrak/issues/301
+#[test]
+fn link_sourcepos_newline() {
+    assert_ast_match!(
+        [],
+        "[AB\nCD](/)\n",
+        (document (1:1-2:6) [
+            (paragraph (1:1-2:6) [
+                (link (1:1-2:6) [
+                    (text (1:2-1:3) "AB")
+                    (softbreak (1:4-1:4))
+                    (text (2:1-2:2) "CD")
+                ])
+            ])
+        ])
+    );
+}
+
+// Ignored per https://github.com/kivikakk/comrak/pull/439#issuecomment-2225129960.
+#[ignore]
+#[test]
+fn link_sourcepos_truffle() {
+    assert_ast_match!(
+        [],
+        "- A\n[![B](/B.png)](/B)\n",
+        (document (1:1-2:18) [
+            (list (1:1-2:18) [
+                (item (1:1-2:18) [
+                    (paragraph (1:3-2:18) [
+                        (text (1:3-1:3) "A")
+                        (softbreak (1:4-1:4))
+                        (link (2:1-2:18) [
+                            (image (2:2-2:13) [
+                                (text (2:4-2:4) "B")
+                            ])
+                        ])
+                    ])
+                ])
+            ])
+        ])
+    );
+}
+
+#[test]
+fn link_sourcepos_truffle_twist() {
+    assert_ast_match!(
+        [],
+        "- A\n  [![B](/B.png)](/B)\n",
+        (document (1:1-2:20) [
+            (list (1:1-2:20) [
+                (item (1:1-2:20) [
+                    (paragraph (1:3-2:20) [
+                        (text (1:3-1:3) "A")
+                        (softbreak (1:4-1:4))
+                        (link (2:3-2:20) [
+                            (image (2:4-2:15) [
+                                (text (2:6-2:6) "B")
+                            ])
+                        ])
+                    ])
+                ])
+            ])
+        ])
+    );
+}
+
+// Ignored per https://github.com/kivikakk/comrak/pull/439#issuecomment-2225129960.
+#[ignore]
+#[test]
+fn link_sourcepos_truffle_bergamot() {
+    assert_ast_match!(
+        [],
+        "- A\n   [![B](/B.png)](/B)\n",
+        (document (1:1-2:21) [
+            (list (1:1-2:21) [
+                (item (1:1-2:21) [
+                    (paragraph (1:3-2:21) [
+                        (text (1:3-1:3) "A")
+                        (softbreak (1:4-1:4))
+                        (link (2:4-2:21) [
+                            (image (2:5-2:16) [
+                                (text (2:7-2:7) "B")
+                            ])
+                        ])
+                    ])
+                ])
+            ])
+        ])
+    );
+}
diff --git a/src/tests/escaped_char_spans.rs b/src/tests/escaped_char_spans.rs
--- a/src/tests/escaped_char_spans.rs
+++ b/src/tests/escaped_char_spans.rs
@@ -1,17 +1,10 @@
 use super::*;
 use ntest::test_case;
 
-// html_opts! does a roundtrip check unless sourcepos is set.
-// These cases don't work roundtrip, because converting to commonmark
-// automatically escapes certain characters.
-#[test_case("\\@user", "<p data-sourcepos=\"1:1-1:6\"><span data-escaped-char data-sourcepos=\"1:1-1:2\">@</span>user</p>\n")]
-#[test_case("This\\@that", "<p data-sourcepos=\"1:1-1:10\">This<span data-escaped-char data-sourcepos=\"1:5-1:6\">@</span>that</p>\n")]
+#[test_case("\\@user", "<p><span data-escaped-char>@</span>user</p>\n")]
+#[test_case("This\\@that", "<p>This<span data-escaped-char>@</span>that</p>\n")]
 fn escaped_char_spans(markdown: &str, html: &str) {
-    html_opts!(
-        [render.escaped_char_spans, render.sourcepos],
-        markdown,
-        html
-    );
+    html_opts!([render.escaped_char_spans], markdown, html, no_roundtrip);
 }
 
 #[test_case("\\@user", "<p>@user</p>\n")]
diff --git a/src/tests/fuzz.rs b/src/tests/fuzz.rs
--- a/src/tests/fuzz.rs
+++ b/src/tests/fuzz.rs
@@ -47,7 +47,7 @@ fn footnote_def() {
             render.hardbreaks
         ],
         "\u{15}\u{b}\r[^ ]:",
-        "<p data-sourcepos=\"1:1-2:5\">\u{15}\u{b}<br data-sourcepos=\"1:3-1:3\" />\n[^ ]:</p>\n",
+        "<p data-sourcepos=\"1:1-2:5\">\u{15}\u{b}<br />\n[^ ]:</p>\n",
     );
 }
 
diff --git a/src/tests/wikilinks.rs b/src/tests/wikilinks.rs
--- a/src/tests/wikilinks.rs
+++ b/src/tests/wikilinks.rs
@@ -1,20 +1,19 @@
 use super::*;
 
-// html_opts! does a roundtrip check unless sourcepos is set.
-// These cases don't work roundtrip, because converting to commonmark
-// automatically escapes certain characters.
 #[test]
 fn wikilinks_does_not_unescape_html_entities_in_link_label() {
     html_opts!(
-        [extension.wikilinks_title_after_pipe, render.sourcepos],
+        [extension.wikilinks_title_after_pipe],
         concat!("This is [[&lt;script&gt;alert(0)&lt;/script&gt;|a &lt;link]]",),
-        concat!("<p data-sourcepos=\"1:1-1:60\">This is <a data-sourcepos=\"1:9-1:60\" href=\"%3Cscript%3Ealert(0)%3C/script%3E\" data-wikilink=\"true\">a &lt;link</a></p>\n"),
+        concat!("<p>This is <a href=\"%3Cscript%3Ealert(0)%3C/script%3E\" data-wikilink=\"true\">a &lt;link</a></p>\n"),
+        no_roundtrip,
     );
 
     html_opts!(
-        [extension.wikilinks_title_before_pipe, render.sourcepos],
+        [extension.wikilinks_title_before_pipe],
         concat!("This is [[a &lt;link|&lt;script&gt;alert(0)&lt;/script&gt;]]",),
-        concat!("<p data-sourcepos=\"1:1-1:60\">This is <a data-sourcepos=\"1:9-1:60\" href=\"%3Cscript%3Ealert(0)%3C/script%3E\" data-wikilink=\"true\">a &lt;link</a></p>\n"),
+        concat!("<p>This is <a href=\"%3Cscript%3Ealert(0)%3C/script%3E\" data-wikilink=\"true\">a &lt;link</a></p>\n"),
+        no_roundtrip,
     );
 }