Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ast: ContentEq for -0 vs +0 #8982

Closed
Boshen opened this issue Feb 9, 2025 · 3 comments · Fixed by #9007
Closed

ast: ContentEq for -0 vs +0 #8982

Boshen opened this issue Feb 9, 2025 · 3 comments · Fixed by #9007
Assignees
Labels
C-bug Category - Bug

Comments

@Boshen
Copy link
Member

Boshen commented Feb 9, 2025

Background

impl ContentEq for NumericLiteral<'_> {
    fn content_eq(&self, other: &Self) -> bool {
        ContentEq::content_eq(&self.value, &other.value)
    }
}
    macro_rules! content_eq_impl {
        ($($t:ty)*) => ($(
            impl ContentEq for $t {
                #[inline]
                fn content_eq(&self, other: &$t) -> bool { (*self) == (*other) }
                #[inline]
                fn content_ne(&self, other: &$t) -> bool { (*self) != (*other) }
            }
        )*)
    }

    content_eq_impl! {
        char &str
        bool isize usize
        u8 u16 u32 u64 u128
        i8 i16 i32 i64 i128
        f32 f64
    }

Motivation

In minifier a(b ? +0 : -0) is currently minified to a((b, 0));, but shouldn't, due to:

        if ctx.expr_eq(&expr.alternate, &expr.consequent) {
            // "a ? b : b" => "a, b"
            let expressions = ctx.ast.vec_from_array([
                ctx.ast.move_expression(&mut expr.test),
                ctx.ast.move_expression(&mut expr.consequent),
            ]);
            return Some(ctx.ast.expression_sequence(expr.span, expressions));
        }
@Boshen Boshen added the C-bug Category - Bug label Feb 9, 2025
@overlookmotel
Copy link
Contributor

Should NumericLiteral ever contain a negative number? Aren't negative numbers expressed as a UnaryExpression wrapping a NumericLiteral? Playground

That's a question, not an argument! We can adapt ContentEq to handle this case no problem, but I'm wondering if we should need to, or whether the problem is best addressed elsewhere.

@Boshen
Copy link
Member Author

Boshen commented Feb 10, 2025

Should NumericLiteral ever contain a negative number?

Our minifier folds unary numbers, Infinity, NaN into negative number, f64::inf and f64:NaN.

"Infinity" if ident.is_global_reference(ctx.symbols()) => {
Some(ctx.ast.expression_numeric_literal(
ident.span,
f64::INFINITY,
None,
NumberBase::Decimal,
))
}
"NaN" if ident.is_global_reference(ctx.symbols()) => Some(
ctx.ast.expression_numeric_literal(ident.span, f64::NAN, None, NumberBase::Decimal),
),

_ => ctx.eval_unary_expression(e).map(|v| ctx.value_to_expr(e.span, v)),

graphite-app bot pushed a commit that referenced this issue Feb 10, 2025
Fixes #8982.

Previously `0f64.content_eq(-0f64)` returned `true`. Now it returns `false`.

This also affects the behavior for `NaN`. Previously `f64::NAN.content_eq(f64::NAN)` returned `false`. Now it returns `true`. But it's complicated:

```rs
f64::NAN.content_eq(f64::NAN) == true
f64::NAN.content_eq(-f64::NAN) == false
f64::NAN.content_eq(--f64::NAN) == true
```

This does *not* align with JS's `Object.is` which returns `true` for *any* two `NaN` values.

If this matters, we could instead implement `f64::content_eq` as:

```rs
if self.is_nan() && other.is_nan() {
    true
} else {
    self.to_bits() == other.to_bits()
}
```

But this generates quite a lot of assembly for all `f64` values, just to cover this small edge case with `NaN`: https://godbolt.org/z/q9zYodn4e

So I suggest that we go with it as in this PR for now, and see if `NaN` causes us problems in reality or not.
@overlookmotel
Copy link
Contributor

Fixed in #9007.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category - Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants