Skip to content

Typescript should warn when checking for null or undefined on non-nullable type #17896

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wizarrc opened this issue Aug 18, 2017 · 5 comments
Closed
Labels
Duplicate An existing issue was already created

Comments

@wizarrc
Copy link

wizarrc commented Aug 18, 2017

Proposal: Add an error or warning when strictNullCheck is turned on and a null check is applied to a non-null type.

Motivation: The developer is often times converting a code base that does not originally have strict null checking turned on. After turning it on, bugs are often revealed and require refactoring, moving where the types are null and not null in the code. When refactoring, it is often times difficult to spot redundancy in null checks. By informing the user with an error or warning, it allows the developer to easily spot where their logic is flawed.

TypeScript Version: 2.4.0

Code

function normalizeName(name: string): string {
    if(name === undefined) return ''; // Should warn that this is unnessary and suggest removing or adding ` | undefined` to name type
    if(name.length === 0) return name;
    return name[0].toUpperCase() + name.substr(1);
}

function alertName(name: string | undefined) {
    alert(normalizeName(name!));
}

(function () {
    alertName(undefined);
}());

Expected behavior:

This should give warning that null check is not needed or should be moved outside the inner function. This would allow me to spot where my error is and remove the ! operator with code like this:

function normalizeName(name: string): string {
    if(name.length === 0) return name;
    return name[0].toUpperCase() + name.substr(1);
}

function alertName(name: string | undefined) {
    alert(name === undefined ? '' : normalizeName(name));
}

(function () {
    alertName(undefined);
}());

Obviously this is a very simple example to demonstrate my point, but this can get much more challenging with a very large and deeply nested code base.

Actual behavior:

No warning or error is displayed.

@wizarrc wizarrc changed the title Non-nullable types should warn when checking for null or undefined on non-nullable type Typescript should warn when checking for null or undefined on non-nullable type Aug 18, 2017
@ghost
Copy link

ghost commented Aug 18, 2017

As a quick fix for now there is a lint rule: https://palantir.github.io/tslint/rules/strict-type-predicates/

@wizarrc
Copy link
Author

wizarrc commented Aug 18, 2017

@Andy-MS Thanks for the quick reply! I didn't see in the description where it warns for this behavior, but if it does I can see it being quite useful.

I think it should be baked into the language service itself being that it's less of a coding pattern and more of a coding contract feature like their example TypeScript won’t let you compare ‘1 === 2’.

Update I got it to work using TSLint (vnext) 0.0.4 in vscode. For some reason, I can't figure out how to pass the stable version of TSLint the types with my webpack project. Oddly enough, I had the option already set to true, but it never mattered since it never actually applied. Nice job on that new plugin model in tsconfig.js, it worked flawlessly the first time I tried it.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Aug 18, 2017
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Aug 18, 2017

Dupliate of #11920 / #14764

@wizarrc
Copy link
Author

wizarrc commented Aug 18, 2017

@Andy-MS since there is a working workaround, would you like me to close this issue? I'm not sure if there is a bigger issue about bringing important linting rules to TypeScript. I can leave it open and pivit the title if there isn't another issue covering this already.

@wizarrc
Copy link
Author

wizarrc commented Aug 18, 2017

@RyanCavanaugh missed that one. closing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

2 participants