Skip to content

Fix for issue #712 #823

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

Conversation

chrisbubernak
Copy link
Contributor

Fix for issue #712. The specific issue mentioned there concerns providing a more helpful error message when the ^ operator is used with 2 boolean operands. My change is slightly more general in that it generates an error for &, &=, ^, ^=, |, and |=. I used the error message mentioned in the issue:

"error TS0000: The ^ operator is not allowed for boolean types. Consider using !== instead."

When & or | are used with boolean values I suggest && and || respectively.

One interesting thing here is how to handle the assignment operators. Right now I show the same recommendation i.e. "consider using !==" shows up for both "^" and "^=" which, is maybe a bit confusing, but I wasn't sure the best way to do this. I've spoken briefly with @DanielRosenwasser about this but would be open to any other feedback for how to handle this.

@@ -5090,6 +5098,23 @@ module ts {
case SyntaxKind.CommaToken:
return rightType;
}

// if a user tries to apply an innappropriate operator to 2 boolean operands try and return them a helpful suggestion
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this to "a bitwise operator" and split this into two lines.

@DanielRosenwasser
Copy link
Member

I don't see any tests affected that demonstrate the change for compound assignment. You may want to add a test in either tests/cases/compiler or tests/cases/conformance; I don't think it matters that much which, but the former is usually easy for a quick & dirty test, and I would just try out all 3 bitwise assignment operators with every permutation of number and boolean.

if ((leftType.flags & TypeFlags.Boolean) && (rightType.flags & TypeFlags.Boolean) && (suggestedOperator = getSuggestedBooleanOperator(node.operator)) !== undefined) {
error(node, Diagnostics.The_0_operator_is_not_allowed_for_boolean_types_Consider_using_1_instead, tokenToString(node.operator), tokenToString(suggestedOperator));
}
// otherwise just check each operand seperately and report errors as normal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't place comments above 'else' blocks. Just put it inside the else block. Thanks!

@CyrusNajmabadi
Copy link
Contributor

Chris, this LGTM. Thanks for contributing. I'm not sure if you are approved for submitting to TS yet. You'll have to talk to @mhegazy about the necessary paperwork to contribute. Once that goes through, then we can proceed on this.

Thanks!

@chrisbubernak
Copy link
Contributor Author

@CyrusNajmabadi I actually work at MS and after talking with @mhegazy it looks like I don't have to fill out the official paperwork.

error(node, Diagnostics.The_0_operator_is_not_allowed_for_boolean_types_Consider_using_1_instead, tokenToString(node.operator), tokenToString(suggestedOperator));
}
else {
// otherwise just check each operand seperately and report errors as normal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misspelled "separately"

@DanielRosenwasser
Copy link
Member

👍 after that last change is addressed, thanks Chris!

@mhegazy
Copy link
Contributor

mhegazy commented Oct 5, 2014

thanks!

mhegazy added a commit that referenced this pull request Oct 5, 2014
@mhegazy mhegazy merged commit 432fff1 into microsoft:master Oct 5, 2014
@chrisbubernak chrisbubernak deleted the betterErrorsForBitwiseOperatorsOnBooleans branch October 5, 2014 23:39
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants