Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 6c79e39

Browse files
MaxGraeyMaxGraey
authoredAug 31, 2022
feat: Add warning about NaN invariant and -0.0 for direct comparisons (#2475)
1 parent f52f612 commit 6c79e39

File tree

6 files changed

+154
-5
lines changed

6 files changed

+154
-5
lines changed
 

‎src/compiler.ts

+37-3
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,9 @@ import {
5252
SideEffects,
5353
SwitchBuilder,
5454
ExpressionRunnerFlags,
55-
isConstZero
55+
isConstZero,
56+
isConstNegZero,
57+
isConstExpressionNaN
5658
} from "./module";
5759

5860
import {
@@ -3992,7 +3994,23 @@ export class Compiler extends DiagnosticEmitter {
39923994
this.currentType = contextualType;
39933995
return module.unreachable();
39943996
}
3995-
3997+
if (commonType.isFloatValue) {
3998+
if (
3999+
isConstExpressionNaN(module, rightExpr) ||
4000+
isConstExpressionNaN(module, leftExpr)
4001+
) {
4002+
this.warning(
4003+
DiagnosticCode._NaN_does_not_compare_equal_to_any_other_value_including_itself_Use_isNaN_x_instead,
4004+
expression.range
4005+
);
4006+
}
4007+
if (isConstNegZero(rightExpr) || isConstNegZero(leftExpr)) {
4008+
this.warning(
4009+
DiagnosticCode.Comparison_with_0_0_is_sign_insensitive_Use_Object_is_x_0_0_if_the_sign_matters,
4010+
expression.range
4011+
);
4012+
}
4013+
}
39964014
leftExpr = this.convertExpression(leftExpr, leftType, commonType, false, left);
39974015
leftType = commonType;
39984016
rightExpr = this.convertExpression(rightExpr, rightType, commonType, false, right);
@@ -4028,7 +4046,23 @@ export class Compiler extends DiagnosticEmitter {
40284046
this.currentType = contextualType;
40294047
return module.unreachable();
40304048
}
4031-
4049+
if (commonType.isFloatValue) {
4050+
if (
4051+
isConstExpressionNaN(module, rightExpr) ||
4052+
isConstExpressionNaN(module, leftExpr)
4053+
) {
4054+
this.warning(
4055+
DiagnosticCode._NaN_does_not_compare_equal_to_any_other_value_including_itself_Use_isNaN_x_instead,
4056+
expression.range
4057+
);
4058+
}
4059+
if (isConstNegZero(rightExpr) || isConstNegZero(leftExpr)) {
4060+
this.warning(
4061+
DiagnosticCode.Comparison_with_0_0_is_sign_insensitive_Use_Object_is_x_0_0_if_the_sign_matters,
4062+
expression.range
4063+
);
4064+
}
4065+
}
40324066
leftExpr = this.convertExpression(leftExpr, leftType, commonType, false, left);
40334067
leftType = commonType;
40344068
rightExpr = this.convertExpression(rightExpr, rightType, commonType, false, right);

‎src/diagnosticMessages.json

+2
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@
5656
"Indexed access may involve bounds checking.": 904,
5757
"Explicitly returning constructor drops 'this' allocation.": 905,
5858
"Unnecessary definite assignment.": 906,
59+
"'NaN' does not compare equal to any other value including itself. Use isNaN(x) instead.": 907,
60+
"Comparison with -0.0 is sign insensitive. Use Object.is(x, -0.0) if the sign matters.": 908,
5961

6062
"Unterminated string literal.": 1002,
6163
"Identifier expected.": 1003,

‎src/module.ts

+52-2
Original file line numberDiff line numberDiff line change
@@ -2788,7 +2788,7 @@ export class Module {
27882788
maxLoopIterations: i32 = 1
27892789
): ExpressionRef {
27902790
var runner = binaryen._ExpressionRunnerCreate(this.ref, flags, maxDepth, maxLoopIterations);
2791-
var precomp = binaryen._ExpressionRunnerRunAndDispose(runner, expr);
2791+
var precomp = binaryen._ExpressionRunnerRunAndDispose(runner, expr);
27922792
if (precomp) {
27932793
if (!this.isConstExpression(precomp)) return 0;
27942794
assert(getExpressionType(precomp) == getExpressionType(expr));
@@ -2810,7 +2810,11 @@ export class Module {
28102810
case BinaryOp.MulI32:
28112811
case BinaryOp.AddI64:
28122812
case BinaryOp.SubI64:
2813-
case BinaryOp.MulI64: return this.isConstExpression(getBinaryLeft(expr)) && this.isConstExpression(getBinaryRight(expr));
2813+
case BinaryOp.MulI64:
2814+
return (
2815+
this.isConstExpression(getBinaryLeft(expr)) &&
2816+
this.isConstExpression(getBinaryRight(expr))
2817+
);
28142818
}
28152819
}
28162820
break;
@@ -2934,6 +2938,52 @@ export function isConstNonZero(expr: ExpressionRef): bool {
29342938
return false;
29352939
}
29362940

2941+
export function isConstNegZero(expr: ExpressionRef): bool {
2942+
if (getExpressionId(expr) != ExpressionId.Const) return false;
2943+
var type = getExpressionType(expr);
2944+
if (type == TypeRef.F32) {
2945+
let d = getConstValueF32(expr);
2946+
return d == 0 && f32_as_i32(d) < 0;
2947+
}
2948+
if (type == TypeRef.F64) {
2949+
let d = getConstValueF64(expr);
2950+
return d == 0 && i64_signbit(f64_as_i64(d));
2951+
}
2952+
return false;
2953+
}
2954+
2955+
export function isConstNaN(expr: ExpressionRef): bool {
2956+
if (getExpressionId(expr) != ExpressionId.Const) return false;
2957+
var type = getExpressionType(expr);
2958+
if (type == TypeRef.F32) return isNaN(getConstValueF32(expr));
2959+
if (type == TypeRef.F64) return isNaN(getConstValueF64(expr));
2960+
return false;
2961+
}
2962+
2963+
export function isConstExpressionNaN(module: Module, expr: ExpressionRef): bool {
2964+
var id = getExpressionId(expr);
2965+
var type = getExpressionType(expr);
2966+
if (type == TypeRef.F32 || type == TypeRef.F64) {
2967+
if (id == ExpressionId.Const) {
2968+
return isNaN(
2969+
type == TypeRef.F32
2970+
? getConstValueF32(expr)
2971+
: getConstValueF64(expr)
2972+
);
2973+
} else if (id == ExpressionId.GlobalGet) {
2974+
let precomp = module.runExpression(expr, ExpressionRunnerFlags.Default, 8);
2975+
if (precomp) {
2976+
return isNaN(
2977+
type == TypeRef.F32
2978+
? getConstValueF32(precomp)
2979+
: getConstValueF64(precomp)
2980+
);
2981+
}
2982+
}
2983+
}
2984+
return false;
2985+
}
2986+
29372987
export function getLocalGetIndex(expr: ExpressionRef): Index {
29382988
return binaryen._BinaryenLocalGetGetIndex(expr);
29392989
}

‎tests/compiler/number.debug.wat

+29
Original file line numberDiff line numberDiff line change
@@ -4972,6 +4972,35 @@
49724972
call $~lib/builtins/abort
49734973
unreachable
49744974
end
4975+
f64.const 1
4976+
global.get $~lib/builtins/f32.NaN
4977+
f64.promote_f32
4978+
f64.eq
4979+
i32.eqz
4980+
drop
4981+
global.get $~lib/number/F32.NaN
4982+
f32.const nan:0x400000
4983+
f32.eq
4984+
i32.eqz
4985+
drop
4986+
f64.const nan:0x8000000000000
4987+
f64.const 1
4988+
f64.eq
4989+
i32.eqz
4990+
drop
4991+
f64.const 1
4992+
global.get $~lib/builtins/f32.NaN
4993+
f64.promote_f32
4994+
f64.ne
4995+
drop
4996+
global.get $~lib/number/F32.NaN
4997+
f32.const nan:0x400000
4998+
f32.ne
4999+
drop
5000+
f64.const nan:0x8000000000000
5001+
f64.const 1
5002+
f64.ne
5003+
drop
49755004
global.get $~lib/memory/__stack_pointer
49765005
i32.const 8
49775006
i32.add

‎tests/compiler/number.json

+14
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,18 @@
11
{
22
"asc_flags": [
3+
],
4+
"stderr": [
5+
"AS907: 'NaN' does not compare equal to any other value including itself. Use isNaN(x) instead.",
6+
"AS907: 'NaN' does not compare equal to any other value including itself. Use isNaN(x) instead.",
7+
"AS907: 'NaN' does not compare equal to any other value including itself. Use isNaN(x) instead.",
8+
"AS907: 'NaN' does not compare equal to any other value including itself. Use isNaN(x) instead.",
9+
"AS907: 'NaN' does not compare equal to any other value including itself. Use isNaN(x) instead.",
10+
"AS907: 'NaN' does not compare equal to any other value including itself. Use isNaN(x) instead.",
11+
"AS908: Comparison with -0.0 is sign insensitive. Use Object.is(x, -0.0) if the sign matters.",
12+
"AS908: Comparison with -0.0 is sign insensitive. Use Object.is(x, -0.0) if the sign matters.",
13+
"AS908: Comparison with -0.0 is sign insensitive. Use Object.is(x, -0.0) if the sign matters.",
14+
"AS908: Comparison with -0.0 is sign insensitive. Use Object.is(x, -0.0) if the sign matters.",
15+
"AS908: Comparison with -0.0 is sign insensitive. Use Object.is(x, -0.0) if the sign matters.",
16+
"AS908: Comparison with -0.0 is sign insensitive. Use Object.is(x, -0.0) if the sign matters."
317
]
418
}

‎tests/compiler/number.ts

+20
Original file line numberDiff line numberDiff line change
@@ -65,3 +65,23 @@ assert(F64.isInteger(f64.MIN_SAFE_INTEGER) == true);
6565
assert(F64.isInteger(f64.MAX_SAFE_INTEGER) == true);
6666
assert(F64.isInteger(+0.5) == false);
6767
assert(F64.isInteger(-1.5) == false);
68+
69+
// Should produce warnings
70+
71+
// always false
72+
assert(!(1.0 == NaN));
73+
assert(!(NaN == F32.NaN));
74+
assert(!(F64.NaN == 1.0));
75+
76+
// always true
77+
assert(1.0 != NaN);
78+
assert(NaN != F32.NaN);
79+
assert(f64.NaN != 1.0);
80+
81+
// always true
82+
assert(+.0 == -.0);
83+
assert(-.0 != -.0);
84+
assert(-.0 == +.0);
85+
assert(f32(+.0) == f32(-.0));
86+
assert(f32(-.0) != f32(-.0));
87+
assert(f32(-.0) == f32(+.0));

0 commit comments

Comments
 (0)
Failed to load comments.