-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add default_box_assignments
lint
#14953
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
base: master
Are you sure you want to change the base?
Conversation
r? @Alexendoo rustbot has assigned @Alexendoo. Use |
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.
This is a good start.
Can you include tests involving macros and make sure this gets handled? For example, what if Box::default()
comes from a macro when called with certain parameters? This should not lint here. Or when the assignment happens in a macro with arguments coming from the user, they may not be aware that this will be assigned to the same variable.
r? samueltardieu. @rustbot author |
Reminder, once the PR becomes ready for a review, use |
This comment has been minimized.
This comment has been minimized.
2226b0e
to
b936ee1
Compare
@rustbot ready |
This comment has been minimized.
This comment has been minimized.
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.
This is looking good, although some things still need to be adjusted, but I like it so far!
You might also want to add tests to ensure that this deals correctly with generics (it should in the current state already), and also with unsized types to make sure they are (and stay) properly rejected:
fn with_default<T: Default>(b: &mut Box<T>) {
*b = Box::new(T::default());
//~^ default_box_assignments
}
fn with_sized<T>(b: &mut Box<T>, t: T) {
*b = Box::new(t);
//~^ default_box_assignments
}
fn with_unsized<const N: usize>(b: &mut Box<[u32]>) {
*b = Box::new([42; N]);
}
#[clippy::version = "1.89.0"] | ||
pub DEFAULT_BOX_ASSIGNMENTS, | ||
perf, | ||
"assigning `Default::default()` to `Box<T>` is inefficient" |
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.
Can you please update the lint description? Even if we don't have a definite name for it, the description should be accurate so that we can start a FCP (final comment period) to let people a chance to comment on the lint.
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.
OK, I’ve changed it to “assigning a newly created box to Box<T>
is inefficient”.
Co-authored-by: Samuel Tardieu <[email protected]>
6ef2726
to
0805610
Compare
I've started the FCP on Zulip. |
Adds a new lint that detects
var = Default::default()
whenvar
isBox<T>
andT
implementsDefault
.changelog: new lint: [
default_box_assignments
].stderr
file)cargo test
passes locallycargo dev update_lints
cargo dev fmt