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

new lint: borrow_deref_ref #7930

Merged
merged 3 commits into from
Jun 1, 2022
Merged

Conversation

lengyijun
Copy link
Contributor

@lengyijun lengyijun commented Nov 4, 2021

changelog: [`borrow_deref_ref`]

Related pr: #6837 #7577
@Jarcho Could you please give a review?

cargo lintcheck gives no false negative (but tested crates are out-of-date).

TODO:

  1. Not sure the name. deref_on_immutable_ref or some others?

@rust-highfive
Copy link

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 4, 2021
@Manishearth
Copy link
Member

r? @Jarcho

Copy link
Member

@flip1995 flip1995 left a 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.

clippy_lints/src/needless_deref.rs Outdated Show resolved Hide resolved
clippy_lints/src/needless_deref.rs Outdated Show resolved Hide resolved
tests/ui/needless_deref.rs Outdated Show resolved Hide resolved
clippy_lints/src/needless_deref.rs Outdated Show resolved Hide resolved
@camsteffen
Copy link
Contributor

This lint is very similar to deref_addrof but with the operators swapped. How about the name addrof_deref_ref - &*<ref-typed-expr>.

@camsteffen
Copy link
Contributor

Though we probably should say "borrow" instead of "addrof" since that's what the docs say - so maybe borrow_deref_ref.

@lengyijun lengyijun force-pushed the needless_deref_new branch 3 times, most recently from 9f62d21 to b28f8ef Compare November 7, 2021 09:30
@lengyijun
Copy link
Contributor Author

lengyijun commented Nov 9, 2021

Conclusion and current status:

We lint on:

  1. let x = &* (&T)
  2. let x = &mut &* foo()
  3. *&* (&T) if DEREF_ADDROF is silent

We don't lint on:

  1. &&* (&T)
  2. &mut &* (&T)

@flip1995
Copy link
Member

flip1995 commented Nov 9, 2021

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);
}

clippy_lints/src/borrow_deref_ref.rs Outdated Show resolved Hide resolved
clippy_lints/src/borrow_deref_ref.rs Outdated Show resolved Hide resolved
clippy_lints/src/borrow_deref_ref.rs Outdated Show resolved Hide resolved
clippy_lints/src/borrow_deref_ref.rs Outdated Show resolved Hide resolved
@bors
Copy link
Collaborator

bors commented Nov 23, 2021

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

@lengyijun lengyijun force-pushed the needless_deref_new branch 2 times, most recently from 779c88f to c5d8828 Compare November 24, 2021 02:27
@lengyijun lengyijun changed the title new lint: needless_deref new lint: borrow_deref_ref Nov 24, 2021
@bors
Copy link
Collaborator

bors commented Dec 6, 2021

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

@bors
Copy link
Collaborator

bors commented Dec 28, 2021

☔ The latest upstream changes (presumably #8127) 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.

Sorry for the late review. December's been very busy.

Both help messages should be suggestions instead.

clippy_lints/src/borrow_deref_ref.rs Outdated Show resolved Hide resolved
clippy_lints/src/borrow_deref_ref.rs Show resolved Hide resolved
@lengyijun
Copy link
Contributor Author

lengyijun commented Jan 3, 2022

Both help messages should be suggestions instead.

Suggestion looks much better!

@bors
Copy link
Collaborator

bors commented May 15, 2022

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

@xFrednet
Copy link
Member

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? 🙃

@Jarcho
Copy link
Contributor

Jarcho commented May 27, 2022

Can do.

@Manishearth
Copy link
Member

@bors delegate=Jarcho

but yeah sure!

@bors
Copy link
Collaborator

bors commented May 27, 2022

✌️ @Jarcho can now approve this pull request

@Jarcho
Copy link
Contributor

Jarcho commented May 28, 2022

Everything looks good. Just have the changes to rustc_tools_util/LICENSE-APACHE and rustc_tools_util/LICENSE-MIT to revert. Feel free to squash while you're at it.

@lengyijun
Copy link
Contributor Author

Run cargo test --features deny-warnings,internal locally emit no error,
but CI give some errors.

@xFrednet
Copy link
Member

xFrednet commented May 29, 2022

On master, we've added a new test, that ensures that lints with automatically applicable suggestions also have a .fixed file. It looks like the CI failed due to this. It might be good to rebase on master again :)

Even though, I'm surprised that this fails in the CI for you

@Jarcho
Copy link
Contributor

Jarcho commented May 31, 2022

You're going to need to split the test file into two: borrow_deref_ref.rs and borrow_deref_ref_unfixable.rs. Move everything that produces two suggestions over to the unfixable one and then add an exemption to RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS in compile-test.rs for the unfixable test.

@Jarcho
Copy link
Contributor

Jarcho commented Jun 1, 2022

Thanks for keeping up with everything!

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 1, 2022

📌 Commit 202fdb9 has been approved by Jarcho

@bors
Copy link
Collaborator

bors commented Jun 1, 2022

⌛ Testing commit 202fdb9 with merge c4c413b...

@bors
Copy link
Collaborator

bors commented Jun 1, 2022

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

@bors bors merged commit c4c413b into rust-lang:master Jun 1, 2022
@lengyijun lengyijun deleted the needless_deref_new branch October 27, 2023 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants