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.

Sign up for GitHub

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

Jump to bottom

WIP New lint: borrowed_option #11463

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

WIP New lint: borrowed_option #11463

wants to merge 1 commit into from

Conversation

tom-anders
Copy link
Contributor

@tom-anders tom-anders commented Sep 4, 2023 •
edited

changelog: new lint: [borrowed_option]

This is inspired by this excellent video: https://www.youtube.com/watch?v=6c7pZYP_iIE

Some things I'd appreciate feedback on:

  • I've added this to the borrowed_box.rs check, since these two lints would share a lot of common code. We should probably rename that file, but to what? borrowed_box_or_option.rs? (same goes for the tests) (Edit: looks like the corresponding issue has been closed for now, so let's keep it like it is for now?
  • There's a special case for ignoring &Box<Any> (see borrowed_box lint and Any #1884), I'm not sure if this is needed for &Option<Any> as well.
  • The wording of What it does and Why is this bad could probably use some improvements, suggestions are welcome!

@rustbot
Copy link
Collaborator

rustbot commented Sep 4, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @blyxyas (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 4, 2023
@blyxyas blyxyas marked this pull request as draft September 4, 2023 15:36
@Centri3
Copy link
Member

Centri3 commented Sep 4, 2023 •
edited

Regarding &Option<dyn Any>, we don't need the same check since T must be Sized. It's impossible to use &Option<dyn Any>, only &Option<Box<dyn Any>> (or anything else that introduces indirection). This will then suggest Option<&Box<dyn Any>> which is already checked for in the borrowed_box lint.

The lint documentation should provide examples of why it does this (this i.e. exposes implementation details). The example given is fine, but some who aren't too familiar with the borrow checker may question why this exposes implementation details.

@blyxyas
Copy link
Member

blyxyas commented Sep 7, 2023

I've added this to the borrowed_box.rs check, since these two lints would share a lot of common code. We should probably rename that file, but to what? borrowed_box_or_option.rs? (same goes for the tests)

We should probably make a new lint type for borrows and references. cc #11464
I currently have a mildly long review queue, so it will take some days. Until that lint type is created, I'll review this independently ^w^

blyxyas
blyxyas reviewed Sep 8, 2023
View reviewed changes
clippy_lints/src/types/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/types/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/types/mod.rs Outdated Show resolved Hide resolved
@bors
Copy link
Collaborator

bors commented Oct 23, 2023

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

@kawadakk kawadakk mentioned this pull request Dec 21, 2023
Closed
kawadakk added a commit to kawadakk/arrow-datafusion that referenced this pull request Dec 21, 2023
@kawadakk
Take Option<&DataType> instead of &Option<DataType> in `ParamValu…
5acf9a0 
…e::get_placeholders_with_values`

<rust-lang/rust-clippy#11463>
alamb added a commit to apache/datafusion that referenced this pull request Dec 22, 2023
@kawadakk @alamb
Optimize the parameter types of ParamValues's methods (#8613)
e467492 
* Take `&str` instead of `&String` in `ParamValue::get_placeholders_with_values`

<https://rust-lang.github.io/rust-clippy/master/index.html#/ptr_arg>

* Take `Option<&DataType>` instead of `&Option<DataType>` in `ParamValue::get_placeholders_with_values`

<rust-lang/rust-clippy#11463>

* Take `&[_]` instead of `&Vec<_>` in `ParamValues::verify`

<https://rust-lang.github.io/rust-clippy/master/index.html#/ptr_arg>

---------

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
@xFrednet
Copy link
Member

Hey @tom-anders, this is a ping from triage, since there hasn't been any activity in some time. Could you rebase the branch on master?

If you have any questions, you're always welcome to ask them in this PR or on Zulip.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 29, 2024
@tom-anders
Copy link
Contributor Author

@xFrednet Thanks for the reminder! I rebased and also incorporated @blyxyas's suggestions

@tom-anders
Copy link
Contributor Author

(fixed formatting)

@xFrednet
Copy link
Member

Perfect, thank you for the swift update :D

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blyxyas blyxyas blyxyas left review comments

Assignees

@blyxyas blyxyas

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tom-anders @rustbot @Centri3 @blyxyas @bors @xFrednet