Change Request: Update RuleVisitor interface

[nzakas]

Which packages would you like to change?

• @eslint/compat
• @eslint/config-array
• @eslint/core
• @eslint/migrate-config
• @eslint/object-schema
• @eslint/plugin-kit

What problem do you want to solve?

The RuleVisitor interface is currently defined as follows:

export interface RuleVisitor {
	/**
	 * Called for each node in the AST or at specific times during the traversal.
	 */
	[key: string]: (...args: any[]) => void;
}

However, this is causing problems when trying to extend RuleVisitor under strict TypeScript mode, as seen in these PRs:

• fix: Use updated types from @eslint/core json#66
• fix: Bring types in sync with @eslint/core eslint#19157

The errors take the form of the following:

error TS2411: Property 'Document' of type '((node: DocumentNode$1) => void) | undefined' is not assignable to 'string' index type '(...args: any[]) => void'.

What do you think is the correct solution?

The problem is that the method defined in RuleVisitor is not optional.

I can see a few different options:

1. Remove all methods. We can remove any predefined methods from RuleVisitor and leave essentially a blank interface:

export interface RuleVisitor {
	/**
	 * Called for each node in the AST or at specific times during the traversal.
	 */
-	[key: string]: (...args: any[]) => void;
}

That would give consumers something to extend from so that it logically looks like there's some relationship between the core types and consumer types. I'm not sure it's valuable.

2. Convert to a mapped type. We can change it to a mapped type as suggested by @fasttime here:

- export interface RuleVisitor {
+ export type RuleVisitor = {
	/**
	 * Called for each node in the AST or at specific times during the traversal.
	 */
-	[key: string]: (...args: any[]) => void;
+	[key in string]?: (...args: any[]) => void;
}

Defining all methods to be optional also seems to make RuleVisitor mostly useless.

3. Eliminate RuleVisitor interface completely. Because we don't know what the visitor will look like for any particular language, maybe it's a bad idea to try and enforce anything in particular. We could just define the RuleDefinitionTypeOptions#Visitor as Object, which would allow any non-primitive type to be used, and then we don't need RuleVisitor at all:

export interface RuleDefinitionTypeOptions {
	LangOptions: LanguageOptions;
	Code: SourceCode;
	RuleOptions: unknown[];
-	Visitor: RuleVisitor;
+	Visitor: Object;
	Node: unknown;
	MessageIds: string;
	ExtRuleDocs: unknown;
}

I think this is my favorite approach, but I'd like some insights from @JoshuaKGoldberg and @fasttime on all of the options and their ramifications.

Participation

• I am willing to submit a pull request for this change.

Additional comments

No response

[JoshuaKGoldberg]

The problem with Visitor: object is that it allows any object to be passed in. So you could pass { value: "gotcha!" } and the types would be fine with it.

I poked around a bit in eslint/json and think the [key in string]?: mapped type option is where I'd go. It makes sense as treating the root issue:

1. Per strictNullChecks, null and undefined can't be used in places that don't say they can take them in (common phrasing: are a nullable union)
2. The current interface RuleVisitor explicitly says that for any given key, the value must be a non-nullable function
3. Rule visitors such as JSONRuleVisitor are saying that for some keys, the value may be undefined

Mismatch!

As I understand it, *RuleVisitor interfaces aren't meant to say that all defined keys must exist. They define the set of allowed keys and then implementers can choose some of them to exist, right?

[fasttime]

Because we don't know what the visitor will look like for any particular language, maybe it's a bad idea to try and enforce anything in particular. We could just define the RuleDefinitionTypeOptions#Visitor as Object, which would allow any non-primitive type to be used

I think it's object (with a small o) that can be assigned any non-primitive type, whereas Object allows any value other than null or undefined in TS strict mode, or any value in non-strict mode (Demo).

Apart from that it isn't clear to me how a Visitor would work if its properties aren't functions. Currently ESLint uses property keys of a Visitor object as selectors and property values as listeners. The listeners are called at some point:

const ruleListenerReturn = ruleListener(...listenerArgs);

https://github.com/eslint/eslint/blob/01557cec24203be72222858a3912da0a474ac75c/lib/linter/linter.js#L1098

So I was under the impression that the expectation was for a Visitor to be an object with callable functions as property values. Are you planning to change the way Visitors work?

[nzakas]

@JoshuaKGoldberg

As I understand it, *RuleVisitor interfaces aren't meant to say that all defined keys must exist. They define the set of allowed keys and then implementers can choose some of them to exist, right?

That's correct. So for JSONRuleVisitor, we want to define all of the known possible methods that rules could use but rule authors are not required to implement any of them.

@fasttime

So I was under the impression that the expectation was for a Visitor to be an object with callable functions as property values.

That's correct. The RuleVisitor interface is only really intended for use by language plugins authors, who will in turn extend that type to define any actual methods that the language will accept on rule objects. If a rule returns an object with non-function key values, that will result in an error if the key is an expected method name or otherwise it will be ignored.

[nzakas]

It looks like you're both in the mapped type camp, so put together a PR here:
#135