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

Suggestion: warn on always-false typeguards #34936

Open
4 of 5 tasks
shicks opened this issue Nov 6, 2019 · 5 comments
Open
4 of 5 tasks

Suggestion: warn on always-false typeguards #34936

shicks opened this issue Nov 6, 2019 · 5 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

@shicks
Copy link
Contributor

shicks commented Nov 6, 2019

Search Terms

type guard always-false warning

Suggestion

When a typeguard is always false, the guarded variable is type never within the body. While you can't actually look up any properties on never, you can pass it anywhere or assign it to anything and there's absolutely no warnings. Calling a typeguard function with an argument that has no overlap with the guard should produce a warning.

Use Cases

My immediate motivation is #24436 (comment), where if Number.isFinite could be written as a typeguard (this would also need a narrower number subtype, see e.g. #32188) then it could easily be allowed to be called with a number|string, but no additional gymnastics would be required to warn if calling with a guaranteed non-number.

In general there's not a good reason to call an always-false typeguard. This is quite different from the always-true case where defensive coding comes into play.

Examples

Example:

declare const x: string;
if (isNumber(x)) { // Should warn: "condition is always false"
    usePrivate(x);
}

If usePrivate requires a private type that this scope doesn't have access to construct, then it would be nice if this code warned somewhere. Instead, it currently just narrows x to never and allows doing whatever you want with it.

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
    • I don't have a good sense of how much this would break, but I expect any breakages would be exposing legitimate errors.
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.
@AnyhowStep
Copy link
Contributor

declare const x: string;
declare function isString(x: unknown): x is string;

if (!isString(x)) {
    //`x` is now of type `never`
    //but I want this to compile
    //because defensive programming against JS code
    throw new Error(`Expected string; received ${typeof x}`);
}

Playground

@fatcerberus
Copy link

@AnyhowStep He mentioned that, actually.

This is quite different from the always-true case where defensive coding comes into play.

isString() is always true (per the type system), you're just negating it afterwards.

@RyanCavanaugh
Copy link
Member

I don't think it's justifiable that defensive checks are always written in always-true format. You could easily imagine writing:

function fn(x: number) {
  if (isNullOrUndefined(x)) Debug.fail("x must be provided");
}

It's not clear what distinguishes this from the case in the OP.

@shicks
Copy link
Contributor Author

shicks commented Nov 9, 2019

That's a good point, though I suspect it's a much less common case. There's always the option to cast to unknown, though it's not particularly satisfying. It's also less relevant for this to be a type guard, since it's not actually narrowing anything.

@RyanCavanaugh - my underlying motivation was a comment you made about wanting to warn on Number.isFinite(shouldNeverBeANumber), which is somewhat similar - I think your example here is more compelling because it's about null/undefined, which we naturally understand to generally be a possible exception to the type system.

Maybe a cleverer approach would be to warn if typeguard(x | undefined | null) would be always false. It lacks a certain symmetry and in general cleverness is bad, but it seems to pass the gut test a little better...

@RyanCavanaugh RyanCavanaugh added 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 labels Nov 11, 2019
@fatcerberus
Copy link

fatcerberus commented Nov 11, 2019

I suppose one could declare the check as

declare function isNullOrUndefined(x: any): x is null | undefined;

But that doesn't seem useful; when do you ever need a value that's guaranteed to be nullish? And if you're not using it as a type guard... well, then that's what distinguishes it from the OP. The issue is about warning on always-false type predicate calls.

I can't think of a single time I've ever seen a type guard which is expected to always be false in normal operation. Generic defensive boolean checks, sure, but no always-false type guards.

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

4 participants