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

raise a type error on attempt to strict-equal-compare a value of string | null to undefined #14764

Open
zpdDG4gta8XKpMCd opened this issue Mar 21, 2017 · 8 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@zpdDG4gta8XKpMCd
Copy link

zpdDG4gta8XKpMCd commented Mar 21, 2017

the following examples are almost certainly bugs:

declare var one: string | undefined;
// expected a type error: Operator '===' cannot be applied to types 'string | undefined' and 'null'
// actual no problem
if (one === null) { 
   // unreachable, because one can't ever be null, undefined was likely meant instead
}
declare var another: string | null;
// expected a type error: Operator '===' cannot be applied to types 'string | null' and 'undefined'
// actual no problem
if (another === undefined) {
   // unreachable, because one can't ever be undefined, null was likely meant instead
}

consider raising a type error for the above situations

@zpdDG4gta8XKpMCd zpdDG4gta8XKpMCd changed the title raising a type error on attempt to compare a value of T | null to undefined raise a type error on attempt to strict-equal-compare a value of string | null to undefined Mar 21, 2017
@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Mar 21, 2017
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Mar 21, 2017

Interested in your thoughts here. We special-cased comparisons against null and undefined specifically for defensive checks:

// TypeScript callers: Don't give me 'undefined'
function fn(x: string) {
  // JS callers: Read the docs please
  if (x === undefined) throw new Error('x cannot be undefined');
}

I think that part is non-negotiable -- it should be "cheap" to check for those sorts of errors, by some means or another.

Checking a null against string | undefined does feel really suspicious to me. Seems like once you've got one sentinel non-value in there, looking for the other is not what you meant. I could see C# programmers especially writing code like this

function fn(x?: string) {
  if (x === null) { x = "some default" } // This is how it works, right?
}

One concern I have is that strictNullChecks disallows the assertion you'd want to write if you were doing this on purpose:

if (another === <string>undefined) { // Error, can't convert undefined to string

Perhaps a type assertion from null or undefined to any type should always succeed.

@zpdDG4gta8XKpMCd
Copy link
Author

zpdDG4gta8XKpMCd commented Mar 21, 2017

i am not questioning #11920 here, this is what i've been through today though, i had:

interface Data { value: number | null; }
declare var data: Data;
if (data.value === null) {
   // do things
}

then i refactored Data to

interface Data { value: number | undefined; }

and

if (data.value === null) {
   // do things
}

hasn't been caught, bang! a problem

so it wasn't a silly comparison by choice, just something vulnerable to refactoring that typescript can't see

@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature and removed In Discussion Not yet reached consensus labels Apr 3, 2017
@nippur72
Copy link

experienced the same problem today: I refactored a number|null to number|undefined and my code stopped to work because there were ===null checks still left behind.

@RyanCavanaugh
Copy link
Member

I really want some strictNullChecks real-world projects so we could test this stuff out

@zpdDG4gta8XKpMCd
Copy link
Author

i have one, willing to test it

@jfirebaugh
Copy link

Ironically, I work on a codebase where we're incrementally enabling strictNullChecks and was bit by this.

To support strictNullChecks , the following change was made:

- let applyMemoryInitializer: () => void;
+ let applyMemoryInitializer: (() => void) | null = null;

A later condition was overlooked, and became always-true.

if (applyMemoryInitializer !== undefined) {
   ...
}

@monsanto
Copy link

monsanto commented May 9, 2024

@RyanCavanaugh It's been several years, is more feedback still desired? This something I've been bit by in the past and might have gotten bit by just now, if I didn't recall TypeScript's behavior in this situation.

Real simple case. I had some code that explicitly returned null, but I was thinking about code-golfing it using the ?. operator, and that would have returned undefined instead. It's easy to make this kind of mistake and TypeScript otherwise encourages you to lean on it for always true/always false comparisons.

@ItsaMeTuni
Copy link

ItsaMeTuni commented Nov 13, 2024

+1 on this. TypeScript is wonderful, but you still can't fully trust it to prevent you from bugs when refactoring because of holes in the type-system like this one.

Interested in your thoughts here. We special-cased comparisons against null and undefined specifically for defensive checks:

// TypeScript callers: Don't give me 'undefined'
function fn(x: string) {
  // JS callers: Read the docs please
  if (x === undefined) throw new Error('x cannot be undefined');
}

I think that part is non-negotiable -- it should be "cheap" to check for those sorts of errors, by some means or another.

@RyanCavanaugh I think that if you're typing it a value and still expecting it to be something else (for any reason, even because of interfacing with plain JS) you're doing something wrong. IMO a better approach would be receiving an unknown as the type of x and then validating it or wrapping the call to fn in another function that does this validation.

Some may disagree with me, and that's fine, but at the very least I don't think this is a valid argument for putting a "hole" in the type system. If you really want to validate x against a type it's not supposed to have you could use // @ts-expect-error.

Workaround

For those that end up here looking for a solution, there's an eslint rule that implements this.

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 Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

7 participants