Skip to content
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

feat: Add rule types #110

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions packages/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
},
"homepage": "https://github.com/eslint/rewrite#readme",
"devDependencies": {
"json-schema": "^0.4.0",
"typescript": "^5.4.5"
},
"engines": {
Expand Down
366 changes: 365 additions & 1 deletion packages/core/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@
* @fileoverview Shared types for ESLint Core.
*/

//------------------------------------------------------------------------------
// Imports
//------------------------------------------------------------------------------

import { JSONSchema4 } from "json-schema";

//------------------------------------------------------------------------------
// Helper Types
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -75,6 +81,365 @@
*/
export type SourceRange = [number, number];

//------------------------------------------------------------------------------
// Rules
//------------------------------------------------------------------------------

/**
* What the rule is responsible for finding:
* - `problem` means the rule has noticed a potential error.
* - `suggestion` means the rule suggests an alternate or better approach.
* - `layout` means the rule is looking at spacing, indentation, etc.
*/
export type RuleType = "problem" | "suggestion" | "layout";

/**
* The type of fix the rule can provide:
* - `code` means the rule can fix syntax.
* - `whitespace` means the rule can fix spacing and indentation.
*/
export type RuleFixType = "code" | "whitespace";

/**
* An object containing visitor information for a rule. Each method is either the
* name of a node type or a selector, or is a method that will be called at specific
* times during the traversal.
*/
export interface RuleVisitor {

Check failure on line 108 in packages/core/src/types.ts

View workflow job for this annotation

GitHub Actions / Verify Files

A record is preferred over an index signature
/**
* Called for each node in the AST or at specific times during the traversal.
*/
[key: string]:
| ((node: unknown, parent?: unknown) => void)
| ((...unknown: unknown[]) => void);
}
Comment on lines +103 to +115
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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


/**
* Rule meta information used for documentation.
*/
export interface RulesMetaDocs {
/**
* A short description of the rule.
*/
description?: string | undefined;

/**
* The URL to the documentation for the rule.
*/
url?: string | undefined;

/**
* The category the rule falls under.
* @deprecated No longer used.
Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

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 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.

*/
category?: string | undefined;

/**
* Indicates if the rule is generally recommended for all users.
*/
recommended?: boolean | undefined;
}

/**
* Meta information about a rule.
*/
export interface RulesMeta {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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'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.

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg Sep 5, 2024

Choose a reason for hiding this comment

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

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;
  }
>

/**
* Properties that are used when documenting the rule.
*/
docs?: RulesMetaDocs | undefined;

/**
* The type of rule.
*/
type?: RuleType | undefined;

/**
* The schema for the rule options. Required if the rule has options.
*/
schema?: JSONSchema4 | JSONSchema4[] | false | undefined;

/**
* The messages that the rule can report.
*/
messages: Record<string, string>;

/**
* The visitor keys for the rule.
*/
visitorKeys?: Record<string, string[]>;
Comment on lines +167 to +170
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


/**
* The deprecated rules for the rule.
*/
deprecated?: boolean | undefined;

/**
* When a rule is deprecated, indicates the rule ID(s) that should be used instead.
*/
replacedBy?: string[] | undefined;

/**
* Indicates if the rule is fixable, and if so, what type of fix it provides.
*/
fixable?: RuleFixType | undefined;

/**
* Indicates if the rule may provide suggestions.
*/
hasSuggestions?: boolean | undefined;
}

/**
* Represents the context object that is passed to a rule. This object contains
* information about the current state of the linting process and is the rule's
* view into the outside world.
*/
export interface RuleContext {
/**
* The current working directory for the session.
*/
cwd: string;

/**
* Returns the current working directory for the session.
* @deprecated Use `cwd` instead.
*/
getCwd(): string;

/**
* The filename of the file being linted.
*/
filename: string;

/**
* Returns the filename of the file being linted.
* @deprecated Use `filename` instead.
*/
getFilename(): string;

/**
* The physical filename of the file being linted.
*/
physicalFilename: string;

/**
* Returns the physical filename of the file being linted.
* @deprecated Use `physicalFilename` instead.
*/
getPhysicalFilename(): string;

/**
* The source code object that the rule is running on.
*/
sourceCode: SourceCode;

/**
* Returns the source code object that the rule is running on.
* @deprecated Use `sourceCode` instead.
*/
getSourceCode(): SourceCode;

/**
* Shared settings for the configuration.
*/
settings: Record<string, unknown>;

/**
* Parser-specific options for the configuration.
* @deprecated Use `languageOptions.parserOptions` instead.
*/
parserOptions: Record<string, unknown>;

/**
* The language options for the configuration.
*/
languageOptions: LanguageOptions;

/**
* The CommonJS path to the parser used while parsing this file.
* @deprecated No longer used.
*/
parserPath: string;

/**
* The rule ID.
*/
id: string;

/**
* The rule's configured options.
*/
options: unknown[];

/**
* The report function that the rule should use to report problems.
* @param violation The violation to report.
*/
report(violation: ViolationReport): void;
}

// #region Rule Fixing

/**
* Manager of text edits for a rule fix.
*/
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;
Comment on lines +287 to +293
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor

Choose a reason for hiding this comment

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

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;

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg Sep 17, 2024

Choose a reason for hiding this comment

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

👍 if the RuleContext cares about the type of nodes, and RuleDefinition therefore does as well, then it makes sense that RuleDefinition would have a type parameter for the kind of nodes.

In other words: if some consumer of RuleContext/RuleDefinition wanted to write a function that takes one of those in...

// js version
function workOnContextNode(context: RuleContext) {
 context.sourceCode.ast.exampleProperty; // (or some equivalent)
}

...then TS users will want to be able to have that be well-typed without an as.

interface MyLanguageRootAST {
  exampleProperty: true;
}

function workOnContextNode(context: RuleContext<MyLanguage>) {
 context.sourceCode.ast.exampleProperty;
}

...or at least that's what I see this as. Is that the right context?


/**
* Inserts text after the specified range.
* @param range The range to insert after.
* @param text The edit to insert after the range.
*/
insertTextAfterRange(range: SourceRange, text: string): RuleTextEdit;

/**
* Inserts text before the specified node or token.
* @param nodeOrToken The node or token to insert before.
* @param text The edit to insert before the node or token.
*/
insertTextBefore(nodeOrToken: object, text: string): RuleTextEdit;

/**
* Inserts text before the specified range.
* @param range The range to insert before.
* @param text The edit to insert before the range.
*/
insertTextBeforeRange(range: SourceRange, text: string): RuleTextEdit;

/**
* Removes the specified node or token.
* @param nodeOrToken The node or token to remove.
* @returns The edit to remove the node or token.
*/
remove(nodeOrToken: object): RuleTextEdit;

/**
* Removes the specified range.
* @param range The range to remove.
* @returns The edit to remove the range.
*/
removeRange(range: SourceRange): RuleTextEdit;

/**
* Replaces the specified node or token with the given text.
* @param nodeOrToken The node or token to replace.
* @param text The text to replace the node or token with.
* @returns The edit to replace the node or token.
*/
replaceText(nodeOrToken: object, text: string): RuleTextEdit;

/**
* Replaces the specified range with the given text.
* @param range The range to replace.
* @param text The text to replace the range with.
* @returns The edit to replace the range.
*/
replaceTextRange(range: SourceRange, text: string): RuleTextEdit;
}

/**
* Represents a fix for a rule violation implemented as a text edit.
*/
export interface RuleTextEdit {
/**
* The range to replace.
*/
range: SourceRange;

/**
* The text to insert.
*/
text: string;
}

// #endregion

interface ViolationReportBase {
/**
* The type of node that the violation is for.
* @deprecated May be removed in the future.
*/
nodeType?: string | undefined;

/**
* The data to insert into the message.
*/
data?: Record<string, string> | undefined;

/**
* The fix to be applied for the violation.
* @param fixer The text editor to apply the fix.
* @returns The fix(es) for the violation.
*/
fix?(
fixer: RuleTextEditor,
): RuleTextEdit | RuleTextEdit[] | IterableIterator<RuleTextEdit> | null;

/**
* An array of suggested fixes for the problem. These fixes may change the
* behavior of the code, so they are not applied automatically.
*/
suggest?: Array<SuggestedEdit>;

Check failure on line 389 in packages/core/src/types.ts

View workflow job for this annotation

GitHub Actions / Verify Files

Array type using 'Array<SuggestedEdit>' is forbidden. Use 'SuggestedEdit[]' instead
}

type ViolationMessage = { message: string } | { messageId: string };
type ViolationLocation = { loc: SourceLocation } | { node: object };

export type ViolationReport = ViolationReportBase &
ViolationMessage &
ViolationLocation;

// #region Suggestions

interface SuggestedEditBase {
/**
* The data to insert into the message.
*/
data?: Record<string, string> | undefined;

/**
* The fix to be applied for the suggestion.
* @param fixer The text editor to apply the fix.
* @returns The fix for the suggestion.
*/
fix?(
fixer: RuleTextEditor,
): RuleTextEdit | RuleTextEdit[] | IterableIterator<RuleTextEdit> | null;
}

type SuggestionMessage = { desc: string } | { messageId: string };

/**
* A suggested edit for a rule violation.
*/
export type SuggestedEdit = SuggestedEditBase & SuggestionMessage;

// #endregion

/**
* The definition of an ESLint rule.
*/
export interface RuleDefinition<T extends RuleVisitor = RuleVisitor> {
Copy link
Member Author

@nzakas nzakas Aug 22, 2024

Choose a reason for hiding this comment

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

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.

/**
* The meta information for the rule.
*/
meta?: RulesMeta;

/**
* Creates the visitor that ESLint uses to apply the rule during traversal.
* @param context The rule context.
* @returns The rule visitor.
*/
create(context: RuleContext): T;
}

//------------------------------------------------------------------------------
// Config
//------------------------------------------------------------------------------
Expand All @@ -90,7 +455,6 @@
* - `0` means off.
* - `1` means warn.
* - `2` means error.
*
*/
export type SeverityLevel = 0 | 1 | 2;

Expand Down
Loading