#[rustc_inherit_overflow_checks] should be documented instead of replaced with e.g. Add::add calls.

[eddyb]

(Note: this was originally posted as a comment on #62429 - also cc #35310 but not sure how relevant)

In #62429 (comment) @scottmcm said:

Might be worth double-checking there's a test for overflow checks here? I'm never confident in what rustc_inherit_overflow_checks does. (And I hear that one can use Add::add(count, 1) instead of using the attribute.)

While adding more tests is better, I don't agree with removing the attribute. If anything the attribute should be better documented, but relying on more implicit mechanisms that internally use the attribute obscures the fact that something unusual is happening.
(i.e. that some of the behavior isn't decided by the crate being compiled, but by a downstream using crate)

The reason Add::add works at all is because the attribute is used in the impls` for the integer types, e.g.:

rust/library/core/src/ops/arith.rs

Lines 92 to 107 in 186f7ae

	macro_rules! add_impl {
	($($t:ty)*) => ($(
	#[stable(feature = "rust1", since = "1.0.0")]
	impl Add for $t {
	type Output = $t;
	#[inline]
	#[rustc_inherit_overflow_checks]
	fn add(self, other: $t) -> $t { self + other }
	}
	forward_ref_binop! { impl Add, add for $t, $t }
	)*)
	}
	add_impl! { usize u8 u16 u32 u64 u128 isize i8 i16 i32 i64 i128 f32 f64 }

There's nothing magical about it being a trait call, you're just calling another function with #[rustc_inherit_overflow_checks] on it, instead of using #[rustc_inherit_overflow_checks] yourself, and relying on generic/#[inline] instantiation to get the behavior.

The only reason the trait impls have the attribute is because of things generic over those traits, not to call directly.

EDIT: see also documentation issue by @m-ou-se at rust-lang/std-dev-guide#13

[scottmcm]

+1 to having something in the guidelines for what the correct approach is -- I only said this because it was mentioned to me in #45754 (comment)

[eddyb]

Wow, that's really old, thanks for tracking it down! I wasn't sure how far back it might go, I was only shown this pattern recently.

[eddyb]

Looks like that was potentially a misunderstanding of my comment at #36372 (comment), heh:

I suggested replacing iter.fold(0, |a, b| a + b) with iter.fold(0, Add::add), not calling Add::add directly.
In retrospect, for documentation purposes, maybe this would've been better:

iter.fold(
    0,
    #[rustc_inherit_overflow_checks]
    |a, b| a + b,
)

[eddyb]

Regarding the suboptimal name, I wrote (in #81732 (comment)):

Yeah, "inherit" refers to the crate it's codegen'd in. So it's more "defer"? Or we can go more explicit really:

#[rustc_use_overflow_checks_settings_from_user_crate]

I also had an idea that might be useful for avoiding misuse in most cases:

We could also make the attribute warn/error if building the MIR doesn't end up making a decision based on it, because that's what usually happens if it's applied to the wrong thing (e.g. a function doing iter.fold(...) instead of a closure).

[hkBst]

Is there anything to do here?

[bionicles]

one question is, how does this interact with clippy::arithmetic_side_effects ?

could this lead to developers missing instances where arithmetic could panic or produce unexpected results?

https://rust-lang.github.io/rust-clippy/master/index.html?groups=restriction#arithmetic_side_effects