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

[WIP] Also lint Box::new in unused_allocation #97774

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/rustc_hir/src/lang_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,7 @@ language_item_table! {
EhCatchTypeinfo, sym::eh_catch_typeinfo, eh_catch_typeinfo, Target::Static, GenericRequirement::None;

OwnedBox, sym::owned_box, owned_box, Target::Struct, GenericRequirement::Minimum(1);
OwnedBoxNew, sym::owned_box_new, owned_box_new, Target::Method(MethodKind::Inherent), GenericRequirement::None;
Copy link
Contributor

Choose a reason for hiding this comment

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

Making it a diagnostic item instead of a lang item would be preferable in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've been planning, as a followup to this PR, to also use this for the #[rustc_box] feature. Right now, #[rustc_box] foobar(foo) is lowered to box foo, irrespective of what foobar is, even if it isn't Box::new. In order to make it error if it's not Box::new, I would be needing something that gives me a def id i suppose. Is a diagnostic item a good choice for such an error as well?

To explain, in a box_syntax free world, some users might still want to use #[rustc_box] over Box::new and they should get a more polished feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right now, #[rustc_box] foobar(foo) is lowered to box foo, irrespective of what foobar is, even if it isn't Box::new.

Which seems fine to me, given that #[rustc_box] is an internal feature that is supposed to be removed in the future.

free world, some users might still want to use #[rustc_box] over Box::new and they should get a more polished feature.

If it's supposed to be used by people and be polished, then perhaps it needs a better syntax than #[rustc_box] Box::new(expr), for example box expr, oh wait.

Copy link
Contributor

@petrochenkov petrochenkov Jun 6, 2022

Choose a reason for hiding this comment

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

I didn't follow the story behind #97293 but the state in which you need to write #[rustc_box] Box::new(expr) instead of just Box::new(expr) or box expr (*) seems like the worse from both worlds.

(*) Preferably the former, because whatever made Box::new(expr) slow in #97293 can also make other much more common things like Vec::from()/String::from() slow, so it may be a good win if it's fixed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Box::new(foo) is not slow in most of cases, thanks to LLVM being really good, especially post version 12 (minimum LLVM is 12 now). The issue is just that switching vec![] to Box::new would also switch it for the remaining few cases where it does cause regressions. For these, the PR added #[rustc_box] as an alternative to box foo. My ultimate goal is to remove box foo support from the compiler, this PR is another preparation of it. Eventually this will need a lang team FCP and a "go forward" from them. Anyways, the general future of box syntax is not discussed here but in the tracking issue (still need to make a proposal there).

It might be better if instead of doing empty promises, I add the lang item in a separate PR first that actually needs it, and then switch this PR to it.


PhantomData, sym::phantom_data, phantom_data, Target::Struct, GenericRequirement::Exact(1);

Expand Down
36 changes: 28 additions & 8 deletions compiler/rustc_lint/src/unused.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1149,18 +1149,17 @@ declare_lint! {
/// ### Example
///
/// ```rust
/// #![feature(box_syntax)]
/// fn main() {
/// let a = (box [1, 2, 3]).len();
/// let a = (Box::new([1, 2, 3])).len();
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// When a `box` expression is immediately coerced to a reference, then
/// the allocation is unnecessary, and a reference (using `&` or `&mut`)
/// When a `Box::new()` expression is immediately coerced to a reference,
/// then the allocation is unnecessary, and a reference (using `&` or `&mut`)
/// should be used instead to avoid the allocation.
pub(super) UNUSED_ALLOCATION,
Warn,
Expand All @@ -1171,14 +1170,35 @@ declare_lint_pass!(UnusedAllocation => [UNUSED_ALLOCATION]);

impl<'tcx> LateLintPass<'tcx> for UnusedAllocation {
fn check_expr(&mut self, cx: &LateContext<'_>, e: &hir::Expr<'_>) {
match e.kind {
hir::ExprKind::Box(_) => {}
let span = match e.kind {
hir::ExprKind::Box(_) => {
// Ideally, we'd underline the `box` part of the expression here,
// but at this point we have lost the span of the `box` keyword.
// Constructing a span from the inner and the entire expression's
// span won't work in many cases, so we just return the entire
// span of the `box foo`.
e.span
}
hir::ExprKind::Call(ref callee, _) => {
// Look for Box::new(foo)
if let hir::ExprKind::Path(ref qpath) = callee.kind &&
// `Res::Local` indicates a closure
let Res::Def(_, def_id) = cx.qpath_res(qpath, callee.hir_id) &&
let Some(owned_box_new_def_id) = cx.tcx.lang_items().owned_box_new() &&
def_id == owned_box_new_def_id
{
// We have a Box::new() call here
callee.span
} else {
return
}
}
_ => return,
}
};

for adj in cx.typeck_results().expr_adjustments(e) {
if let adjustment::Adjust::Borrow(adjustment::AutoBorrow::Ref(_, m)) = adj.kind {
cx.struct_span_lint(UNUSED_ALLOCATION, e.span, |lint| {
cx.struct_span_lint(UNUSED_ALLOCATION, span, |lint| {
lint.build(match m {
adjustment::AutoBorrowMutability::Not => fluent::lint::unused_allocation,
adjustment::AutoBorrowMutability::Mut { .. } => {
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1037,6 +1037,7 @@ symbols! {
out,
overlapping_marker_traits,
owned_box,
owned_box_new,
packed,
panic,
panic_2015,
Expand Down
1 change: 1 addition & 0 deletions library/alloc/src/boxed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ impl<T> Box<T> {
/// let five = Box::new(5);
/// ```
#[cfg(all(not(no_global_oom_handling)))]
#[lang = "owned_box_new"]
#[inline(always)]
#[stable(feature = "rust1", since = "1.0.0")]
#[must_use]
Expand Down
4 changes: 2 additions & 2 deletions library/alloc/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@ fn any_move() {

match a.downcast::<i32>() {
Ok(a) => {
assert!(a == Box::new(8));
assert!(*a == 8);
}
Err(..) => panic!(),
}
match b.downcast::<Test>() {
Ok(a) => {
assert!(a == Box::new(Test));
assert!(*a == Test);
}
Err(..) => panic!(),
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/iterators/into-iter-on-arrays-lint.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// run-rustfix
// rustfix-only-machine-applicable

#[allow(unused_must_use)]
#[allow(unused_must_use, unused_allocation)]
fn main() {
let small = [1, 2];
let big = [0u8; 33];
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/iterators/into-iter-on-arrays-lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// run-rustfix
// rustfix-only-machine-applicable

#[allow(unused_must_use)]
#[allow(unused_must_use, unused_allocation)]
fn main() {
let small = [1, 2];
let big = [0u8; 33];
Expand Down
10 changes: 10 additions & 0 deletions src/test/ui/lint/unused/unused-allocation.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#![feature(box_syntax)]
#![deny(unused_allocation)]

fn main() {
let foo = (box [1, 2, 3]).len(); //~ ERROR: unnecessary allocation
let one = (box vec![1]).pop(); //~ ERROR: unnecessary allocation

let foo = Box::new([1, 2, 3]).len(); //~ ERROR: unnecessary allocation
let one = Box::new(vec![1]).pop(); //~ ERROR: unnecessary allocation
}
32 changes: 32 additions & 0 deletions src/test/ui/lint/unused/unused-allocation.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
error: unnecessary allocation, use `&` instead
--> $DIR/unused-allocation.rs:5:15
|
LL | let foo = (box [1, 2, 3]).len();
| ^^^^^^^^^^^^^^^
|
note: the lint level is defined here
--> $DIR/unused-allocation.rs:2:9
|
LL | #![deny(unused_allocation)]
| ^^^^^^^^^^^^^^^^^

error: unnecessary allocation, use `&mut` instead
--> $DIR/unused-allocation.rs:6:15
|
LL | let one = (box vec![1]).pop();
| ^^^^^^^^^^^^^

error: unnecessary allocation, use `&` instead
--> $DIR/unused-allocation.rs:8:15
|
LL | let foo = Box::new([1, 2, 3]).len();
| ^^^^^^^^

error: unnecessary allocation, use `&mut` instead
--> $DIR/unused-allocation.rs:9:15
|
LL | let one = Box::new(vec![1]).pop();
| ^^^^^^^^

error: aborting due to 4 previous errors

1 change: 1 addition & 0 deletions src/test/ui/self/arbitrary_self_types_trait.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// run-pass
#![allow(unused_allocation)]

use std::rc::Rc;

Expand Down
1 change: 1 addition & 0 deletions src/test/ui/structs-enums/align-struct.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// run-pass
#![allow(dead_code)]
#![allow(unused_allocation)]

use std::mem;

Expand Down