Change Request: Make the `data` property stricter

[lumirlumir]

Which packages would you like to change?

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

What problem do you want to solve?

This request proposes tightening the SuggestedEditBase's data property, which is used by context.report.

rewrite/packages/core/src/types.ts

Lines 520 to 530 in d9a760d

	export interface SuggestedEditBase {
	/**
	* The data to insert into the message.
	*/
	data?: Record<string, unknown> | undefined;
	/**
	* The fix to be applied for the suggestion.
	*/
	fix: RuleFixer;
	}

In PR #298, the data property's type was widened from Record<string, string> to Record<string, unknown>.

However, I think this type is overly broad for the following reason:

---

All data properties can be stringified, but I'm not sure using unknown here is the best approach. I think it's too broad a type to accept.

Although Symbol, Function, and Object values can be stringified, the result is often meaningless to be used as a message and they aren't stringified like normal primitive values. For example:

console.log(String(Symbol("symbol")));

// Output: Symbol(symbol)

console.log(
	String({
		hi: {
			bye: {
				hi: "hi",
			},
		},
	}),
);

// Output: [object Object]

console.log(String(() => {}));

// Output: () => {}

For the following reason, I think using string | number | boolean | bigint, or adding another type if necessary, would be more appropriate here.

What do you think is the correct solution?

Narrow the type to string | number | boolean | bigint (or something similar) from unknown.

Participation

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

Additional comments

No response

[DMartens]

I agree that the type is too loose.
I think we can orient on possible JSON values and include possible nullish values: string | number | boolean | bigint | null | undefined.
Maybe we should also include arrays which have straightforward stringification but that stringification can be made explicit with a join(,).

[nzakas]

@lumirlumir can you update your description to include where data is used? There's nothing in the description that indicates the problem you're trying to solve or where one might see it. Right now we're left to click through to a PR, then to the diff, to understand what the problem might be.

Also, just a process note: @DMartens when you respond to an issue, it's considered that you are triaging it, which means that you need to move it through the Triage board to the appropriate spot. If you're waiting for feedback from others, then please move to "Feedback Needed" and at-mention the team.

[kirkwaiblinger]

Unsolicited opinion: the original type was correct; just require a string. This most clearly signals to consumers how the value is intended to be used and that there isn't different logic that kicks in when a different type is provided.

[DMartens]

@eslint/eslint-team We want to change the type of values which can be used as values in the data property of the argument of context.report. ESLint currently only uses the values for replacing placeholders specified in rule.meta.messages without special logic for stringification.
Current suggestions:

• be explicit (only string, was the value before the change to unknown)
• common values: string | number | boolean | bigint
• unknown gives the rule developer complete freedom (e.g. stringification of arrays)
• primitives (except Symbol): string | number | boolean | bigint | null | undefined

[lumirlumir]

Oops, I missed that the link had gone stale. I've updated the issue description.

Also, @DMartens's comment nicely describes what I wanted to suggest. :)

common values: string | number | boolean | bigint

Personally, my first choice is the one above.

be explicit (only string, was the value before the change to unknown)
primitives (except Symbol): string | number | boolean | bigint | null | undefined

I'm fine with the above as well if there's consensus.

unknown gives the rule developer complete freedom (e.g. stringification of arrays)

But, I'm not sure unknown is the desired approach here 🤔

[nzakas]

When making changes like this, we need be data-driven rather than opinion-driven. We don't want to change the type to something that will all of a sudden cause existing rules to indicate type errors even though they work as intended.

Here's the code that uses data:
https://github.com/eslint/eslint/blob/main/lib/linter/interpolate.js

We are doing a replacement of the exact value from data without coercing to a string. Coercion happens behind the scenes using the ECMAScript ToString operation (basically the String() function).

A quick search of the ESLint repo reveals several rules that are passing numbers as values in data:
https://github.com/eslint/eslint/blob/a2ee188ea7a38a0c6155f3d39e2b00e1d0f36e14/lib/rules/max-depth.js#L103
https://github.com/eslint/eslint/blob/a2ee188ea7a38a0c6155f3d39e2b00e1d0f36e14/lib/rules/max-len.js#L469-L472
https://github.com/eslint/eslint/blob/a2ee188ea7a38a0c6155f3d39e2b00e1d0f36e14/lib/rules/max-lines-per-function.js#L223

(Basically, any rule that begins with max- seems to do this.)

--
Given that, I think the original suggestion of string | number | boolean | bigint is most appropriate. This will allow already-existing rules to validate while discouraging the passing of objects that may or may not coerce to string correctly.

Important: We use this same type for both fixes and suggestions, so whatever we do, it should be the same in both places.