Bug: [prefer-nullish-coalescing] unexpected fails on condition with generics variable which can be null or undefined

[zhangbenber]

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.2.2&fileType=.tsx&code=GYVwdgxgLglg9mABFApgZygHgCoD4AUAbgIYA2AXItogD6LgAmKwMYKDAlIgN4BQiiAE4ooIQUhKlEAQgC8s%2BmCYs2DRAH5EkxJQDkwOHF0BuXgF9evVBnxgQpUh2NA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Y6RAM0WlqYSNkAC1pkA9gEMkyMswDm6MAG1IvaOOiQAuuDABfEPqA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA&tokens=false

Repro Code

function test<T>(val: T | undefined) {
  return val !== undefined ? val : 'foo';
}

ESLint Config

{
  "rules": {
    "@typescript-eslint/prefer-nullish-coalescing": ["error"]
  }
}

tsconfig

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

Expected Result

I expect there's no errors reported.

Actual Result

I get the following error:

Prefer using nullish coalescing operator (`??`) instead of a ternary expression, as it is simpler to read. 2:10 - 2:41

And I get a Fix to nullish coalescing operator (??), which changes the logic of the function.

Additional Info

If I change the type of val to T | undefined | null, everything goes OK.

However, the generic T can also be null. The error does not disappear even if I make T extends null. In that situation, val !== undefined ? val : 'foo' is not equivalent to val ?? 'foo' and the fix may break the code if I call test(null).

The same thing also happens in the following code:

function test<T>(val: T | null) {
  return val !== null ? val : 'foo';
}
test(undefined);

[bradzacher]

In the most technical sense - yes, you are correct - T could be null, or it could be U | null.

It does seem pretty odd to me that any code written in this manner would be making an intentional and clear distinction between null and undefined. It would certainly be a niche edge-case.

Can you provide a little more context behind the code? Does the distinction matter?

[zhangbenber]

Can you provide a little more context behind the code? Does the distinction matter?

@bradzacher Consider that I have a Property class, which stores a field value in a property set, and provide the getter and setter for the user. It also has a default value, which is indicated by undefined in value.

class Property<T> {
  defaultValue: T;
  value: T | undefined; // undefined indicates default fallback

  constructor(defaultValue: T) {
    this.defaultValue = defaultValue;
  }

  set(value?: T | undefined) {
    this.value = value;
  }

  get() {
    // We focused on this line
    return this.value !== undefined ? this.value : this.defaultValue;
  }

  // ...
}

And we can use like this:

const httpRequestProperties = {
  host: new Property('localhost'),
  // Property<string>, which must be set and cannot be null

  port: new Property(80),
  // Property<number>, which must be set and cannot be null

  username: new Property<string | null>('root'),
  // Can be null, and null means an empty value (anonymous)

  password: new Property<string | null>('p@ssword'),
  // Can be null, and null means an empty value (anonymous)

  // We should not specify a default value for usernames and passwords, and it's only for demonstration

  // ...
}

Notice that we explicitly defined the string | null type for the generics of username and password field. If we want an anonymous access, we can:

httpRequestProperties.username.set(null);
httpRequestProperties.password.set(null);

and this will override their default values, and return null when get() method is called.

However, the code above gets the error:

Prefer using nullish coalescing operator (`??`) instead of a ternary expression, as it is simpler to read. 14:12 - 14:69

And if I accept the fix and change to:

  get() {
    return this.value ?? this.defaultValue;
  }

you may find the logic is broken. I will get 'root' when I call httpRequestProperties.username.get() even if it is set to null.

An attempt to solve this is to redefine the type of value to T | undefined | null, and now typescript-eslint is happy. But it changes the signature of the get() method, especially for these non-nullish fields.

class Property<T> {
  defaultValue: T;
  value: T | undefined | null; // We add the explicit `null`
  // ...

  get() {
    return this.value !== undefined ? this.value : this.defaultValue;
  }
}

const httpRequestProperties = {
  host: new Property('localhost'),
  // Property<string>, which must be set and cannot be null
  // ...
}

const host = httpRequestProperties.host.get();
typeof host // => `string | null`, but it should never be `null` here

And I must cast them into the right type manually, in the ungraceful manner.

In this situation to my view, null is part of the generic T and should be explicitly defined by the user. It is related to the user's usage and has the indication for the possibility of this generic being null. Defining T | null may break this indication, so that we get to the wrong way.

I will be happy if you want to fix this, or give any suggestions for this situation. Thank you!

[bradzacher]

personally I've found that adding meaning to the distinction between null vs undefined is generally a code smell given how easily undefined can creep in accidentally. If I were to design it I'd instead use a symbol to make it explicit that a value is supposed to be unset so that you can never accidentally fall into the default value case.

EG:

const EMPTY = Symbol();
class Property<T> {
  static readonly EMPTY = EMPTY;
  defaultValue: T;
  value: T | null | typeof EMPTY = EMPTY;

  set(value: T | typeof EMPTY) {
    this.value = value;
  }
  get(): T {
    return this.value === EMPTY ? this.defaultValue : this.value;
  }
}

That way you can also support cases like new Property<string | undefined>() without anything breaking unintentionally.

[zhangbenber]

Yes, I agree with you and that was what I exactly did to solve this. It's the perfect solution to this situation.

But this may have other shortcomings, not easy for serializing, for example. And I personally do not think distinction between null and undefined is unacceptable, since TypeScript does distinguish them (comparing to undefined vs void). JSON is another example, null and absence is different.

Or, could you report another type of rule violate for this case, or add some options to ignore this, as what the ignorePrimitives does? In this way one can prevent the dangerous fix action, especially for these auto-fixers.

[JoshuaKGoldberg]

Although I agree with Brad's points that this is some funky behavior, I also agree that it's technically a bug in the rule 😄. Accepting PRs.

Let's have the rule give a more specific error message because I think a lot of people get tripped up on type parameters being allowed to be null. TypeScript added a similar special casing in microsoft/TypeScript#30394.

[ulrichstark]

I've noticed a similar issue that's #7055 but with a generic function.
If the logical or operator is applied on two generic and potentially null function parameters, it will show a false positive if the function is called with boolean as type for generic.

Reproduction Sandbox:
https://typescript-eslint.io/play/#ts=5.3.2&fileType=.tsx&code=MYewdgzgLgBMCuAnRBTMUBqBDANvFAXDAEYgg4pZgwA%2BMY8OOMAvDAGa4QoDcAsAChQkWAAdUANwCWIeBGx5CJMhSq16jZmyiJ8-IeGgxpEKcQoL8rGAHMUmKafMpLKABQJkaTLnwAaGHEUaVl5XxQASh4YAHoYmABRAA9RFGAoFAATIhMzC3DrHXxBQXZ4MHSZajsHJ3zFAB4AFQA%2BDyRUdFciJvUGJgCgkLlumF66fpwInr7NGABvQRgYVCgkak9On0VaOiGZEfDouJgAflOYAHdZHEySFCvEcBsYAAsUVHvgLDkHqHfqLlnK5rJwcNxBABfIA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Y6RAM0WlqYSNkAC1pkA9gEMkyMswDm6KL2jjokcGAC%2BILUA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA&tokens=false

Do we need a flag like ignoreGenerics or is this a bug?