Skip to content

Commit

Permalink
Fix issue #2907.
Browse files Browse the repository at this point in the history
Update the "borrow box" lint to avoid recommending the following
conversion:

```
  // Old
  pub fn f(&mut Box<T>) {...}

  // New
  pub fn f(&mut T) {...}
```

Given a mutable reference to a box, functions may want to change
"which" object the Box is pointing at.

This change avoids recommending removing the "Box" parameter
for mutable references.
  • Loading branch information
smklein committed Apr 19, 2020
1 parent f6b07db commit 0ef5dee
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 19 deletions.
14 changes: 7 additions & 7 deletions clippy_lints/src/types.rs
Expand Up @@ -531,11 +531,12 @@ impl Types {
} else {
format!("{} ", lt.name.ident().as_str())
};
let mutopt = if mut_ty.mutbl == Mutability::Mut {
"mut "
} else {
""
};

if mut_ty.mutbl == Mutability::Mut {
// Ignore `&mut Box<T>` types; see issue #2907 for
// details.
return;
}
let mut applicability = Applicability::MachineApplicable;
span_lint_and_sugg(
cx,
Expand All @@ -544,9 +545,8 @@ impl Types {
"you seem to be trying to use `&Box<T>`. Consider using just `&T`",
"try",
format!(
"&{}{}{}",
"&{}{}",
ltopt,
mutopt,
&snippet_with_applicability(cx, inner.span, "..", &mut applicability)
),
Applicability::Unspecified,
Expand Down
18 changes: 18 additions & 0 deletions tests/ui/borrow_box.rs
Expand Up @@ -4,6 +4,14 @@
#![allow(dead_code)]

pub fn test1(foo: &mut Box<bool>) {
// Although this function could be changed to "&mut bool",
// avoiding the Box, mutable references to boxes are not
// flagged by this lint.
//
// This omission is intentional: By passing a mutable Box,
// the memory location of the pointed-to object could be
// modified. By passing a mutable reference, the contents
// could change, but not the location.
println!("{:?}", foo)
}

Expand Down Expand Up @@ -71,6 +79,16 @@ impl<'a> Test12 for Test11<'a> {
}
}

pub fn test13(boxed_slice: &mut Box<[i32]>) {
// Unconditionally replaces the box pointer.
//
// This cannot be accomplished if "&mut [i32]" is passed,
// and provides a test case where passing a reference to
// a Box is valid.
let mut data = vec![12];
*boxed_slice = data.into_boxed_slice();
}

fn main() {
test1(&mut Box::new(false));
test2();
Expand Down
18 changes: 6 additions & 12 deletions tests/ui/borrow_box.stderr
@@ -1,8 +1,8 @@
error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
--> $DIR/borrow_box.rs:6:19
--> $DIR/borrow_box.rs:19:14
|
LL | pub fn test1(foo: &mut Box<bool>) {
| ^^^^^^^^^^^^^^ help: try: `&mut bool`
LL | let foo: &Box<bool>;
| ^^^^^^^^^^ help: try: `&bool`
|
note: the lint level is defined here
--> $DIR/borrow_box.rs:1:9
Expand All @@ -11,22 +11,16 @@ LL | #![deny(clippy::borrowed_box)]
| ^^^^^^^^^^^^^^^^^^^^

error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
--> $DIR/borrow_box.rs:11:14
|
LL | let foo: &Box<bool>;
| ^^^^^^^^^^ help: try: `&bool`

error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
--> $DIR/borrow_box.rs:15:10
--> $DIR/borrow_box.rs:23:10
|
LL | foo: &'a Box<bool>,
| ^^^^^^^^^^^^^ help: try: `&'a bool`

error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
--> $DIR/borrow_box.rs:19:17
--> $DIR/borrow_box.rs:27:17
|
LL | fn test4(a: &Box<bool>);
| ^^^^^^^^^^ help: try: `&bool`

error: aborting due to 4 previous errors
error: aborting due to 3 previous errors

0 comments on commit 0ef5dee

Please sign in to comment.