Skip to content

feat: add no-empty-definitions rule #364

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 14, 2025

Conversation

Pixel998
Copy link
Contributor

@Pixel998 Pixel998 commented May 8, 2025

Prerequisites checklist

What is the purpose of this pull request?

This PR adds a new rule no-empty-definitions to prevent empty URL definitions

What changes did you make? (Give an overview)

Added the no-empty-definitions rule that warns on empty URLs (<>) or empty fragment URLs (#), along with documentation and tests.

Related Issues

Closes #359

@lumirlumir About including [foo]: in the no-empty-definitions rule, I held off because a few weird things popped up when I looked closer.

Take these for example:

[foo]:
[bar]:

You'd think that's two errors, right? But this actually one definition where foo is the label and the URL is [bar].

So, because of these quirks, I think we should do what remark-lint does and actually check the definition nodes themselves in the code. That way, we're looking at the real definitions and not just the text, which seems much more reliable.

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

@lumirlumir lumirlumir added this to Triage May 9, 2025
@github-project-automation github-project-automation bot moved this to Needs Triage in Triage May 9, 2025
@lumirlumir lumirlumir moved this from Needs Triage to Implementing in Triage May 9, 2025
@lumirlumir
Copy link
Member

Currently, remark-lint-no-empty-url performs a fairly simple check, similar to the code you implemented. While there are some minor differences, the core behavior is the same:

definition(node) {
    if (!node.url || node.url === "#") {
        context.report({
            loc: node.position,
            messageId: "emptyDefinition",
        });
    }
}

If our goal is merely to lint empty definitions, this approach works well.

However, my original intention in proposing this rule was to catch more error-prone definition patterns. For example:

> [def]: def

- [def]: def

* [def]: def

+ [def]: def

[def]: def

[def]: def
[def]: def

[def]: def
[def]: def
[def]: def

AST

These are all valid definitions and are correctly parsed as such. But if the URL is omitted, it should be flagged. For example:

> [def]:

- [def]:

* [def]:

+ [def]:

[def]:

[def]:
[def]: 

[def]: 
[def]: 
[def]: 

AST

All of the above should be detected as empty or invalid definitions.

The challenge is that some of these aren't parsed as proper definition nodes, so a more thorough check is required. This makes the rule more complex to implement.

In fact, the no-missing-label-refs rule takes a similar approach for the same reason—it needs to handle cases where definitions aren’t recognized as valid AST nodes.

These examples cover many invalid cases, but there may still be additional edge cases to consider.

@nzakas nzakas requested a review from Copilot May 9, 2025 14:30
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces the new ESLint rule "no-empty-definitions" to prevent empty URL definitions in Markdown documents.

  • Adds a new rule implementation in src/rules/no-empty-definitions.js
  • Provides tests in tests/rules/no-empty-definitions.test.js to validate the rule
  • Updates the documentation in docs/rules/no-empty-definitions.md and README.md

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
tests/rules/no-empty-definitions.test.js Adds test cases to ensure the rule flags empty URL definitions
src/rules/no-empty-definitions.js Implements the rule and reports empty definitions
docs/rules/no-empty-definitions.md Documents the rule with examples and guidance
README.md Updates the rule list to include the new rule

@Pixel998
Copy link
Contributor Author

Pixel998 commented May 9, 2025

All of the above should be detected as empty or invalid definitions.

As I mentioned in the issue this is not two empty definitions. This is a single valid definition with label def and url [def]: (AST). Are you saying we should treat this as two definition nodes?

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. Would like @lumirlumir to review before merging.

@lumirlumir
Copy link
Member

lumirlumir commented May 12, 2025

As I mentioned in the comment above, we should make sure to handle all cases, including the definition node that Pixel998 pointed out.

Right now, the rule doesn't seem to be serving much purpose. Since definition node types are often used in comments as well — sometimes with empty definitions — the current implementation doesn't quite align with what I had in mind.

If I'm asking for too much with this rule, I'm happy to hear feedback from the rest of the team.

@eslint/eslint-team

@fasttime
Copy link
Member

As I mentioned in the comment above, we should make sure to handle all cases, including the definition node that Pixel998 pointed out.

@lumirlumir Can you clarify what cases you have in mind? Do you think that a text like [def]: should be flagged if it is at the end of a line?

@lumirlumir
Copy link
Member

Sorry for not clarifying this rule earlier.

Here's my suggestion that I originally planned to implement.

no-empty-definitions

This rule aims to disallow empty definitions.

Then, what are empty definitions? Empty definitions can fall into the following two categories:

  1. empty-definition
  2. empty-definition-prone-syntax (used for explaining my intention, not an actual term)

Defintions

empty-definition

empty-definition is a node that is interpreted as a valid definition node with an empty url property.

For example, the following Markdown syntax can be interpreted as empty-definition:

[earth]: <>
[earth]: #

empty-definition-prone-syntax

empty-definition-prone-syntax is a text that could be interpreted as a valid definition node, if a user types a string right next to the [def]: syntax.

It may be represented as a text, nested definition, or other node types in the real world examples.

For example, below Markdown syntax can be interpreted as empty-definition-prone-syntax:

> [def]:

- [def]:

* [def]:

+ [def]:

[def]:

[def]:
[def]: 

[def]: 
[def]: 
[def]: 

Then, why the above code are empty-definition-prone-syntax?

> [def]: def

- [def]: def

* [def]: def

+ [def]: def

[def]: def

[def]: def
[def]: def

[def]: def
[def]: def
[def]: def

It is because, if a user type a string right next to [def]: syntax, it is interpreted as a valid definition node.

Cautions

[def]: syntax must appear at the beginning of the line. If a user types Hello, [def]: def, the [def]: def cannot be interpreted as a definition node. So, this one should not be detected.

Conclusion

We should validate both empty-definition and empty-definition-prone-syntax in this rule.

@nzakas
Copy link
Member

nzakas commented May 13, 2025

@lumirlumir Instead of holding up this PR, can we merge this and you can add the additional functionality you have in mind?

lumirlumir
lumirlumir previously approved these changes May 13, 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.

Yes, creating a separate PR for my suggestions would be a better choice.

LGTM, would like @nzakas to review before merging.

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 cb5a956 into eslint:main May 14, 2025
17 checks passed
@github-project-automation github-project-automation bot moved this from Implementing to Complete in Triage May 14, 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.

New Rule: no-empty-definitions
4 participants