feat: Add rule types

[nzakas]

Prerequisites checklist

• I have read the contributing guidelines.

What is the purpose of this pull request?

Add types for rules that will work regardless of the language being used.

What changes did you make? (Give an overview)

I added several types that let us completely type check rules. These types are as generic as possible to allow for language plugins to override or better define their types, too.

These are not guaranteed to match the types in @types/eslint, which are very specific to the JavaScript language. The intent is to define something that will work and then see if we can backport into @types/eslint in a way that still works with the JavaScript rules.

Related Issues

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

Does the naming make sense for the types? I tried to be more specific to avoid ambiguity (i.e., RuleFixer is now RuleTextEditor).

[nzakas]

Using a type parameter here allows language plugins to insert their own RuleVisitor interface for better type checking. I imagine we can turn the one in @types/eslint into a RuleVisitor so those definitions will still work.

This is really the only difference between rules for different languages.

[nzakas]

ping @eslint/eslint-tsc @JoshuaKGoldberg

[JoshuaKGoldberg]

Sorry for the delay, and thanks for the ping! I still don't feel very confident reviewing functionally (#73), but have made some educated inferences just for what already exists in ESLint.

[JoshuaKGoldberg]

😄 not a fan of the stylistic config? Any reason in particular to remove this?

From running locally, I see two complaints:

/Users/josh/repos/rewrite/packages/core/src/types.ts
  108:8   error  A record is preferred over an index signature                                        @typescript-eslint/consistent-indexed-object-style
  389:12  error  Array type using 'Array<SuggestedEdit>' is forbidden. Use 'SuggestedEdit[]' instead  @typescript-eslint/array-type

• @typescript-eslint/array-type: purely stylistic for consistency. If you prefer Array<...> that's available as an option.
• @typescript-eslint/consistent-indexed-object-style: it's meant to be purely stylistic, but there are some changes between interfaces and records. If you want to use catch-all string signatures the way RuleVisitor is set up now, I'd just disable the rule with a comment. Either inline or in the config altogether.

[nzakas]

Because I didn't think either of these warnings were helpful, and I didn't feel like fighting other rules that might come up (generally not a fan of applying rules blindly, especially ones that just seem to be enforcing stylistic preferences that I don't have).

[JoshuaKGoldberg]

The PRs in this repo are probably the heaviest users of : object I've seen this year 😄. I'm getting the impression types using them are split across two use cases:

• "Base" (generalized) types meant to be the constraints on type parameters, such as the current RuleVisitor
• Generic types where there's a clear injectable, shared type

For "Base" (generalized) types, it makes sense to use types like object and unknown. It's also common to name them something like Any* or *Base to make it clear that they're base types, not the ones end users directly use. Some of the types here are named like that, but some aren't - is that intentional?

For generic types, it'd be more typical to use a type parameter. That way consumers of the type can substitute in their intended types, instead of having to do some extends strategy. Which would be very inconvenient for types like this with a lot of : objects!

Is there anything blocking making this type generic?

Suggested change

	export interface RuleTextEditor {
	/**
	* Inserts text after the specified node or token.
	* @param nodeOrToken The node or token to insert after.
	* @param text The edit to insert after the node or token.
	*/
	insertTextAfter(nodeOrToken: object, text: string): RuleTextEdit;
	export interface RuleTextEditor<NodeOrToken> {
	/**
	* Inserts text after the specified node or token.
	* @param nodeOrToken The node or token to insert after.
	* @param text The edit to insert after the node or token.
	*/
	insertTextAfter(nodeOrToken: NodeOrToken, text: string): RuleTextEdit;

Note that the same logic could be extended to any place that includes : object or : unknown.

• ViolationLocation's node: which would make ViolationReport generic, which would make RuleContext generic.
• RuleContext's options
• RuleVisitor's node and parent - though that type looks like it might be more of a "Base" generalized type?

[nzakas]

In this case, there's only ever one instance of RuleTextEditor, which exists in the core and needs to work on everything...would that still be a case for a generic? 🤔

(Still trying to wrap my brain around this.)

[JoshuaKGoldberg]

Type parameters are for when you want to substitute out different parts of a type for different uses. If there's only ever one instance of something, then you likely don't need type parameters - but then the type for nodeOrToken should be known as something more specific than object, right?

If the type for nodeOrToken is always going to be some known thing, then a different suggestion could be:

Suggested change

	export interface RuleTextEditor {
	/**
	* Inserts text after the specified node or token.
	* @param nodeOrToken The node or token to insert after.
	* @param text The edit to insert after the node or token.
	*/
	insertTextAfter(nodeOrToken: object, text: string): RuleTextEdit;
	// or some existing type, or some union thereof...
	export interface EditableNodeOrToken {
	/* ... */
	}
	export interface RuleTextEditor {
	/**
	* Inserts text after the specified node or token.
	* @param nodeOrToken The node or token to insert after.
	* @param text The edit to insert after the node or token.
	*/
	insertTextAfter(nodeOrToken: EditableNodeOrToken, text: string): RuleTextEdit;

[nzakas]

To fully flesh out what we have here: this same RuleTextEditor is used inside the core for applying fixes to any language that ESLint is linting (via a Language object). So for JavaScript, it works on ESTree(ish) nodes, while for Markdown it works on Unist nodes. While these two types of nodes only shared a type, there is no guarantee that any particular language will have that.

So, a rule is passing some representation of a node (or token) into these methods on RuleTextEditor. ESLint itself doesn't really need to know what those are, because we just pass them back into Language#getRange() to get the range information for that node or token.

🤔 I suppose the audience for types here would be the rule developer, who may not be able to tell what to pass in. In that case, I could see it being helpful to specify the type as a parameter...but that would then bubble up into RuleContext and then up to RuleDefinition. Maybe that's not a bad thing in TypeScript land?

[JoshuaKGoldberg]

Similar to the note on RuleTextEditor below, messages and could be made generic. We do this in typescript-eslint's types so that rule tests have a type error reported if an incorrect message ID is used.

For reference, https://github.com/typescript-eslint/typescript-eslint/blob/2ad3404690cd601d1bbb80daa7f861ab5bb127e4/packages/utils/src/eslint-utils/RuleCreator.ts#L32-L38:

export interface RuleWithMeta<
  Options extends readonly unknown[],
  MessageIds extends string,
  Docs = unknown,
> extends RuleCreateAndOptions<Options, MessageIds> {
  meta: RuleMetaData<MessageIds, Docs>;
}

That Docs generic is something to consider here too. Plugins can define their own docs, and we found often want to have their own properties on top of ESLint's. The RuleMetadata type for docs is an & intersection of the provided Docs and the ESLint standard RuleMetaDataDocs. https://typescript-eslint.io/blog/announcing-typescript-eslint-v8#custom-rule-metadocs-types

[nzakas]

I'll need to look at this a bit closer. The code snippet you pasted above makes my head hurt -- I have a hard time reasoning about what it actually means.

[JoshuaKGoldberg]

Yeah we're not huge fans of how complicated it's gotten ourselves 😄. But such is the complexity we've had to go with. A general summary if it helps...

What's happening is that each RuleWithMeta has three "generic" parts that can be substituted in:

• What Options the rule has specified, which must be* a readonly array
• The MessageIds it can report, which must be* a string
• The shape of Documentation, which doesn't need to be explicitly specified - it defaults to unknown

*"must be" here is "assignable to": as in, whatever type we provide must satisfy that type:

• readonly unknown[] can be satisfied by, say, [] or [{ allowInterfaces: boolean; allowObjects: boolean; }]
• MessageIds can be satisfied by, say, "noEmptyInterface" | "noEmptyObject"

It then extends the interface RuleCreateAndOptions, which defines the standard ESLint create() method and typescript-eslint's defaultOptions. Plus then it adds a meta object of type RuleMetaData.

Putting it all together, this is a rough subset of @typescript-eslint/no-empty-object-type:

type NoEmptyObjectType = RuleWithMeta<
  // Options
  [
    {
      allowInterfaces: boolean;
      allowObjects: boolean;
    }
  ],

  // MessageIds
  "noEmptyInterface" | "noEmptyObject",

  // Docs
  {
    recommended: 'recommended';
    requiresTypeChecking: true;
  }
>

[JoshuaKGoldberg]

Err, what is this? Rules will have their own visitor keys now?

[nzakas]

Oops, that looks like a copy-paste error. Good catch!

[JoshuaKGoldberg]

@deprecated

A thought: it would be nice if these types don't need to include deprecated things... is there a strong need to include them?

For example: if these are meant to be used in ESLint core, but ESLint core wouldn't include them until the next major version, maybe anything meant to be dropped in that next major version can be omitted?

[nzakas]

I don't have a strong opinion, it just seemed like it would be helpful to see something rather than not so that you know it wasn't an accidental omission in the type.

[fasttime]

How will this interface be used? Will it be extended to declare language-specific interfaces where node and parent have a particular type, or will rules simply create and return a RuleVisitor object?

[nzakas]

It's designed to be extended but still work as a default. (see the RuleDefinition interface)

[fasttime]

I tried to declare an interface that extends RuleVisitor with something more specific, but it's not clear to me how the TypeScript syntax would look like (Playground link).

[fasttime]

A possible solution: we could use generics in place of the actual argument types, e.g.:

export interface RuleVisitor<NodeType = unknown, ArgsType extends unknown[] = unknown[]> {
	[key: string]:
		| ((node: NodeType, parent?: NodeType) => void)
		| ((...unknown: ArgsType) => void);
}

Implementors could specify those types explicitly:

interface MyRuleVisitor extends RuleVisitor<MyNode> {
	[key: string]: (node: MyNode, parent?: MyNode) => void;
}

or

interface MyRuleVisitor extends RuleVisitor<never, [string, number, boolean]> {
	[key: string]: (arg1: string, arg2: number, arg3: boolean) => void;
}

Playground link