fix: guard for missing position in no-invalid-label-refs

[DMartens]

Prerequisites checklist

• I have read the contributing guidelines.

What is the purpose of this pull request?

The GFM parser can create nodes without a position property for example as reported in #619.
The example code "Hello [World][https://example.com]" is split into 3 subnodes while the commonmark parser keeps it a single text node.

What changes did you make? (Give an overview)

Ignore text nodes without a position property.
This leaves the example code as a false negative but for a proper fix we would need to rethink how the rule works.

Related Issues

Fixes #619

Is there anything you'd like reviewers to focus on?

Should we also check other rules for a potentially missing position property?

[nzakas]

Thanks for investigating. Under what circumstances is a node created without a position? Is this possible a bug in the parser?

I don't think we should just ignore nodes without position. There must be a way to calculate the actual position, and maybe that logic should be embedded in getLoc() directly.

[DMartens]

The problem is not that we cannot get the position but that the one text node in commonmark is 3 nodes in GFM (each without position set, see linked playground):

• { type: "text", value: "Hello [World][" }
• { type: "link", children: [{ type: "text", value: "https://example.com" }] }
• { type: "text", value: "]" }

Currently the node listens for text nodes, so we get the text in chunks and need to reconstruct it.
We could add a workaround for this pattern "[...][", followed by a link and "]..." or scan the whole text at once instead of per text node.

[lumirlumir]

Currently, all Markdown rules assume every node has a position property. I think this change is only a workaround for a quick fix, not a fundamental solution.

I've examined various Markdown AST cases, and in my experience the only time a node lacks position is when the mode is GFM and the node contains autolink literals. I encountered the same issue while working on #597, but at the time I didn't have enough time to investigate further, so I left it as-is.

Therefore, I strongly suspect the missing position is a parser bug rather than something that can be fundamentally fixed here. If we proceed in this direction, the fix would need to be applied across all Markdown rules, since nodes without a position property can occur elsewhere.

Looking at mdast-util-from-markdown and micromark, the tokenizer and parser don't clearly state when a node's position property is included or not, but in almost every case the AST includes position, it only seems to be missing in certain scenarios, so I suspect the bug is more likely caused by a specific extension in GFM mode.

[DMartens]

As I said this is a quick band aid fix to make sure this rule does not crash.
I would also prefer a proper fix, so I raised an issue for the autolink plugin, which is part of the GFM plugin.

[nzakas]

I agree with @lumirlumir in that every rule expects every node to have a position property, so updating one rule to ignore nodes without a position property does little to address this problem.

While I don't mind a temporary solution, I think that solution should get to attempt to calculate the actual position inside of getLoc(), which would then distribute that solution to all consuming rules.

[DMartens]

How should the position be calculated as we cannot make any assumptions of the context.
Naively you could search for the "value" of the node in the source code (basically trying to stringify the node again) for the start offset, but:

1. Markdown allows different syntax for the same node, e.g. using a single * or _ to create an emphasis node
2. The same text could be present multiple times in the text

I think we should rather return null and handle this in each rule.

[nzakas]

Handling in each rule is a footgun that will be easy to forget and a pain to remember to fix later. I don't see that as a valid option.

Can we not look for Text nodes that end with [ and extrapolate from that?

[DMartens]

I thought that you meant we should try to get to position in the general case, not for this specific case. As lumir mentioned he also encountered missing position properties when testing, so there may be additional cases.

In this specific case all three nodes do not have a position:

1. text[ (text node)
2. https://example.com (link node)
3. ]

Adding characters after ] only creates two nodes:

1. text[
2. https://example.com]text

So we would also need to handle that.
In that case I would rather wait for the maintainers to respond to the raised issue.

[lumirlumir]

After further investigation, I found this pattern only occurs when [ appears immediately before autolink literals.

The cases below all include position information:

Playground

https://example.com

http://example.com

xxx@google.com

However, the cases below do not (the number of whitespace characters after [ doesn't seem to matter):

Playground

[https://example.com

[ https://example.com

[http://example.com

[ http://example.com

[xxx@google.com

[ xxx@google.com

---

If a quick workaround doesn't make sense, I think waiting for the mdast maintainer's response or submitting a PR there would be more appropriate.