Merge pull request #33178 from jwbay/nonNullableCallSignaturesTreeWalk

Flag non-nullable functions in `if` statements as errors (tree walk version)
This commit is contained in:
Orta 2019-09-25 14:06:24 -04:00 committed by GitHub
commit 91be3689f0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 701 additions and 15 deletions

View File

@ -28306,7 +28306,8 @@ namespace ts {
// Grammar checking
checkGrammarStatementInAmbientContext(node);
checkTruthinessExpression(node.expression);
const type = checkTruthinessExpression(node.expression);
checkTestingKnownTruthyCallableType(node, type);
checkSourceElement(node.thenStatement);
if (node.thenStatement.kind === SyntaxKind.EmptyStatement) {
@ -28316,6 +28317,57 @@ namespace ts {
checkSourceElement(node.elseStatement);
}
function checkTestingKnownTruthyCallableType(ifStatement: IfStatement, type: Type) {
if (!strictNullChecks) {
return;
}
const testedNode = isIdentifier(ifStatement.expression)
? ifStatement.expression
: isPropertyAccessExpression(ifStatement.expression)
? ifStatement.expression.name
: undefined;
if (!testedNode) {
return;
}
const possiblyFalsy = getFalsyFlags(type);
if (possiblyFalsy) {
return;
}
// While it technically should be invalid for any known-truthy value
// to be tested, we de-scope to functions unrefenced in the block as a
// heuristic to identify the most common bugs. There are too many
// false positives for values sourced from type definitions without
// strictNullChecks otherwise.
const callSignatures = getSignaturesOfType(type, SignatureKind.Call);
if (callSignatures.length === 0) {
return;
}
const testedFunctionSymbol = getSymbolAtLocation(testedNode);
if (!testedFunctionSymbol) {
return;
}
const functionIsUsedInBody = forEachChild(ifStatement.thenStatement, function check(childNode): boolean | undefined {
if (isIdentifier(childNode)) {
const childSymbol = getSymbolAtLocation(childNode);
if (childSymbol && childSymbol.id === testedFunctionSymbol.id) {
return true;
}
}
return forEachChild(childNode, check);
});
if (!functionIsUsedInBody) {
error(ifStatement.expression, Diagnostics.This_condition_will_always_return_true_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead);
}
}
function checkDoStatement(node: DoStatement) {
// Grammar checking
checkGrammarStatementInAmbientContext(node);

View File

@ -1654,7 +1654,7 @@ namespace ts {
*/
export function compose<T>(...args: ((t: T) => T)[]): (t: T) => T;
export function compose<T>(a: (t: T) => T, b: (t: T) => T, c: (t: T) => T, d: (t: T) => T, e: (t: T) => T): (t: T) => T {
if (e) {
if (!!e) {
const args: ((t: T) => T)[] = [];
for (let i = 0; i < arguments.length; i++) {
args[i] = arguments[i];

View File

@ -2722,7 +2722,10 @@
"category": "Error",
"code": 2773
},
"This condition will always return true since the function is always defined. Did you mean to call it instead?": {
"category": "Error",
"code": 2774
},
"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
"code": 4000

View File

@ -1389,18 +1389,16 @@ namespace ts {
// try to verify results of module resolution
for (const { oldFile: oldSourceFile, newFile: newSourceFile } of modifiedSourceFiles) {
const newSourceFilePath = getNormalizedAbsolutePath(newSourceFile.originalFileName, currentDirectory);
if (resolveModuleNamesWorker) {
const moduleNames = getModuleNames(newSourceFile);
const resolutions = resolveModuleNamesReusingOldState(moduleNames, newSourceFilePath, newSourceFile);
// ensure that module resolution results are still correct
const resolutionsChanged = hasChangesInResolutions(moduleNames, resolutions, oldSourceFile.resolvedModules, moduleResolutionIsEqualTo);
if (resolutionsChanged) {
oldProgram.structureIsReused = StructureIsReused.SafeModules;
newSourceFile.resolvedModules = zipToMap(moduleNames, resolutions);
}
else {
newSourceFile.resolvedModules = oldSourceFile.resolvedModules;
}
const moduleNames = getModuleNames(newSourceFile);
const resolutions = resolveModuleNamesReusingOldState(moduleNames, newSourceFilePath, newSourceFile);
// ensure that module resolution results are still correct
const resolutionsChanged = hasChangesInResolutions(moduleNames, resolutions, oldSourceFile.resolvedModules, moduleResolutionIsEqualTo);
if (resolutionsChanged) {
oldProgram.structureIsReused = StructureIsReused.SafeModules;
newSourceFile.resolvedModules = zipToMap(moduleNames, resolutions);
}
else {
newSourceFile.resolvedModules = oldSourceFile.resolvedModules;
}
if (resolveTypeReferenceDirectiveNamesWorker) {
// We lower-case all type references because npm automatically lowercases all packages. See GH#9824.

View File

@ -0,0 +1,91 @@
tests/cases/compiler/truthinessCallExpressionCoercion.ts(2,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
tests/cases/compiler/truthinessCallExpressionCoercion.ts(18,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
tests/cases/compiler/truthinessCallExpressionCoercion.ts(36,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
tests/cases/compiler/truthinessCallExpressionCoercion.ts(50,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
tests/cases/compiler/truthinessCallExpressionCoercion.ts(66,13): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
==== tests/cases/compiler/truthinessCallExpressionCoercion.ts (5 errors) ====
function onlyErrorsWhenTestingNonNullableFunctionType(required: () => boolean, optional?: () => boolean) {
if (required) { // error
~~~~~~~~
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
}
if (optional) { // ok
}
if (!!required) { // ok
}
if (required()) { // ok
}
}
function onlyErrorsWhenUnusedInBody() {
function test() { return Math.random() > 0.5; }
if (test) { // error
~~~~
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
console.log('test');
}
if (test) { // ok
console.log(test);
}
if (test) { // ok
test();
}
if (test) { // ok
[() => null].forEach(() => {
test();
});
}
if (test) { // error
~~~~
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
[() => null].forEach(test => {
test();
});
}
}
function checksPropertyAccess() {
const x = {
foo: {
bar() { return true; }
}
}
if (x.foo.bar) { // error
~~~~~~~~~
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
}
if (x.foo.bar) { // ok
x.foo.bar;
}
}
class Foo {
maybeIsUser?: () => boolean;
isUser() {
return true;
}
test() {
if (this.isUser) { // error
~~~~~~~~~~~
!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
}
if (this.maybeIsUser) { // ok
}
}
}

View File

@ -0,0 +1,134 @@
//// [truthinessCallExpressionCoercion.ts]
function onlyErrorsWhenTestingNonNullableFunctionType(required: () => boolean, optional?: () => boolean) {
if (required) { // error
}
if (optional) { // ok
}
if (!!required) { // ok
}
if (required()) { // ok
}
}
function onlyErrorsWhenUnusedInBody() {
function test() { return Math.random() > 0.5; }
if (test) { // error
console.log('test');
}
if (test) { // ok
console.log(test);
}
if (test) { // ok
test();
}
if (test) { // ok
[() => null].forEach(() => {
test();
});
}
if (test) { // error
[() => null].forEach(test => {
test();
});
}
}
function checksPropertyAccess() {
const x = {
foo: {
bar() { return true; }
}
}
if (x.foo.bar) { // error
}
if (x.foo.bar) { // ok
x.foo.bar;
}
}
class Foo {
maybeIsUser?: () => boolean;
isUser() {
return true;
}
test() {
if (this.isUser) { // error
}
if (this.maybeIsUser) { // ok
}
}
}
//// [truthinessCallExpressionCoercion.js]
function onlyErrorsWhenTestingNonNullableFunctionType(required, optional) {
if (required) { // error
}
if (optional) { // ok
}
if (!!required) { // ok
}
if (required()) { // ok
}
}
function onlyErrorsWhenUnusedInBody() {
function test() { return Math.random() > 0.5; }
if (test) { // error
console.log('test');
}
if (test) { // ok
console.log(test);
}
if (test) { // ok
test();
}
if (test) { // ok
[function () { return null; }].forEach(function () {
test();
});
}
if (test) { // error
[function () { return null; }].forEach(function (test) {
test();
});
}
}
function checksPropertyAccess() {
var x = {
foo: {
bar: function () { return true; }
}
};
if (x.foo.bar) { // error
}
if (x.foo.bar) { // ok
x.foo.bar;
}
}
var Foo = /** @class */ (function () {
function Foo() {
}
Foo.prototype.isUser = function () {
return true;
};
Foo.prototype.test = function () {
if (this.isUser) { // error
}
if (this.maybeIsUser) { // ok
}
};
return Foo;
}());

View File

@ -0,0 +1,153 @@
=== tests/cases/compiler/truthinessCallExpressionCoercion.ts ===
function onlyErrorsWhenTestingNonNullableFunctionType(required: () => boolean, optional?: () => boolean) {
>onlyErrorsWhenTestingNonNullableFunctionType : Symbol(onlyErrorsWhenTestingNonNullableFunctionType, Decl(truthinessCallExpressionCoercion.ts, 0, 0))
>required : Symbol(required, Decl(truthinessCallExpressionCoercion.ts, 0, 54))
>optional : Symbol(optional, Decl(truthinessCallExpressionCoercion.ts, 0, 78))
if (required) { // error
>required : Symbol(required, Decl(truthinessCallExpressionCoercion.ts, 0, 54))
}
if (optional) { // ok
>optional : Symbol(optional, Decl(truthinessCallExpressionCoercion.ts, 0, 78))
}
if (!!required) { // ok
>required : Symbol(required, Decl(truthinessCallExpressionCoercion.ts, 0, 54))
}
if (required()) { // ok
>required : Symbol(required, Decl(truthinessCallExpressionCoercion.ts, 0, 54))
}
}
function onlyErrorsWhenUnusedInBody() {
>onlyErrorsWhenUnusedInBody : Symbol(onlyErrorsWhenUnusedInBody, Decl(truthinessCallExpressionCoercion.ts, 12, 1))
function test() { return Math.random() > 0.5; }
>test : Symbol(test, Decl(truthinessCallExpressionCoercion.ts, 14, 39))
>Math.random : Symbol(Math.random, Decl(lib.es5.d.ts, --, --))
>Math : Symbol(Math, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>random : Symbol(Math.random, Decl(lib.es5.d.ts, --, --))
if (test) { // error
>test : Symbol(test, Decl(truthinessCallExpressionCoercion.ts, 14, 39))
console.log('test');
>console.log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
>console : Symbol(console, Decl(lib.dom.d.ts, --, --))
>log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
}
if (test) { // ok
>test : Symbol(test, Decl(truthinessCallExpressionCoercion.ts, 14, 39))
console.log(test);
>console.log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
>console : Symbol(console, Decl(lib.dom.d.ts, --, --))
>log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --))
>test : Symbol(test, Decl(truthinessCallExpressionCoercion.ts, 14, 39))
}
if (test) { // ok
>test : Symbol(test, Decl(truthinessCallExpressionCoercion.ts, 14, 39))
test();
>test : Symbol(test, Decl(truthinessCallExpressionCoercion.ts, 14, 39))
}
if (test) { // ok
>test : Symbol(test, Decl(truthinessCallExpressionCoercion.ts, 14, 39))
[() => null].forEach(() => {
>[() => null].forEach : Symbol(Array.forEach, Decl(lib.es5.d.ts, --, --))
>forEach : Symbol(Array.forEach, Decl(lib.es5.d.ts, --, --))
test();
>test : Symbol(test, Decl(truthinessCallExpressionCoercion.ts, 14, 39))
});
}
if (test) { // error
>test : Symbol(test, Decl(truthinessCallExpressionCoercion.ts, 14, 39))
[() => null].forEach(test => {
>[() => null].forEach : Symbol(Array.forEach, Decl(lib.es5.d.ts, --, --))
>forEach : Symbol(Array.forEach, Decl(lib.es5.d.ts, --, --))
>test : Symbol(test, Decl(truthinessCallExpressionCoercion.ts, 36, 29))
test();
>test : Symbol(test, Decl(truthinessCallExpressionCoercion.ts, 36, 29))
});
}
}
function checksPropertyAccess() {
>checksPropertyAccess : Symbol(checksPropertyAccess, Decl(truthinessCallExpressionCoercion.ts, 40, 1))
const x = {
>x : Symbol(x, Decl(truthinessCallExpressionCoercion.ts, 43, 9))
foo: {
>foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 43, 15))
bar() { return true; }
>bar : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 44, 14))
}
}
if (x.foo.bar) { // error
>x.foo.bar : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 44, 14))
>x.foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 43, 15))
>x : Symbol(x, Decl(truthinessCallExpressionCoercion.ts, 43, 9))
>foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 43, 15))
>bar : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 44, 14))
}
if (x.foo.bar) { // ok
>x.foo.bar : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 44, 14))
>x.foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 43, 15))
>x : Symbol(x, Decl(truthinessCallExpressionCoercion.ts, 43, 9))
>foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 43, 15))
>bar : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 44, 14))
x.foo.bar;
>x.foo.bar : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 44, 14))
>x.foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 43, 15))
>x : Symbol(x, Decl(truthinessCallExpressionCoercion.ts, 43, 9))
>foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 43, 15))
>bar : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 44, 14))
}
}
class Foo {
>Foo : Symbol(Foo, Decl(truthinessCallExpressionCoercion.ts, 55, 1))
maybeIsUser?: () => boolean;
>maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion.ts, 57, 11))
isUser() {
>isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion.ts, 58, 32))
return true;
}
test() {
>test : Symbol(Foo.test, Decl(truthinessCallExpressionCoercion.ts, 62, 5))
if (this.isUser) { // error
>this.isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion.ts, 58, 32))
>this : Symbol(Foo, Decl(truthinessCallExpressionCoercion.ts, 55, 1))
>isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion.ts, 58, 32))
}
if (this.maybeIsUser) { // ok
>this.maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion.ts, 57, 11))
>this : Symbol(Foo, Decl(truthinessCallExpressionCoercion.ts, 55, 1))
>maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion.ts, 57, 11))
}
}
}

View File

@ -0,0 +1,179 @@
=== tests/cases/compiler/truthinessCallExpressionCoercion.ts ===
function onlyErrorsWhenTestingNonNullableFunctionType(required: () => boolean, optional?: () => boolean) {
>onlyErrorsWhenTestingNonNullableFunctionType : (required: () => boolean, optional?: (() => boolean) | undefined) => void
>required : () => boolean
>optional : (() => boolean) | undefined
if (required) { // error
>required : () => boolean
}
if (optional) { // ok
>optional : (() => boolean) | undefined
}
if (!!required) { // ok
>!!required : true
>!required : false
>required : () => boolean
}
if (required()) { // ok
>required() : boolean
>required : () => boolean
}
}
function onlyErrorsWhenUnusedInBody() {
>onlyErrorsWhenUnusedInBody : () => void
function test() { return Math.random() > 0.5; }
>test : () => boolean
>Math.random() > 0.5 : boolean
>Math.random() : number
>Math.random : () => number
>Math : Math
>random : () => number
>0.5 : 0.5
if (test) { // error
>test : () => boolean
console.log('test');
>console.log('test') : void
>console.log : (message?: any, ...optionalParams: any[]) => void
>console : Console
>log : (message?: any, ...optionalParams: any[]) => void
>'test' : "test"
}
if (test) { // ok
>test : () => boolean
console.log(test);
>console.log(test) : void
>console.log : (message?: any, ...optionalParams: any[]) => void
>console : Console
>log : (message?: any, ...optionalParams: any[]) => void
>test : () => boolean
}
if (test) { // ok
>test : () => boolean
test();
>test() : boolean
>test : () => boolean
}
if (test) { // ok
>test : () => boolean
[() => null].forEach(() => {
>[() => null].forEach(() => { test(); }) : void
>[() => null].forEach : (callbackfn: (value: () => null, index: number, array: (() => null)[]) => void, thisArg?: any) => void
>[() => null] : (() => null)[]
>() => null : () => null
>null : null
>forEach : (callbackfn: (value: () => null, index: number, array: (() => null)[]) => void, thisArg?: any) => void
>() => { test(); } : () => void
test();
>test() : boolean
>test : () => boolean
});
}
if (test) { // error
>test : () => boolean
[() => null].forEach(test => {
>[() => null].forEach(test => { test(); }) : void
>[() => null].forEach : (callbackfn: (value: () => null, index: number, array: (() => null)[]) => void, thisArg?: any) => void
>[() => null] : (() => null)[]
>() => null : () => null
>null : null
>forEach : (callbackfn: (value: () => null, index: number, array: (() => null)[]) => void, thisArg?: any) => void
>test => { test(); } : (test: () => null) => void
>test : () => null
test();
>test() : null
>test : () => null
});
}
}
function checksPropertyAccess() {
>checksPropertyAccess : () => void
const x = {
>x : { foo: { bar(): boolean; }; }
>{ foo: { bar() { return true; } } } : { foo: { bar(): boolean; }; }
foo: {
>foo : { bar(): boolean; }
>{ bar() { return true; } } : { bar(): boolean; }
bar() { return true; }
>bar : () => boolean
>true : true
}
}
if (x.foo.bar) { // error
>x.foo.bar : () => boolean
>x.foo : { bar(): boolean; }
>x : { foo: { bar(): boolean; }; }
>foo : { bar(): boolean; }
>bar : () => boolean
}
if (x.foo.bar) { // ok
>x.foo.bar : () => boolean
>x.foo : { bar(): boolean; }
>x : { foo: { bar(): boolean; }; }
>foo : { bar(): boolean; }
>bar : () => boolean
x.foo.bar;
>x.foo.bar : () => boolean
>x.foo : { bar(): boolean; }
>x : { foo: { bar(): boolean; }; }
>foo : { bar(): boolean; }
>bar : () => boolean
}
}
class Foo {
>Foo : Foo
maybeIsUser?: () => boolean;
>maybeIsUser : (() => boolean) | undefined
isUser() {
>isUser : () => boolean
return true;
>true : true
}
test() {
>test : () => void
if (this.isUser) { // error
>this.isUser : () => boolean
>this : this
>isUser : () => boolean
}
if (this.maybeIsUser) { // ok
>this.maybeIsUser : (() => boolean) | undefined
>this : this
>maybeIsUser : (() => boolean) | undefined
}
}
}

View File

@ -1153,6 +1153,8 @@ node_modules/npm/test/tap/check-engine-reqs.js(4,20): error TS2307: Cannot find
node_modules/npm/test/tap/check-install-self.js(4,20): error TS2307: Cannot find module 'tap'.
node_modules/npm/test/tap/check-os-reqs.js(4,20): error TS2307: Cannot find module 'tap'.
node_modules/npm/test/tap/check-permissions.js(4,20): error TS2307: Cannot find module 'tap'.
node_modules/npm/test/tap/check-permissions.js(26,7): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
node_modules/npm/test/tap/check-permissions.js(42,7): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead?
node_modules/npm/test/tap/ci-header.js(3,20): error TS2307: Cannot find module 'tap'.
node_modules/npm/test/tap/ci-header.js(4,18): error TS2307: Cannot find module 'npm-registry-mock'.
node_modules/npm/test/tap/ci-header.js(5,21): error TS2307: Cannot find module 'tacks'.

View File

@ -0,0 +1,74 @@
// @strictNullChecks:true
function onlyErrorsWhenTestingNonNullableFunctionType(required: () => boolean, optional?: () => boolean) {
if (required) { // error
}
if (optional) { // ok
}
if (!!required) { // ok
}
if (required()) { // ok
}
}
function onlyErrorsWhenUnusedInBody() {
function test() { return Math.random() > 0.5; }
if (test) { // error
console.log('test');
}
if (test) { // ok
console.log(test);
}
if (test) { // ok
test();
}
if (test) { // ok
[() => null].forEach(() => {
test();
});
}
if (test) { // error
[() => null].forEach(test => {
test();
});
}
}
function checksPropertyAccess() {
const x = {
foo: {
bar() { return true; }
}
}
if (x.foo.bar) { // error
}
if (x.foo.bar) { // ok
x.foo.bar;
}
}
class Foo {
maybeIsUser?: () => boolean;
isUser() {
return true;
}
test() {
if (this.isUser) { // error
}
if (this.maybeIsUser) { // ok
}
}
}