fix(eslint-plugin): [no-unnecessary-type-constraint] correctly fix in cts/mts files #6795

WoodyWoodsta · 2023-03-30T11:54:20Z

PR Checklist

Addresses an existing open issue: fixes Bug: [no-unnecessary-type-constraint] False positive for generics on arrow functions in .mts files #5449
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

Suggest comma disambiguation on .mts, cts as well as jsx variant files

typescript-eslint · 2023-03-30T11:54:23Z

Thanks for the PR, @WoodyWoodsta!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

WoodyWoodsta · 2023-03-30T11:57:17Z

I've specifically left out unit tests for this, as it doesn't appear that any of the fixtures for eslint-plugin include .mts or .cts files. It seems like something which should require test-wide/project-wide consensus. Please correct me if I'm wrong!

nx-cloud · 2023-03-30T11:57:52Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 1ab20aa. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 35 targets

Sent with 💌 from NxCloud.

WoodyWoodsta · 2023-03-30T12:09:38Z

I've noticed two things:

My changes don't address the problem (at least in my project via linking)
I can add tests as you can specify the filename on the RuleTester. Will do that now.

netlify · 2023-03-30T14:05:04Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`1ab20aa`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/64b492583501ff00083d9842
😎 Deploy Preview	https://deploy-preview-6795--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

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

WoodyWoodsta · 2023-03-30T14:13:43Z

@bradzacher I'm trying to understand why the proposition you made which was simply to use the getScriptKind util as a "fix", because getScriptKind still interprets .mts and .cts files as TS files, not TSX.

Regardless, there was no logic in the rule which checked for jsx/js, only the fix method. So I've added a new util to serve as a bypass for this rule. I have no idea if this is the correct approach: to me it doesn't seem elegant.

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

codecov · 2023-03-30T14:23:43Z

Codecov Report

Merging #6795 (1ab20aa) into main (d67fd6d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6795   +/-   ##
=======================================
  Coverage   87.47%   87.47%           
=======================================
  Files         379      379           
  Lines       13228    13234    +6     
  Branches     3905     3906    +1     
=======================================
+ Hits        11571    11577    +6     
  Misses       1279     1279           
  Partials      378      378

Flag	Coverage Δ
unittest	`87.47% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...plugin/src/rules/no-unnecessary-type-constraint.ts	`100.00% <100.00%> (ø)`

bradzacher

Apologies - it appears that I've lead you astray due to my misunderstanding of TS.
I've never actually used .mts for JSX, but I knew it errored on JSX-ambiguous syntax, so I had assumed it actually supported JSX.

In testing I now see that this isn't actually the case - .mts/.cts doesn't support JSX, it just errors on ambiguous syntax, and does not actually support JSX in the files. I assume that this just the TS team future-proofing - reading the design meeting notes it appears like they didn't want to lock in one-way or the other for the first release, so they just made sure they could make it JSX later if they wanted to by banning ambiguous syntax.

This obviously the reason why the getScriptKind function returns "TS" as the kind for the file instead of "TSX" - which was another thing I hadn't noticed in passing earlier.

Which provides a problem as you've noticed our suggested solution obviously won't fix it.

So what would be a good solution?
Now that I know what we know - I think we should just go back to hard-coding this. Something like this would do the job:

function requiresGenericDeclarationDisambiguation(filename: string) {
  const ext = path.ext(filename).toLowerCase();
  switch (ext) {
    case ts.Extension.Tsx:
    case ts.Extension.Cts:
    case ts.Extension.Mts:
      return true;

    default:
      return false;
  }
}

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

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

This reverts commit 5a5bc87. Revert "fix: detect new module syntax" This reverts commit 815b248. Revert "fix: correctly resolve file type" This reverts commit af6784e.

WoodyWoodsta · 2023-04-02T21:02:26Z

@bradzacher To sum up your comments and relate to the issue:

Original Issue

<T> requires disambiguation in mts, cts and JSX language variant files
<T extends any> as a disambiguation reports no-unnecessary-type-constraint in mts and cts files
rule fix, <T extends any> -> <T,>, is stripped of the dangling comma with at least prettier as a formatter returning it to the place of needing disambiguation

Fix

Only report no-unnecessary-type-constraint for <T extends any> in files which do not require disambiguation (everything other than mts, cts and JSX language variant files)

No changes to eslint fix required.

Let me know if this sounds correct.

Of course this could also be resolved in some way in formatters (i.e. to not strip the dangling comma here), but:

it's a matter of opinion which of <T,> and <T extends any> is "better"
stripping dangling commas from single-line argument lists except for in this case requires quite a lot of contextual awareness which I'm not sure whether these formatters have 🤷

bradzacher · 2023-04-02T21:20:50Z

rule fix, -> <T,>, is stripped of the dangling comma with at least prettier as a formatter returning it to the place of needing disambiguation

If prettier is stripping the syntactically necessary comma when formatting a .cts/mts file, then that's a prettier bug to be reported and fixed.

It has already been reported: prettier/prettier#14518

stripping dangling commas from single-line argument lists except for in this case requires quite a lot of contextual awareness which I'm not sure whether these formatters have

Formatters are powered by the AST - they know exactly and unambiguously that <T,>() => {} is an arrow function with a type parameter declaration because the parser has encoded that in the AST.

The formatter then decides how to print the code from the AST - if it decides to prints the type parameter on a single line, its trivial to also insert the comma.

bradzacher · 2023-04-02T21:23:15Z

Only report no-unnecessary-type-constraint for in files which do not require disambiguation (everything other than mts, cts and JSX language variant files)

This is not correct, no.

The rule should always report on the unnecessary extends any and the fix is to remove it in .ts files, and replace it with a comma in .tsx/cts/mts files.

The current rule behaviour is to always report - but the erroneous behaviour is that it removes it in .cts/mts files. Thus the fix is more or less to qualify those extensions as TSX files so that they are treated the same way, and receive a comma.

WoodyWoodsta · 2023-04-03T08:08:59Z

Formatters are powered by the AST

@bradzacher Ok you confirm what I thought. So if I understand correctly, there is actually nothing to do here in typscript-eslint? 😆

bradzacher · 2023-04-03T08:10:52Z

My last comment describes the change we need to make to the rule - we need to fix to <T,> in mts/cts files.

WoodyWoodsta · 2023-04-03T09:03:39Z

@bradzacher Hopefully I've now understood. Please see 6bfccc6

WoodyWoodsta · 2023-04-20T16:46:20Z

@bradzacher Any thoughts on the above?

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

JoshuaKGoldberg · 2023-07-16T23:49:49Z

@bradzacher is this still something you have time to review?

bradzacher

this LGTM!

fix: correctly resolve file type

af6784e

WoodyWoodsta mentioned this pull request Mar 30, 2023

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

Closed

4 tasks

fix: detect new module syntax

815b248

Josh-Cena reviewed Mar 30, 2023

View reviewed changes

packages/typescript-estree/src/create-program/getScriptKind.ts Outdated Show resolved Hide resolved

armano2 reviewed Mar 30, 2023

View reviewed changes

packages/eslint-plugin/src/rules/no-unnecessary-type-constraint.ts Outdated Show resolved Hide resolved

chore: refactor to isNodeNextModuleExtension

5a5bc87

bradzacher requested changes Mar 31, 2023

View reviewed changes

packages/typescript-estree/src/create-program/getScriptKind.ts Outdated Show resolved Hide resolved

bradzacher added the bug Something isn't working label Mar 31, 2023

bradzacher reviewed Mar 31, 2023

View reviewed changes

packages/eslint-plugin/src/rules/no-unnecessary-type-constraint.ts Outdated Show resolved Hide resolved

revert changes

58e2505

This reverts commit 5a5bc87. Revert "fix: detect new module syntax" This reverts commit 815b248. Revert "fix: correctly resolve file type" This reverts commit af6784e.

feat: widen comma fix filetypes

6bfccc6

WoodyWoodsta requested a review from bradzacher April 3, 2023 09:03

armano2 reviewed Apr 21, 2023

View reviewed changes

packages/eslint-plugin/src/rules/no-unnecessary-type-constraint.ts Outdated Show resolved Hide resolved

WoodyWoodsta added 2 commits April 21, 2023 16:37

Merge branch 'main' into 5449/fix-esm-extensions

cc21d45

chore: refactor 'node:path' to 'path'

ff2a403

WoodyWoodsta requested a review from armano2 April 21, 2023 15:41

fix: linting

3a04d71

bradzacher previously approved these changes Jul 17, 2023

View reviewed changes

Merge branch 'main' into 5449/fix-esm-extensions

1ab20aa

bradzacher dismissed their stale review via 1ab20aa July 17, 2023 00:59

bradzacher added the 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready label Jul 17, 2023

bradzacher approved these changes Jul 17, 2023

View reviewed changes

bradzacher changed the title ~~fix(eslint-plugin): [no-unnecessary-type-constraint] resolve jsx language variant correctly~~ fix(eslint-plugin): [no-unnecessary-type-constraint] correctly fix in cts/mts files Jul 17, 2023

bradzacher merged commit 1404796 into typescript-eslint:main Jul 17, 2023
48 of 49 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [no-unnecessary-type-constraint] correctly fix in cts/mts files #6795

fix(eslint-plugin): [no-unnecessary-type-constraint] correctly fix in cts/mts files #6795

WoodyWoodsta commented Mar 30, 2023 •

edited

typescript-eslint bot commented Mar 30, 2023

WoodyWoodsta commented Mar 30, 2023

nx-cloud bot commented Mar 30, 2023 •

edited

WoodyWoodsta commented Mar 30, 2023

netlify bot commented Mar 30, 2023 •

edited

WoodyWoodsta commented Mar 30, 2023

codecov bot commented Mar 30, 2023 •

edited

bradzacher left a comment

WoodyWoodsta commented Apr 2, 2023 •

edited

bradzacher commented Apr 2, 2023 •

edited

bradzacher commented Apr 2, 2023 •

edited

WoodyWoodsta commented Apr 3, 2023

bradzacher commented Apr 3, 2023

WoodyWoodsta commented Apr 3, 2023

WoodyWoodsta commented Apr 20, 2023

JoshuaKGoldberg commented Jul 16, 2023

bradzacher left a comment

fix(eslint-plugin): [no-unnecessary-type-constraint] correctly fix in cts/mts files #6795

fix(eslint-plugin): [no-unnecessary-type-constraint] correctly fix in cts/mts files #6795

Conversation

WoodyWoodsta commented Mar 30, 2023 • edited

PR Checklist

Overview

typescript-eslint bot commented Mar 30, 2023

WoodyWoodsta commented Mar 30, 2023

nx-cloud bot commented Mar 30, 2023 • edited

☁️ Nx Cloud Report

WoodyWoodsta commented Mar 30, 2023

netlify bot commented Mar 30, 2023 • edited

✅ Deploy Preview for typescript-eslint ready!

WoodyWoodsta commented Mar 30, 2023

codecov bot commented Mar 30, 2023 • edited

Codecov Report

bradzacher left a comment

Choose a reason for hiding this comment

WoodyWoodsta commented Apr 2, 2023 • edited

Original Issue

Fix

bradzacher commented Apr 2, 2023 • edited

bradzacher commented Apr 2, 2023 • edited

WoodyWoodsta commented Apr 3, 2023

bradzacher commented Apr 3, 2023

WoodyWoodsta commented Apr 3, 2023

WoodyWoodsta commented Apr 20, 2023

JoshuaKGoldberg commented Jul 16, 2023

bradzacher left a comment

Choose a reason for hiding this comment

WoodyWoodsta commented Mar 30, 2023 •

edited

nx-cloud bot commented Mar 30, 2023 •

edited

netlify bot commented Mar 30, 2023 •

edited

codecov bot commented Mar 30, 2023 •

edited

WoodyWoodsta commented Apr 2, 2023 •

edited

bradzacher commented Apr 2, 2023 •

edited

bradzacher commented Apr 2, 2023 •

edited