Skip to content

Suggest removing Box::new instead of unboxing it #146259

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
Merged
merged 1 commit into from
Sep 17, 2025
Merged

Suggest removing Box::new instead of unboxing it #146259

merged 1 commit into from
Sep 17, 2025
+49 −3

Conversation

camsteffen
Copy link
Contributor

No description provided.

@rustbot
Copy link
Collaborator

rustbot commented Sep 6, 2025

r? @jackh726

rustbot has assigned @jackh726.
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 S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 6, 2025
fmease
fmease reviewed Sep 6, 2025
View reviewed changes
compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs Outdated
vec![
(expr.span.until(arg.span), "".to_string()),
(
expr.span.with_lo(expr.span.hi() - BytePos(1)),
Copy link
Member

@fmease fmease Sep 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Does your code crash on this?

fn main() {
    let _: String = Box::new(String::new()❩;
}

I've replaced the final ) (U+0028) with ❩ (U+2769). CC #128790.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I didn't want to use the argument span since it could be a parent or child expansion...

Copy link
Member

@fmease fmease Sep 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If you want to keep basing things off of expr.span, consider using one of SourceMap::{start,end,next}_point or similar.

Otherwise, shouldn't you be able to use things like find_ancestor_inside{,_same_ctxt}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I saw your next comment after a refresh. end_point is good!

Otherwise, shouldn't you be able to use things like find_ancestor_inside{,_same_ctxt}?

That would work if the arg is a child expansion, but not if it's a parent. -- Box::new($x)

@camsteffen camsteffen force-pushed the remove-the-box branch 5 times, most recently from be71edc to 09dca95 Compare September 6, 2025 03:41
hkBst
hkBst reviewed Sep 7, 2025
View reviewed changes
jackh726
jackh726 reviewed Sep 16, 2025
View reviewed changes
@jackh726
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 16, 2025

📌 Commit d66fb49 has been approved by jackh726

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 16, 2025
@bors
Copy link
Collaborator

bors commented Sep 16, 2025

⌛ Testing commit d66fb49 with merge f199c62...

bors added a commit that referenced this pull request Sep 16, 2025
@bors
Auto merge of #146259 - camsteffen:remove-the-box, r=jackh726
f199c62 
Suggest removing Box::new instead of unboxing it
@bors
Copy link
Collaborator

bors commented Sep 16, 2025

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 16, 2025
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@Zalathar
Copy link
Contributor

@bors retry (flaked out)

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 16, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 16, 2025
@Zalathar
Rollup merge of rust-lang#146259 - camsteffen:remove-the-box, r=jackh726
f30be46 
Suggest removing Box::new instead of unboxing it
@Zalathar Zalathar mentioned this pull request Sep 16, 2025
Closed
bors added a commit that referenced this pull request Sep 16, 2025
@bors
Auto merge of #146628 - Zalathar:rollup-ecm8zaj, r=Zalathar
40e88bc 
Rollup of 3 pull requests

Successful merges:

 - #145181 (remove FIXME block from `has_significant_drop`, it never encounters inference variables)
 - #146259 (Suggest removing Box::new instead of unboxing it)
 - #146588 (tests/run-make: Update list of statically linked musl targets)

r? `@ghost`
`@rustbot` modify labels: rollup
jhpratt added a commit to jhpratt/rust that referenced this pull request Sep 17, 2025
@jhpratt
Rollup merge of rust-lang#146259 - camsteffen:remove-the-box, r=jackh726
5c4cef6 
Suggest removing Box::new instead of unboxing it
@jhpratt jhpratt mentioned this pull request Sep 17, 2025
Closed
bors added a commit that referenced this pull request Sep 17, 2025
@bors
Auto merge of #146662 - jhpratt:rollup-8pggsqv, r=jhpratt
bcb42b1 
Rollup of 9 pull requests

Successful merges:

 - #144871 (Stabilize `btree_entry_insert` feature)
 - #145181 (remove FIXME block from `has_significant_drop`, it never encounters inference variables)
 - #145838 (don't apply temporary lifetime extension rules to non-extended `super let`)
 - #146259 (Suggest removing Box::new instead of unboxing it)
 - #146410 (Iterator repeat: no infinite loop for `last` and `count`)
 - #146460 (Add tidy readme)
 - #146581 (Detect attempt to use var-args in closure)
 - #146588 (tests/run-make: Update list of statically linked musl targets)
 - #146647 (Move `#[rustc_coherence_is_core]` to the `crate_level` file)

r? `@ghost`
`@rustbot` modify labels: rollup
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 17, 2025
@Zalathar
Rollup merge of rust-lang#146259 - camsteffen:remove-the-box, r=jackh726
1bc4e25 
Suggest removing Box::new instead of unboxing it
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 17, 2025
@Zalathar
Rollup merge of rust-lang#146259 - camsteffen:remove-the-box, r=jackh726
7cb0086 
Suggest removing Box::new instead of unboxing it
This was referenced Sep 17, 2025
Closed
Merged
bors added a commit that referenced this pull request Sep 17, 2025
@bors
Auto merge of #146666 - Zalathar:rollup-m2b8low, r=Zalathar
2ebb126 
Rollup of 14 pull requests

Successful merges:

 - #142807 (libtest: expose --fail-fast as an unstable command-line option)
 - #144871 (Stabilize `btree_entry_insert` feature)
 - #145071 (Update the minimum external LLVM to 20)
 - #145181 (remove FIXME block from `has_significant_drop`, it never encounters inference variables)
 - #145660 (initial implementation of the darwin_objc unstable feature)
 - #145838 (don't apply temporary lifetime extension rules to non-extended `super let`)
 - #146259 (Suggest removing Box::new instead of unboxing it)
 - #146410 (Iterator repeat: no infinite loop for `last` and `count`)
 - #146460 (Add tidy readme)
 - #146552 (StateTransform: Do not renumber resume local.)
 - #146564 (Remove Rvalue::Len again.)
 - #146581 (Detect attempt to use var-args in closure)
 - #146588 (tests/run-make: Update list of statically linked musl targets)
 - #146631 (cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 3))

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 79f8acb into rust-lang:master Sep 17, 2025
10 of 11 checks passed
@rustbot rustbot added this to the 1.92.0 milestone Sep 17, 2025
rust-timer added a commit that referenced this pull request Sep 17, 2025
@rust-timer
Unrolled build for #146259
9fddfc4 
Rollup merge of #146259 - camsteffen:remove-the-box, r=jackh726

Suggest removing Box::new instead of unboxing it
@camsteffen camsteffen deleted the remove-the-box branch September 17, 2025 11:48
@github-actions github-actions bot mentioned this pull request Sep 18, 2025
Draft
github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request Sep 22, 2025
@bors
Auto merge of #146666 - Zalathar:rollup-m2b8low, r=Zalathar
f81ad3e 
Rollup of 14 pull requests

Successful merges:

 - rust-lang/rust#142807 (libtest: expose --fail-fast as an unstable command-line option)
 - rust-lang/rust#144871 (Stabilize `btree_entry_insert` feature)
 - rust-lang/rust#145071 (Update the minimum external LLVM to 20)
 - rust-lang/rust#145181 (remove FIXME block from `has_significant_drop`, it never encounters inference variables)
 - rust-lang/rust#145660 (initial implementation of the darwin_objc unstable feature)
 - rust-lang/rust#145838 (don't apply temporary lifetime extension rules to non-extended `super let`)
 - rust-lang/rust#146259 (Suggest removing Box::new instead of unboxing it)
 - rust-lang/rust#146410 (Iterator repeat: no infinite loop for `last` and `count`)
 - rust-lang/rust#146460 (Add tidy readme)
 - rust-lang/rust#146552 (StateTransform: Do not renumber resume local.)
 - rust-lang/rust#146564 (Remove Rvalue::Len again.)
 - rust-lang/rust#146581 (Detect attempt to use var-args in closure)
 - rust-lang/rust#146588 (tests/run-make: Update list of statically linked musl targets)
 - rust-lang/rust#146631 (cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 3))

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmease fmease fmease left review comments

@jackh726 jackh726 jackh726 left review comments

+1 more reviewer

@hkBst hkBst hkBst left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@jackh726 jackh726

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.92.0
Development

Successfully merging this pull request may close these issues.
8 participants
@camsteffen @rustbot @jackh726 @bors @rust-log-analyzer @Zalathar @fmease @hkBst