Skip to content

Commit

Permalink
[strict-boolean-expressions] Consider assertion function argument a b…
Browse files Browse the repository at this point in the history
…oolean context
  • Loading branch information
kirkwaiblinger committed May 11, 2024
1 parent af47bc9 commit 524c870
Show file tree
Hide file tree
Showing 4 changed files with 266 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ The following nodes are considered boolean expressions and their type is checked
- Operands of logical binary operators (`lhs || rhs` and `lhs && rhs`).
- Right-hand side operand is ignored when it's not a descendant of another boolean expression.
This is to allow usage of boolean operators for their short-circuiting behavior.
- Asserted argument of an assertion function (`assert(arg)`).

## Examples

Expand Down Expand Up @@ -55,6 +56,11 @@ let obj = {};
while (obj) {
obj = getObj();
}

// assertion functions without an `is` are boolean contexts.
declare function myAssert(value: unknown): asserts value;
let maybeString = Math.random() > 0.5 ? '' : undefined;
myAssert(maybeString);
```

</TabItem>
Expand Down
118 changes: 117 additions & 1 deletion packages/eslint-plugin/src/rules/strict-boolean-expressions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ export default createRule<Options, MessageId>({
WhileStatement: traverseTestExpression,
'LogicalExpression[operator!="??"]': traverseLogicalExpression,
'UnaryExpression[operator="!"]': traverseUnaryLogicalExpression,
CallExpression: traverseCallExpression,
};

type TestExpression =
Expand Down Expand Up @@ -232,10 +233,125 @@ export default createRule<Options, MessageId>({
// left argument is always treated as a condition
traverseNode(node.left, true);
// if the logical expression is used for control flow,
// then it's right argument is used for it's side effects only
// then its right argument is used for its side effects only
traverseNode(node.right, isCondition);
}

function traverseCallExpression(node: TSESTree.CallExpression): void {
// If the call looks like `assert(expr1, expr2, ...c, d, e, f)`, then we can
// only care if `expr1` or `expr2` is asserted, since anything that happens
// within or after a spread argument is out of scope to reason about.
const checkableArguments: TSESTree.Expression[] = [];
for (const argument of node.arguments) {
if (argument.type === AST_NODE_TYPES.SpreadElement) {
break;
} else {
checkableArguments.push(argument);
}
}

// nothing to do
if (checkableArguments.length === 0) {
return;
}

// Game plan: we're going to check the type of the callee. If it has call
// signatures and they _ALL_ agree that they assert on a parameter at the
// _SAME_ position, we'll consider the argument in that position to be a
// boolean context.
const calleeType = getConstrainedTypeAtLocation(services, node.callee);
const callSignatures = tsutils.getCallSignaturesOfType(calleeType);

let assertedParameterIndex: number | undefined;
for (const signature of callSignatures) {
const declaration = signature.getDeclaration();
const returnTypeAnnotation = declaration.type;

// Be sure we're dealing with a truthiness assertion function.
if (
!(
returnTypeAnnotation != null &&
ts.isTypePredicateNode(returnTypeAnnotation) &&
// This eliminates things like `x is string` and `asserts x is T`
// leaving us with just the `asserts x` cases.
returnTypeAnnotation.type == null &&
// I think this is redundant but, still, it needs to be true
returnTypeAnnotation.assertsModifier != null
)
) {
return;
}

const assertionTarget = returnTypeAnnotation.parameterName;
if (assertionTarget.kind !== ts.SyntaxKind.Identifier) {
// This can happen when asserting on `this`. Ignore!
return;
}

const firstParameter = declaration.parameters.at(0);
const nonThisParameters =
firstParameter != null &&
firstParameter.name.kind === ts.SyntaxKind.Identifier &&
firstParameter.name.text === 'this'
? declaration.parameters.slice(1)
: declaration.parameters;

// Don't even bother inspecting parameters past the number of
// arguments we have at the call site.
const checkableNonThisParameters = nonThisParameters.slice(
0,
checkableArguments.length,
);

let assertedParameterIndexForThisSignature: number | undefined;
for (const [index, parameter] of checkableNonThisParameters.entries()) {
if (parameter.dotDotDotToken != null) {
// Cannot assert a rest parameter, and can't have a rest parameter
// before the asserted parameter. It's not only a TS error, it's
// not something we can logically make sense of, so give up here.
return;
}

if (parameter.name.kind !== ts.SyntaxKind.Identifier) {
// Only identifiers are valid for assertion targets, so skip over
// anything like `{ destructuring: parameter }: T`
continue;
}

// we've found a match between the "target"s in
// `function asserts(target: T): asserts target;`
if (parameter.name.text === assertionTarget.text) {
assertedParameterIndexForThisSignature = index;
break;
}
}

if (assertedParameterIndexForThisSignature == null) {
// Didn't find an assertion target in this signature that could match
// the call site.
return;
}

if (
assertedParameterIndex != null &&
assertedParameterIndex !== assertedParameterIndexForThisSignature
) {
// Found asserted parameter didn't match previous signatures.
return;
}

assertedParameterIndex = assertedParameterIndexForThisSignature;
}

// Didn't find a unique assertion index.
if (assertedParameterIndex == null) {
return;
}

const assertedParameter = checkableArguments[assertedParameterIndex];
traverseNode(assertedParameter, true);
}

/**
* Inspects any node.
*
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

137 changes: 137 additions & 0 deletions packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,63 @@ if (y) {
},
],
},
`
declare function assert(a: number, b: unknown): asserts a;
declare const nullableString: string | null;
declare const boo: boolean;
assert(boo, nullableString);
`,
`
declare function assert(a: number, b: unknown): asserts b is string;
declare const nullableString: string | null;
declare const boo: boolean;
assert(boo, nullableString);
`,
`
declare function assert(a: number, b: unknown): asserts b;
declare const nullableString: string | null;
declare const boo: boolean;
assert(nullableString, boo);
`,
`
declare function assert(a: number, b: unknown): asserts b;
declare const nullableString: string | null;
declare const boo: boolean;
assert(...nullableString, nullableString);
`,
`
declare function assert(
this: object,
a: number,
b?: unknown,
c?: unknown,
): asserts c;
declare const nullableString: string | null;
declare const foo: number;
const o: { assert: typeof assert } = {
assert,
};
o.assert(foo, nullableString);
`,
{
code: `
declare function assert(x: unknown): x is string;
declare const nullableString: string | null;
assert(nullableString);
`,
},
{
code: `
class ThisAsserter {
assertThis(this: unknown, arg2: unknown): asserts this {}
}
declare const lol: string | number | unknown | null;
const thisAsserter: ThisAsserter = new ThisAsserter();
thisAsserter.assertThis(lol);
`,
},
],

invalid: [
Expand Down Expand Up @@ -1726,5 +1783,85 @@ if (x) {
},
],
},
{
code: `
declare function assert(x: unknown): asserts x;
declare const nullableString: string | null;
assert(nullableString);
`,
output: null,
errors: [
{
messageId: 'conditionErrorNullableString',
line: 4,
column: 8,
},
],
},
{
code: `
declare function assert(a: number, b: unknown): asserts b;
declare const nullableString: string | null;
assert(foo, nullableString);
`,
output: null,
errors: [
{
messageId: 'conditionErrorNullableString',
line: 4,
column: 13,
},
],
},
{
code: `
declare function assert(a: number, b: unknown): asserts b;
declare function assert(one: number, two: unknown): asserts two;
declare const nullableString: string | null;
assert(foo, nullableString);
`,
output: null,
errors: [
{
messageId: 'conditionErrorNullableString',
line: 5,
column: 13,
},
],
},
{
code: `
declare function assert(this: object, a: number, b: unknown): asserts b;
declare const nullableString: string | null;
assert(foo, nullableString);
`,
output: null,
errors: [
{
messageId: 'conditionErrorNullableString',
line: 4,
column: 13,
},
],
},
{
code: `
function asserts1(x: string | number | undefined): asserts x {}
function asserts2(x: string | number | undefined): asserts x {}
const maybeString = Math.random() ? 'string'.slice() : undefined;
const someAssert: typeof asserts1 | typeof asserts2 =
Math.random() > 0.5 ? asserts1 : asserts2;
someAssert(maybeString);
`,
output: null,
errors: [
{
messageId: 'conditionErrorNullableString',
},
],
},
],
});

0 comments on commit 524c870

Please sign in to comment.