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

[JoshuaKGoldberg]

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

💖

[typescript-eslint]

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]

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	2508d84
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/665cbc9e868fb20008d1efd7
😎 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: 92 (🔴 down 6 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (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]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 2508d84. 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 11 targets

• nx run types:build
• nx test visitor-keys --coverage=false
• nx run-many --target=build --parallel --exclude=website --exclude=website-eslint
• nx run repo-tools:postinstall-script
• nx test parser --coverage=false
• nx test rule-schema-to-typescript-types --coverage=false
• nx test repo-tools --coverage=false
• nx test eslint-plugin-internal --coverage=false
• nx test typescript-estree --coverage=false
• nx test ast-spec --coverage=false
• nx run repo-tools:generate-configs

---

Sent with 💌 from NxCloud.

[JoshuaKGoldberg]

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 🙏

[kirkwaiblinger]

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 🤷

[bachmacintosh]

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

[JoshuaKGoldberg]

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.

[JoshuaKGoldberg]

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

[jakebailey]

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.

[bennycode]

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]

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

[JoshuaKGoldberg]

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

[JoshuaKGoldberg]

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

[kirkwaiblinger]

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 🤷

[kirkwaiblinger]

[bradzacher]

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.

[JoshuaKGoldberg]

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

[bradzacher]

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?

[JoshuaKGoldberg]

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.

[JoshuaKGoldberg]

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

[kirkwaiblinger]

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

[bradzacher]

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

[JoshuaKGoldberg]

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.

[Josh-Cena]

I think @bradzacher has given similar opinions on the code

[Josh-Cena]

FWIW I still don't fully entertain the rule name😢 I know I'm being super pedantic, but Function and Object aren't really "wrapper objects". Anything that involves "wrapper", "object", and "primitive" would sound off to me. I would only prefer names that involve "lower/uppercase", "intrinsic", and "structural".

[JoshuaKGoldberg]

OK THAT DOES IT! I'm splitting out a dedicated no-unsafe-function-type rule. 😂

Having an absurdly difficult time naming a rule is a strong indicator that the rule's trying to do too much. Even if the two areas (functions and objects) are conceptually very close, and/or implemented very similarly, having a clear user-friendly name is very valuable.