feat(eslint-plugin): [ban-types] rework default options

[bradzacher]

BREAKING CHANGE:

Updated the config based on feedback:

• Removed fixer from object and Object
  • Updated the messages to be much clearer.
• Added {}
• Added Function

{
// ... config for lower case primitives ...

Function: {
  message: [
    'The `Function` type accepts any function-like value.',
    'It provides no type-safety when calling the function, which can be a common source of bugs.',
    'It also accepts things like class declarations, which will throw at runtime as they will not be called with `new`.',
    'If you are expecting the function to accept certain arguments, you should explicitly define function shape.',
  ].join('\n'),
},

Object: {
  message: [
    'The `Object` type actually means "any non-nullish value", so it is marginally better than `unknown`.',
    '- If you want a type meaning "any object", you probably want `Record<string, unknown>` instead.',
    '- If you want a type meaning "any value", you probably want `unknown` instead.',
  ].join('\n'),
},
'{}': {
  message: [
    '`{}` actually means "any non-nullish value".',
    '- If you want a type meaning "any object", you probably want `Record<string, unknown>` instead.',
    '- If you want a type meaning "any value", you probably want `unknown` instead.',
  ].join('\n'),
},

object: {
  message: [
    'The `object` type is currently hard to use (see https://github.com/microsoft/TypeScript/issues/21732).',
    'Consider using `Record<string, unknown>` instead, as it allows you to more easily inspect and use the keys.',
  ].join('\n'),
},
}

Fixes #842

[typescript-eslint]

Thanks for the PR, @bradzacher!

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

[glen-84]

I'm not sure if it makes sense to include object here. This rule has always been about object wrappers (of the pascal-case variety). A developer can always add object to the configuration if they really want it.

Also, as mentioned in #842, I'm not sure if fixing the Object case makes sense.

[bradzacher]

updated code and OP description. LMK your thoughts on this.

---

This rule has always been about object wrappers

This is not a correct statement.
The default options were just a set of sensible bans that could be applied to any and every codebase. The defaults existed to apply consistency to a codebase's types.

The fact that they were all "use camelCase instead of PascalCase" was purely coincidental.

[glen-84]

updated code and OP description. LMK your thoughts on this.

It's definitely an improvement, although I'm still not 100% sure about object. It would be good to hear some more opinions. 🙂

This is not a correct statement.

I meant that the defaults have always been about object wrappers, that's not to say that other objects cannot be used, just that it would be a (breaking, BTW) change to how things have been for quite a while now, and as I said, developers can always add this type to their configurations if they have no use for it in their codebases.

[JamesHenry]

I'm inclined to agree that this is a breaking change, so I've tagged it for v3 (which won't be miles away, we won't leave as big of a gap as we did between 1 and 2)

[eole1712]

@JamesHenry Maybe until then we can give a way to disable the default behaviour for Object type ? Should I do a PR for this?

[glen-84]

@bradzacher,

Have you considered adding Function and {} to the default configuration? (I know that the latter has only been possible in recent times.)

Function -> Prefer a specific function type, like () => void.
{} -> Every type except null and undefined is assignable to {}.

(no fixers)

[threehams]

Function has uses in conditional types and generic constraints, even if though it's incredibly confusing in other usages.

[bradzacher]

Yeah I haven't revisited this in a while, but both of those seem reasonable.

For context of the types here: I really want the default config to reflect things you shouldn't use 99% of the time, because the 1% is easily covered by disable comments, with a supporting comment explaining why it's a reasonable usage.

I would definitely argue that Function should be banned by default as well, as the vast, vast majority of the time you shouldn't be using it.

This is why Object and object are banned.

For Object/object, there should never be a time that you should use them. There is always a better type, and IMO it's really confusing to people that number is assignable to Object. Similarly, it can be pretty confusing to people that any function is assignable to Object/object.
I would say that the majority of the time that people use object/Object, they are saying "please provide an actual object", not "please provide an object-like value" (see examples below).

The problem is that I added a fixer with the original defaults, and I think there shouldn't be one for these types. "Basic" users probably want the fixer, as they meant an object type, but "advanced" users know what the type means, so they use it "correctly", so the fixer breaks their code. A very clear message explaining exactly what the types do is the best approach for them (same for {}).

function test_Record(obj: Record<string, unknown>) {
    obj.foo.toString(); // ✅ Expected Error - obj.foo is of type unknown

    if ('foo' in obj) {
        obj.foo.toString(); // ✅ Expected Error - obj.foo is of type unknown
    }

    if (typeof obj.foo === 'object' && obj.foo) {
        obj.foo.toString(); // ✅ No Error
    }

    if (obj instanceof Date) {
        obj.getDate(); // ✅ No Error
    }
}
function test_object(obj: object) {
    obj.foo.toString(); // ✅ Expected Error - Property 'foo' does not exist on type 'object'.

    if ('foo' in obj) {
        obj.foo.toString(); // ❌ Weird error - Property 'foo' does not exist on type 'object'.
    }

    if ('foo' in obj && typeof obj.foo === 'object' && obj.foo) { // ❌ Unexpected Error - Property 'foo' does not exist on type 'object'.
        obj.foo.toString(); // ❌ Unexpected error - same
    }
}
function test_Object(obj: Object) {
    obj.foo.toString(); // ✅ Expected Error

    if ('foo' in obj) {
        obj.foo.toString(); // ❌ Weird error - Property 'foo' does not exist on type 'object'.
    }

    if ('foo' in obj && typeof obj.foo === 'object' && obj.foo) { // ❌ Unexpected Error - Property 'foo' does not exist on type 'object'.
        obj.foo.toString(); // ❌ Unexpected error - same
    }
}

test_Record({}); // ✅ Expected No Error
test_object({}); // ✅ Expected No Error
test_Object({}); // ✅ Expected No Error

test_Record(1); // ✅ Expected Error
test_object(1); // ✅ Expected Error
test_Object(1); // ❌ Unexpected No Error

test_Record(true); // ✅ Expected Error
test_object(true); // ✅ Expected Error
test_Object(true); // ❌ Unexpected No Error

test_Record(null); // ✅ Expected Error
test_object(null); // ✅ Expected Error
test_Object(null); // ✅ Expected Error

test_Record(() => { }); // ✅ Expected Error
test_object(() => { }); // ❌ Unexpected No Error
test_Object(() => { }); // ❌ Unexpected No Error

repl

[glen-84]

and IMO it's really confusing to people that number is assignable to object.

Did you mean Object?

---

I'm still a bit unsure about object. When you say:

The only way to actually interrogate a variable typed with object is via an explicit type assertion

... I assume that you mean:

(obj as Date).getDate();

... but you can also use instanceof to narrow the type:

function test(obj: object) {
    if (obj instanceof Date) {
        obj.getDate();
    } else if (obj instanceof Location) {
        obj.hash;
    }
}

[bradzacher]

Sorry, I just realised I messed up my example because TS only reported one error per expression. I've updated it so it's correct.

But yes, Object is essentially "any non-null value", and object is essentially "any non-null, object-like value".

Object is almost always better represented as unknown, as IMO this is much clearer as to your intent, and forces you to use strict type assertions from the get-go.
object is almost always better represented as Record<string, unknown>, as it is much easier to work with (works with property checks (in etc), works with instance of), whilst still requiring strict type assertions on the properties.

I'm still a bit unsure about object. When you say ...

Instance of is definitely one way to refine it, but that only works when you have a well defined class structure, etc. But then if you're expecting a Date object, why wouldn't you just union in the Date type on your argument?

What I'm talking about expressly is the "I accept any object; anything that can use the string indexer and works correctly with Object.keys". If you ever want to refine the object down to have certain keys in it ('key' in obj, obj.key != null), it will not work - you have to explicitly cast it (see my example in the above comment).

[glen-84]

'foo' in obj doesn't work for object because of microsoft/TypeScript#21732.

I think the main difference between the two is that with Record<string, unknown>, you have access to any property with type unknown, which you can use to check for its existence and type.

It would be interesting to see actual use cases of both types.

But then if you're expecting a Date object, why wouldn't you just union in the Date type on your argument?

It was just a contrived example to show the usage of instanceof.

[Retsam]

Overall, personally, I wouldn't ban object.

---

object is a fairly niche type - there's relatively few cases where I actually think it's the right signature for a function (though it extends object is sometimes useful as a generic constraint) - but as far as I've seen in most cases where beginners use object or Object, the "correct" fix is either a more specific type that specifies the intended contract for the object, or a case where they should be using generics. Stuff like:

function noviceCode(x: object) {
   return (x as any).a + (x as any).b;
}

And Record<string, unknown> isn't any better for these cases, it's just as "niche" as object, I just don't think "arbitrary, non-primitive value, without any restrictions on its keys" is very a useful type (outside of generics). So a linter rule suggesting to replace object with Record<string, unknown> isn't particularly helpful in these cases. Here it might be more helpful to say "consider using a more specific type or a generic constraint", and not have a fixer.

---

The narrowing thing is a shame, but unknown doesn't narrow very well, either. (I think TypeScript#25720 is the most relevant issue).

In fact, it's specifically because object doesn't narrow well that unknown doesn't narrow well, as the first step in narrowing unknown to a specific type like {foo: string} is to narrow it to object. (e.g. typeof x === "object" && x)

I imagine this issue will get fixed eventually, (which would mean fixing object so it can narrow).

And using Record<string, unknown> instead is only a partial workaround for the narrowing issue; it works for flat objects, but doesn't help if you're trying to narrow to something like {foo: {bar: string}}, (as you'll run into the same issue when you try to narrow unknown to {bar: string}.

---

And in general, banning object seems to diverge from the best practices of the Typescript team - it's a fundamental part of the language: as typeof x === "object" narrows to object | null, and some of the built-in typings use it (Object.keys being the most prominent example). And I've never seen it recommended to be avoided like the other types banned by default.

The new (unreleased) handbook section on object, doesn't recommend avoiding it, and in fact specifically recommends it over Object or {}.

object is not Object. Always use object!

As I mentioned above, it's rare that I think object is the right type to use; I think banning object can be a useful linter rule, but IMO, that's a fairly opinionated decision, while I think banning the things the handbook specifically recommends avoiding - Object, String, Number, Boolean, (Symbol and Function aren't specifically mentioned in the handbook, but I think it's largely the same situation) - is unopinionated.

[bradzacher]

object is a fairly niche type there's relatively few cases where I actually think it's the right signature for a function

Which is the point I was making. 99.9% of the time, you don't want to use it, and the vast majority of codebases won't want to use it. IMO that makes it a great candidate for banning by default.
In the 0.1% of the cases, you can use a disable comment and clearly justify why you're using the type, or you can override the default config (should have a better config solution for the 3.0 release).

I agree though, that a fixer was the wrong approach. For both Object and object, I will change it to have a clear message as to why they're discouraged.

[bradzacher]

Note: I have updated the PR based on feedback.
LMK your thoughts on the new error messages.

[codecov]

Codecov Report

Merging #848 into v3 will decrease coverage by 0.03%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##               v3     #848      +/-   ##
==========================================
- Coverage   93.91%   93.87%   -0.04%     
==========================================
  Files         171      172       +1     
  Lines        7789     7804      +15     
  Branches     2240     2239       -1     
==========================================
+ Hits         7315     7326      +11     
- Misses        217      221       +4     
  Partials      257      257              

Flag	Coverage Δ
#unittest	93.87% <84.61%> (-0.04%)	⬇️

Impacted Files	Coverage Δ
...xperimental-utils/src/eslint-utils/applyDefault.ts	100.00% <ø> (ø)
.../experimental-utils/src/ts-eslint-scope/analyze.ts	100.00% <ø> (ø)
packages/eslint-plugin/src/util/objectIterators.ts	66.66% <66.66%> (ø)
packages/eslint-plugin/src/rules/ban-types.ts	100.00% <100.00%> (ø)
...ackages/eslint-plugin/src/rules/member-ordering.ts	98.46% <100.00%> (ø)