assert_eq! with a literal float should probably fire float_cmp_const not float_cmp

[CAD97]

Example:

pub const RADIANS_PER_FURMAN: f64 = f64::consts::PI / 32_768_f64;
pub const FURMANS_PER_RADIAN: f64 = 32_768_f64 / f64::consts::PI;
pub const DEGREES_PER_FURMAN: f64 = 45_f64 / 8192_f64;
pub const FURMANS_PER_DEGREE: f64 = 8192_f64 / 45_f64;

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn constants_are_right() {
        // decimal expansions retrieved with WolframAlpha and truncated by clippy
        assert_eq!(RADIANS_PER_FURMAN, 0.000_095_873_799_242_852_57); // exact
        assert_eq!(FURMANS_PER_RADIAN, 10_430.378_350_470_453); // exact
        assert_eq!(DEGREES_PER_FURMAN, 0.005_493_164_062_5); // exact
        assert_eq!(FURMANS_PER_DEGREE, 182.044_444_444_444_45); // exact
    }
}

Here float_cmp fires. Given that this is comparing a const to a literal float, however, this should almost certainly be float_cmp_const instead (which is in the restriction group, as opposed to float_cmp in pedantic). (float_cmp used to be more aggressively linted, but both lints are allow by default nowadays.)

This seems like a reasonable usage of exact floating point equality: defining consts with the intuitive definition and asserting that the produced value is precise within ±½ ULP, by checking against an alternatively produced value.

(Yes, furmans are a real unit, even if unusual/whimsical. Furman is the name for 1/65536th of a revolution, such that one revolution divides evenly into u16.)

[ghost]

@rustbot claim

[ghost]

Hi, I need some help with this, I see that the is_named_constant() in clippy_lints/src/misc.rs returns false, I see there is a difference in expression objects between the left and right expressions in a regular equality check and an assert_eq macro, could someone point me in the right direction to tackle this, CC @flip1995

[flip1995]

Hmm. I would start to add dbg!() around the left and right expression passed to is_named_constant in misc and then see what expressions get passed to constant().

The constant function uses something from the rustc API directly. Maybe this has something to do with the macro context? But that would surprise me, because MIR shouldn't care about where constants come from.

[ghost]

This is how the debug expressions look like

    hir_id: HirId {
        owner: DefId(0:14 ~ float_cmp[317d]::main),
        local_id: 430,
    },
    kind: AddrOf(
        Ref,
        Not,
        Expr {
            hir_id: HirId {
                owner: DefId(0:14 ~ float_cmp[317d]::main),
                local_id: 429,
            },
            kind: AddrOf(
                Ref,
                Not,
                Expr {
                    hir_id: HirId {
                        owner: DefId(0:14 ~ float_cmp[317d]::main),
                        local_id: 428,
                    },
                    kind: AddrOf(
                        Ref,
                        Not,
                        Expr {
                            hir_id: HirId {
                                owner: DefId(0:14 ~ float_cmp[317d]::main),
                                local_id: 427,
                            },
                            kind: AddrOf(
                                Ref,
                                Not,
                                Expr {
                                    hir_id: HirId {
                                        owner: DefId(0:14 ~ float_cmp[317d]::main),
                                        local_id: 426,
                                    },
                                    kind: Lit(
                                        Spanned {
                                            node: Float(
                                                "0.0",
                                                Unsuffixed,
                                            ),
                                            span: tests/ui/float_cmp.rs:115:9: 115:12 (#0),
                                        },
                                    ),
                                    attrs: ThinVec(
                                        None,
                                    ),
                                    span: tests/ui/float_cmp.rs:115:9: 115:12 (#0),
                                },
                            ),
                            attrs: ThinVec(
                                None,
                            ),
                            span: tests/ui/float_cmp.rs:115:8: 115:12 (#0),
                        },
                    ),
                    attrs: ThinVec(
                        None,
                    ),
                    span: tests/ui/float_cmp.rs:115:7: 115:12 (#0),
                },
            ),
            attrs: ThinVec(
                None,
            ),
            span: tests/ui/float_cmp.rs:115:6: 115:12 (#0),
        },
    ),
    attrs: ThinVec(
        None,
    ),
    span: tests/ui/float_cmp.rs:115:5: 115:12 (#0),
}
[clippy_lints/src/misc.rs:728] right = Expr {
    hir_id: HirId {
        owner: DefId(0:14 ~ float_cmp[317d]::main),
        local_id: 460,
    },
    kind: Unary(
        Deref,
        Expr {
            hir_id: HirId {
                owner: DefId(0:14 ~ float_cmp[317d]::main),
                local_id: 459,
            },
            kind: Path(
                Resolved(
                    None,
                    Path {
                        span: /Users/srenganathan/.rustup/toolchains/nightly-2021-02-25-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/macros/mod.rs:39:36: 39:45 (#7),
                        res: Local(
                            HirId {
                                owner: DefId(0:14 ~ float_cmp[317d]::main),
                                local_id: 452,
                            },
                        ),
                        segments: [
                            PathSegment {
                                ident: right_val#7,
                                hir_id: Some(
                                    HirId {
                                        owner: DefId(0:14 ~ float_cmp[317d]::main),
                                        local_id: 458,
                                    },
                                ),
                                res: Some(
                                    Local(
                                        HirId {
                                            owner: DefId(0:14 ~ float_cmp[317d]::main),
                                            local_id: 452,
                                        },
                                    ),
                                ),
                                args: None,
                                infer_args: true,
                            },
                        ],
                    },
                ),
            ),
            attrs: ThinVec(
                None,
            ),
            span: /Users/srenganathan/.rustup/toolchains/nightly-2021-02-25-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/macros/mod.rs:39:36: 39:45 (#7),
        },
    ),
    attrs: ThinVec(
        None,
    ),
    span: /Users/srenganathan/.rustup/toolchains/nightly-2021-02-25-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/macros/mod.rs:39:35: 39:45 (#7),
}

[flip1995]

Ah this makes sense. Looking at the macro expansion:

        match (&RADIANS_PER_FURMAN, &0.000_095_873_799_242_852_57) {
            (left_val, right_val) => {
                if !(*left_val == *right_val) {
                    let kind = ::core::panicking::AssertKind::Eq;
                    ::core::panicking::assert_failed(kind, &*left_val,
                                                     &*right_val,
                                                     ::core::option::Option::None);
                }
            }
        }

Kind of weird, that rustc can't figure out that right_val is a constant. Something seems to go wrong in

rust-clippy/clippy_utils/src/consts.rs

Lines 327 to 328 in 63aca96

	/// Lookup a possibly constant expression from a `ExprKind::Path`.
	fn fetch_path(&mut self, qpath: &QPath<'_>, id: HirId, ty: Ty<'tcx>) -> Option<Constant> {

Can you verify that this function gets called, while evaluating the constants? And possibly what the result of this is:

rust-clippy/clippy_utils/src/consts.rs

Lines 339 to 350 in 63aca96

	let result = self
	.lcx
	.tcx
	.const_eval_resolve(
	self.param_env,
	ty::Unevaluated {
	def: ty::WithOptConstParam::unknown(def_id),
	substs,
	promoted: None,
	},
	None,
	)

[ghost]

This function indeed gets called, result of left and right expressions

[clippy_utils/src/consts.rs:331] res = Local(
    HirId {
        owner: DefId(0:14 ~ float_cmp[317d]::main),
        local_id: 451,
    },
)
[clippy_utils/src/consts.rs:329] "Fetch path called" = "Fetch path called"
[clippy_utils/src/consts.rs:331] res = Local(
    HirId {
        owner: DefId(0:14 ~ float_cmp[317d]::main),
        local_id: 452,
    },
)

For a normal expression (==) the value of result is as follows
[clippy_utils/src/consts.rs:331] res = Def(
    Const,
    DefId(0:22 ~ float_cmp[317d]::main::ABRA),
)

[flip1995]

So the fetch_path function does not deal with Res::Local. So to fix this issue, you have to figure out how to evaluate a constant from a Local/HirId (if at all possible) and add this here:

rust-clippy/clippy_utils/src/consts.rs

Lines 359 to 360 in 63aca96

	// FIXME: cover all usable cases.
	_ => None,

[ghost]

Thanks, I'll look into that

[ghost]

I'll not be able to look at this for the next 15 days, unassigning if anyone wants to take it up in the mean time, I was last working on lowering the expression to extract the type

[torfsen]

Here float_cmp fires. Given that this is comparing a const to a literal float, however, this should almost certainly be float_cmp_const instead (which is allow-by-default, as opposed to deny-by-default float_cmp).

As far as I can see, float_cmp is currently in the pedantic category, which is allow-by-default (float_cmp_const is in restriction, which is also allow-by-default). It was moved there in #7692 and released in Rust 1.57. So changing the emitted lint would not create much of a difference here, at this point.

[CAD97]

Yeah, that's changed since I originally filed the issue. I updated the OP to not be incorrect.

Adjusting which lint gets emitted would still be more correct, but yes, it's significantly less impactful of a change with both lints being allow by default now. But it does still matter for when one but not the other is enabled.