Skip to content

Commit

Permalink
feat(49962): disallow comparison against NaN
Browse files Browse the repository at this point in the history
  • Loading branch information
a-tarasyuk committed Sep 3, 2022
1 parent 7910c50 commit a51aa40
Show file tree
Hide file tree
Showing 16 changed files with 476 additions and 10 deletions.
18 changes: 18 additions & 0 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34386,6 +34386,7 @@ namespace ts {
const eqType = operator === SyntaxKind.EqualsEqualsToken || operator === SyntaxKind.EqualsEqualsEqualsToken;
error(errorNode, Diagnostics.This_condition_will_always_return_0_since_JavaScript_compares_objects_by_reference_not_value, eqType ? "false" : "true");
}
checkNaNEquality(errorNode, operator, left, right);
reportOperatorErrorUnless((left, right) => isTypeEqualityComparableTo(left, right) || isTypeEqualityComparableTo(right, left));
return booleanType;

Expand Down Expand Up @@ -34618,6 +34619,23 @@ namespace ts {
return undefined;
}
}

function checkNaNEquality(errorNode: Node | undefined, operator: SyntaxKind, left: Expression, right: Expression) {
const isLeftNaN = isNaN(skipParentheses(left));
const isRightNaN = isNaN(skipParentheses(right));
if (isLeftNaN || isRightNaN) {
const err = error(errorNode, Diagnostics.This_condition_will_always_return_0,
tokenToString(operator === SyntaxKind.EqualsEqualsEqualsToken || operator === SyntaxKind.EqualsEqualsToken ? SyntaxKind.FalseKeyword : SyntaxKind.TrueKeyword));
if (isLeftNaN && isRightNaN) return;
const location = isLeftNaN ? right : left;
addRelatedInfo(err, createDiagnosticForNode(location, Diagnostics.Did_you_mean_0,
`${operator === SyntaxKind.ExclamationEqualsEqualsToken || operator === SyntaxKind.ExclamationEqualsToken ? tokenToString(SyntaxKind.ExclamationToken) : ""}Number.isNaN(${getTextOfNode(location)})`));
}
}

function isNaN(expr: Expression): boolean {
return isIdentifier(expr) && expr.escapedText === "NaN";
}
}

function getBaseTypesIfUnrelated(leftType: Type, rightType: Type, isRelated: (left: Type, right: Type) => boolean): [Type, Type] {
Expand Down
13 changes: 12 additions & 1 deletion src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -3559,6 +3559,10 @@
"category": "Error",
"code": 2844
},
"This condition will always return '{0}'.": {
"category": "Error",
"code": 2845
},

"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
Expand Down Expand Up @@ -7352,7 +7356,14 @@
"category": "Message",
"code": 95173
},

"Use `{0}`.": {
"category": "Message",
"code": 95174
},
"Use `Number.isNaN` in all conditions.": {
"category": "Message",
"code": 95175
},

"No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": {
"category": "Error",
Expand Down
9 changes: 0 additions & 9 deletions src/services/codefixes/fixAddMissingConstraint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,6 @@ namespace ts.codefix {
}
}

function findAncestorMatchingSpan(sourceFile: SourceFile, span: TextSpan): Node {
const end = textSpanEnd(span);
let token = getTokenAtPosition(sourceFile, span.start);
while (token.end < end) {
token = token.parent;
}
return token;
}

function tryGetConstraintFromDiagnosticMessage(messageText: string | DiagnosticMessageChain) {
const [_, constraint] = flattenDiagnosticMessageText(messageText, "\n", 0).match(/`extends (.*)`/) || [];
return constraint;
Expand Down
65 changes: 65 additions & 0 deletions src/services/codefixes/fixNaNEquality.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/* @internal */
namespace ts.codefix {
const fixId = "fixNaNEquality";
const errorCodes = [
Diagnostics.This_condition_will_always_return_0.code,
];

registerCodeFix({
errorCodes,
getCodeActions(context) {
const { sourceFile, span, program } = context;
const info = getInfo(program, sourceFile, span);
if (info === undefined) return;

const { suggestion, expression, arg } = info;
const changes = textChanges.ChangeTracker.with(context, t => doChange(t, sourceFile, arg, expression));
return [createCodeFixAction(fixId, changes, [Diagnostics.Use_0, suggestion], fixId, Diagnostics.Use_Number_isNaN_in_all_conditions)];
},
fixIds: [fixId],
getAllCodeActions: context => {
return codeFixAll(context, errorCodes, (changes, diag) => {
const info = getInfo(context.program, diag.file, createTextSpan(diag.start, diag.length));
if (info) {
doChange(changes, diag.file, info.arg, info.expression);
}
});
}
});

interface Info {
suggestion: string;
expression: BinaryExpression;
arg: Expression;
}

function getInfo(program: Program, sourceFile: SourceFile, span: TextSpan): Info | undefined {
const diag = find(program.getSemanticDiagnostics(sourceFile), diag => diag.start === span.start && diag.length === span.length);
if (diag === undefined || diag.relatedInformation === undefined) return;

const related = find(diag.relatedInformation, related => related.code === Diagnostics.Did_you_mean_0.code);
if (related === undefined || related.file === undefined || related.start === undefined || related.length === undefined) return;

const token = findAncestorMatchingSpan(related.file, createTextSpan(related.start, related.length));
if (token === undefined) return;

if (isExpression(token) && isBinaryExpression(token.parent)) {
return { suggestion: getSuggestion(related.messageText), expression: token.parent, arg: token };
}
return undefined;
}

function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, arg: Expression, expression: BinaryExpression) {
const callExpression = factory.createCallExpression(
factory.createPropertyAccessExpression(factory.createIdentifier("Number"), factory.createIdentifier("isNaN")), /*typeArguments*/ undefined, [arg]);
const operator = expression.operatorToken.kind ;
changes.replaceNode(sourceFile, expression,
operator === SyntaxKind.ExclamationEqualsEqualsToken || operator === SyntaxKind.ExclamationEqualsToken
? factory.createPrefixUnaryExpression(SyntaxKind.ExclamationToken, callExpression) : callExpression);
}

function getSuggestion(messageText: string | DiagnosticMessageChain) {
const [_, suggestion] = flattenDiagnosticMessageText(messageText, "\n", 0).match(/\'(.*)\'/) || [];
return suggestion;
}
}
9 changes: 9 additions & 0 deletions src/services/codefixes/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -743,4 +743,13 @@ namespace ts.codefix {
export function importSymbols(importAdder: ImportAdder, symbols: readonly Symbol[]) {
symbols.forEach(s => importAdder.addImportFromExportedSymbol(s, /*isValidTypeOnlyUseSite*/ true));
}

export function findAncestorMatchingSpan(sourceFile: SourceFile, span: TextSpan): Node {
const end = textSpanEnd(span);
let token = getTokenAtPosition(sourceFile, span.start);
while (token.end < end) {
token = token.parent;
}
return token;
}
}
1 change: 1 addition & 0 deletions src/services/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
"codefixes/fixConstructorForDerivedNeedSuperCall.ts",
"codefixes/fixEnableExperimentalDecorators.ts",
"codefixes/fixEnableJsxFlag.ts",
"codefixes/fixNaNEquality.ts",
"codefixes/fixModuleAndTargetOptions.ts",
"codefixes/fixPropertyAssignment.ts",
"codefixes/fixExtendsInterfaceBecomesImplements.ts",
Expand Down
89 changes: 89 additions & 0 deletions tests/baselines/reference/nanEquality.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
tests/cases/compiler/nanEquality.ts(3,5): error TS2845: This condition will always return 'false'.
tests/cases/compiler/nanEquality.ts(4,5): error TS2845: This condition will always return 'false'.
tests/cases/compiler/nanEquality.ts(6,5): error TS2845: This condition will always return 'false'.
tests/cases/compiler/nanEquality.ts(7,5): error TS2845: This condition will always return 'false'.
tests/cases/compiler/nanEquality.ts(9,5): error TS2845: This condition will always return 'true'.
tests/cases/compiler/nanEquality.ts(10,5): error TS2845: This condition will always return 'true'.
tests/cases/compiler/nanEquality.ts(12,5): error TS2845: This condition will always return 'true'.
tests/cases/compiler/nanEquality.ts(13,5): error TS2845: This condition will always return 'true'.
tests/cases/compiler/nanEquality.ts(15,5): error TS2845: This condition will always return 'false'.
tests/cases/compiler/nanEquality.ts(16,5): error TS2845: This condition will always return 'false'.
tests/cases/compiler/nanEquality.ts(18,5): error TS2845: This condition will always return 'true'.
tests/cases/compiler/nanEquality.ts(19,5): error TS2845: This condition will always return 'true'.
tests/cases/compiler/nanEquality.ts(21,5): error TS2845: This condition will always return 'false'.
tests/cases/compiler/nanEquality.ts(22,5): error TS2845: This condition will always return 'true'.
tests/cases/compiler/nanEquality.ts(24,5): error TS2845: This condition will always return 'false'.
tests/cases/compiler/nanEquality.ts(25,5): error TS2845: This condition will always return 'true'.


==== tests/cases/compiler/nanEquality.ts (16 errors) ====
declare const x: number;

if (x === NaN) {}
~~~~~~~~~
!!! error TS2845: This condition will always return 'false'.
!!! related TS1369 tests/cases/compiler/nanEquality.ts:3:5: Did you mean 'Number.isNaN(x)'?
if (NaN === x) {}
~~~~~~~~~
!!! error TS2845: This condition will always return 'false'.
!!! related TS1369 tests/cases/compiler/nanEquality.ts:4:13: Did you mean 'Number.isNaN(x)'?

if (x == NaN) {}
~~~~~~~~
!!! error TS2845: This condition will always return 'false'.
!!! related TS1369 tests/cases/compiler/nanEquality.ts:6:5: Did you mean 'Number.isNaN(x)'?
if (NaN == x) {}
~~~~~~~~
!!! error TS2845: This condition will always return 'false'.
!!! related TS1369 tests/cases/compiler/nanEquality.ts:7:12: Did you mean 'Number.isNaN(x)'?

if (x !== NaN) {}
~~~~~~~~~
!!! error TS2845: This condition will always return 'true'.
!!! related TS1369 tests/cases/compiler/nanEquality.ts:9:5: Did you mean '!Number.isNaN(x)'?
if (NaN !== x) {}
~~~~~~~~~
!!! error TS2845: This condition will always return 'true'.
!!! related TS1369 tests/cases/compiler/nanEquality.ts:10:13: Did you mean '!Number.isNaN(x)'?

if (x != NaN) {}
~~~~~~~~
!!! error TS2845: This condition will always return 'true'.
!!! related TS1369 tests/cases/compiler/nanEquality.ts:12:5: Did you mean '!Number.isNaN(x)'?
if (NaN != x) {}
~~~~~~~~
!!! error TS2845: This condition will always return 'true'.
!!! related TS1369 tests/cases/compiler/nanEquality.ts:13:12: Did you mean '!Number.isNaN(x)'?

if (x === ((NaN))) {}
~~~~~~~~~~~~~
!!! error TS2845: This condition will always return 'false'.
!!! related TS1369 tests/cases/compiler/nanEquality.ts:15:5: Did you mean 'Number.isNaN(x)'?
if (((NaN)) === x) {}
~~~~~~~~~~~~~
!!! error TS2845: This condition will always return 'false'.
!!! related TS1369 tests/cases/compiler/nanEquality.ts:16:17: Did you mean 'Number.isNaN(x)'?

if (x !== ((NaN))) {}
~~~~~~~~~~~~~
!!! error TS2845: This condition will always return 'true'.
!!! related TS1369 tests/cases/compiler/nanEquality.ts:18:5: Did you mean '!Number.isNaN(x)'?
if (((NaN)) !== x) {}
~~~~~~~~~~~~~
!!! error TS2845: This condition will always return 'true'.
!!! related TS1369 tests/cases/compiler/nanEquality.ts:19:17: Did you mean '!Number.isNaN(x)'?

if (NaN === NaN) {}
~~~~~~~~~~~
!!! error TS2845: This condition will always return 'false'.
if (NaN !== NaN) {}
~~~~~~~~~~~
!!! error TS2845: This condition will always return 'true'.

if (NaN == NaN) {}
~~~~~~~~~~
!!! error TS2845: This condition will always return 'false'.
if (NaN != NaN) {}
~~~~~~~~~~
!!! error TS2845: This condition will always return 'true'.

45 changes: 45 additions & 0 deletions tests/baselines/reference/nanEquality.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
//// [nanEquality.ts]
declare const x: number;

if (x === NaN) {}
if (NaN === x) {}

if (x == NaN) {}
if (NaN == x) {}

if (x !== NaN) {}
if (NaN !== x) {}

if (x != NaN) {}
if (NaN != x) {}

if (x === ((NaN))) {}
if (((NaN)) === x) {}

if (x !== ((NaN))) {}
if (((NaN)) !== x) {}

if (NaN === NaN) {}
if (NaN !== NaN) {}

if (NaN == NaN) {}
if (NaN != NaN) {}


//// [nanEquality.js]
if (x === NaN) { }
if (NaN === x) { }
if (x == NaN) { }
if (NaN == x) { }
if (x !== NaN) { }
if (NaN !== x) { }
if (x != NaN) { }
if (NaN != x) { }
if (x === ((NaN))) { }
if (((NaN)) === x) { }
if (x !== ((NaN))) { }
if (((NaN)) !== x) { }
if (NaN === NaN) { }
if (NaN !== NaN) { }
if (NaN == NaN) { }
if (NaN != NaN) { }
68 changes: 68 additions & 0 deletions tests/baselines/reference/nanEquality.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
=== tests/cases/compiler/nanEquality.ts ===
declare const x: number;
>x : Symbol(x, Decl(nanEquality.ts, 0, 13))

if (x === NaN) {}
>x : Symbol(x, Decl(nanEquality.ts, 0, 13))
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))

if (NaN === x) {}
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))
>x : Symbol(x, Decl(nanEquality.ts, 0, 13))

if (x == NaN) {}
>x : Symbol(x, Decl(nanEquality.ts, 0, 13))
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))

if (NaN == x) {}
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))
>x : Symbol(x, Decl(nanEquality.ts, 0, 13))

if (x !== NaN) {}
>x : Symbol(x, Decl(nanEquality.ts, 0, 13))
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))

if (NaN !== x) {}
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))
>x : Symbol(x, Decl(nanEquality.ts, 0, 13))

if (x != NaN) {}
>x : Symbol(x, Decl(nanEquality.ts, 0, 13))
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))

if (NaN != x) {}
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))
>x : Symbol(x, Decl(nanEquality.ts, 0, 13))

if (x === ((NaN))) {}
>x : Symbol(x, Decl(nanEquality.ts, 0, 13))
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))

if (((NaN)) === x) {}
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))
>x : Symbol(x, Decl(nanEquality.ts, 0, 13))

if (x !== ((NaN))) {}
>x : Symbol(x, Decl(nanEquality.ts, 0, 13))
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))

if (((NaN)) !== x) {}
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))
>x : Symbol(x, Decl(nanEquality.ts, 0, 13))

if (NaN === NaN) {}
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))

if (NaN !== NaN) {}
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))

if (NaN == NaN) {}
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))

if (NaN != NaN) {}
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))
>NaN : Symbol(NaN, Decl(lib.es5.d.ts, --, --))

Loading

0 comments on commit a51aa40

Please sign in to comment.