alloy-rs / core

High-performance, well-tested & documented core libraries for Ethereum, in Rust
https://alloy.rs
Apache License 2.0
790 stars 155 forks source link

[Bug] `LogData::topics_mut_unchecked` and `LogData::topics_mut` are useless without `DerefMut` #775

Closed wtdcode closed 1 month ago

wtdcode commented 1 month ago

Component

primitives

What version of Alloy are you on?

alloy-primitives v0.8.8

Operating System

Linux

Describe the bug

main.rs

fn main() {
    let log = alloy::primitives::Log::empty();

    let topics = log.topics_mut();

    dbg!(&topics);
}

cargo.toml

[package]
name = "hello"
version = "0.1.0"
edition = "2021"

[dependencies]
alloy = "0.4.2"

This gives error:

error[E0596]: cannot borrow data in dereference of `Log` as mutable
 --> src/main.rs:4:18
  |
4 |     let topics = log.topics_mut();
  |                  ^^^ cannot borrow as mutable
  |
  = help: trait `DerefMut` is required to modify through a dereference, but it is not implemented for `Log`

For more information about this error, try `rustc --explain E0596`.
error: could not compile `hello` (bin "hello") due to 1 previous error

I have no idea when LogData::topics_mut_unchecked and LogData::topics_mut should be used without DerefMut implemented. It looks like it is by design not to implement DerefMut but it makes it impossible to call these two functions (and other functions taking &mut self).

wtdcode commented 1 month ago

@DaniPopes @mattsse Hey guys, any advice on this? I can also draft a PR but I need to know the intended way to handle this:

Or do I miss something?

mattsse commented 1 month ago
let mut log = alloy_primitives::Log::empty();
let topics = log.data.topics_mut();

Log only has Deref for the wrapped data, but since data is pub anyway, we could also just impl derefmut as well imo

wtdcode commented 1 month ago

let mut log = alloy_primitives::Log::empty(); let topics = log.data.topics_mut();

Ohhhhh, you are correct! I didn't notice data is public XD. I will draft a PR for DerefMut.

yash-atreya commented 1 month ago

Closed by #786