feat(eslint-plugin): [ban-types] allow banning null and undefined

[Validark]

Allows ban-types to block the null type.

Can be used like so:

types: {
  String: {
    message: 'Use string instead',
    fixWith: 'string',
  },
  null: {
    message: 'Use undefined instead',
    fixWith: 'undefined',
  },
};

[typescript-eslint]

Thanks for the PR, @Validark!

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

[bradzacher]

thanks for your contribution!

While you're at it, could you please do two things:

• add support for undefined as well (I see no reason to not to have mirrored support for TSUndefinedKeyword)
• add tests

stale

[octogonz]

My understanding is that ban-types is normally used to make code more consistent by completely prohibiting a type. For example, we can replace String with string and have more idiomatic code, without affecting the meaning. Or if the MD5 class had a known security vulnerability, we could ban it and require a replacement.

The null vs undefined debate is much more nuanced than this. Some background:

There's a reasonable case that null was an actual mistake in the original JavaScript language design: Most other programming languages only have one "null" or "nil" value, and it serves many purposes:

• As the default initial value for an uninitialized variable
• As the default value of an omitted optional function parameter
• In dynamic languages, as the value of x.y or x["y"] when the x object does not have any such key
• As a special token that developers can assign explicitly, if they want to specify an unknown state or mark the end of a linked list or whatever

In JavaScript, the undefined value has all of these behavioral properties, i.e. undefined has the same semantics as "null" or "nil" keywords in other programming languages. But then JavaScript confusingly also provides a second special token called null. The null token can only arise when a developer explicitly assigns it; null does not behave like "null" or "nil" in other languages, because it fails 3 of the 4 roles above.

And it's problematic to allow both null and undefined together. Code that has to deal with string | undefined | null has worse performance, is harder to read, and has lots of extra edge cases to test.

The obvious solution is to say "Okay, null was a mistake, so let's fix things by simply using undefined everywhere because it actually has all the desirable properties of a "null" or "nil" value."

But there's two obstacles to that plan:

• Some really ubiquitous system APIs such as JSON.parse() will return null, and it's too late to change them
• A bunch of JavaScript developers grew up observing that (1) the language designers seemingly blessed null by using it in the system APIs, and (2) the word "null" appears in many popular languages, whereas the word "undefined" is less common. These developers are understandably confused into believing that undefined is the bad one, and so they continue to proliferate APIs that return null.

Thus, we cannot categorically ban null in the way we banned String. We will always have to deal with annoying libraries that return null.

Instead, what we really want is a lint rule that prevents new introductions of null, but without requiring tons of lint suppressions wherever an external library returns null.

In other words:

// This should be a lint error:
const x: string | null = null;

// This should NOT be a lint error:
const y: string | null = JSON.parse('null');

That is what TSLint's no-null-keyword did, and I'm pretty sure it's what eslint-plugin-no-null does.

@Validark In this light, maybe this PR isn't needed? Or if there are TypeScript-specific aspects to the problem, perhaps we should create a separate no-null-keyword rule rather than trying to integrate this into ban-types?

[bradzacher]

I understand your argument, but I don't think it's a bad thing to support all named types - including type keywords - with this rule (except for any, because it lives outside this rule because it's so common).

I wouldn't use the option personally, because I haven't found that there's any lost clarity from using both.

I have seen codebases go the other way - never explicitly write out undefined, and only ever null - i.e. treat undefined as a JS/language level value, and null as a user level value.

---

no-null-keyword does only work on the null value, and not the type. And considering there is already eslint-plugin-no-null, we don't want to implement the tslint rule here (we want to avoid stepping on other plugin's toes where possible).

[octogonz]

Seems reasonable.

[octogonz]

I have seen codebases go the other way - never explicitly write out undefined, and only ever null - i.e. treat undefined as a JS/language level value, and null as a user level value.

I can see how someone might be able to ban this:

let x = undefined;

But how would they realistically be able to ban this?

let x: string | undefined = y.z;

As the PR is written currently, won't it apply to both forms?

[Validark]

In my free time I'm one of the main developers for a TypeScript to Lua compiler, which is basically our way of adding types (and promises, built-in functional methods, classes, extra operators) to otherwise regular Lua development. We want to completely ban null, and only ever plan on supporting undefined (we don't support vanilla TS). We can have a custom rule for banning null for our case if necessary. I already wrote one, hence I submitted this PR and the one that adds Literal as a bindable rule. I just wanted to submit this PR so other people won't get confused why ban-types doesn't work for any type one might define (null). If you don't want this functionality, I think it should error for types which it doesn't let you block, instead of silently not working.

@bradzacher requested that I add in similar functionality for undefined, probably more out of completeness than usability. If a person was testing it, you'd want it to work, even if nobody ever wants to block undefined.

[octogonz]

Weird, but reasonable! 😁

[bradzacher]

I've seen people do things to emulate flow's maybe operator (and other languages' maybe meta types):

// flow
const x: ?number; // type === number | null | undefined

// ts
type Maybe<T> = T | null | undefined;
const x: Maybe<number>;

In which case banning null and undefined makes sense, because you'd want people to use the defined generics instead.

At work I've pretty much never explicitly ever written the null or undefined types, instead we just use ?number and all our conditional checks are == null to cover both cases. It's a really nice way to write code - concise and easy to understand. It just takes some time to change the muscle memory for typing == null instead of === null.

[octogonz]

At work I've pretty much never explicitly ever written the null or undefined types, instead we just use ?number and all our conditional checks are == null to cover both cases. It's a really nice way to write code - concise and easy to understand. It just takes some time to change the muscle memory for typing == null instead of === null.

Interesting... But how would you write this without using null or undefined?

class TreeNode {
  public parent: TreeNode | undefined = undefined;
  public readonly children: TreeNode[] = [];

  public addChild(child: TreeNode): void {
    if (child.parent !== undefined) {
      throw new Error('Already has a parent')
    }
    this.children.push(child);
    child.parent = this;
  }

  public removeChild(child: TreeNode): void {
    const index = this.children.indexOf(child);
    if (index >= 0) {
      this.children.splice(index, 1);
      child.parent = undefined;  // <=== ??
    }    
  }
}

[bradzacher]

You'd need to write null | undefined exactly once in a TS codebase - to define the Maybe type:

// eslint-disable-next-line @typescript-eslint/ban-types
type Maybe<T> = T | null | undefined;

In flow you don't need to because ?T is built in and means the same thing.

class TreeNode {

// flow
public parent: ?TreeNode = undefined;
// ts
public parent: Maybe<TreeNode> = undefined;
// also valid ts but I hate it
public parent!: Maybe<TreeNode>;

Then your check is:

if (child.parent != null) {
// or
if (child.parent != undefined) {

Because == null will match both null and undefined.
I guess that we use null to do checks because it's shorter?

[bradzacher]

code LGTM. Thanks for your contribution!

[octogonz]

@bradzacher What about this line from my example:

    if (index >= 0) {
      this.children.splice(index, 1);
      child.parent = undefined;  // <=== ??
    }    

[bradzacher]

could use either null or undefined. With the change to the logic and types, they both have the same meaning.

If you really wanted to do it without referencing either, you could do delete child.parent, but that's weird, and I hate it. Plus it would break code that does 'parent' in child. Though I don't think I've ever seen someone do an in check for against an object backed by a class...

Or you could technically accomplish it with this combination, but optional members on classes are weird, and I hate it.

class TreeNode {
  public parent?: TreeNode;
  // ...
  public removeChild(child: TreeNode): void {
    // ...
    delete child.parent;
  }
}

[octogonz]

Thanks for taking the time to share these examples. It's somewhat obscure trivia, but very interesting to see all the different styles that developers use!

[JamesHenry]

Sorry for conflicts introduced by recent merges @Validark, looks like there might have been a build failure as well. Just let me know when this is synced back up and we'll get it merged, thanks a lot!

[JamesHenry]

Just following up again @Validark, barring the conflicts and build failure this is good to merge

[Validark]

Sorry for the delay. I will fix this as soon as I can. Just got a lot of code writing on my to-do list and haven't had another chance to fix this up. I will address this as soon as I can.

[Validark]

Sorry for taking so many months, haha. A lot has happened. Anyway, I updated this PR again to the latest master. I still can't get the tests to work, however. I really don't know what the problem is.

[bradzacher]

@Validark - the current CI step that's failing is code formatting.
run yarn format and then push the changes.

See CI output here:
https://dev.azure.com/typescript-eslint/TypeScript%20ESLint/_build/results?buildId=3263&view=logs&j=18ce2c0c-8e5f-561b-2a1a-c5027ed6632f&t=86003bb4-c15c-5206-371f-9fafed070759&l=28

[bradzacher]

LGTM - thanks for seeing this through.