Bug: [no-unnecessary-type-constraint] False positive for generics on arrow functions in .mts files

[spalger]

Before You File a Bug Report Please Confirm You Have Done The Following...

• I have tried restarting my IDE and the issue persists.
• I have updated to the latest version of the packages.
• I have searched for related issues and found none that matched my issue.
• I have read the FAQ and my problem is not listed.

Issue Description

When using the new (as of TS 4.7) .mts file extension the const foo = <T>() => ... syntax for generic arguments is reserved similar to JSX files. The suggestion from tsc recommends defining an explicit constraint:

Since my function doesn't need or nor do I want a type constraint, but I need one for the syntax, I reached for the noop constraint of extends any, but then I was violating the no-unnecessary-type-constraint rule.

In this case, with the .mts extension and it's limited syntax I think it's unfair to say it "doesn't do anything and is unnecessary".

Reproduction Repository Link

https://github.com/spalger/reproduce-issues/tree/ts-eslint-no-unnecessary-type-constraint

Repro Steps

1. clone the repo with --branch

   git clone git@github.com:spalger/reproduce-issues.git --branch ts-eslint-no-unnecessary-type-constraint

2. npm i

3. npm run lint

Versions

package	version
@typescript-eslint/eslint-plugin	5.33.0
@typescript-eslint/parser	5.33.0
TypeScript	4.7.4
ESLint	8.21.0
node	18.7.0

[JoshuaKGoldberg]

Somehow, I had a feeling this line of code would come back to haunt us 😄:

typescript-eslint/packages/eslint-plugin/src/rules/no-unnecessary-type-constraint.ts

Line 62 in 48c4ed6

const inJsx = context.getFilename().toLowerCase().endsWith('tsx');

Agreed, this is a bug. The rule should understand .cts and .mts files to need the constraint too.

[bradzacher]

We should probably somehow export the utils here to reuse them so we can share logic across packages?

typescript-eslint/packages/typescript-estree/src/create-program/getScriptKind.ts

Lines 4 to 48 in 399b3b1

	function getScriptKind(filePath: string, jsx: boolean): ts.ScriptKind {
	const extension = path.extname(filePath).toLowerCase();
	// note - we only respect the user's jsx setting for unknown extensions
	// this is so that we always match TS's internal script kind logic, preventing
	// weird errors due to a mismatch.
	// https://github.com/microsoft/TypeScript/blob/da00ba67ed1182ad334f7c713b8254fba174aeba/src/compiler/utilities.ts#L6948-L6968
	switch (extension) {
	case ts.Extension.Js:
	case ts.Extension.Cjs:
	case ts.Extension.Mjs:
	return ts.ScriptKind.JS;
	case ts.Extension.Jsx:
	return ts.ScriptKind.JSX;
	case ts.Extension.Ts:
	case ts.Extension.Cts:
	case ts.Extension.Mts:
	return ts.ScriptKind.TS;
	case ts.Extension.Tsx:
	return ts.ScriptKind.TSX;
	case ts.Extension.Json:
	return ts.ScriptKind.JSON;
	default:
	// unknown extension, force typescript to ignore the file extension, and respect the user's setting
	return jsx ? ts.ScriptKind.TSX : ts.ScriptKind.TS;
	}
	}
	function getLanguageVariant(scriptKind: ts.ScriptKind): ts.LanguageVariant {
	// https://github.com/microsoft/TypeScript/blob/d6e483b8dabd8fd37c00954c3f2184bb7f1eb90c/src/compiler/utilities.ts#L6281-L6285
	switch (scriptKind) {
	case ts.ScriptKind.TSX:
	case ts.ScriptKind.JSX:
	case ts.ScriptKind.JS:
	case ts.ScriptKind.JSON:
	return ts.LanguageVariant.JSX;
	default:
	return ts.LanguageVariant.Standard;
	}
	}

[viell-dev]

TS7060:

This syntax is reserved in files with the .mts or .cts extension. Add a trailing comma or explicit constraint.

A trailing comma should also be fine. Like so:

const func = <T,>(name: T) => {
  console.log(name);
}

Though if you're using a formatter, it might end up removing the trailing comma.

[pkyeck]

hey, I'm wondering what the status on this is? same happens with .cts files.

(type parameter) T in <T>(baseConfig: T, process: NodeJS.Process): T
This syntax is reserved in files with the .mts or .cts extension. Add a trailing comma or explicit constraint.ts(7060)

[bradzacher]

With any issue opened in this project — it either has visible progress in the form of an attached PR, or it has no progress.

We are a community-run project. The volunteer maintainers spend most of their time triaging issues and reviewing PRs. This means that most issues will not progress unless a member of the community steps up and champions it.

If this issue is important to you — consider being that champion.

If not — please just subscribe to the issue and wait patiently.
Commenting asking for status updates does not bump issue priority in any way and just serves to spam everyone subscribed to the issue.

[WoodyWoodsta]

@bradzacher This is all very well, but the reality is that these packages require significant amounts of context before approaching being the "champion".

This is an issue that's cropped up for me as well, and I've spent a fair amount of my time trying to understand why this issue even exists in the first place. I'm still not anywhere close to understanding what needs to change in order to fix.

Any help would be appreciated? I'm more than happy to open a pr.

If an issue is labelled good-first-issue, I'd expect either a rough sketch of the cause of the issue, or of the fix required, or it to be a relatively trivial issue.

[bradzacher]

@WoodyWoodsta Josh was pretty succinct in his analysis of the problem in the very first comment on this thread

.mts files are seen as JSX files - but the rule was written when .tsx files were the only JSX files - thus it gates the rule's JSX logic behind the file's extension - which is obviously now incorrect.

My second comment in this thread suggested a utility we could expose from another one of our packages to use instead of the current and incorrect check.

So the PR to fix this would be

1. expose the linked utility from typesript-estree - probably through the type-utils package.
2. replace the incorrect check with this utility that will behave correctly.

[WoodyWoodsta]

@bradzacher Appreciate the response - Josh was very succinct about the symptom, but to create the link between that and the actual problem wasn't as easy. Instead the member analysis was "this came back to bite us" and "We should probably somehow export the utils here..." which doesn't help anyone understand what you've crucially highlighted which is that somehow mts files are regarded as JSX files. I still don't understand how that's possible when the highlighted line is .startsWith('tsx') but I'm willing to replace that with the utility. Thank you for the input.

[bradzacher]

To clarify - in JSX files the single generic argument arrow function syntax (<T>() => {}) is considered "syntactically ambiguous" by TS - it needs something to disambiguate the <T> as a generic parameter definition as opposed to an opening JSX tag. That disambiguation can either be <T,> or <T extends something>.

Hence the correct fix in a JSX file is from <T extends any> to <T,>.

When TS introduced .mts files the decision was made that those files would be treated as JSX files by TS (off topic but I'd say that decision was made to avoid needing to introduce .mts, .mtsx, .cts, and .ctsx - they dropped the x and halved the number of new extensions added).

So hence there's a problem in the rule. The rule was built in a time when the only JSX file was .tsx - hence it gates its JSX logic behind the hardcoded extension .tsx. This hardcoded string is (as mentioned) no longer correct.

So the options to solve this are two fold:

1. add more hardcoded conditions for the new extensions and hope no new extensions are added in the future (bad).
2. remove the hardcoded check and use the same utility the parser uses to check if the file is a JSX file (good).

[WoodyWoodsta]

The background of JSX tags and generic argument syntax isn't new to me, that makes complete sense.

To clarify, is

typescript-eslint/packages/utils/src/eslint-utils/rule-tester/RuleTester.ts

Line 127 in 8ee0cbb

const filename = `file.ts${resolvedOptions.ecmaFeatures?.jsx ? 'x' : ''}`;

the actual source of the issue?

But I think I have enough understanding for a fix. Thank you!

[bradzacher]

@WoodyWoodsta the source of the issue is specifically the line of code Josh linked in the initial comment:

typescript-eslint/packages/eslint-plugin/src/rules/no-unnecessary-type-constraint.ts

Line 63 in 8ee0cbb

const inJsx = context.getFilename().toLowerCase().endsWith('tsx');

[WoodyWoodsta]

Sorry @bradzacher, my snippet I linked was from the jest rule tester, not the actual plugin code. Can't digest an entire monorepo in one day.

Let's continue the discussion in #6795

[JoseLion]

I'm seeing this issue on .tsx files but with some differences:

• Autofix does not seem to change const bar = <T extends any>(t: T): T => t; into const bar = <T,>(t: T): T => t;
• After changing to the correct fix, both the comma-dangle and comma-spacing rules report a problem.

Are these related, or should I open a separate issue?

Versions:

"@typescript-eslint/eslint-plugin": "^5.61.0",
"@typescript-eslint/parser": "^5.61.0",
"eslint": "^8.44.0",

Rules Config:

"@typescript-eslint/comma-dangle": ["error", "always-multiline"],
"@typescript-eslint/no-unnecessary-type-constraint": "error",
"comma-spacing": "error",

[bradzacher]

After changing to the correct fix, both the comma-dangle and comma-spacing rules report a problem.

Those are separate issues.