[ban-types] Conflict with no-explicit-any

[glen-84]

Repro

{
  "rules": {
    "@typescript-eslint/ban-types": "error",
    "@typescript-eslint/no-explicit-any": "error"
  }
}

createComponent(component: any, params: Object) {}

Expected Result

Error on params for ban-types, fixes to {} or suggests object, or at least something that doesn't include an explicit any.

Actual Result

Error on params for ban-types, fixes to Record<string, any> which conflicts with the no-explicit-any rule.

Additional Info

n/a

Versions

package	version
@typescript-eslint/eslint-plugin	1.13.0
@typescript-eslint/parser	1.13.0
TypeScript	3.4.5
ESLint	6.1.0
node	10.16.0
npm	6.9.0

[bradzacher]

It's not a good idea to use object/Object as it's an open ended type with zero type safety. On top of that it's not obvious from looking at it that it's not typesafe.
Record<string, any> is a much better type to fix to because it's obviously not typesafe due to the any.

Now a days, this should instead fix to Record<string, unknown> instead.
The rule was written before unknown existed, so any was the preferred type.

How come you're writing such a loosely typed function? There's almost always better options.

[glen-84]

It's not a good idea to use object/Object as it's an open ended type with zero type safety.

object is stricter than Object, as it excludes primitive types, so I'm not sure about "zero" type safety.

How is Record<string, any> (or unknown) better than object? It just seems unnecessarily verbose, and they both could be stricter, depending on the use case (and there are valid use cases for using object).

However, I'm wondering if any of these options are actually appropriate, since they narrow the type which could be a breaking change. Object allows primitives, while object, {}, and Record do not.

Would it not be best to simply not provide a fixer for this case, and rather just have it be a suggestion that the developer consider a more type-safe alternative?

Now a days, this should instead fix to Record<string, unknown> instead.

Again, more restrictive, but ultimately not a good type to use in most cases, so there's little point in fixing to it IMO.

How come you're writing such a loosely typed function? There's almost always better options.

It's not my code – I'm adding linting to an existing project. There are cases where methods are returning Observables, which are generic and require a type parameter (f.e. Observable<object>). The method isn't returning an observable of a specific type, it could be any object. The method itself could probably be made generic, but that's beside the point, and not something that this rule could suggest/fix.

[bradzacher]

In testing, it looks like object is a lot stricter than I originally thought, but it allows no keys at all, meaning it effectively narrows the type to something usable without an explicit type assertion.

const x: object = { a: 1 };

x.foo // error - key foo not in x
const key = 'a';
x[key] // error - no string index on x

if ('a' in x) {
  // typeof x === never
  x.a // error - key a not in type never
}

const y: { a: 1 } = x; // error - cannot assign {} to {a: 1}
const z = x as { a: 1 };

IMO Record<string, unknown> is a more usable type, because it allows you to do property access, whilst forcing you to specifically check the type of the returned keys:

const x: Record<string, unknown> = { a: 1 };

x.foo // typeof === unknown
const key = 'a';
x[key] // typeof === unknown

if ('a' in x) {
  x.a // typeof === unknown
}

const y: { a: 1 } = x; // error - cannot assign Record<string, unknown> to {a: 1}
const z = x as { a: 1 };

---

object is stricter than Object, as it excludes primitive types
Object allows primitives, while object, {}, and Record do not.

const y1: object = {};
const y2: object = new Map();
const y3: object = 1;      // error
const y4: object = '';     // error
const y5: object = false;  // error
const y6: object = [];

// no errors at all!
const x1: Object = {};
const x2: Object = new Map();
const x3: Object = 1;
const x4: Object = '';
const x5: Object = false;
const x6: Object = [];

I'm struggling to understand why you'd ever use Object, as it seems to functionally equivalent to unknown?
As in - it looks like it accepts any value in, but doesn't really let any value out (without a type guard).

edit - reading into it more, it looks like unknown is essentially type unknown = Object | null | undefined
So there is a slight difference between the two. But I'm not sure how the distinction matters, when there's no guarantee of the value's type?

---

If the observable requires something which you don't care about the value of, why not define the observable as having a default value for the generic Observable<T = Record<string, unknown>?
Ofc this doesn't work if that's a provided library type you don't have access to (in which case, ignore this comment).

[glen-84]

... meaning it effectively narrows the type to something usable without an explicit type assertion

Did you mean unusable?

IMO Record<string, unknown> is a more usable type ...

You might be right, but I think it's best to leave this decision to the developer, who may have specific reasons for choosing between the three options (or better, a more specific type).

I'm struggling to understand why you'd ever use Object, as it seems to functionally equivalent to unknown?

I never said that I wanted to use Object, in fact that's the purpose of this rule, that it's seldom an appropriate type. 🙂

So there is a slight difference between the two. But I'm not sure how the distinction matters, when there's no guarantee of the value's type?

Well if your theory is correct, the answer is there – the developer may not have wanted to allow null or undefined.

Ofc this doesn't work if that's a provided library type you don't have access to (in which case, ignore this comment).

Ya, it's an rxjs thing. If we make the method itself generic, then that could perhaps be the default type (but then it's probably best for it to actually not have a generic default).

[bradzacher]

Sorry by usable I mean that you can actually operate on, and use Record as an object, where as object cannot have anything done to it without an explicit as assertion:

const key = 'a';

const x1: object = { a: 1 };
x1.a // error key a not on x
x1[key] // error no index accessor on x
if ('a' in x1) {
  x1.a // error key a not on type never
}

const x2 = x1 as { a: 1 };
// now I can actually do the above things, but there's no type safety afforded by this cast, i.e.:
const x3 = ({ } as object) as { a: 1 }; // no errors

// compared to

const y1: Record<string, unknown> = { a: 1 };
y1.a // works with type unknown
y1[key] // works with type unknown
if ('a' in y1) {
  y1.a // works with type unknown
}

const y2 = y1 as { a: 1 };
// note that this unsafe explicit assertion still works without error, so this use case is no more safe
const y3 = ({ } as object) as Record<string, unknown>;

[glen-84]

Yes, I understood, but you were referring to object as usable instead of unusable. It's a minor thing, I was just making sure that I wasn't getting mixed up. I apologize, my brain reads things like a grammatical parser and throws errors when sentences seem to be contradictory. 😄

(here as well [personally /don't/ use]) 😛

[bradzacher]

woops - sometimes I forget to proof read my comments after I restructuring the sentences..

[ux-engineer]

No activity on this issue even though this is still open?

Shouldn't the autofix logic change Object to Record<string, unknown> instead of Record<string, any> (which causes a further type error)?

[glen-84]

@ux-engineer,

See #848.

The fixer will be removed.

It's considered to be a breaking change because object was also added. If that was split into a separate PR, then I don't think that the removal of the fixer for Object should be considered a breaking change.

[bradzacher]

We'll be planning to start on the 3.0 release soon™.
As the core maintainers are volunteers who provide their spare time for free, it's a question of finding time where we can all collaborate together so we don't block the release for too long.