Skip to content
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

Add size_of_ref lint #10098

Merged
merged 1 commit into from
Dec 24, 2022
Merged

Add size_of_ref lint #10098

merged 1 commit into from
Dec 24, 2022

Conversation

lukaslueg
Copy link
Contributor

@lukaslueg lukaslueg commented Dec 17, 2022

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, 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, and). 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

@rustbot
Copy link
Collaborator

rustbot commented Dec 17, 2022

r? @Jarcho

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 17, 2022
@bors
Copy link
Contributor

bors commented Dec 19, 2022

☔ The latest upstream changes (presumably #10099) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Dec 20, 2022

☔ The latest upstream changes (presumably #10063) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Contributor

@Jarcho Jarcho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be better as suspicious. It's an easy mistake to make, but it's not inherently wrong and the alternative requires writing out the type name which could be a long.

clippy_lints/src/size_of_ref.rs Outdated Show resolved Hide resolved
clippy_lints/src/size_of_ref.rs Outdated Show resolved Hide resolved
@lukaslueg lukaslueg requested a review from Jarcho December 24, 2022 13:31
@lukaslueg
Copy link
Contributor Author

Ah sorry, I was lazy :-/ Will squash if needed

@Jarcho
Copy link
Contributor

Jarcho commented Dec 24, 2022

It's better if it's squashed, but not a big deal.

@lukaslueg
Copy link
Contributor Author

squashed

@Jarcho
Copy link
Contributor

Jarcho commented Dec 24, 2022

Thank you. @bors r+

@bors
Copy link
Contributor

bors commented Dec 24, 2022

📌 Commit d7b9e19 has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Dec 24, 2022

⌛ Testing commit d7b9e19 with merge e8703a0...

@bors
Copy link
Contributor

bors commented Dec 24, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing e8703a0 to master...

@bors bors merged commit e8703a0 into rust-lang:master Dec 24, 2022
@lukaslueg lukaslueg deleted the size_of_ref branch December 24, 2022 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants