strict boolean expressions #1820

subash-a · 2016-12-05T07:16:09Z

PR checklist

Addresses an existing issue: New rule to only allow boolean statements in logical expressions #253
New feature, bugfix, or enhancement
Includes tests
Documentation update

Introduced a rule to type check boolean expressions using type aware rules. This rule checks if the given binary expression, if statement, while statement or for statement comprises of correctly formed boolean expressions or literals. This is a first cut of the implementation and would like to get feedback on how to improve this and finally make this rule a part of the tslint rule set.
@myitcv tagging you here since we both discussed this rule offline, @adidahiya any feedback is welcome on how we can improve this.

palantirtech · 2016-12-05T07:16:11Z

Thanks for your interest in palantir/tslint, @subash-a! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

myitcv · 2016-12-05T10:06:29Z

Looks great to me 👍

nchen63

Looks pretty good overall. Good tests. Needs some minor changes

nchen63 · 2016-12-05T16:24:42Z

src/rules/strictBooleanExpressionsRule.ts

@@ -0,0 +1,116 @@
+import * as ts from "typescript";


license, metadata

nchen63 · 2016-12-05T16:27:21Z

src/rules/strictBooleanExpressionsRule.ts

+
+class StrictBooleanExpressionsRule extends Lint.ProgramAwareRuleWalker {
+    public visitBinaryExpression(node: ts.BinaryExpression): void {
+        let checker = this.getTypeChecker();


call this once. move to constructor

nchen63 · 2016-12-05T16:27:57Z

src/rules/strictBooleanExpressionsRule.ts

+    public visitBinaryExpression(node: ts.BinaryExpression): void {
+        let checker = this.getTypeChecker();
+        // check all Boolean and BooleanLiteral expressions for validity
+        let nodeType = checker.getTypeAtLocation(node);


move this and as much logic as possible inside of if statement for perf

nchen63 · 2016-12-05T16:38:44Z

src/rules/strictBooleanExpressionsRule.ts

+}
+
+class StrictBooleanExpressionsRule extends Lint.ProgramAwareRuleWalker {
+    public visitBinaryExpression(node: ts.BinaryExpression): void {


we typically don't explicitly write the void return type

nchen63 · 2016-12-05T16:43:05Z

src/rules/strictBooleanExpressionsRule.ts

+    }
+
+    private isBooleanType(btype: ts.Type): boolean {
+        let boolType = /* tslint:disable:no-bitwise */(btype.flags & ts.TypeFlags.BooleanLike)/* tslint:enable:no-bitwise */;


use utils.isNodeFlagSet

@nchen63 this would not work because I am checking the type flags and not the node flags, there are two options available

I can leave this line as is

I can introduce a new function isTypeFlagsSet into language/utils.ts which can be used to check the type flags here
which option do you suggest I go with?

Please add this to utils

nchen63 · 2016-12-05T16:44:13Z

src/rules/strictBooleanExpressionsRule.ts

+
+    private isBooleanType(btype: ts.Type): boolean {
+        let boolType = /* tslint:disable:no-bitwise */(btype.flags & ts.TypeFlags.BooleanLike)/* tslint:enable:no-bitwise */;
+        if (boolType === ts.TypeFlags.Boolean || boolType === ts.TypeFlags.BooleanLiteral || boolType === ts.TypeFlags.BooleanLike) {


don't need if statement, just return the condition

nchen63 · 2016-12-05T16:44:51Z

test/rules/strict-boolean-expressions/test.ts.lint

@@ -0,0 +1,184 @@
+/* tslint:disable:no-unused-parameters */


shouldn't need these disable comments

also this is not a tslint rule

nchen63 · 2016-12-05T16:52:13Z

src/rules/strictBooleanExpressionsRule.ts

+                this.addFailure(this.createFailure(node.getStart(), node.getWidth(), Rule.BINARY_EXPRESSION_ERROR));
+            }
+        }
+        super.visitBinaryExpression(node);


Don't call super.visitBinaryExpression if a failure is detected at this node. Seems excessive to flag true && 1 && 1 twice since errors overlap

same with other super calls below

nchen63 · 2016-12-05T17:00:05Z

src/rules/strictBooleanExpressionsRule.ts

+export class Rule extends Lint.Rules.TypedRule {
+    public static BINARY_EXPRESSION_ERROR = "Invalid Expression: Operands for the && or || operator should be of type boolean";
+    public static UNARY_EXPRESSION_ERROR = "Invalid Expression: Operand for the ! operator should be of type boolean";
+    public static IF_STATEMENT_ERROR = "Invalid Expression: If statement condition needs to be a boolean expression or literal";


Combine lines 7 to 11 into Statement is not a boolean expression. After that, combine some of the checker logic below to reduce code duplication.

nchen63 · 2016-12-05T17:03:47Z

src/rules/strictBooleanExpressionsRule.ts

+import * as Lint from "../index";
+
+export class Rule extends Lint.Rules.TypedRule {
+    public static BINARY_EXPRESSION_ERROR = "Invalid Expression: Operands for the && or || operator should be of type boolean";


Remove Invalid Expression:

subash-a · 2016-12-06T08:11:07Z

@nchen63 I have made all the changes suggested, but one of the comments had a problem with the implementation for which I have left you a note, will make changes based on your answer there. Thanks for the quick and detailed review

nchen63 · 2016-12-06T19:21:32Z

src/rules/strictBooleanExpressionsRule.ts

+    public visitIfStatement(node: ts.IfStatement) {
+        let hasError = this.checkStatement(node);
+        if (!hasError) {
+            super.visitIfStatement(node);


Won't this skip the block/body of the if statement?

nchen63 · 2016-12-06T19:31:14Z

src/rules/strictBooleanExpressionsRule.ts

+        if (!this.isBooleanType(expType)) {
+            switch (node.kind) {
+            case ts.SyntaxKind.IfStatement:
+                this.addFailure(this.createFailure(node.getStart(), node.getWidth(), `If ${Rule.STATEMENT_ERROR}`));


Don't necessarily need different error message for each type of conditional, but this works

nchen63 · 2016-12-06T19:34:03Z

src/rules/strictBooleanExpressionsRule.ts

+                let rhsExpression = node.right;
+                let rhsType = this.checker.getTypeAtLocation(rhsExpression);
+                if (!this.isBooleanType(lhsType) || !this.isBooleanType(rhsType)) {
+                    this.addFailure(this.createFailure(node.getStart(), node.getWidth(), Rule.BINARY_EXPRESSION_ERROR));


The failure should underline the offending operand, not the entire statement

subash-a · 2016-12-07T09:51:13Z

@nchen63 changed the code to point out only the offending expression and also changed the logic of checking statements even after error has been flagged to find further errors in the statement block.

subash-a · 2016-12-07T10:17:50Z

@nchen63 However I had a question about the super.visit call for the various statements and expressions that I have in my code, is calling super.visitIfStatement (for example) mandatory after the error checking logic is executed? How does this affect the other rules, if it does at all, and would it stop errors from other rules showing up? Because if that is the case then what we are doing inside the visitBooleanExpression would stop other errors for the boolean expression to show up when there is an error in my rule and the other error would show up only after correcting the first error. Any suggestion on how we can structure this rule to avoid affecting other rules? perhaps @adidahiya and @jkillian can also pitch in with their thoughts on how best to do this.

nchen63 · 2016-12-07T16:04:21Z

we have rules like this that don't call super. See noShadowedVariableRule.ts

adidahiya · 2016-12-07T16:06:39Z

Rules are evaluated individually in isolation, so omitting a super call will not affect other rules. If such a call is omitted at a non-leaf node in the AST, the only effect is that the children in that subtree are not linted for that rule.

nchen63 · 2016-12-07T16:07:35Z

test/rules/strict-boolean-expressions/test.ts.lint

+/*** If Statement ***/
+/*** Invalid ***/
+if (numType) { /* statements */ }
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [If statement condition needs to be a boolean expression or literal]


underline the condition, not the entire if statement. Same with others

nchen63 · 2016-12-07T16:09:28Z

test/rules/strict-boolean-expressions/test.ts.lint

+bwrapType || boolType;
+~~~~~~~~~              [Operands for the && or || operator should be of type boolean]
+objType || boolType || enumType;
+~~~~~~~~~~~~~~~~~~~              [Operands for the && or || operator should be of type boolean]


I don't think it makes sense to underline objType || boolType, objType should only be underlined. If we can avoid flagging this, we can go back to the way it was before using errorFlagged and call super since that address the issue of overlapping errors

This rule helps in enforcing usage of boolean expressions in if statements and other constructs where boolean expressions are used also restricts the && and || operators to work with boolean types so that the operator behaves consistently without the need for type coercion.

subash-a · 2016-12-08T07:01:10Z

Thank you @nchen63 and @adidahiya for that explanation, things are much clearer to me now, so based on the feedback left in the earlier comments I have made changes to eliminate the duplication of error messages and also narrow down the error message to point to the specific expression or identifier, please take a look and let me know if the changes are fine, this should work as good first cut for this rule and probably improved upon as we go. Please let me know what you guys think.

subash-a · 2016-12-08T18:32:23Z

@nchen63 and @adidahiya Thanks for the review, how do we get the documentation updated, i.e add the description about the new rule at https://palantir.github.io/tslint/rules/

nchen63 · 2016-12-08T19:03:09Z

We don't update that site until a release is made. You can run npm run verify to generate the docs, but you don't need to

subash-a changed the title ~~Strict boolean exprs~~ [WIP] strict boolean exprs Dec 5, 2016

subash-a changed the title ~~[WIP] strict boolean exprs~~ [WIP] strict boolean expressions Dec 5, 2016

subash-a force-pushed the strict-boolean-exprs branch from 67c6d3a to 6743e25 Compare December 5, 2016 08:57

adidahiya assigned nchen63 Dec 5, 2016

nchen63 suggested changes Dec 5, 2016

View reviewed changes

subash-a force-pushed the strict-boolean-exprs branch from 6743e25 to b1a04bb Compare December 6, 2016 08:09

subash-a force-pushed the strict-boolean-exprs branch from b1a04bb to 47072f1 Compare December 6, 2016 08:26

nchen63 suggested changes Dec 6, 2016

View reviewed changes

subash-a force-pushed the strict-boolean-exprs branch from 6c73667 to 815912a Compare December 7, 2016 09:26

subash-a mentioned this pull request Dec 7, 2016

[WIP] empty object type inference #1821

Merged

4 tasks

nchen63 suggested changes Dec 7, 2016

View reviewed changes

subash-a force-pushed the strict-boolean-exprs branch 3 times, most recently from 5a83f90 to 8cfc9d0 Compare December 8, 2016 06:12

subash-a force-pushed the strict-boolean-exprs branch from 8cfc9d0 to a984107 Compare December 8, 2016 06:16

subash-a changed the title ~~[WIP] strict boolean expressions~~ strict boolean expressions Dec 8, 2016

nchen63 approved these changes Dec 8, 2016

View reviewed changes

nchen63 merged commit 47212df into palantir:master Dec 8, 2016

adidahiya mentioned this pull request Feb 26, 2017

New rule to only allow boolean statements in logical expressions #253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

strict boolean expressions #1820

strict boolean expressions #1820

subash-a commented Dec 5, 2016

palantirtech commented Dec 5, 2016

myitcv commented Dec 5, 2016

nchen63 left a comment

nchen63 Dec 5, 2016

nchen63 Dec 5, 2016

nchen63 Dec 5, 2016

nchen63 Dec 5, 2016

nchen63 Dec 5, 2016

subash-a Dec 6, 2016

nchen63 Dec 6, 2016

nchen63 Dec 5, 2016

nchen63 Dec 5, 2016

adidahiya Dec 5, 2016

nchen63 Dec 5, 2016

nchen63 Dec 5, 2016

nchen63 Dec 5, 2016

nchen63 Dec 5, 2016

subash-a commented Dec 6, 2016

nchen63 Dec 6, 2016

nchen63 Dec 6, 2016

nchen63 Dec 6, 2016

subash-a commented Dec 7, 2016

subash-a commented Dec 7, 2016

nchen63 commented Dec 7, 2016

adidahiya commented Dec 7, 2016

nchen63 Dec 7, 2016

nchen63 Dec 7, 2016

subash-a commented Dec 8, 2016

subash-a commented Dec 8, 2016

nchen63 commented Dec 8, 2016

strict boolean expressions #1820

strict boolean expressions #1820

Conversation

subash-a commented Dec 5, 2016

PR checklist

palantirtech commented Dec 5, 2016

myitcv commented Dec 5, 2016

nchen63 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subash-a commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subash-a commented Dec 7, 2016

subash-a commented Dec 7, 2016

nchen63 commented Dec 7, 2016

adidahiya commented Dec 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subash-a commented Dec 8, 2016

subash-a commented Dec 8, 2016

nchen63 commented Dec 8, 2016