Skip to content

Conversation

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Sep 15, 2025

Prerequisites checklist

What is the purpose of this pull request?

Refactors no-reference-like-urls to visit image and link nodes directly and thus simplify the code, and also fixes report location when the link/image contains lone <CR> as a line break (see the two updated test cases).

What changes did you make? (Give an overview)

Refactored no-reference-like-urls to visit image and link nodes directly.

Related Issues

Discussion #523 (comment)

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

Parsing of link/image node texts is still needed because of the [link](<uri>) syntax. The regex could probably be simplified further, but that would be a more complicated change.

@eslintbot eslintbot added this to Triage Sep 15, 2025
@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Sep 15, 2025
Comment on lines -270 to +271
endLine: 1,
endColumn: 20,
endLine: 2,
endColumn: 9,
Copy link
Member Author

@mdjermanovic mdjermanovic Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there's a bug in findOffsets. Now that this rule is modified to report node location directly from the AST, lone <CR> increases line number.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly. Currently, findOffsets doesn't handle CR line endings.

I plan to fix this in #493 once #376 is merged.

Comment on lines -336 to +337
endLine: 1,
endColumn: 21,
endLine: 2,
endColumn: 9,
Copy link
Member Author

@mdjermanovic mdjermanovic Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as #525 (comment)


/** Pattern to match both inline links: `[text](url)` and images: `![alt](url)`, with optional title */
const linkOrImagePattern =
/(?<=(?<!\\)(?:\\{2})*)(?<imageBang>!)?\[(?<label>(?:\\.|[^()\\]|\([\s\S]*\))*?)\]\((?<destination>[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*(?:<[^>]*>|[^ \t()]+))(?:[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*(?<title>"[^"]*"|'[^']*'|\([^)]*\)))?[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*\)(?!\()/gu;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what cases were being checked by (?!\() at the end, but no tests are failing now that I've removed it since the regex gets only the image/link node text so it can't check for a parenthesis after the image/link text.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it's safe to remove it, since I tried to find edge cases but couldn't detect any now that we're inspecting the link and image nodes directly.

@lumirlumir lumirlumir self-requested a review September 17, 2025 08:46
@lumirlumir lumirlumir moved this from Needs Triage to Triaging in Triage Sep 18, 2025
Copy link
Member

@lumirlumir lumirlumir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look! The updated logic looks great.

I've left a few minor suggestions and noted some false negatives in the regex to discuss.

Comment on lines -270 to +271
endLine: 1,
endColumn: 20,
endLine: 2,
endColumn: 9,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly. Currently, findOffsets doesn't handle CR line endings.

I plan to fix this in #493 once #376 is merged.

function isInSkipRange(index, skipRanges) {
return skipRanges.some(range => range[0] <= index && index < range[1]);
}
/(?<imageBang>!)?\[(?<label>(?:\\.|[^()\\]|\([\s\S]*\))*?)\]\((?<destination>[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*(?:<[^>]*>|[^ \t()]+))(?:[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*(?<title>"[^"]*"|'[^']*'|\([^)]*\)))?[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*\)$/u;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[[](](link)

[[])](link)

![[](](image)

![[])](image)

According to the AST, the code above are valid links and images.

But, they produces false negatives like the following:

  • test cases:
		{
			code: dedent`
				[[](](mercury)

				[mercury]: https://example.com/mercury
			`,
			output: dedent`
				[[](][mercury]

				[mercury]: https://example.com/mercury
			`,
			errors: [
				{
					messageId: "referenceLikeUrl",
					data: { type: "link", prefix: "" },
					line: 1,
					column: 1,
					endLine: 1,
					endColumn: 15,
				},
			],
		},
		{
			code: dedent`
				[[])](mercury)

				[mercury]: https://example.com/mercury
			`,
			output: dedent`
				[[])][mercury]

				[mercury]: https://example.com/mercury
			`,
			errors: [
				{
					messageId: "referenceLikeUrl",
					data: { type: "link", prefix: "" },
					line: 1,
					column: 1,
					endLine: 1,
					endColumn: 15,
				},
			],
		},
		{
			code: dedent`
				![[](](mercury)

				[mercury]: https://example.com/mercury
			`,
			output: dedent`
				![[](][mercury]

				[mercury]: https://example.com/mercury
			`,
			errors: [
				{
					messageId: "referenceLikeUrl",
					data: { type: "image", prefix: "!" },
					line: 1,
					column: 1,
					endLine: 1,
					endColumn: 16,
				},
			],
		},
		{
			code: dedent`
				![[])](mercury)

				[mercury]: https://example.com/mercury
			`,
			output: dedent`
				![[])][mercury]

				[mercury]: https://example.com/mercury
			`,
			errors: [
				{
					messageId: "referenceLikeUrl",
					data: { type: "image", prefix: "!" },
					line: 1,
					column: 1,
					endLine: 1,
					endColumn: 16,
				},
			],
		},
  • result:
image

The regex could probably be simplified further, but that would be a more complicated change.

But, I'm not sure this fix should be included in this PR.

As you mentioned above, would it be better to address this bug in a separate PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, they produces false negatives like the following

Is this a bug that this PR would introduce, or does it already exist in the current version of this rule in the main branch?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this bug already exists. It also fails on the main branch.

Copy link
Member Author

@mdjermanovic mdjermanovic Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I think we could fix it in a separate PR as it's unrelated to this change (unless this change would make it more difficult to fix this bug and we'd need to revert it?). This change was meant to be just a refactor to simplify the rule, and the <CR> bug fix came as a side effect.


/** Pattern to match both inline links: `[text](url)` and images: `![alt](url)`, with optional title */
const linkOrImagePattern =
/(?<=(?<!\\)(?:\\{2})*)(?<imageBang>!)?\[(?<label>(?:\\.|[^()\\]|\([\s\S]*\))*?)\]\((?<destination>[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*(?:<[^>]*>|[^ \t()]+))(?:[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*(?<title>"[^"]*"|'[^']*'|\([^)]*\)))?[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*\)(?!\()/gu;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it's safe to remove it, since I tried to find edge cases but couldn't detect any now that we're inspecting the link and image nodes directly.

@lumirlumir lumirlumir moved this from Triaging to Implementing in Triage Sep 18, 2025
mdjermanovic and others added 2 commits September 22, 2025 09:59
Co-authored-by: 루밀LuMir <rpfos@naver.com>
Copy link
Member

@lumirlumir lumirlumir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Would like one more review before merging.

@lumirlumir lumirlumir moved this from Implementing to Second Review Needed in Triage Sep 22, 2025
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

@nzakas nzakas merged commit 28723c2 into main Sep 22, 2025
23 checks passed
@nzakas nzakas deleted the refactor-no-reference-like-urls branch September 22, 2025 17:44
@github-project-automation github-project-automation bot moved this from Second Review Needed to Complete in Triage Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

4 participants