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 borrowed_reborrowable lint #164

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

MrGVSV
Copy link
Contributor

@MrGVSV MrGVSV commented Oct 27, 2024

Fixes #127

Adds the pedantic borrowed_reborrowable lint to suggest that parameters using a mutable reference to a re-borrowable type instead switch to an owned instance of that type.

See #127 for details.

@BD103 BD103 self-requested a review November 1, 2024 18:35
@BD103 BD103 added A-Linter Related to the linter and custom lints C-Feature Make something new possible labels Nov 1, 2024
Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

Overall some great stuff, thank you! I have a few comments, but nothing critical.

One more note, can we shorten the name? Maybe borrowed_reborrowable (borrowed_commands, borrowed_resource, etc.)?

bevy_lint/src/lints/borrow_of_reborrowable.rs Outdated Show resolved Hide resolved
bevy_lint/src/lints/borrow_of_reborrowable.rs Outdated Show resolved Hide resolved
bevy_lint/src/lints/borrow_of_reborrowable.rs Outdated Show resolved Hide resolved
bevy_lint/src/lints/borrow_of_reborrowable.rs Outdated Show resolved Hide resolved
bevy_lint/src/lints/borrow_of_reborrowable.rs Outdated Show resolved Hide resolved
bevy_lint/src/lints/borrow_of_reborrowable.rs Outdated Show resolved Hide resolved
bevy_lint/src/lints/borrow_of_reborrowable.rs Outdated Show resolved Hide resolved
@MrGVSV
Copy link
Contributor Author

MrGVSV commented Nov 2, 2024

One more note, can we shorten the name? Maybe borrowed_reborrowable (borrowed_commands, borrowed_resource, etc.)?

Sure thing! I'll change the name to borrowed_reborrowable

@MrGVSV MrGVSV force-pushed the mrgvsv/borrow_of_reborrowable branch from 428c708 to cc9c4a9 Compare November 2, 2024 17:24
@MrGVSV MrGVSV changed the title Add borrow_of_reborrowable lint Add borrowed_reborrowable lint Nov 2, 2024
@MrGVSV MrGVSV force-pushed the mrgvsv/borrow_of_reborrowable branch 2 times, most recently from 28f8071 to 0161b02 Compare November 2, 2024 22:29
Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

Overall really good! Once all the outstanding comments are addressed, I'm cool to merge this!

Thank you for your hard work and patience :D

bevy_lint/tests/ui/borrowed_reborrowable/commands.rs Outdated Show resolved Hide resolved
Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

I just ran this over the Bevy engine source code and found this error:

warning: parameter takes `&mut ResMut` instead of a re-borrowed `ResMut`
   --> crates/bevy_ecs/src/change_detection.rs:327:28
    |
327 |             fn set_changed(&mut self) {
    |                            ^^^^^^^^^ help: use `ResMut` instead: `mut self: change_detection::ResMut<'w, T>`
...
661 | change_detection_mut_impl!(ResMut<'w, T>, T, Resource);
    | ------------------------------------------------------ in this macro invocation
    |
    = note: `#[warn(bevy::borrowed_resource)]` on by default

Could you add a special case that skips self parameters?

@MrGVSV MrGVSV force-pushed the mrgvsv/borrow_of_reborrowable branch from b86e281 to fb41fe4 Compare November 9, 2024 23:46
@MrGVSV MrGVSV requested a review from BD103 November 9, 2024 23:46
@BD103 BD103 mentioned this pull request Nov 17, 2024
BD103 added a commit that referenced this pull request Nov 17, 2024
This, following the [release
checklist](https://github.com/TheBevyFlock/bevy_cli/blob/663298e1e24a6422abd001b6142dc53f371c3bcd/bevy_lint/docs/release.md),
updates `bevy_lint` to v0.2.0-dev. I also added some missing information
to the release checklist, based off of my experience releasing v0.1.0.

Once this is merged, the feature freeze will be lifted, letting #164 and
#173 be merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Related to the linter and custom lints C-Feature Make something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add lint: &mut Commands function argument
2 participants