From 4223df0014ab03a0c46c45999cab7df9fede80c9 Mon Sep 17 00:00:00 2001 From: MaxGraey Date: Sat, 27 Aug 2022 13:12:43 +0300 Subject: [PATCH 1/7] add warning about NaN invariant during direct comparision --- src/compiler.ts | 21 ++++++++++++++++++--- src/diagnosticMessages.json | 1 + src/module.ts | 8 ++++++++ tests/compiler/number.debug.wat | 29 +++++++++++++++++++++++++++++ tests/compiler/number.json | 6 ++++++ tests/compiler/number.ts | 10 ++++++++++ 6 files changed, 72 insertions(+), 3 deletions(-) diff --git a/src/compiler.ts b/src/compiler.ts index e937313480..89c9b65222 100644 --- a/src/compiler.ts +++ b/src/compiler.ts @@ -52,7 +52,8 @@ import { SideEffects, SwitchBuilder, ExpressionRunnerFlags, - isConstZero + isConstZero, + isConstNaN } from "./module"; import { @@ -3992,7 +3993,14 @@ export class Compiler extends DiagnosticEmitter { this.currentType = contextualType; return module.unreachable(); } - + if (commonType.isFloatValue) { + if (isConstNaN(rightExpr) || isConstNaN(leftExpr)) { + this.warning( + DiagnosticCode.Comparision_with_NaN_is_invariant_You_need_to_use_isNaN_x, + expression.range + ); + } + } leftExpr = this.convertExpression(leftExpr, leftType, commonType, false, left); leftType = commonType; rightExpr = this.convertExpression(rightExpr, rightType, commonType, false, right); @@ -4028,7 +4036,14 @@ export class Compiler extends DiagnosticEmitter { this.currentType = contextualType; return module.unreachable(); } - + if (commonType.isFloatValue) { + if (isConstNaN(rightExpr) || isConstNaN(leftExpr)) { + this.warning( + DiagnosticCode.Comparision_with_NaN_is_invariant_You_need_to_use_isNaN_x, + expression.range + ); + } + } leftExpr = this.convertExpression(leftExpr, leftType, commonType, false, left); leftType = commonType; rightExpr = this.convertExpression(rightExpr, rightType, commonType, false, right); diff --git a/src/diagnosticMessages.json b/src/diagnosticMessages.json index 99486b1396..8e4bd2d25f 100644 --- a/src/diagnosticMessages.json +++ b/src/diagnosticMessages.json @@ -56,6 +56,7 @@ "Indexed access may involve bounds checking.": 904, "Explicitly returning constructor drops 'this' allocation.": 905, "Unnecessary definite assignment.": 906, + "Comparision with NaN is invariant. You need to use isNaN(x).": 907, "Unterminated string literal.": 1002, "Identifier expected.": 1003, diff --git a/src/module.ts b/src/module.ts index 9a4f079786..2577b773dc 100644 --- a/src/module.ts +++ b/src/module.ts @@ -2917,6 +2917,14 @@ export function isConstNonZero(expr: ExpressionRef): bool { return false; } +export function isConstNaN(expr: ExpressionRef): bool { + if (getExpressionId(expr) != ExpressionId.Const) return false; + var type = getExpressionType(expr); + if (type == TypeRef.F32) return isNaN(getConstValueF32(expr)); + if (type == TypeRef.F64) return isNaN(getConstValueF64(expr)); + return false; +} + export function getLocalGetIndex(expr: ExpressionRef): Index { return binaryen._BinaryenLocalGetGetIndex(expr); } diff --git a/tests/compiler/number.debug.wat b/tests/compiler/number.debug.wat index a91c2a331b..c2bf156863 100644 --- a/tests/compiler/number.debug.wat +++ b/tests/compiler/number.debug.wat @@ -4972,6 +4972,35 @@ call $~lib/builtins/abort unreachable end + f64.const 1 + global.get $~lib/builtins/f32.NaN + f64.promote_f32 + f64.eq + i32.eqz + drop + global.get $~lib/number/F32.NaN + f32.const nan:0x400000 + f32.eq + i32.eqz + drop + f64.const nan:0x8000000000000 + f64.const 1 + f64.eq + i32.eqz + drop + f64.const 1 + global.get $~lib/builtins/f32.NaN + f64.promote_f32 + f64.ne + drop + global.get $~lib/number/F32.NaN + f32.const nan:0x400000 + f32.ne + drop + f64.const nan:0x8000000000000 + f64.const 1 + f64.ne + drop global.get $~lib/memory/__stack_pointer i32.const 8 i32.add diff --git a/tests/compiler/number.json b/tests/compiler/number.json index 1bdd02b1be..7baf2c9fd1 100644 --- a/tests/compiler/number.json +++ b/tests/compiler/number.json @@ -1,4 +1,10 @@ { "asc_flags": [ + ], + "stderr": [ + "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", + "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", + "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", + "AS907: Comparision with NaN is invariant. You need to use isNaN(x)." ] } diff --git a/tests/compiler/number.ts b/tests/compiler/number.ts index cf7c76dbfa..ebef46aee2 100644 --- a/tests/compiler/number.ts +++ b/tests/compiler/number.ts @@ -65,3 +65,13 @@ assert(F64.isInteger(f64.MIN_SAFE_INTEGER) == true); assert(F64.isInteger(f64.MAX_SAFE_INTEGER) == true); assert(F64.isInteger(+0.5) == false); assert(F64.isInteger(-1.5) == false); + +// Should produce warnings + +// always false +assert(!(1.0 == NaN)); +assert(!(NaN == F32.NaN)); + +// always true +assert(1.0 != NaN); +assert(NaN != F32.NaN); From d67747caf97885d481f1f6ff804a07f753fdf90b Mon Sep 17 00:00:00 2001 From: MaxGraey Date: Sat, 27 Aug 2022 13:43:09 +0300 Subject: [PATCH 2/7] minor refactorings --- src/module.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/module.ts b/src/module.ts index 2577b773dc..c8e9173bc6 100644 --- a/src/module.ts +++ b/src/module.ts @@ -2771,7 +2771,7 @@ export class Module { maxLoopIterations: i32 = 1 ): ExpressionRef { var runner = binaryen._ExpressionRunnerCreate(this.ref, flags, maxDepth, maxLoopIterations); - var precomp = binaryen._ExpressionRunnerRunAndDispose(runner, expr); + var precomp = binaryen._ExpressionRunnerRunAndDispose(runner, expr); if (precomp) { if (!this.isConstExpression(precomp)) return 0; assert(getExpressionType(precomp) == getExpressionType(expr)); @@ -2793,7 +2793,11 @@ export class Module { case BinaryOp.MulI32: case BinaryOp.AddI64: case BinaryOp.SubI64: - case BinaryOp.MulI64: return this.isConstExpression(getBinaryLeft(expr)) && this.isConstExpression(getBinaryRight(expr)); + case BinaryOp.MulI64: + return ( + this.isConstExpression(getBinaryLeft(expr)) && + this.isConstExpression(getBinaryRight(expr)) + ); } } break; From 73413e8e354f675a2ee71dd7361158ebe59fee66 Mon Sep 17 00:00:00 2001 From: MaxGraey Date: Sat, 27 Aug 2022 14:04:50 +0300 Subject: [PATCH 3/7] allow more complex expressions. Useful for namespaced globals --- src/compiler.ts | 12 +++++++++--- src/module.ts | 24 ++++++++++++++++++++++++ tests/compiler/number.json | 2 ++ tests/compiler/number.ts | 2 ++ 4 files changed, 37 insertions(+), 3 deletions(-) diff --git a/src/compiler.ts b/src/compiler.ts index 89c9b65222..f911669430 100644 --- a/src/compiler.ts +++ b/src/compiler.ts @@ -53,7 +53,7 @@ import { SwitchBuilder, ExpressionRunnerFlags, isConstZero, - isConstNaN + isConstExpressionNaN } from "./module"; import { @@ -3994,7 +3994,10 @@ export class Compiler extends DiagnosticEmitter { return module.unreachable(); } if (commonType.isFloatValue) { - if (isConstNaN(rightExpr) || isConstNaN(leftExpr)) { + if ( + isConstExpressionNaN(module, rightExpr) || + isConstExpressionNaN(module, leftExpr) + ) { this.warning( DiagnosticCode.Comparision_with_NaN_is_invariant_You_need_to_use_isNaN_x, expression.range @@ -4037,7 +4040,10 @@ export class Compiler extends DiagnosticEmitter { return module.unreachable(); } if (commonType.isFloatValue) { - if (isConstNaN(rightExpr) || isConstNaN(leftExpr)) { + if ( + isConstExpressionNaN(module, rightExpr) || + isConstExpressionNaN(module, leftExpr) + ) { this.warning( DiagnosticCode.Comparision_with_NaN_is_invariant_You_need_to_use_isNaN_x, expression.range diff --git a/src/module.ts b/src/module.ts index c8e9173bc6..7cecd0875a 100644 --- a/src/module.ts +++ b/src/module.ts @@ -2929,6 +2929,30 @@ export function isConstNaN(expr: ExpressionRef): bool { return false; } +export function isConstExpressionNaN(module: Module, expr: ExpressionRef): bool { + var id = getExpressionId(expr); + var type = getExpressionType(expr); + if (type == TypeRef.F32 || type == TypeRef.F64) { + if (id == ExpressionId.Const) { + return isNaN( + type == TypeRef.F32 + ? getConstValueF32(expr) + : getConstValueF64(expr) + ); + } else if (id == ExpressionId.GlobalGet) { + let precomp = module.runExpression(expr, ExpressionRunnerFlags.Default, 8); + if (precomp) { + return isNaN( + type == TypeRef.F32 + ? getConstValueF32(precomp) + : getConstValueF64(precomp) + ); + } + } + } + return false; +} + export function getLocalGetIndex(expr: ExpressionRef): Index { return binaryen._BinaryenLocalGetGetIndex(expr); } diff --git a/tests/compiler/number.json b/tests/compiler/number.json index 7baf2c9fd1..604fffa882 100644 --- a/tests/compiler/number.json +++ b/tests/compiler/number.json @@ -2,6 +2,8 @@ "asc_flags": [ ], "stderr": [ + "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", + "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", diff --git a/tests/compiler/number.ts b/tests/compiler/number.ts index ebef46aee2..05eed298a9 100644 --- a/tests/compiler/number.ts +++ b/tests/compiler/number.ts @@ -71,7 +71,9 @@ assert(F64.isInteger(-1.5) == false); // always false assert(!(1.0 == NaN)); assert(!(NaN == F32.NaN)); +assert(!(F64.NaN == 1.0)); // always true assert(1.0 != NaN); assert(NaN != F32.NaN); +assert(f64.NaN != 1.0); From 96090532b0dd71b9036586a61c2eb046102ee0de Mon Sep 17 00:00:00 2001 From: MaxGraey Date: Sat, 27 Aug 2022 16:27:08 +0300 Subject: [PATCH 4/7] add same warning for -0.0 comparisions --- src/compiler.ts | 13 +++++++++++++ src/diagnosticMessages.json | 1 + src/module.ts | 17 +++++++++++++++++ tests/compiler/number.json | 9 ++++++++- tests/compiler/number.ts | 8 ++++++++ 5 files changed, 47 insertions(+), 1 deletion(-) diff --git a/src/compiler.ts b/src/compiler.ts index f911669430..f3ee88987b 100644 --- a/src/compiler.ts +++ b/src/compiler.ts @@ -53,6 +53,7 @@ import { SwitchBuilder, ExpressionRunnerFlags, isConstZero, + isConstNegZero, isConstExpressionNaN } from "./module"; @@ -4003,6 +4004,12 @@ export class Compiler extends DiagnosticEmitter { expression.range ); } + if (isConstNegZero(rightExpr) || isConstNegZero(leftExpr)) { + this.warning( + DiagnosticCode.Comparision_with_0_0_is_sign_insensitive_Perhaps_you_want_to_use_Object_is_x_0_0, + expression.range + ); + } } leftExpr = this.convertExpression(leftExpr, leftType, commonType, false, left); leftType = commonType; @@ -4049,6 +4056,12 @@ export class Compiler extends DiagnosticEmitter { expression.range ); } + if (isConstNegZero(rightExpr) || isConstNegZero(leftExpr)) { + this.warning( + DiagnosticCode.Comparision_with_0_0_is_sign_insensitive_Perhaps_you_want_to_use_Object_is_x_0_0, + expression.range + ); + } } leftExpr = this.convertExpression(leftExpr, leftType, commonType, false, left); leftType = commonType; diff --git a/src/diagnosticMessages.json b/src/diagnosticMessages.json index 8e4bd2d25f..cfe8a1ee9a 100644 --- a/src/diagnosticMessages.json +++ b/src/diagnosticMessages.json @@ -57,6 +57,7 @@ "Explicitly returning constructor drops 'this' allocation.": 905, "Unnecessary definite assignment.": 906, "Comparision with NaN is invariant. You need to use isNaN(x).": 907, + "Comparision with -0.0 is sign insensitive. Perhaps you want to use Object.is(x, -0.0).": 908, "Unterminated string literal.": 1002, "Identifier expected.": 1003, diff --git a/src/module.ts b/src/module.ts index 7cecd0875a..6c693df0e7 100644 --- a/src/module.ts +++ b/src/module.ts @@ -2921,6 +2921,23 @@ export function isConstNonZero(expr: ExpressionRef): bool { return false; } +export function isConstNegZero(expr: ExpressionRef): bool { + if (getExpressionId(expr) != ExpressionId.Const) return false; + var type = getExpressionType(expr); + if (type == TypeRef.F32) { + let d = getConstValueF32(expr); + if (d == 0) return f32_as_i32(d) == -0x80000000; + } + if (type == TypeRef.F64) { + let d = getConstValueF64(expr); + if (d == 0) { + let u = f64_as_i64(d); + return !i64_low(u) && i64_high(u) == -0x80000000; + } + } + return false; +} + export function isConstNaN(expr: ExpressionRef): bool { if (getExpressionId(expr) != ExpressionId.Const) return false; var type = getExpressionType(expr); diff --git a/tests/compiler/number.json b/tests/compiler/number.json index 604fffa882..881e47599b 100644 --- a/tests/compiler/number.json +++ b/tests/compiler/number.json @@ -7,6 +7,13 @@ "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", - "AS907: Comparision with NaN is invariant. You need to use isNaN(x)." + "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", + + "AS908: Comparision with -0.0 is sign insensitive. Perhaps you want to use Object.is(x, -0.0).", + "AS908: Comparision with -0.0 is sign insensitive. Perhaps you want to use Object.is(x, -0.0).", + "AS908: Comparision with -0.0 is sign insensitive. Perhaps you want to use Object.is(x, -0.0).", + "AS908: Comparision with -0.0 is sign insensitive. Perhaps you want to use Object.is(x, -0.0).", + "AS908: Comparision with -0.0 is sign insensitive. Perhaps you want to use Object.is(x, -0.0).", + "AS908: Comparision with -0.0 is sign insensitive. Perhaps you want to use Object.is(x, -0.0)." ] } diff --git a/tests/compiler/number.ts b/tests/compiler/number.ts index 05eed298a9..84c45020c9 100644 --- a/tests/compiler/number.ts +++ b/tests/compiler/number.ts @@ -77,3 +77,11 @@ assert(!(F64.NaN == 1.0)); assert(1.0 != NaN); assert(NaN != F32.NaN); assert(f64.NaN != 1.0); + +// always true +assert(+.0 == -.0); +assert(-.0 != -.0); +assert(-.0 == +.0); +assert(f32(+.0) == f32(-.0)); +assert(f32(-.0) != f32(-.0)); +assert(f32(-.0) == f32(+.0)); From f625ca7e43cc60958a97f0001dc72990d6348543 Mon Sep 17 00:00:00 2001 From: MaxGraey Date: Sun, 28 Aug 2022 22:36:03 +0300 Subject: [PATCH 5/7] simplify isConstNegZero --- src/module.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/module.ts b/src/module.ts index 6c693df0e7..ff5bc990ac 100644 --- a/src/module.ts +++ b/src/module.ts @@ -2926,14 +2926,11 @@ export function isConstNegZero(expr: ExpressionRef): bool { var type = getExpressionType(expr); if (type == TypeRef.F32) { let d = getConstValueF32(expr); - if (d == 0) return f32_as_i32(d) == -0x80000000; + return d == 0 && f32_as_i32(d) < 0; } if (type == TypeRef.F64) { let d = getConstValueF64(expr); - if (d == 0) { - let u = f64_as_i64(d); - return !i64_low(u) && i64_high(u) == -0x80000000; - } + return d == 0 && i64_signbit(f64_as_i64(d)); } return false; } From cd9819e774a1837cc3aa62a843dc2d371df21518 Mon Sep 17 00:00:00 2001 From: MaxGraey Date: Wed, 31 Aug 2022 21:36:54 +0300 Subject: [PATCH 6/7] this fail. But why? --- src/compiler.ts | 4 ++-- src/diagnosticMessages.json | 4 ++-- tests/compiler/number.json | 25 ++++++++++++------------- 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/src/compiler.ts b/src/compiler.ts index 96ab97466f..687f1df482 100644 --- a/src/compiler.ts +++ b/src/compiler.ts @@ -4000,13 +4000,13 @@ export class Compiler extends DiagnosticEmitter { isConstExpressionNaN(module, leftExpr) ) { this.warning( - DiagnosticCode.Comparision_with_NaN_is_invariant_You_need_to_use_isNaN_x, + DiagnosticCode._NaN_does_not_compare_equal_to_any_other_value_including_itself_Use_isNaN_x_instead, expression.range ); } if (isConstNegZero(rightExpr) || isConstNegZero(leftExpr)) { this.warning( - DiagnosticCode.Comparision_with_0_0_is_sign_insensitive_Perhaps_you_want_to_use_Object_is_x_0_0, + DiagnosticCode.Comparison_with_0_0_is_sign_insensitive_Use_Object_is_x_0_0_if_the_sign_matters, expression.range ); } diff --git a/src/diagnosticMessages.json b/src/diagnosticMessages.json index cfe8a1ee9a..2e8efed630 100644 --- a/src/diagnosticMessages.json +++ b/src/diagnosticMessages.json @@ -56,8 +56,8 @@ "Indexed access may involve bounds checking.": 904, "Explicitly returning constructor drops 'this' allocation.": 905, "Unnecessary definite assignment.": 906, - "Comparision with NaN is invariant. You need to use isNaN(x).": 907, - "Comparision with -0.0 is sign insensitive. Perhaps you want to use Object.is(x, -0.0).": 908, + "'NaN' does not compare equal to any other value including itself. Use isNaN(x) instead.": 907, + "Comparison with -0.0 is sign insensitive. Use Object.is(x, -0.0) if the sign matters.": 908, "Unterminated string literal.": 1002, "Identifier expected.": 1003, diff --git a/tests/compiler/number.json b/tests/compiler/number.json index 881e47599b..d823d836bf 100644 --- a/tests/compiler/number.json +++ b/tests/compiler/number.json @@ -2,18 +2,17 @@ "asc_flags": [ ], "stderr": [ - "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", - "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", - "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", - "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", - "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", - "AS907: Comparision with NaN is invariant. You need to use isNaN(x).", - - "AS908: Comparision with -0.0 is sign insensitive. Perhaps you want to use Object.is(x, -0.0).", - "AS908: Comparision with -0.0 is sign insensitive. Perhaps you want to use Object.is(x, -0.0).", - "AS908: Comparision with -0.0 is sign insensitive. Perhaps you want to use Object.is(x, -0.0).", - "AS908: Comparision with -0.0 is sign insensitive. Perhaps you want to use Object.is(x, -0.0).", - "AS908: Comparision with -0.0 is sign insensitive. Perhaps you want to use Object.is(x, -0.0).", - "AS908: Comparision with -0.0 is sign insensitive. Perhaps you want to use Object.is(x, -0.0)." + "AS907: 'NaN' does not compare equal to any other value including itself. Use isNaN(x) instead.", + "AS907: 'NaN' does not compare equal to any other value including itself. Use isNaN(x) instead.", + "AS907: 'NaN' does not compare equal to any other value including itself. Use isNaN(x) instead.", + "AS907: 'NaN' does not compare equal to any other value including itself. Use isNaN(x) instead.", + "AS907: 'NaN' does not compare equal to any other value including itself. Use isNaN(x) instead.", + "AS907: 'NaN' does not compare equal to any other value including itself. Use isNaN(x) instead.", + "AS908: Comparison with -0.0 is sign insensitive. Use Object.is(x, -0.0) if the sign matters.", + "AS908: Comparison with -0.0 is sign insensitive. Use Object.is(x, -0.0) if the sign matters.", + "AS908: Comparison with -0.0 is sign insensitive. Use Object.is(x, -0.0) if the sign matters.", + "AS908: Comparison with -0.0 is sign insensitive. Use Object.is(x, -0.0) if the sign matters.", + "AS908: Comparison with -0.0 is sign insensitive. Use Object.is(x, -0.0) if the sign matters.", + "AS908: Comparison with -0.0 is sign insensitive. Use Object.is(x, -0.0) if the sign matters." ] } From 74195299427c4537411762c7d086289259a5793a Mon Sep 17 00:00:00 2001 From: MaxGraey Date: Wed, 31 Aug 2022 21:39:51 +0300 Subject: [PATCH 7/7] fix --- src/compiler.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/compiler.ts b/src/compiler.ts index 687f1df482..02ffda461d 100644 --- a/src/compiler.ts +++ b/src/compiler.ts @@ -4052,13 +4052,13 @@ export class Compiler extends DiagnosticEmitter { isConstExpressionNaN(module, leftExpr) ) { this.warning( - DiagnosticCode.Comparision_with_NaN_is_invariant_You_need_to_use_isNaN_x, + DiagnosticCode._NaN_does_not_compare_equal_to_any_other_value_including_itself_Use_isNaN_x_instead, expression.range ); } if (isConstNegZero(rightExpr) || isConstNegZero(leftExpr)) { this.warning( - DiagnosticCode.Comparision_with_0_0_is_sign_insensitive_Perhaps_you_want_to_use_Object_is_x_0_0, + DiagnosticCode.Comparison_with_0_0_is_sign_insensitive_Use_Object_is_x_0_0_if_the_sign_matters, expression.range ); }