Bug: [no-unnecessary-condition] does not report an error for typeof/instanceof or "in" typeguards

[kkmuffme]

Before You File a Bug Report Please Confirm You Have Done The Following...

• I have tried restarting my IDE and the issue persists.
• I have updated to the latest version of the packages.
• I have searched for related issues and found none that matched my issue.
• I have read the FAQ and my problem is not listed.

Playground Link

https://typescript-eslint.io/play/#ts=5.0.4&sourceType=module&code=PQKgBApgzgNglgOwC5gEQAEkE8AO0DGATnDkgLTTzLAID2ZArgghPtFAIaFZn60IATOEjj9UALjABtVBEKFahVAF0wIYAFgAUNjxgAtlgAquCGAC8YAN7awYAOQALCDBi17kqEmIIA5toBfAG5tbQAzJnwRfjAw2loACi5fSUMTPABKa1swODCwJMJfLKswYGAwJEcOJHsoWMQIUK07AOa7PIKnFzd7XIQwZJKyiqq4evGwQggBJgEOZHawNq0czoSAQgTu13d+waKM4fLKx0nJ6dnBBaQllZXwyOiBgCMuQpSwAFEANwhkEprfIffpeBZsWj5X7-JDHUZnCb1S5zG45Ow5B4tXLA3QQSEHXwWcyWey0F4AK1YtThp3OSJmKMWqyx9yAA&eslintrc=N4KABGBEBOCuA2BTAzpAXGYBfEWg&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA

Repro Code

type myType = {
  'hello': string
};

function foo(arg: myType) {
  if (arg) { // that's fine

  }

  if ('hello' in arg) { // this is redundant

  }

  if (!('hello' in arg)) { // this is redundant

  }
}

function bar(arg: Event) {
  if (arg instanceof Event) { // this is redundant
    
  }

  if (typeof arg === 'object') { // this is redundant

  }
}

### ESLint Config

```javascript
module.exports = {
  parser: "@typescript-eslint/parser",
  rules: {
    "@typescript-eslint/no-unnecessary-condition": ["error"],
  },
};

tsconfig

{
  "compilerOptions": {
    "strictNullChecks": true
  }
}

Expected Result

Errors in line 7, 11, 15, 21 and 25

Actual Result

Only error in line 7

Additional Info

No response

[bradzacher]

TypeScript types are a lower-bound, not an exact match. This means that the type {hello: string} doesn't mean "an object with exactly one property named hello whose type is string", it instead means "a value that has at least one property named hello whose type is at least string".

So this does create a little bit of weirdness - for example if you check for a property that isn't in the type - this isn't an unnecessary check and can actually execute at runtime!

However I do agree that we could detect redundant checks like "this property definitely exists in the type and is not optional".

---

In the same vein - an instanceof check might seem redundant, but actually again TS class types aren't nominal types - they're just a special way of writing an object type - so any class means "a value with at least these properties". Put another way it's actually valid to pass a plain object of the correct shape to a location expecting a class instance! So again that instanceof check might actually do something at runtime!!

So I don't think we should check instanceof checks.

---

Once more in the same vein - the result of a typeof check might seem like it's always going to be 'object', but there are a lot of cases where an "object type" actually will accept a non-object value!!!

For example you may assign any non-nullish value to {} as it means "a value with at least zero properties". And similarly you may assign any non-nullish value to {toString(): string}. And you may assign functions to the type {name: string}.

There are so many cases where the typeof can result in something you're not expecting which is why TS doesn't actually narrow the possible return strings for the operator. We won't be handling this case due to the complexity - if TS can't narrow it safely then we can't either.

Relevant issue microsoft/TypeScript#46399

[Josh-Cena]

Before microsoft/TypeScript#46399 is fixed, it would still be valuable to detect the following case, right?

declare const x: string/* possibly intersecting with other things */;

if (typeof x === "string");

[bradzacher]

We could potentially - but the thing I'm weary of (just like in that issue) is that we can handle number | boolean | string | bigint | symbol | undefined in the rule - but we cannot handle object | function types.

I'm also not 100% if it should exist in this rule or not, it feels a little weird because:

1. this rule is powered by the available types
2. this functionality could change at any time within TS and we'd duplicate their behaviour and have to gate it or remove it to avoid double-reporting.

Maybe a new rule like no-unnecessary-typeof-comparison or something?
I think a new rule would be good as we could consider also handling switch (which is a common way to check the typeof result) - eg error on cases the types say are unreachable.

[Josh-Cena]

FWIW, if TS lands narrower types for typeof, we won't be duplicating any report; we would simply be migrating our logic off the custom type comparison and using the simple assignability check. TS won't be reporting if (typeof x === "string") above under any setting anyway.

[bradzacher]

The difficult thing is that we'd need ad version-specific logic to support both versions because we support older versions of TS. It's all doable - just a maintenance burden for us.

[kkmuffme]

Thanks for the elaborate answer!

TS already reports an error when I assign a property that doesn't exist on it:
TS2339
https://typescript-eslint.io/play/#ts=5.0.4&sourceType=module&code=PQKgBApgzgNglgOwC5gEQAEkE8AO0DGATnDkgLTTzLAID2ZArgghPtFAIaFZn60IATOEjj9UALjABtVBEKFahVAF0wIYAFgAUNjxgAtlgAquCGAC8YAN7awYAOQALCDBi17kqEmIIA5toBfAG5tbQAzJnwRfjAw2loACi5fSUMTPABKa1swODCwJMJfLKswYGAwJEcOJHsoWMQIUK07AOa7ZIA6DigBfMt7AA8sAC97EK0cvIKnFzd7XIQwZJKyiqq4es2wQggBJgEOZHawNsmW3PyEgEIE2dd3ReWijNXyysdt7d39wSOkE5nM7hSLRJYAIy4hRSYAAogA3CDIEpTK7JRZeI5sWj5BFIpBvdafLb1H4Hf45Ow5YEXaYJXQQHHPXwWcwDWjggBWrFqhI+X1Je3Jx3OrUCQA&eslintrc=N4KABGBEBOCuA2BTAzpAXGYBfEWg&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA

Therefore the reverse isn't far fetched either?
Why would arg.asdf = 'xyz'; give an error (by TS natively) but if ('asdf' in arg) { not?

---

instanceof:
if a variable is type Event then chekcing instanceof Event doesn't make sense? Could you perhaps give me an example where you think it would be a false positive error?

---

typeof:
The issue isn't with "object" per se, but with typeof even in the simplest cases. E.g.

function bar(arg: string) {
  if (typeof arg === 'boolean') { // this is redundant

  }
}

[Josh-Cena]

Why would arg.asdf = 'xyz'; give an error (by TS natively) but if ('asdf' in arg) { not?

Because "asdf" in arg is implemented in TS as an infallible operation, as long as arg is an object. Rather, it narrows arg. For example, within the branch of if ('asdf' in arg), you will find that arg is typed as typeof arg & { asdf: unknown }.

if a variable is type Event then checking instanceof Event doesn't make sense? Could you perhaps give me an example where you think it would be a false positive error?

TypeScript is structurally typed.

class Something {
  a = 2;
}

const obj: Something = { a: 1 }; // Works because the object satisfies the shape of Something
if (obj instanceof Something) {
  console.log(obj);
} else {
  console.log(obj);
}

In this example, TS believes you go down the if branch, but you actually go down the else branch. instanceof is not type-safe.

The issue isn't with "object" per se, but with typeof even in the simplest cases

The "simplest case" is exactly what we are discussing above: typeof is only sound to the extent of dealing with primitives. Primitives are assignable to object types but not vice versa.

function bar(arg: { toString(): string }) {
  if (typeof arg === "string") {
    console.log(arg); // This is NOT redundant
  }
}

bar("foo"); // Works

[bradzacher]

Josh hit the nail on the head in all fronts!

To further expand on the typeof point - the toString case is the most obvious case where an object type isn't always an object.

But there's also the case of functions that make it impossible to truly narrow - eg

declare function acceptsName(arg: {name: string}): void;
acceptsName(function () {}); // no error!!!

declare function acceptsT(arg: {prop: string}): void;
acceptsT(function () {}); // correctly errors as expected

function foo() {}
namespace foo {
  export const prop = "";
}
acceptsT(foo); // no error!!!

typeof foo === "function"
// foo is a function with properties!

playground

TS is a lot looser than people believe. If you're really strict with your code then you can be pretty safe with it but it's easy to be unsafe with it.

For example the following is actually valid TS code that typechecks with no errors:

const totallyAString1 = {} as string;
const totallyAString2 = {length: 1} as string;

playground

Which is one of the reasons that the TS team allows "unnecessary" checks without reporting and they don't narrow typeof.

[kkmuffme]

To summarize: what is/would be possible to do here? If I understood it correctly the typeof and instanceof won't be possible, but the "in" might be? (basically when typescripts starts to create an intersection type typeof arg & { asdf: unknown } it's unnecessary?

[bradzacher]

Correct - instanceof and typeof are not possible because TS isn't sound for these cases.

However in we can handle, for sure.
Thinking more - I actually think this should instead be handled as a separate rule, which I proposed a while ago here: #5677.

There's a bit of nuance to in that would probably be better served as a separate rule so we can provide better configurability as well as better error messages.