Skip to content

fix: correct parent typing for rule visitors #571

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+36 −67
Open

fix: correct parent typing for rule visitors #571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/language/markdown-source-code.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { lineEndingPattern } from "../util.js";
* @import { Position } from "unist";
* @import { Root, Node, Html } from "mdast";
* @import { TraversalStep, FileProblem, DirectiveType, RulesConfig } from "@eslint/core";
* @import { MarkdownLanguageOptions } from "../types.js";
* @import { MarkdownLanguageOptions, MarkdownSyntaxElement } from "../types.js";
*/

//-----------------------------------------------------------------------------
Expand Down Expand Up @@ -113,7 +113,7 @@ function extractInlineConfigCommentsFromHTML(node, sourceCode) {

/**
* Markdown Source Code Object
* @extends {TextSourceCodeBase<{LangOptions: MarkdownLanguageOptions, RootNode: Root, SyntaxElementWithLoc: Node, ConfigNode: { value: string; position: Position }}>}
* @extends {TextSourceCodeBase<{LangOptions: MarkdownLanguageOptions, RootNode: Root, SyntaxElementWithLoc: MarkdownSyntaxElement, ConfigNode: { value: string; position: Position }}>}
*/
export class MarkdownSourceCode extends TextSourceCodeBase {
/**
Expand Down
99 changes: 34 additions & 65 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,35 +8,11 @@ import type {
Data,
Literal,
Parent,
// Nodes
Blockquote,
Break,
Parents,
// Node unions
Nodes,
Code,
Definition,
Emphasis,
Heading,
Html,
Image,
ImageReference,
InlineCode,
Link,
LinkReference,
List,
ListItem,
Paragraph,
Root,
Strong,
Text,
ThematicBreak,
// Extensions (GFM)
Delete,
FootnoteDefinition,
FootnoteReference,
Table,
TableCell,
TableRow,
// Extensions (front matter)
Yaml,
Comment on lines -15 to -39
Copy link
Member

@lumirlumir lumirlumir Nov 3, 2025

Choose a reason for hiding this comment

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

Originally, there was consensus not to use the RootContent type.

Currently, the Nodes type is equivalent to Root | RootContent, so I'm not sure that using Nodes here is the desired approach.

I personally prefer not to use the Nodes type and to keep the original enumeration style.

Ref: #334 (comment)

} from "mdast";
import type {
CustomRuleDefinitionType,
Expand All @@ -58,6 +34,25 @@ type WithExit<RuleVisitorType extends RuleVisitor> = {
| `${Key & string}:exit`]: RuleVisitorType[Key];
};

/**
* Compute the precise parent type for a given node `T`.
* - Root has no parent (returns `never`).
* - Custom frontmatter nodes (`toml` / `json`) only appear directly under `Root`.
* - For nodes, include every parent `P` where `T` is assignable to one of
* `P['children'][number]` (i.e. `T` can appear in `P.children`).
*/
type ParentOf<T extends MarkdownNode> = T extends Root
Copy link
Contributor

@DMartens DMartens Oct 29, 2025

Choose a reason for hiding this comment

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

I think this should be static for performance and simplicity reasons as the parent of the various markdown nodes should be easily groupable (inside paragraph (e.g. Strong and Emphasis) and root (e.g. Heading and Code).

Copy link
Member

@lumirlumir lumirlumir Nov 3, 2025

Choose a reason for hiding this comment

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

Agreed. Also, deeply nested conditional types make the code somewhat unclear to me.

? never
: T extends Toml | Json
? Root
: Parents extends infer P
? P extends Parent
? T extends P["children"][number]
? P
: never
: never
: never;

//------------------------------------------------------------------------------
// Exports
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -129,45 +124,19 @@ export interface MarkdownLanguageOptions extends LanguageOptions {
*/
export type MarkdownLanguageContext = LanguageContext<MarkdownLanguageOptions>;

export type MarkdownSyntaxElement = Node;

type MarkdownNode = Nodes | Json | Toml;

type MarkdownNodeVisitor = {
[Node in MarkdownNode as Node["type"]]: Node extends Root
? ((node: Node) => void) | undefined
: ((node: Node, parent: ParentOf<Node>) => void) | undefined;
};

export interface MarkdownRuleVisitor
extends RuleVisitor,
WithExit<
{
root?(node: Root): void;
} & {
[NodeType in
| Blockquote // Nodes
| Break
| Code
| Definition
| Emphasis
| Heading
| Html
| Image
| ImageReference
| InlineCode
| Link
| LinkReference
| List
| ListItem
| Paragraph
| Strong
| Text
| ThematicBreak
| Delete // Extensions (GFM)
| FootnoteDefinition
| FootnoteReference
| Table
| TableCell
| TableRow
| Yaml // Extensions (front matter)
| Toml
| Json as NodeType["type"]]?: (
node: NodeType,
parent?: Parent,
) => void;
}
> {}
Partial<WithExit<MarkdownNodeVisitor>> {}

export type MarkdownRuleDefinitionTypeOptions = CustomRuleTypeDefinitions;

Expand All @@ -178,7 +147,7 @@ export type MarkdownRuleDefinition<
LangOptions: MarkdownLanguageOptions;
Code: MarkdownSourceCode;
Visitor: MarkdownRuleVisitor;
Node: Node;
Node: MarkdownSyntaxElement;
},
Options
>;