Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Commit

Permalink
Add check-rhs option for strict-boolean-expressions
Browse files Browse the repository at this point in the history 
By default, the right-hand operator of `&&` and `||` will not be checked
as a strict boolean expression.  The old behavior can be enabled by
adding the `check-rhs` option to `strict-boolean-expressions`.
  • Loading branch information
@dobesv
dobesv committed Sep 8, 2018
1 parent 0437cd9 commit f702d97
Show file tree
Hide file tree
Showing 7 changed files with 365 additions and 87 deletions.
208 changes: 142 additions & 66 deletions src/rules/strictBooleanExpressionsRule.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ const OPTION_ALLOW_ENUM = "allow-enum";
const OPTION_ALLOW_NUMBER = "allow-number";
const OPTION_ALLOW_MIX = "allow-mix";
const OPTION_ALLOW_BOOLEAN_OR_UNDEFINED = "allow-boolean-or-undefined";
const OPTION_CHECK_RHS = "check-rhs";

// tslint:disable object-literal-sort-keys
// tslint:disable object-literal-sort-keys no-bitwise

export class Rule extends Lint.Rules.TypedRule {
public static metadata: Lint.IRuleMetadata = {
Expand Down Expand Up @@ -64,6 +65,7 @@ export class Rule extends Lint.Rules.TypedRule {
- Also allows \`true | false | undefined\`.
- Does not allow \`false | undefined\`.
- This option is a subset of \`${OPTION_ALLOW_UNDEFINED_UNION}\`, so you don't need to enable both options at the same time.
* \`${OPTION_CHECK_RHS}\` checks the right-hand side of \`&&\` and \`||\'
`,
options: {
type: "array",
Expand All @@ -76,23 +78,34 @@ export class Rule extends Lint.Rules.TypedRule {
OPTION_ALLOW_ENUM,
OPTION_ALLOW_NUMBER,
OPTION_ALLOW_BOOLEAN_OR_UNDEFINED,
],
OPTION_CHECK_RHS
]
},
minLength: 0,
maxLength: 5,
maxLength: 7
},
optionExamples: [
true,
[true, OPTION_ALLOW_NULL_UNION, OPTION_ALLOW_UNDEFINED_UNION, OPTION_ALLOW_STRING, OPTION_ALLOW_ENUM, OPTION_ALLOW_NUMBER],
[true, OPTION_ALLOW_BOOLEAN_OR_UNDEFINED],
[
true,
OPTION_ALLOW_NULL_UNION,
OPTION_ALLOW_UNDEFINED_UNION,
OPTION_ALLOW_STRING,
OPTION_ALLOW_ENUM,
OPTION_ALLOW_NUMBER
],
[true, OPTION_ALLOW_BOOLEAN_OR_UNDEFINED]
],
type: "functionality",
typescriptOnly: true,
requiresTypeInfo: true,
requiresTypeInfo: true
};

public applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): Lint.RuleFailure[] {
const options = parseOptions(this.ruleArguments, Lint.isStrictNullChecksEnabled(program.getCompilerOptions()));
const options = parseOptions(
this.ruleArguments,
Lint.isStrictNullChecksEnabled(program.getCompilerOptions())
);
return this.applyWithFunction(sourceFile, walk, options, program.getTypeChecker());
}
}
Expand All @@ -106,6 +119,7 @@ interface Options {
allowNumber: boolean;
allowMix: boolean;
allowBooleanOrUndefined: boolean;
checkRhs: boolean;
}

function parseOptions(ruleArguments: string[], strictNullChecks: boolean): Options {
Expand All @@ -118,6 +132,7 @@ function parseOptions(ruleArguments: string[], strictNullChecks: boolean): Optio
allowNumber: has(OPTION_ALLOW_NUMBER),
allowMix: has(OPTION_ALLOW_MIX),
allowBooleanOrUndefined: has(OPTION_ALLOW_BOOLEAN_OR_UNDEFINED),
checkRhs: has(OPTION_CHECK_RHS)
};

function has(name: string): boolean {
Expand All @@ -133,14 +148,19 @@ function walk(ctx: Lint.WalkContext<Options>, checker: ts.TypeChecker): void {
const b = node as ts.BinaryExpression;
if (binaryBooleanExpressionKind(b) !== undefined) {
const { left, right } = b;
const checkHalf = (expr: ts.Expression) => {
// If it's another boolean binary expression, we'll check it when recursing.
if (!isBooleanBinaryExpression(expr)) {
checkExpression(expr, b);
}
};
checkHalf(left);
checkHalf(right);
// If check-rhs is on, we don't have to analyze a boolean binary expression
// on the left side because it will checked well enough on its own. However,
// if check-rhs is off, we have to analyze the overall result of the left
// side no matter what, because its right side might not follow the rules.
if (!(options.checkRhs && isBooleanBinaryExpression(left))) {
checkExpression(left, b);
}
// If check-rhs is off, we don't have to analyze the right hand side
// We also don't have to analyze the right hand side if it is also a
// boolean binary expression; its own inner check is sufficient.
if (options.checkRhs && !isBooleanBinaryExpression(right)) {
checkExpression(right, b);
}
}
break;
}
Expand All @@ -165,7 +185,10 @@ function walk(ctx: Lint.WalkContext<Options>, checker: ts.TypeChecker): void {
}

case ts.SyntaxKind.ConditionalExpression:
checkExpression((node as ts.ConditionalExpression).condition, node as ts.ConditionalExpression);
checkExpression(
(node as ts.ConditionalExpression).condition,
node as ts.ConditionalExpression
);
break;

case ts.SyntaxKind.ForStatement: {
Expand All @@ -183,16 +206,16 @@ function walk(ctx: Lint.WalkContext<Options>, checker: ts.TypeChecker): void {
const type = checker.getTypeAtLocation(node);
const failure = getTypeFailure(type, options);
if (failure !== undefined) {
if (failure === TypeFailure.AlwaysTruthy &&
if (
failure === TypeFailure.AlwaysTruthy &&
!options.strictNullChecks &&
(options.allowNullUnion || options.allowUndefinedUnion)) {
(options.allowNullUnion || options.allowUndefinedUnion)
) {
// OK; It might be null/undefined.
return;
}

ctx.addFailureAtNode(
node,
showFailure(location, failure, isUnionType(type), options));
ctx.addFailureAtNode(node, showFailure(location, failure, isUnionType(type), options));
}
}
}
Expand All @@ -203,19 +226,22 @@ function getTypeFailure(type: ts.Type, options: Options): TypeFailure | undefine
}

const kind = getKind(type);
const failure = failureForKind(kind, /*isInUnion*/false, options);
const failure = failureForKind(kind, /*isInUnion*/ false, options);
if (failure !== undefined) {
return failure;
}

switch (triState(kind)) {
case true:
// Allow 'any'. Allow 'true' itself, but not any other always-truthy type.
// tslint:disable-next-line no-bitwise
return isTypeFlagSet(type, ts.TypeFlags.Any | ts.TypeFlags.BooleanLiteral) ? undefined : TypeFailure.AlwaysTruthy;
return isTypeFlagSet(type, ts.TypeFlags.Any | ts.TypeFlags.BooleanLiteral)
? undefined
: TypeFailure.AlwaysTruthy;
case false:
// Allow 'false' itself, but not any other always-falsy type
return isTypeFlagSet(type, ts.TypeFlags.BooleanLiteral) ? undefined : TypeFailure.AlwaysFalsy;
return isTypeFlagSet(type, ts.TypeFlags.BooleanLiteral)
? undefined
: TypeFailure.AlwaysFalsy;
case undefined:
return undefined;
}
Expand All @@ -228,7 +254,7 @@ function isBooleanUndefined(type: ts.UnionType): boolean | undefined {
isTruthy = true;
} else if (isTypeFlagSet(ty, ts.TypeFlags.BooleanLiteral)) {
isTruthy = isTruthy || (ty as ts.IntrinsicType).intrinsicName === "true";
} else if (!isTypeFlagSet(ty, ts.TypeFlags.Void | ts.TypeFlags.Undefined)) { // tslint:disable-line:no-bitwise
} else if (!isTypeFlagSet(ty, ts.TypeFlags.Void | ts.TypeFlags.Undefined)) {
return undefined;
}
}
Expand All @@ -251,7 +277,7 @@ function handleUnion(type: ts.UnionType, options: Options): TypeFailure | undefi

for (const ty of type.types) {
const kind = getKind(ty);
const failure = failureForKind(kind, /*isInUnion*/true, options);
const failure = failureForKind(kind, /*isInUnion*/ true, options);
if (failure !== undefined) {
return failure;
}
Expand All @@ -269,13 +295,21 @@ function handleUnion(type: ts.UnionType, options: Options): TypeFailure | undefi
}
}

return seenFalsy === 0 ? TypeFailure.AlwaysTruthy
: !anyTruthy ? TypeFailure.AlwaysFalsy
: !options.allowMix && seenFalsy > 1 ? TypeFailure.Mixes : undefined;
return seenFalsy === 0
? TypeFailure.AlwaysTruthy
: !anyTruthy
? TypeFailure.AlwaysFalsy
: !options.allowMix && seenFalsy > 1
? TypeFailure.Mixes
: undefined;
}

/** Fails if a kind of falsiness is not allowed. */
function failureForKind(kind: TypeKind, isInUnion: boolean, options: Options): TypeFailure | undefined {
function failureForKind(
kind: TypeKind,
isInUnion: boolean,
options: Options
): TypeFailure | undefined {
switch (kind) {
case TypeKind.String:
case TypeKind.FalseStringLiteral:
Expand Down Expand Up @@ -311,7 +345,7 @@ export const enum TypeFailure {
Null,
Undefined,
Enum,
Mixes,
Mixes
}

const enum TypeKind {
Expand All @@ -324,7 +358,7 @@ const enum TypeKind {
Null,
Undefined,
Enum,
AlwaysTruthy,
AlwaysTruthy
}

/** Divides a type into always true, always false, or unknown. */
Expand All @@ -349,19 +383,31 @@ function triState(kind: TypeKind): boolean | undefined {
}

function getKind(type: ts.Type): TypeKind {
return is(ts.TypeFlags.String) ? TypeKind.String
: is(ts.TypeFlags.Number) ? TypeKind.Number
: is(ts.TypeFlags.Boolean) ? TypeKind.Boolean
: is(ts.TypeFlags.Null) ? TypeKind.Null
: is(ts.TypeFlags.Undefined | ts.TypeFlags.Void) ? TypeKind.Undefined // tslint:disable-line:no-bitwise
: is(ts.TypeFlags.EnumLike) ? TypeKind.Enum
: is(ts.TypeFlags.NumberLiteral) ?
(numberLiteralIsZero(type as ts.NumberLiteralType) ? TypeKind.FalseNumberLiteral : TypeKind.AlwaysTruthy)
: is(ts.TypeFlags.StringLiteral) ?
(stringLiteralIsEmpty(type as ts.StringLiteralType) ? TypeKind.FalseStringLiteral : TypeKind.AlwaysTruthy)
: is(ts.TypeFlags.BooleanLiteral) ?
((type as ts.IntrinsicType).intrinsicName === "true" ? TypeKind.AlwaysTruthy : TypeKind.FalseBooleanLiteral)
: TypeKind.AlwaysTruthy;
return is(ts.TypeFlags.String)
? TypeKind.String
: is(ts.TypeFlags.Number)
? TypeKind.Number
: is(ts.TypeFlags.Boolean)
? TypeKind.Boolean
: is(ts.TypeFlags.Null)
? TypeKind.Null
: is(ts.TypeFlags.Undefined | ts.TypeFlags.Void)
? TypeKind.Undefined
: is(ts.TypeFlags.EnumLike)
? TypeKind.Enum
: is(ts.TypeFlags.NumberLiteral)
? numberLiteralIsZero(type as ts.NumberLiteralType)
? TypeKind.FalseNumberLiteral
: TypeKind.AlwaysTruthy
: is(ts.TypeFlags.StringLiteral)
? stringLiteralIsEmpty(type as ts.StringLiteralType)
? TypeKind.FalseStringLiteral
: TypeKind.AlwaysTruthy
: is(ts.TypeFlags.BooleanLiteral)
? (type as ts.IntrinsicType).intrinsicName === "true"
? TypeKind.AlwaysTruthy
: TypeKind.FalseBooleanLiteral
: TypeKind.AlwaysTruthy;

function is(flags: ts.TypeFlags) {
return isTypeFlagSet(type, flags);
Expand All @@ -379,7 +425,10 @@ function stringLiteralIsEmpty(type: ts.StringLiteralType): boolean {

/** Matches `&&` and `||` operators. */
function isBooleanBinaryExpression(node: ts.Expression): boolean {
return node.kind === ts.SyntaxKind.BinaryExpression && binaryBooleanExpressionKind(node as ts.BinaryExpression) !== undefined;
return (
node.kind === ts.SyntaxKind.BinaryExpression &&
binaryBooleanExpressionKind(node as ts.BinaryExpression) !== undefined
);
}

function binaryBooleanExpressionKind(node: ts.BinaryExpression): "&&" | "||" | undefined {
Expand Down Expand Up @@ -431,23 +480,43 @@ function showLocation(n: Location): string {
}
}

function showFailure(location: Location, ty: TypeFailure, unionType: boolean, options: Options): string {
function showFailure(
location: Location,
ty: TypeFailure,
unionType: boolean,
options: Options
): string {
const expectedTypes = showExpectedTypes(options);
const expected = expectedTypes.length === 1
? `Only ${expectedTypes[0]}s are allowed`
: `Allowed types are ${stringOr(expectedTypes)}`;
const expected =
expectedTypes.length === 1
? `Only ${expectedTypes[0]}s are allowed`
: `Allowed types are ${stringOr(expectedTypes)}`;
const tyFail = showTypeFailure(ty, unionType, options.strictNullChecks);
return `This type is not allowed in the ${showLocation(location)} because it ${tyFail}. ${expected}.`;
return `This type is not allowed in the ${showLocation(
location
)} because it ${tyFail}. ${expected}.`;
}

function showExpectedTypes(options: Options): string[] {
const parts = ["boolean"];
if (options.allowNullUnion) { parts.push("null-union"); }
if (options.allowUndefinedUnion) { parts.push("undefined-union"); }
if (options.allowString) { parts.push("string"); }
if (options.allowEnum) { parts.push("enum"); }
if (options.allowNumber) { parts.push("number"); }
if (options.allowBooleanOrUndefined) { parts.push("boolean-or-undefined"); }
if (options.allowNullUnion) {
parts.push("null-union");
}
if (options.allowUndefinedUnion) {
parts.push("undefined-union");
}
if (options.allowString) {
parts.push("string");
}
if (options.allowEnum) {
parts.push("enum");
}
if (options.allowNumber) {
parts.push("number");
}
if (options.allowBooleanOrUndefined) {
parts.push("boolean-or-undefined");
}
return parts;
}

Expand All @@ -458,14 +527,21 @@ function showTypeFailure(ty: TypeFailure, unionType: boolean, strictNullChecks:
return strictNullChecks
? "is always truthy"
: "is always truthy. It may be null/undefined, but neither " +
`'${OPTION_ALLOW_NULL_UNION}' nor '${OPTION_ALLOW_UNDEFINED_UNION}' is set`;
case TypeFailure.AlwaysFalsy: return "is always falsy";
case TypeFailure.String: return `${is} a string`;
case TypeFailure.Number: return `${is} a number`;
case TypeFailure.Null: return `${is} null`;
case TypeFailure.Undefined: return `${is} undefined`;
case TypeFailure.Enum: return `${is} an enum`;
case TypeFailure.Mixes: return "unions more than one truthy/falsy type";
`'${OPTION_ALLOW_NULL_UNION}' nor '${OPTION_ALLOW_UNDEFINED_UNION}' is set`;
case TypeFailure.AlwaysFalsy:
return "is always falsy";
case TypeFailure.String:
return `${is} a string`;
case TypeFailure.Number:
return `${is} a number`;
case TypeFailure.Null:
return `${is} null`;
case TypeFailure.Undefined:
return `${is} undefined`;
case TypeFailure.Enum:
return `${is} an enum`;
case TypeFailure.Mixes:
return "unions more than one truthy/falsy type";
}
}

Expand Down
3 changes: 1 addition & 2 deletions test/rules/strict-boolean-expressions/allow-boolean-undefined-union/test.ts.lint
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,8 @@ if (get<RegExp | undefined>() || get<boolean>()) {}
// This still fails of course!
if (get<RegExp>() || get<RegExp>()) {}
~~~~~~~~~~~~~ [err % ("operand for the '||' operator", 'is always truthy')]
~~~~~~~~~~~~~ [err % ("operand for the '||' operator", 'is always truthy')]

if (get<number | undefined>()) {}
~~~~~~~~~~~~~~~~~~~~~~~~~ [err % ("'if' condition", 'could be undefined')]

[err]: This type is not allowed in the %s because it %s. Allowed types are boolean or boolean-or-undefined.
[err]: This type is not allowed in the %s because it %s. Allowed types are boolean or boolean-or-undefined.
1 change: 0 additions & 1 deletion test/rules/strict-boolean-expressions/allow-undefined-union/test.ts.lint
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ if (get<RegExp | undefined>() || get<boolean>()) {}
// This still fails of course!
if (get<RegExp>() || get<RegExp>()) {}
~~~~~~~~~~~~~ [This type is not allowed in the operand for the '||' operator because it is always truthy. Allowed types are boolean or undefined-union.]
~~~~~~~~~~~~~ [This type is not allowed in the operand for the '||' operator because it is always truthy. Allowed types are boolean or undefined-union.]

if (get<number | undefined>()) {}
~~~~~~~~~~~~~~~~~~~~~~~~~ [This type is not allowed in the 'if' condition because it could be a number. Allowed types are boolean or undefined-union.]
Loading

0 comments on commit f702d97

Please sign in to comment.