Skip to content

Commit

Permalink
feat(49962): Disallow comparison against NaN (#50626)
Browse files Browse the repository at this point in the history
* feat(49962): disallow comparison against NaN

* change diagnostic message

* use global NaN symbol for NaN equality comparisons
  • Loading branch information
a-tarasyuk authored Sep 20, 2022
1 parent 23746af commit e002159
Show file tree
Hide file tree
Showing 17 changed files with 651 additions and 10 deletions.
29 changes: 29 additions & 0 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -999,6 +999,7 @@ namespace ts {
let deferredGlobalOmitSymbol: Symbol | undefined;
let deferredGlobalAwaitedSymbol: Symbol | undefined;
let deferredGlobalBigIntType: ObjectType | undefined;
let deferredGlobalNaNSymbol: Symbol | undefined;
let deferredGlobalRecordSymbol: Symbol | undefined;

const allPotentiallyUnusedIdentifiers = new Map<Path, PotentiallyUnusedIdentifier[]>(); // key is file name
Expand Down Expand Up @@ -14343,6 +14344,10 @@ namespace ts {
return (deferredGlobalBigIntType ||= getGlobalType("BigInt" as __String, /*arity*/ 0, /*reportErrors*/ false)) || emptyObjectType;
}

function getGlobalNaNSymbol(): Symbol | undefined {
return (deferredGlobalNaNSymbol ||= getGlobalValueSymbol("NaN" as __String, /*reportErrors*/ false));
}

function getGlobalRecordSymbol(): Symbol | undefined {
deferredGlobalRecordSymbol ||= getGlobalTypeAliasSymbol("Record" as __String, /*arity*/ 2, /*reportErrors*/ true) || unknownSymbol;
return deferredGlobalRecordSymbol === unknownSymbol ? undefined : deferredGlobalRecordSymbol;
Expand Down Expand Up @@ -34495,6 +34500,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 @@ -34727,6 +34733,29 @@ namespace ts {
return undefined;
}
}

function checkNaNEquality(errorNode: Node | undefined, operator: SyntaxKind, left: Expression, right: Expression) {
const isLeftNaN = isGlobalNaN(skipParentheses(left));
const isRightNaN = isGlobalNaN(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 operatorString = operator === SyntaxKind.ExclamationEqualsEqualsToken || operator === SyntaxKind.ExclamationEqualsToken ? tokenToString(SyntaxKind.ExclamationToken) : "";
const location = isLeftNaN ? right : left;
const expression = skipParentheses(location);
addRelatedInfo(err, createDiagnosticForNode(location, Diagnostics.Did_you_mean_0,
`${operatorString}Number.isNaN(${isEntityNameExpression(expression) ? entityNameToString(expression) : "..."})`));
}
}

function isGlobalNaN(expr: Expression): boolean {
if (isIdentifier(expr) && expr.escapedText === "NaN") {
const globalNaNSymbol = getGlobalNaNSymbol();
return !!globalNaNSymbol && globalNaNSymbol === getResolvedSymbol(expr);
}
return false;
}
}

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 @@ -3563,6 +3563,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 @@ -7356,7 +7360,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 @@ -737,4 +737,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
109 changes: 109 additions & 0 deletions tests/baselines/reference/nanEquality.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
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(29,5): error TS2845: This condition will always return 'false'.


==== tests/cases/compiler/nanEquality.ts (17 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'.

// ...
declare let y: any;
if (NaN === y[0][1]) {}
~~~~~~~~~~~~~~~
!!! error TS2845: This condition will always return 'false'.
!!! related TS1369 tests/cases/compiler/nanEquality.ts:29:13: Did you mean 'Number.isNaN(...)'?

function t1(value: number, NaN: number) {
return value === NaN; // ok
}

function t2(value: number, NaN: number) {
return NaN == value; // ok
}

function t3(NaN: number) {
return NaN === NaN; // ok
}

71 changes: 71 additions & 0 deletions tests/baselines/reference/nanEquality.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
//// [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) {}

// ...
declare let y: any;
if (NaN === y[0][1]) {}

function t1(value: number, NaN: number) {
return value === NaN; // ok
}

function t2(value: number, NaN: number) {
return NaN == value; // ok
}

function t3(NaN: number) {
return NaN === NaN; // ok
}


//// [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) { }
if (NaN === y[0][1]) { }
function t1(value, NaN) {
return value === NaN; // ok
}
function t2(value, NaN) {
return NaN == value; // ok
}
function t3(NaN) {
return NaN === NaN; // ok
}
Loading

0 comments on commit e002159

Please sign in to comment.