-
Notifications
You must be signed in to change notification settings - Fork 1.8k
new lint: borrow_deref_ref
#7930
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
Conversation
|
r? @Manishearth (rust-highfive has picked a reviewer for you, use r? to override) |
|
r? @Jarcho |
flip1995
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general comments. I wasn't involved in the previous discussions about this lint, so I don't really know how we wanted to move forward with this lint.
|
This lint is very similar to deref_addrof but with the operators swapped. How about the name |
|
Though we probably should say "borrow" instead of "addrof" since that's what the docs say - so maybe |
9f62d21 to
b28f8ef
Compare
Conclusion and current status:We lint on:
We don't lint on:
|
|
Here's another interesting case, I stumbled upon working on rustc today: Playground struct S<'a> {
a: &'a mut u32,
}
fn f(s: &S<'_>) {
let x = &*s.a;
// let x = s.a; // <- errors, since &mut must be moved and cannot be copied
println!("{:?}", x);
} |
ab75530 to
888937f
Compare
082ae7b to
e64f4cb
Compare
|
☔ The latest upstream changes (presumably #8025) made this pull request unmergeable. Please resolve the merge conflicts. |
779c88f to
c5d8828
Compare
c5d8828 to
8b0326c
Compare
|
☔ The latest upstream changes (presumably #8042) made this pull request unmergeable. Please resolve the merge conflicts. |
01afd37 to
699434f
Compare
b2366c1 to
da0578a
Compare
|
☔ The latest upstream changes (presumably #8127) made this pull request unmergeable. Please resolve the merge conflicts. |
Jarcho
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late review. December's been very busy.
Both help messages should be suggestions instead.
b6a6aac to
91f2d88
Compare
Suggestion looks much better! |
a13f43a to
eea3b1d
Compare
|
☔ The latest upstream changes (presumably #8621) made this pull request unmergeable. Please resolve the merge conflicts. |
467b64b to
d3f53e6
Compare
|
Hey @Jarcho, could you leave a normal comment (not a review) on this PR, so I can assign it to you? r? @Manishearth could you afterwards do the final review for this PR? 🙃 |
|
Can do. |
|
@bors delegate=Jarcho but yeah sure! |
|
✌️ @Jarcho can now approve this pull request |
b82872e to
a8b5330
Compare
|
Everything looks good. Just have the changes to |
a8b5330 to
dda27e0
Compare
|
Run |
|
On master, we've added a new test, that ensures that lints with automatically applicable suggestions also have a Even though, I'm surprised that this fails in the CI for you |
dda27e0 to
8430fa2
Compare
|
You're going to need to split the test file into two: |
3da9bbf to
202fdb9
Compare
|
Thanks for keeping up with everything! @bors r+ |
|
📌 Commit 202fdb9 has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
changelog:
[`borrow_deref_ref`]Related pr: #6837 #7577
@Jarcho Could you please give a review?
cargo lintcheckgives no false negative (but tested crates are out-of-date).TODO:
deref_on_immutable_refor some others?