Skip to content

Conversation

bluebear94
Copy link

Adds a new lint that detects var = Default::default() when var is Box<T> and T implements Default.

changelog: new lint: [default_box_assignments]

  • Followed [lint naming conventions][lint_naming]
  • Added passing UI tests (including committed .stderr file)
  • cargo test passes locally
  • Executed cargo dev update_lints
  • Added lint documentation
  • Run cargo dev fmt

@rustbot
Copy link
Collaborator

rustbot commented Jun 2, 2025

r? @Alexendoo

rustbot has assigned @Alexendoo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 2, 2025
Copy link
Member

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

@samueltardieu
Copy link
Member

r? samueltardieu. @rustbot author

@rustbot rustbot assigned samueltardieu and unassigned Alexendoo Jun 2, 2025
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 2, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 2, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Jun 2, 2025
@rustbot

This comment has been minimized.

@bluebear94 bluebear94 force-pushed the mf/box-assign-default branch 2 times, most recently from 2226b0e to b936ee1 Compare June 2, 2025 19:30
@bluebear94
Copy link
Author

@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 Jun 4, 2025
@rustbot

This comment has been minimized.

Copy link
Member

@samueltardieu samueltardieu left a 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"
Copy link
Member

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.

Copy link
Author

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”.

@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 Jun 5, 2025
@bluebear94 bluebear94 force-pushed the mf/box-assign-default branch from 6ef2726 to 0805610 Compare June 5, 2025 20:23
@samueltardieu
Copy link
Member

I've started the FCP on Zulip.

@rustbot

This comment has been minimized.

@bluebear94 bluebear94 force-pushed the mf/box-assign-default branch from 6538301 to bd9ca0a Compare September 8, 2025 16:05
@rustbot

This comment has been minimized.

Copy link

github-actions bot commented Sep 8, 2025

Lintcheck changes for 78a53c2

Lint Added Removed Changed
clippy::default_box_assignments 3 0 0

This comment will be updated if you push new changes

@rustbot

This comment has been minimized.

@bluebear94 bluebear94 force-pushed the mf/box-assign-default branch from 4db6993 to 78a53c2 Compare October 6, 2025 22:46
@rustbot
Copy link
Collaborator

rustbot commented Oct 6, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Copy link
Member

@samueltardieu samueltardieu left a comment

Choose a reason for hiding this comment

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

The commits need to be squashed. Also, what about the name change to replace_box that was suggested in the FCP thread by @llogiq?

@rustbot label S-final-comment-period

View changes since this review

/// let mut b = Box::new(1u32);
/// *b = Default::default();
/// ```
#[clippy::version = "1.89.0"]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[clippy::version = "1.89.0"]
#[clippy::version = "1.92.0"]

@rustbot rustbot added the S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) label Oct 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) 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.

4 participants