Skip to content

std::mem::size_of_val w/ double-reference #9995

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

Closed
crepererum opened this issue Nov 30, 2022 · 0 comments
Closed

std::mem::size_of_val w/ double-reference #9995

crepererum opened this issue Nov 30, 2022 · 0 comments
Labels
A-lint Area: New lints

Comments

@crepererum
Copy link

What it does

Imagine the following code:

struct S {
    field: u32,
    data: Vec<u8>,
}

impl S {
    /// Get size of object including `self`, in bytes.
    pub fn size(&self) -> usize {
        std::mem::size_of_val(self) + (std::mem::size_of::<u8>() * self.data.capacity())
    }
}

Now with a little typo, this is all wrong:

impl S {
    /// Get size of object including `self`, in bytes.
    pub fn size(&self) -> usize {
        // typo is here:      V
        std::mem::size_of_val(&self) + (std::mem::size_of::<u8>() * self.data.capacity())
    }
}

This is because &self will not calculate the size of S but of &S This is most likely NOT what the user wanted, but it's easy to mess up.

This is confusing because the type signature is size_of_val<T>(val: &T) where the the type of the size calculation is NOT a reference but the parameter is (to prevent the value from being moved just for the calculation).

See playground.

Lint Name

size_of_val_on_reference

Category

suspicious

Advantage

Drawbacks

  • there might be good reasons why a users calculates the size of a reference.

Example

fn f(&self) {
    std::mem::size_of_val(&self)
}

Probably meant:

fn f(&self) {
    std::mem::size_of_val(self)
}
@crepererum crepererum added the A-lint Area: New lints label Nov 30, 2022
@crepererum crepererum changed the title std::mem::size_of_val w/ double-refernce std::mem::size_of_val w/ double-reference Nov 30, 2022
lukaslueg added a commit to lukaslueg/rust-clippy that referenced this issue Dec 17, 2022
lukaslueg added a commit to lukaslueg/rust-clippy that referenced this issue Dec 19, 2022
lukaslueg added a commit to lukaslueg/rust-clippy that referenced this issue Dec 20, 2022
bors added a commit that referenced this issue Dec 24, 2022
Add size_of_ref lint

This addresses #9995, which is likely raising a valid point about `std::mem::size_of_val()`: It's [very easy to use double-references as the argument](apache/datafusion#4371 (comment)), which the function will happily accept and give back the size of _the reference_, not the size of the value _behind_ the reference. In the worst case, if the value matches the programmer's expectation, this seems to work, while in fact, everything will go horribly wrong e.g. on a different platform.

The size of a `&T` is independent of what `T` is, and people might want to use `std::mem::size_of_val()` to actually get the size of _any_ reference (e.g. via `&&()`). I would rather suggest that this is always bad behavior, though ([instead](https://doc.rust-lang.org/reference/type-layout.html#pointers-and-references-layout), [and](https://doc.rust-lang.org/stable/std/primitive.usize.html#associatedconstant.BITS)). I, therefore, put this lint into `correctness`.

Since the problem is usually easily fixed by removing extra `&`, I went light on suggesting code.

---

changelog: New lint: [`size_of_ref`]
[#10098](#10098)
<!-- changelog_checked -->
@bors bors closed this as completed in d7b9e19 Dec 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

No branches or pull requests

1 participant