fix: add #[must_use] to volatile types, read, and as_raw_ptr

[mkroening]

No description provided.

[Freax13]

A user might be calling this function to cause side-effects and not necessarily because they're interested in the value.

[mkroening]

Ah, that's true.

For those cases, it might still be beneficial to be explicit about wanting side effects by discarding the value:

let _side_effect = pointer.read();

I can remove it, if you want me to, though. I would prefer #[must_use] and be explicit about side effects, but I have no strong opinion on this.

[phil-opp]

I'm not sure about this PR. I think the #[must_use] attribute is mainly about avoiding potential bugs or accidental misuse. For example, it is often used for methods that look a bit like &mut self methods but are actually returning the new value without modifying the original (e.g. checked_add). The standard lib dev guide describes more examples: https://std-dev-guide.rust-lang.org/policy/must-use.html

I guess it comes down to personal preference. In theory, we could add must_use to all methods with return values, just to remind users that they might want to use the value. But I'm not sure if this is worth it.

As one data point, the standard library didn't make array index or pointer read operations unsafe. The following example compiles without any warnings:

use std::ops::Index;

fn main() {
    let x = [1, 2, 3];
    x[1];
    x.index(1);
    unsafe { std::ptr::read(&x[0]) };
}

(playground)

Clippy throws a warning for the array access, though:

warning: statement with no effect
 --> src/main.rs:5:5
  |
5 |     x[1];
  |     ^^^^^
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_effect
  = note: `#[warn(clippy::no_effect)]` on by default

I don't have a strong opinion about this and I'm happy to dicuss this further.

[mkroening]

The standard lib dev guide describes more examples: https://std-dev-guide.rust-lang.org/policy/must-use.html

"The #[must_use] attribute can be applied to types or functions when failing to explicitly consider them or their output is almost certainly a bug."

I'd argue that this is the case for volatile pointers and references, though maybe not read, as @Freax13 noted, but I would personally prefer explicitly handling cases where only the side effect of reading is needed.

Regarding the types: 0x8000 as *const i32 also has no rustc warnings, but clippy::no_effect. On the other hand, std::ptr::with_exposed_provenance is #[must_use].

Personally, I'd like the hint from the compiler that if I have a volatile pointer, no matter if created via map_field! or otherwise, I should also probably use it. I don't feel strong about this too, but I think this could be nice.

[phil-opp]

Personally, I'd like the hint from the compiler that if I have a volatile pointer, no matter if created via map_field! or otherwise, I should also probably use it. I don't feel strong about this too, but I think this could be nice.

I fully agree with you that compiler warnings are useful in these cases! I'm just trying to decide for some policy that we can follow for this crate.

---

After thinking about this more, I agree that it's a good idea to add the must_use attributes, for the following reasons:

• VolatilePtr is a Copy type whose methods take self, so it's not obvious which methods have side-effects (e.g. modify the data).
• Accessing a volatile register with a non-volatile operation is definitely a bug. Constructing a VolatileRef/VolatilePtr without using it could be an indication for such a bug.
• The API for access etc. is complex enough that mistakes might happen. So extra checks are a good idea.

For read, I'm still unsure, since it can clearly have side-effects as a volatile operation. The linked doc states:

Look for any legitimate use-cases where #[must_use] will cause callers to explicitly ignore values. If these are common then #[must_use] probably isn't appropriate.

I don't know if read operations only for their side-effects are "common", so it's difficult to estimate whether a bare read() is "almost certainly a bug".

[mkroening]

I don't know if read operations only for their side-effects are "common"

Yeah, that's what I am unsure of, too. I would not expect them to be common, but I don't have a lot of experience in this area yet.

[phil-opp]

Let's merge this as-is and see how common such false-positive warnings are. We can still remove the warning later if it needs to be silenced too often.