Repository navigation
Invalid String Literal Comparison Error in Expression Using || and !== Operator #15079
Description
TypeScript Version: 2.2.1
We just stumbled upon an inspection error that doesn't seem to be correct.
When trying to do an || in the console.log the type checker seems to think we're checking two string literals for equality, when we're actually checking if a property value is equal or not equal to a string literal.
To make matters a bit more weird, it only applies when the !== is the first check in the expression.
Note
This inspection error is not shown in tsc --version Version 2.0.10, but starts to show up in tsc --version Version 2.1.4
Code
class Role {
name: string;
}
class User {
role: Role;
}
const user = {
role: { name: "Assistant" }
} as User;
console.log(user.role.name !== "Admin" || user.role.name === "Assistant"); // error TS2365
console.log(user.role.name === "Assistant" || user.role.name !== "Admin"); // works as expectedExpected behavior:
Output
true
trueJavaScript
var Role = (function () {
function Role() {
}
return Role;
}());
var User = (function () {
function User() {
}
return User;
}());
var user = {
role: { name: "Assistant" }
};
console.log(user.role.name !== "Admin" || user.role.name === "Assistant");
console.log(user.role.name === "Assistant" || user.role.name !== "Admin");Actual behavior:
Inspection Error
error TS2365: Operator '===' cannot be applied to types '"Admin"' and '"Assistant"'.
JavaScript
var Role = (function () {
function Role() {
}
return Role;
}());
var User = (function () {
function User() {
}
return User;
}());
var user = {
role: { name: "Assistant" }
};
console.log(user.role.name !== "Admin" || user.role.name === "Assistant");
console.log(user.role.name === "Assistant" || user.role.name !== "Admin");
Activity
gcnew commentedon Apr 7, 2017
See #14983.
This error tells you that you have a logical inconsistency. Basically in:
If
user.role.nameis"Admin"then the second partuser.role.name === "Assistant"will be always false, as we already know thatuser.role.nameis"Admin"from the left side of the operator. If it's not"Admin"then the second part is redundant again because it will never be reached.stehlikio commentedon Apr 7, 2017
Marin Marinov (@gcnew) that issue is definitely similar, although in this example, only one side of the expression is using inequality. The expression I have in the example will not always return
trueorfalse. Also, it only seems to happen when the inequality expression is the left hand operand of the||operator.True Condition
Output
False Condition
Output
gcnew commentedon Apr 7, 2017
The right part of the
||operator is redundant both whenuser.role.nameis"Admin"and when it is not.Consider
user.role.name = "Manager". We evaluate the conditionuser.role.name !== "Admin"and we concludetrue.user.role.name === "Assistant"is never evaluated because of short-circuiting.Conversely, consider the other option -
user.role.manager = "Admin". This timeuser.role.name !== "Admin"evaluates tofalse. The second condition is never true, because we now know thatuser.role.nameis"Admin"and we compare it to"Assistant". The compiler is very smart at its analysis (but not in its way of communication) and points that out to us in the error message.I'm not exactly sure what your intention is, but I think you want to compare the property against a single know value. If it's that "known value", it can't be any other.
stehlikio commentedon Apr 7, 2017
Marin Marinov (@gcnew) Ok, I do see your point there. This is quite weird in my opinion, considering this is more of a code quality inspection vs type error, and it returns a nonzero error level.
RyanCavanaugh commentedon Apr 7, 2017
The only reason the second line isn't an error is because we don't have a type that means "Any string except
Assistant".People almost never write code that doesn't do anything on purpose.
It's like writing
ifDog(foo) && isMammal(foo)-- what is the second condition doing there? It never changes the result of the expression. More often than not, you meant to write some different second condition there, or simply hadn't thought about it hard enough.stehlikio commentedon Apr 7, 2017
Ryan Cavanaugh (@RyanCavanaugh) Yeah, we did end up realizing the condition didn't make a ton of sense and ended up changing it.
I do agree with Yiou Chen (@yiochen) that the message is very misleading and caused more headache than it was worth.
gcnew commentedon Apr 7, 2017
I think it's invaluable - it either warns you of dead code or points you out flawed reasoning. Exactly the things I expect from a compiler. It has saved me several times from mistyped logical operators when my intention was the opposite of what I had written.
stehlikio commentedon Apr 7, 2017
Marin Marinov (@gcnew) Agreed. I do hope we can clean up the message/message level in the future, ambiguous messages can really hurt productivity.
It's also interesting that there are dirty ways to sidestep the errors.
aluanhaddad commentedon Apr 7, 2017
Yes, you can suppress the errors with a type assertion. As for the second case, you are preevaluting the right hand side which creates an arbitrary boolean which naturally suppresses the error as well.
That said, these errors are great. They have caught real bugs in my code. I also like them because they encourage one to be neat and tidy and avoid redundant tests that express a lack of confidence in the code that make it hard to understand and thus hard to maintain.
yiochen commentedon Apr 7, 2017
I personally got tripped by those boolean conditions a lot(I wish I was a better programmer). So I think this is a very nice feature of TypeScript. Just asking, is there anyway to practice those fundamentals? just code a lot?
aluanhaddad commentedon Apr 9, 2017
Yiou Chen (@yiochen) indeed code a lot :) especially in TypeScript since it tells you about the mistakes and you can generalize the experience.
stehlikio commentedon Apr 11, 2017
Ryan Cavanaugh (@RyanCavanaugh) I spent a little bit of time digging through the compiler checks to see if I could get the compiler to do what I was looking for, stehlikio/TypeScript@15d89a7. I know this is a naive implementation...and I don't know all of the downstream impacts it could cause, but all tests current conformance tests pass.
Is there any thought in utilizing the warning diagnostic messages (I didn't see anything currently using them at this time) in the future to provide better messaging?
RyanCavanaugh commentedon Apr 11, 2017
In general we're willing to take changes to improve error messages, but they have to be more correct than the current message. The logic you have there just identifies the code you wrote, not the redundancy. It issues the same error for code like this:
What's happened in your case is that an expression got narrowed to a specific type which ended up being incomparable to some other type. We don't really have the logic to identify why something was narrowed after the fact, nor is it obvious that the actual logic error is a redundancy.