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

Boolean Comparisons Not Working As Expected #56862

Closed
LeeWhite187 opened this issue Dec 24, 2023 · 15 comments
Closed

Boolean Comparisons Not Working As Expected #56862

LeeWhite187 opened this issue Dec 24, 2023 · 15 comments
Labels
Duplicate An existing issue was already created

Comments

@LeeWhite187
Copy link

LeeWhite187 commented Dec 24, 2023

🔎 Search Terms

Searched google, Reddit, stackoverflow, and the issues, here.
As well, went through this with ChatGPT, and asked for a proxy search.
No helpful results.

🕗 Version & Regression Information

I'm using Typescript v5.2.2 (under Angular 17 and React 18.1).
Have not explicitly tried older versions.

⏯ Playground Link

No response

💻 Code

export class testClass1
{
    testmethod1a()
    {
        let x:boolean = true;

        // Gives compiler error:
        // This comparison appears to be unintentional because the types 'false' and 'true' have no overlap.
        if(x === false)
            return;
    }
    testmethod1b()
    {
        let x:boolean = true;

        if((x as boolean) === false)
            return;
    }
    testmethod2()
    {
        let x:boolean = true;

        if(x === true)
            return;
    }
}

export class TestClass2
{
    testmethod3()
    {
        let x:boolean = false;

        if(x === false)
            return;
    }
    testmethod4a()
    {
        let x:boolean = false;

        // Gives compiler error:
        // This comparison appears to be unintentional because the types 'false' and 'true' have no overlap.
        if(x === true)
            return;
    }
    testmethod4b()
    {
        let x:boolean = false;

        if((x as boolean) === true)
            return;
    }
}

🙁 Actual behavior

The compiler is giving errors, for the two commented statements in the sample code, above:
"This comparison appears to be unintentional because the types 'true' and 'false' have no overlap.ts(2367)"

This same behavior occurs when using '===' and '=='.
It as well, gives the same behavior, if I replace the right-hand boolean literal (in the comparison) with strongly-typed variable of the same value.

🙂 Expected behavior

Since the following are true in the above sample methods:

  • Typescript is strongly-typed
  • Each variable is type-asserted at declaration
  • Each variable is assigned a boolean literal at declaration
  • Each comparison is to a boolean literal

I expect the compiler to have no ambiguity of type, nor control flow concerns, at the comparison statements of testmethod1a and testmethod4a, and to compile them (or transpile, in this case), into runnable code.

And taking the given error message literally, I would expect the compiler, to regard "the types 'true' and 'false' have no overlap" as them being NOT equal, and allow the evaluation to derive a boolean false result for the containing 'if' statement.

Only when I cast the variable with a type-assertion, does the compiler error go away.
But, this should not be necessary, since the variable is type-asserted in the immediately prior statement (at declaration) AND is assigned a boolean literal value.

So, the necessity to wrap a type-declared boolean variable inside a type assertion, for the compiler to regard a comparison of boleans as valid (and finish a build), creates a definite risk for bugs, if this edge case is not remembered wherever booleans are compared.

Additional information about the issue

No response

@fatcerberus
Copy link

fatcerberus commented Dec 24, 2023

This is intentional. TypeScript is telling that it knows the value is true (or false) at that point and the always-false if conditional is thus likely indicative of an error in the code.

And taking the given error message literally, I would expect the compiler, to regard "the types 'true' and 'false' have no overlap" as them being NOT equal, and allow the evaluation to derive a boolean false result for the containing 'if' statement.

This is, in effect, what the compiler is telling you. The condition can never be true because the literal type true has no overlap with the literal type false (i.e. they have no values in common); but the fact that you wrote a conditional statement indicates that you think it can be true.

But, this should not be necessary, since the variable is type-asserted in the immediately prior statement (at declaration) AND is assigned a boolean literal value.

let x: boolean = true is not a type assertion. It's a type annotation. The variable still gets narrowed on assignment, the same as if you wrote

declare let x: boolean;
console.log(x);
//          ^? let x: boolean
x = true;
console.log(x);
//          ^? let x: true

@jcalz
Copy link
Contributor

jcalz commented Dec 24, 2023

🔎 Search Terms

These are supposed to be the words you used in your search, like "boolean comparison" and "narrowing", which also serves to help future searchers find your issue.

⏯ Playground Link

This is supposed to be a link to the TS Playground with your example code inside


This looks like #31734 to me, mostly. Each of these features is behaving as intended:

  • the boolean type is actually a union
  • union types are narrowed upon assignment
  • comparisons of types with no overlap are considered to be errors

None of these is a bug, but unfortunately they act in concert to give you this undesirable behavior.

@fatcerberus
Copy link

tl;dr: it's an "unreachable code" error with extra steps

None of these is a bug, but unfortunately they act in concert to give you this undesirable behavior.

I'd argue this isn't really undesirable, except if #9998 comes into play of course.

@LeeWhite187
Copy link
Author

LeeWhite187 commented Dec 24, 2023

Thank you for the replies.
Maybe this will better explain the problem I'm seeing.
I created a second example (below), that doesn't include the non-reachable statement, created during my exploration of the combinatorial cases that the compiler found acceptable or not.

This example has no chance for compile-time realization of non-reachable code.
And, I still get the same compiler error.

And, like my previous example (showing the possible variants (RH true, RH false, and LH type-asserted)) was indicating, the compiler error does NOT occur, if I modify the comparison to either:

  • Switch the right-hand value to false. Or, more accurately, switch the right-hand to be the same literal value as the declaration.
  • Wrap the left-hand value in a type-assertion (to boolean).

Of which, the former should not be an opinion of the compiler for a strongly typed language, unless a boolean is not really a boolean (But, that seems more an existential crisis of booleans that if not 'false', and not 'true', then what?).
And, the latter should not be a requirement (for comparing declared booleans), because a need for an extra step (to tell the compiler that a declared boolean variable is a boolean) creates a risk of software flaws in production, for an already strongly-typed language.

export class testClass
{
    checkforproblems(entries:IEntry[]):boolean
    {
        let problemoccurred:boolean = false;

        entries.forEach(n=>
        {
            if(!n)
            {
                problemoccurred = true;
            }
            if(n.checkforproblem() === true)
            {
                problemoccurred = true;
            }
        });

        // Compiler error, again, here:
        // This comparison appears to be unintentional because the types 'false' and 'true' have no overlap.ts(2367
        if(problemoccurred === true)
        {
            return true;
        }

        return false;
    }
}

export interface IEntry
{
    prop1:number;
    prop2:string;
    checkforproblem:()=>boolean;
}

@LeeWhite187
Copy link
Author

@jcalz Apologies. I did not include a 'playground' example of the code, as the compiler will not execute it.
So, there's not really a way to achieve a runnable example.
This issue is for a compile-time problem.

@LeeWhite187
Copy link
Author

@fatcerberus Yes. You're right about the unreachable code. I created a revised test class, above, that still creates the same compiler error scenario, without the unreachable code (caused by my exploration of scenarios).
Hope this is a better example to discuss.

@jcalz
Copy link
Contributor

jcalz commented Dec 25, 2023

I did not include a 'playground' example of the code, as the compiler will not execute it. So, there's not really a way to achieve a runnable example. This issue is for a compile-time problem.

You seem to be misunderstanding. Maybe if you review other issues in this repository, you’ll see playground links and how they are used to demonstrate compile-time problems.

@fatcerberus
Copy link

fatcerberus commented Dec 25, 2023

@LeeWhite187 Thanks, this is why real-world examples are helpful (I understand you wanted to reduce the repro, but you cut out too much). The incorrect narrowing in the case of .forEach makes this a duplicate of #9998 and/or #11498.

I did not include a 'playground' example of the code, as the compiler will not execute it.

Fun fact: You can execute Playground code even in the presence of compiler errors, as long as it’s still legal JS. This is what the noEmitOnError tsconfig option controls. Even if not, the playground link still helps in general since you can see what types the compiler inferred, etc.

@LeeWhite187
Copy link
Author

@fatcerberus Sorry for my misunderstanding of the terminology in Typescript...
Did you say that 'false' and 'true' are 'types', and not simply the possible values, or states, of a boolean variable?
I'm not a CS major, so maybe I'm missing a concept about datatypes in Typescript, or just blissfully unaware of all the legacy "nuances" the Typescript compiler must contend with, when generating Javascript.

@fatcerberus
Copy link

@LeeWhite187 Yes, TypeScript has literal types, so besides the primitive types like number, string etc. you can write stuff like "foo" | "bar" or 1 | 2 | 3 or whatever. boolean is unique though in that it’s completely equivalent to the union true | false.

@LeeWhite187
Copy link
Author

@fatcerberus I looked at the two issues you cited....
9998 talks about control flow analysis, and specifically the dilution of type-certainty after indirection occurs.

Reading across issues, I found this one 9757, stating that "reachability and control flow analysis is done before typechecking".
I'm not sure how obsolete that remark may be (from 2016). But since control flow checking seems to be the reason for the compiler error I'm getting, and that it occurs before typechecking is performed, then control flow analysis may simply NOT be able to realize type at the compare (which is giving an error).

So, this may be a compiler limitation, since the reference eludes to exhaustive flow-traversed typing, requiring "a massive architectural change".

@fatcerberus
Copy link

#9998 talks about control flow analysis, and specifically the dilution of type-certainty after indirection occurs.

9998 is in fact the relevant issue; the issue is specifically about type narrowing not always being accurate in the presence of callbacks/closures (due to design limitations that aren’t easily solved), which is what’s happening with your forEach example; the side effects of the callback passed to forEach are ignored (because the compiler doesn’t actually know when it will execute) and the variable is assumed to still have its value from initialization.

The order that CFA and typechecking is done is not relevant here.

@LeeWhite187
Copy link
Author

@fatcerberus That makes it a curious problem, then....
Why would a CFA implementation assume no value change, in the obvious presence of intermediate logic that has an indeterminate chance of side-effect? It seems that, any lack of positive state traversal (for an unknown (at compile-time) call graph), would be enough justification to prevent any assumptions of state, preventing this 'error' from occurring.

From what I read so far, this error might be a side-effect of some greater design choice that's attempting to solve 'more painful' compilation problems. And, the apparent 'state myopia', here, is a satisfactory penalty.
If so, I can deal with that.

It is interesting to come across a compiler that attempts to give errors for the utility of my logic... in addition to actual problems in it.

I've employed a wrapper and workaround for the immediate problem I was having.
But, do you know of a compiler switch that safely disables this type of error?
I get enough reminders of the uselessness of my logic. haha

Have a good week.

@fatcerberus
Copy link

Why would a CFA implementation assume no value change, in the obvious presence of intermediate logic that has an indeterminate chance of side-effect?

That's also discussed in #9998, but basically, in JS it's possible for pretty much anything to have arbitrary side effects (including a simple property access!), those effects are not always statically analyzable even theoretically, and being completely pessimistic about it would severely neuter CFA-based type narrowing (which is very useful most of the time).

It is interesting to come across a compiler that attempts to give errors for the utility of my logic... in addition to actual problems in it.

I would argue it is trying to warn you about an actual problem - namely that you wrote a guard against a value that can't occur (or so the compiler thinks). The error is less about "please remove this dead code" and more "this is probably a mistake, you might want to double-check it". In most compilers such message would be a warning, not an error--but TS doesn't have warnings.

But, do you know of a compiler switch that safely disables this type of error?

I'm going to assume you mean the "comparison appears unintentional" error specifically because I don't imagine you would want to disable all type-related errors. 😉 Unfortunately, the answer to that is "no", but you can put a // @ts-ignore or // @ts-expect-error before the statement in question to suppress the error, if it's erroneous1.

Footnotes

  1. No pun intended...

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Jan 2, 2024
@typescript-bot
Copy link
Collaborator

This issue has been marked as "Duplicate" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

5 participants