feat(eslint-plugin): replace ban-types with no-restricted-types, no-unsafe-function-type, no-wrapper-object-types #9102

JoshuaKGoldberg · 2024-05-15T23:36:32Z

BREAKING CHANGE:
Replaces rules in the recommended configs.

PR Checklist

Addresses an existing open issue: fixes Enhancement: [ban-types] Split into default-less no-restricted-types and more targeted type ban rule(s) #8978
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

Finally deprecates the venerable ban-types rule and adds three new rules:

🆕 no-restricted-types: allowing banning a configurable list of type names
🆕 no-unsafe-function-type: banning the built-in Function
🆕 no-wrapper-object-types: banning Object and built-in class wrappers such as Number

💖

…s rules

typescript-eslint · 2024-05-15T23:36:34Z

Thanks for the PR, @JoshuaKGoldberg!

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.

netlify · 2024-05-15T23:36:48Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`75543e9`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/666ab8d63628680008c109fd
😎 Deploy Preview	https://deploy-preview-9102--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (no change from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 90 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

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

nx-cloud · 2024-05-15T23:38:23Z

☁️ Nx Cloud Report

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

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 1 target

nx run repo-tools:postinstall-script

Sent with 💌 from NxCloud.

JoshuaKGoldberg · 2024-05-15T23:38:39Z

packages/eslint-plugin/docs/rules/no-uppercase-wrapper-types.mdx

I don't like this name 😦 but couldn't think of one I really do like... no-primitive-class-wrappers? no-class-wrapper-types? So wordy, but I also like ending with -types... Someone please help 🙏

TL;DR ban-bad-built-in-class-types or even just ban-bad-class-types

wordy? you must hate use-unknown-in-catch-callback-variable 😆

I guess the commonalities in this rule are, for type X flagged by the rule

X is built-in ("built-in", "intrinsic")

the types means "something which has X.prototype in its prototype chain". (gives us "class-type"/"object-type"/"prototype-type"/etc) <-- This makes the rule hard to name

that type is bad for some reason, but not necessarily the same reason...

in the case of BigInt, Boolean, Number, String, Symbol, because they refer to the boxed primitive, but the unboxed primitive is also assignable

Object, because it's terrible; any primitive is assignable, similar to previous

Function, because it's unsound <-- This makes the rule hard to name

Technically we could argue all of these are unsound, but Function is really a difference in kind compared to the others, so it's hard to justify using "unsound"/"unsafe" in the name

X is uppercase - technically true but not really the point 😄

So that would lead me to a rough template of {no/ban}-bad-{built-in/intrinsic}-{instance/prototype/class/object}-types of which my tentative favorite is ban-bad-built-in-class-types.

I'm not a fan of "wrapper" since Object and Function aren't wrappers. And same problem arises with using the word "primitive". If Function weren't part of the rule though I think no-{boxed/wrapped}-primitive-type would work nicely 🤷

👋 What about no-ts-wrapper-types ?

Pro: These types are all defined by TypeScript itself, and the list of banned types not only includes primitive wrappers, but also wrappers for object and a wide, any-ish function which are technically not primitives per MDN. The name indicates it's related to a TypeScript feature itself like ban-ts-comment does.

Con: Might be a little more confusing / need further explanation in the docs.

wordy? you must hate use-unknown-in-catch-callback-variable 😆

Haha, yes, I do! And now I'm realizing maybe we could have called it just use-unknown-in-catch-callback. Ugh.

Long rule names make for less readable/pronouncable editor popups & CLI complaints. They're actively (mildly) worse for users.

Some more suggestions from across the internet:

no-boxed-primitive-types (@molisani)

no-class-wraps (@bumpylumps)

no-class-wrapper-types (@bennycode)

no-object-instances-as-types (@gangsthub)

no-primitive-constructors (@niklas-wortmann)

no-primitive-wrappers (@davidlj95)

no-primordial-constructors (@boneskull)

no-upper-case-primitive-type (@bradzacher)

no-wrapper-types (@jakebailey)

no-wrapper-types or no-primitive-wrapper-types (@sandersn)

prefer-lowercase-intrinsic-type or prefer-intrinsic-type (@Josh-Cena)

prefer-primitive-type (-thingy) (@the-ult)

Some general thoughts...

"Primitive" would be something nice to include in there, but as @Josh-Cena pointed out, object isn't a primitive and so that's inaccurate

Moving Function bans into a separate rule (no-unsafe-call: ban calling Function #9108 (comment)) would help narrow down the naming

It'd be nice for naming to also move out Object bans into its own rule so we could then use "primitive" after all, but I don't know of any justifications on that one 😕

It's hard to justify using a term that isn't in common usage already. Words like "boxed" and "primordial" might (previously or presently) be good descriptors, but if they don't match MDN docs or common StackOverflow nomenclature, users will have a hard time with them

We don't generally put abbreviations in rule names anymore. It's hard enough for users to understand lint rules, let alone have to remember random abbreviations in there too.

I think it'd be nice to include some indication in the name that it's about types / type annotations, not generally using those things. E.g. ending with -types. That'd help disambiguate from other rules that do ban usage of some types (e.g. no-unsafe-* for anys).

That leaves a few finalists IMO:

no-class-wrapper-types

no-intrinsic-wrapper-types

no-intrinsic-class-wrapper-types

no-primitive-class-wrapper-types

prefer-lowercase-intrinsic-types: if we split out Function into its own rule, since lower-case function isn't a type

I'll continue to ruminate...

I personally find the mention of "class" to be very odd, especially given pages like https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number and https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/Number do not contain that string.

I can see how the term "class" could be confusing as it could be associated with limiting the behaviour in a real class. While reading through the MDN docs, I came across this:

When properties are accessed on primitives, JavaScript auto-boxes the value into a wrapper object and accesses the property on that object instead. For example, "foo".includes("f") implicitly creates a String wrapper object and calls String.prototype.includes() on that object. This auto-boxing behavior is not observable in JavaScript code but is a good mental model of various behaviors

Source: https://developer.mozilla.org/en-US/docs/Glossary/Primitive

Stating the above, would these names be acceptable?

no-wrapper-object-types

no-auto-boxing-types

@kirkwaiblinger what do you mean about Object not being a wrapper?

Oh, well, I think I meant the Object type doesn't conceptually refer to a wrapper around a primitive, though as you pointed out (and I wasn't aware) that is a subset of its possible uses. So, feel free to disregard my comment on that; my mind has been changed 🙂

I like no-wrapper-object-types a lot! "Wrapper" might be a bit general on its own, but the built-in upper-case types are the only ones I can think of that that jump to mind as "wrapper object types". Nice @bennycode!

Renaming now, but if anybody doesn't like this then please do continue the conversation. 🙁

Update: following #9102 (comment), I split out a no-unsafe-function-type. @Josh-Cena makes a good point that the function type isn't a wrapper.

That leaves the question of whether the no-wrapper-object-types name is ok to include Object. I.e. being ok with what @kirkwaiblinger mentioned: that it's not technically a wrapper itself. I'm in favor of allowing this: more convenience at the cost of slightly less pedantically correct rule name. But I could be convinced otherwise if we have a good rule name alternative and/or have justification to move Object into its own rule (do we?).

packages/eslint-plugin/docs/rules/no-uppercase-wrapper-types.mdx

kirkwaiblinger · 2024-05-16T06:30:10Z

packages/eslint-plugin/docs/rules/no-uppercase-wrapper-types.mdx

TL;DR ban-bad-built-in-class-types or even just ban-bad-class-types

wordy? you must hate use-unknown-in-catch-callback-variable 😆

I guess the commonalities in this rule are, for type X flagged by the rule

X is built-in ("built-in", "intrinsic")

the types means "something which has X.prototype in its prototype chain". (gives us "class-type"/"object-type"/"prototype-type"/etc) <-- This makes the rule hard to name

that type is bad for some reason, but not necessarily the same reason...

in the case of BigInt, Boolean, Number, String, Symbol, because they refer to the boxed primitive, but the unboxed primitive is also assignable

Object, because it's terrible; any primitive is assignable, similar to previous

Function, because it's unsound <-- This makes the rule hard to name

Technically we could argue all of these are unsound, but Function is really a difference in kind compared to the others, so it's hard to justify using "unsound"/"unsafe" in the name

X is uppercase - technically true but not really the point 😄

So that would lead me to a rough template of {no/ban}-bad-{built-in/intrinsic}-{instance/prototype/class/object}-types of which my tentative favorite is ban-bad-built-in-class-types.

I'm not a fan of "wrapper" since Object and Function aren't wrappers. And same problem arises with using the word "primitive". If Function weren't part of the rule though I think no-{boxed/wrapped}-primitive-type would work nicely 🤷

packages/eslint-plugin/docs/rules/no-uppercase-wrapper-types.mdx

Co-authored-by: Kirk Waiblinger <kirk.waiblinger@gmail.com>

kirkwaiblinger

bradzacher · 2024-05-27T02:03:31Z

packages/eslint-plugin/docs/rules/ban-types.mdx

@@ -15,6 +15,16 @@ It's often a good idea to ban certain types to help with consistency and safety.
 This rule bans specific types and can suggest alternatives.
 Note that it does not ban the corresponding runtime objects from being used.

+:::danger Deprecated
+This rule is deprecated and now split across several rules:


Why are we deprecating the rule?
There's still value in the rule - just that the default config can be empty now.

People use this rule all the time to ban various types across their codebase. It's the only way to reliably target type names only without hitting value names.

@bradzacher because it's essentially being renamed to no-restricted-types, with the default values split into more targeted rules. I'll update the deprecation notice to make it more clear.

Given this is a major - should we just remove the old rule name entirely?
Is there a benefit to keeping it if we're adding the new rules to recommended and the old rule should almost be a 1:1 name swap in config?

I was thinking this is a pretty user-known rule so leaving it in would help with migration. The name swap might be 1:1 but the behavioral change means figuring out which of the three new rules individual config entries / inline comments map to.

...heck, now that I post this, I'm liking deleting ban-types and just leaving a tombstone page. cc @typescript-eslint/triage-team - we can always add back the old deleted rule if trying out on community repos surfaces a lot of pain (#9141), right?

Makes sense to me too...
If we do that for this rule, should we just go ahead and do that for #8821, too, though?

@kirkwaiblinger we can land #8821 against main and then remove it in v8
similarly we could technically land this PR in main and then remove ban-types in v8 (which is what I originally suggested #8977 (comment)).

I.e. we could backport the new rules to v7 so they're available right now, deprecate ban-types, and then do the actual breaking change (removing ban-types) in v8.

I like it. Once this PR goes in I can send a followup to backport the changes to v7 / main, replacing the deletion with a deprecation.

packages/eslint-plugin/src/rules/ban-types.ts

packages/eslint-plugin/docs/rules/no-unsafe-function-type.mdx

packages/eslint-plugin/docs/rules/no-restricted-types.mdx

Josh-Cena · 2024-06-03T02:00:33Z

packages/eslint-plugin/src/rules/no-unsafe-function-type.ts

+    }
+
+    return {
+      TSClassImplements(node): void {


Thought: should the two new rules check implements and extends? You can't extend number or () => {} so the use of Number must either be intentional or refer to another type (since we still don't have scope analysis for this rule)

Someone could, in theory, be doing:

extends: interface MyFunction extends Function { __brand: 'my-function'; }

implements: class Weird implements Function { apply: ...; ... }.

...so for completeness, I think we need to?

Is that a bad idea? If they do it there's no reason they are doing it by accident or ignorance.

I'm not strongly opposed to removing this for simplicity. It does feel a little weird to me to leave in a known thing that nobody should really ever do - even if it's only doable with great intent.

Let's leave them in for now, but at least extends Function feels fine to me.

Josh-Cena · 2024-06-10T15:26:39Z

Leaving my verbal approval, pending some discussion but not a blocker since we have that behavior already

packages/eslint-plugin/docs/rules/no-unsafe-function-type.mdx

kirkwaiblinger

Some nits and stuff but looks good to me 👍

kirkwaiblinger · 2024-06-11T17:13:57Z

packages/eslint-plugin/docs/rules/no-wrapper-object-types.mdx

-Similarly, TypeScript's `Function` type refers to objects created with the `Function` constructor, i.e. `new Function("eval'd code with implicit args")`.
-`Function` allows being called with any number of arguments and returns type `any`.
-It's generally better to specify function parameter and return types with function type syntax.
+Using the primitives like `0` instead of object wrappers like `new Number(0)` is generally considered a JavaScript best practice.


For flow, you might link this and the next sentence with a conjunction such as "..., since ..." or something like that.

I like the idea but can't think of phrasing I like here to go with it. Do you have a suggestion?

@kirkwaiblinger I really want to get this in to unblock testing on community repos, so I'm merging now - but let's definitely revisit the docs before v8 goes stable!

packages/eslint-plugin/docs/rules/no-wrapper-object-types.mdx

packages/website/blog/2024-05-27-announcing-typescript-eslint-v8-beta.mdx

Co-authored-by: Kirk Waiblinger <kirk.waiblinger@gmail.com>

kirkwaiblinger

🔥🔥

bradzacher

My one feedback:
These two rules must use scope analysis to ensure the reference is to the global type, nor a local one.

Eg this code should be fine:

type T<Symbol> = Symbol;
type U<UU> = UU extends T<infer Function>
  ? Function : never;

Without scope analysis I believe the first case above will autofix to type T<Symbol> = symbol; and break the code!

I say this because we specifically received this feedback in the past from users but had rejected it because "ban-types is intentionally dumb and simple and just bans the name".
Given these rules are now targeted and toight - there's no reason we can't now add the scope analysis to be safe.

Otherwise LGTM!

Is this all documented in the v8 blog post?

packages/eslint-plugin/docs/rules/ban-types.md

bradzacher · 2024-06-12T04:58:40Z

packages/eslint-plugin/docs/rules/ban-types.md

@@ -0,0 +1,22 @@
+:::danger Deprecated


bradzacher

JoshuaKGoldberg · 2024-06-13T09:21:44Z

Merging this now with roughly resolved conversations (except some more docs work) to unblock #9141.

Note that this is into v8, not main - we can definitely clean things up & make any changes we need in v8 as needed before this goes stable!

feat(eslint-plugin): add no-restricted-types, no-uppercase-alias-type…

452fe6d

…s rules

JoshuaKGoldberg added the breaking change This change will require a new major version to be released label May 15, 2024

JoshuaKGoldberg added this to the 8.0.0 milestone May 15, 2024

JoshuaKGoldberg commented May 15, 2024

View reviewed changes

JoshuaKGoldberg mentioned this pull request May 15, 2024

Docs: Write blog post on the history of ban-types, {}, and object/Object #9103

Open

2 tasks

Fix lint issues

1791e4f

JoshuaKGoldberg requested a review from a team May 15, 2024 23:57

kirkwaiblinger reviewed May 16, 2024

View reviewed changes

Apply suggestions from code review

7c606fc

Co-authored-by: Kirk Waiblinger <kirk.waiblinger@gmail.com>

JoshuaKGoldberg mentioned this pull request May 24, 2024

no-unsafe-call: ban calling Function #9108

Closed

6 tasks

JoshuaKGoldberg added 5 commits May 24, 2024 05:43

Kirk suggestions

7734da7

yarn test -u

d1127f5

test: fix tests for useProgramFromProjectService

aae4be7

Rename to no-wrapper-object-types

c411cdb

excess myObject in docs

057b34a

JoshuaKGoldberg changed the title ~~feat(eslint-plugin): add no-restricted-types, no-uppercase-alias-types rules~~ feat(eslint-plugin): add no-restricted-types, no-wrapper-object-types rules May 26, 2024

JoshuaKGoldberg requested a review from kirkwaiblinger May 26, 2024 15:16

JoshuaKGoldberg added 2 commits May 26, 2024 17:48

Fix rules/index.ts

d4baaef

Merge branch 'v8' into no-ban-types-splitup

f25f333

kirkwaiblinger approved these changes May 27, 2024

View reviewed changes

kirkwaiblinger added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label May 27, 2024

bradzacher requested changes May 27, 2024

View reviewed changes

Clarify ban-types deprecation message

88bd835

JoshuaKGoldberg changed the title ~~feat(eslint-plugin): add no-restricted-types, no-wrapper-object-types rules~~ feat(eslint-plugin): replace ban-types with no-restricted-types, no-wrapper-object-types rules May 27, 2024

Remove ban-types altogether

874e97a

JoshuaKGoldberg requested a review from bradzacher May 27, 2024 15:17

chore: fix snapshot

22bec9d

Josh-Cena reviewed Jun 3, 2024

View reviewed changes

fix lint complaint

3858d65

JoshuaKGoldberg requested review from Josh-Cena, kirkwaiblinger and bradzacher and removed request for kirkwaiblinger June 3, 2024 11:33

JoshuaKGoldberg removed the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Jun 3, 2024

Address feedback

e078105

kirkwaiblinger reviewed Jun 11, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/no-unsafe-function-type.mdx Outdated Show resolved Hide resolved

kirkwaiblinger reviewed Jun 11, 2024

View reviewed changes

JoshuaKGoldberg and others added 2 commits June 12, 2024 03:52

Apply suggestions from code review

e22cbb9

Co-authored-by: Kirk Waiblinger <kirk.waiblinger@gmail.com>

Mention concepts and equality

8872bdc

JoshuaKGoldberg requested a review from kirkwaiblinger June 12, 2024 02:00

kirkwaiblinger approved these changes Jun 12, 2024

View reviewed changes

bradzacher approved these changes Jun 12, 2024

View reviewed changes

JoshuaKGoldberg added 2 commits June 12, 2024 22:47

fix: scope analysis

2e33cee

ban-types.md rule mentions

184e4ee

JoshuaKGoldberg requested a review from bradzacher June 12, 2024 20:50

bradzacher approved these changes Jun 13, 2024

View reviewed changes

JoshuaKGoldberg added 3 commits June 13, 2024 10:38

Merge branch 'v8'

cad7178

fix no-restricted-types docs test

7d73bf3

wrong list, kronk

75543e9

JoshuaKGoldberg merged commit 5cd80ca into typescript-eslint:v8 Jun 13, 2024
59 checks passed

JoshuaKGoldberg deleted the no-ban-types-splitup branch June 13, 2024 09:34

JoshuaKGoldberg mentioned this pull request Jun 13, 2024

Enhancement: [ban-types] Split into default-less no-restricted-types and more targeted type ban rule(s) #8978

Closed

kirkwaiblinger mentioned this pull request Jun 15, 2024

docs: update main with v8 blog post changes #9365

Merged

3 tasks

github-actions bot locked as resolved and limited conversation to collaborators Jun 25, 2024

feat(eslint-plugin): replace ban-types with no-restricted-types, no-unsafe-function-type, no-wrapper-object-types #9102

feat(eslint-plugin): replace ban-types with no-restricted-types, no-unsafe-function-type, no-wrapper-object-types #9102

Conversation

JoshuaKGoldberg commented May 15, 2024 • edited Loading

PR Checklist

Overview

typescript-eslint bot commented May 15, 2024

netlify bot commented May 15, 2024 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented May 15, 2024 • edited Loading

☁️ Nx Cloud Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bennycode May 24, 2024 • edited Loading

Choose a reason for hiding this comment

kirkwaiblinger May 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirkwaiblinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josh-Cena commented Jun 10, 2024

kirkwaiblinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirkwaiblinger left a comment

Choose a reason for hiding this comment

bradzacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradzacher left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Jun 13, 2024 • edited Loading

JoshuaKGoldberg commented May 15, 2024 •

edited

Loading

netlify bot commented May 15, 2024 •

edited

Loading

nx-cloud bot commented May 15, 2024 •

edited

Loading

bennycode May 24, 2024 •

edited

Loading

kirkwaiblinger May 25, 2024 •

edited

Loading

JoshuaKGoldberg Jun 2, 2024 •

edited

Loading

JoshuaKGoldberg May 27, 2024 •

edited

Loading

JoshuaKGoldberg Jun 10, 2024 •

edited

Loading

JoshuaKGoldberg commented Jun 13, 2024 •

edited

Loading