Skip to content
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

disallow comparing to null and undefined unless they are valid cases in strict null mode #11920

Open
zpdDG4gta8XKpMCd opened this issue Oct 28, 2016 · 16 comments · May be fixed by #59559
Open
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Has Repro This issue has compiler-backed repros: https://aka.ms/ts-repros Suggestion An idea for TypeScript

Comments

@zpdDG4gta8XKpMCd
Copy link

zpdDG4gta8XKpMCd commented Oct 28, 2016

the rationale is the same as for --noUnusedLocals which in this case would be --noUselessNullChecks (i am not proposing a new flag, the existing --strictNullChecks flag should be used, this is for illustration only)

declare const value: number;
if (value !== null) { // <-- somehow valid, expected to be invalid, since number doesn't have null as a possible value
}
if (value !== '') { // <-- invalid as expected 
}
@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Oct 28, 2016
@RyanCavanaugh
Copy link
Member

We intentionally allow this for the sake of defensive programming (i.e. defending against missing inputs from non-TS code). If there's enough demand we could add a flag or something.

@Spongman
Copy link

Spongman commented Dec 9, 2016

i would suggest to make type safety the default, and have interop require explicit casting.

@DanielRosenwasser
Copy link
Member

Internal implementation detail: right now we use the comparability relationship for type assertions (casting) - if we did this, we'd need to have a separate type relationship (or ad-hoc check) for allowing you to cast undefined/null to any other type.

@wizarrc
Copy link

wizarrc commented Aug 18, 2017

#17896 (comment)

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

This seems to work good enough for me right now, but in the long run I would like to see this baked into the language.

@devoto13
Copy link

I would also like to have it implemented for the same reason as #14764: vulnerable refactoring. This proposal is a bit more broad, then #14764. Maybe they should be merged?

In the recent refactoring I've changed public key: string | null; to public key: string;. null was used to mark a special case and now it was decided to use particular string constant for that special case instead. Deep in the codebase there was a bunch of checks like:

if (myObject.key == null) {
  // perform some logic for the special case
}

This silently stopped working and took quite a bit of effort to debug. It was also surprising that this didn't fail type checking. Maybe do it as strictNullComparisons flag or something, so it is true by default under strict mode and then consumers willing to do defensive programming can opt out? This way people will get consistent (and expected) behaviour out of the box and those, who need to defend their code from JavaScript can do so as well 😄

@jfirebaugh
Copy link

I just encountered a bug similar to @devoto13. I was making some code work with --strictNullChecks that was trying to initialize a property declared as prop?: number with null. I figured that given the type declaration, the correct fix was to initialize with undefined instead. This caused a bug because other code did a strict-equality check foo.prop === null which was now always false. This feature would have flagged the comparison so that I would have known to change the type declaration to prop: number | null instead.

@pladaria
Copy link

pladaria commented Aug 13, 2020

We had bugs in our codebase due to this:

For example:

This is a bug:

if (typeof foo === undefined) { // this is impossible
}

it should be:

if (typeof foo === 'undefined') {
}

I know it is a dumb bug but shit happens and this is hard to notice, even with code reviews.

The typesystem shouldn't allow this

@softwareCobbler
Copy link
Contributor

i'd like to vote for a flag or something for this; just refactored a few things from function foo() : T | null to function foo() : T | undefined and plenty of post-use-site checks like result === null became silently borked. I saw no squiggles and thought "wow that refactoring went quick!" but then runtime failures got me.

@ugultopu
Copy link

You can use the no-unnecessary-condition rule from TypeScript ESLint for this.

@RA80533
Copy link

RA80533 commented Jun 8, 2021

Has opinion changed on this at all? I see that in 2016 it was of the opinion that allowing this behavior would be better for defensive programming when dealing with non-TS code.

Strangely enough, this behavior exists on the typeof operator. In other words, you can do the following without error:

 typeof null === null;

The snippet above is likely something a beginner might do if after they attempt to check it against "null" because the compiler will correctly present an error for that (playground):

// @errors: 2367

typeof null === "null";

@typescript-bot typescript-bot added the Has Repro This issue has compiler-backed repros: https://aka.ms/ts-repros label Jun 8, 2021
@c-vetter
Copy link

The rationale behind the current behavior is understandable. However, in my view it lends itself to sloppy typing. My strategy is to guard the boundaries where data comes in from unsafe (=non-TS) sources. And there, null and undefined need to be accounted for, but that is really no special case as far as typing is concerned. Rather, null and undefined are just necessarily possible value types. Based on that, strict typing makes a lot of sense.

Until today, I fully expected the behavior this issue calls for to be present when strict is active. Then I came across a case similar to the OP sample and wondered why there was no error message. I even checked the tsconfig to see if somebody had disabled strict or strictNullChecks.

Please add this behavior, possibly behind a new setting 🙂

@DanielRosenwasser

Internal implementation detail: right now we use the comparability relationship for type assertions (casting) - if we did this, we'd need to have a separate type relationship (or ad-hoc check) for allowing you to cast undefined/null to any other type.
Is this still a blocker? If yes:
How is this casting required? Isn't the problem an unintended non-distinction, which would be functionally equivalent to casting?

@RyanCavanaugh
This issue is marked Awaiting More Feedback. Can you say roughly what this issue would need to get some traction, how far off it is?

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 14, 2022

👋 Hi, I'm the Repro bot. I can help narrow down and track compiler bugs across releases! This comment reflects the current state of this repro running against the nightly TypeScript.


Comment by @RA80533

❌ Failed: -

  • This comparison appears to be unintentional because the types '"string" | "number" | "bigint" | "boolean" | "symbol" | "undefined" | "object" | "function"' and '"null"' have no overlap.

Historical Information
Version Reproduction Outputs
4.2.2, 4.3.2, 4.4.2, 4.5.2, 4.6.2

❌ Failed: -

  • This condition will always return 'false' since the types '"string" | "number" | "bigint" | "boolean" | "symbol" | "undefined" | "object" | "function"' and '"null"' have no overlap.

@bradenhs
Copy link

bradenhs commented Nov 3, 2023

Since this issue is "awaiting more feedback" I'd just like to add my feedback that allowing comparison to null for non-nullable types in strict mode is far from intuitive behavior. I've encountered bugs that would've been prevented had the compiler caught these type of impossible comparisons to null. I would love to see an additional flag for this if adding this behavior to strictNullChecks would be too disruptive to existing code.

@ersjim
Copy link

ersjim commented Apr 2, 2024

Just had an infinite loop that lead me to this issue. I was surprised to find out that I can compare a string to null.

const tokenStream = new Tokenizer(input);
while (tokenStream.peek() !== null) { // will never happen! Returns "" instead of null at EOF.
  processNextToken(tokenStream.next());
}

The problem is I was assuming that tokenStream.peek would return null at the end of the file, but it instead returns an empty string. The return type of the method is a non-nullable string. I really feel like the type system should have caught this before bricking my computer and making me hard reset.

@skondrashov
Copy link

skondrashov commented May 1, 2024

Also adding that I ran into a case where I was allowed to compare a Date to null and was confused why typescript was allowing it.

no-unnecessary-condition from typescript eslint didn't help me either, would really like to know how I can enable type checking for this, or to have that feature added if it's not possible currently.

@lauraharker
Copy link

We also ran into a problem with

if (typeof foo === undefined) { // this is impossible
}

recently - would there be any way to disallow special case expressions that are definitely, syntactically, not null/undefined like typeof foo? I realize that expressing this in the type system itself might get too complicated, but would it be reasonable to have a purely syntactic check?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Has Repro This issue has compiler-backed repros: https://aka.ms/ts-repros Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.