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

borrowed_box incorrectly assumes mutable boxed slice allocation won't change #2907

Closed
okready opened this issue Jul 9, 2018 · 3 comments
Closed
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied

Comments

@okready
Copy link

okready commented Jul 9, 2018

Defining a function that has a &mut Box<[T]> parameter triggers the borrowed_box lint, recommending it be replaced with a &mut [T]. It's possible to have a function that replaces the boxed slice with a different allocation (e.g. a slice with a different size), which cannot be done if the parameter is just a slice reference.

Example:

pub fn maybe_sets_box(boxed_slice: &mut Box<[i32]>) {
    if boxed_slice.is_empty() {
        let mut data = Vec::with_capacity(1);
        data.push(12);
        *boxed_slice = data.into_boxed_slice();
    }
}
@EFanZh
Copy link

EFanZh commented Sep 14, 2019

I think Clippy should allow the usage of &mut Box<T> but disallow the usage of &Box<T> since we might want to change the box pointer instead of the object it points to.

@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy C-bug Category: Clippy is not doing the correct thing I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied labels Sep 17, 2019
@jmaargh
Copy link

jmaargh commented Mar 20, 2020

A (perhaps a little contrived) example: https://godbolt.org/z/LFRvd7

smklein added a commit to smklein/rust-clippy that referenced this issue Apr 19, 2020
Update the "borrow box" lint to avoid recommending the following
conversion:

  &mut Box<T> --> &mut T

Given a mutable reference to a box, functions may want to change
"which" object the Box is pointing at.
smklein added a commit to smklein/rust-clippy that referenced this issue Apr 19, 2020
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.
bors added a commit that referenced this issue Apr 19, 2020
Fix issue #2907.

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.
bors added a commit that referenced this issue Apr 19, 2020
Fix issue #2907.

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.

changelog: Don't trigger [`borrow_box`] lint on `&mut Box` references
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue May 5, 2020
Changes:
````
rustup to rust-lang/rust#70043
map_clone: avoid suggesting `copied()` for &mut
fix redundant_pattern_matching lint
Add tests for rust-lang#1654
Don't trigger while_let_on_iterator when the iterator is recreated every iteration
Update issue_2356.stderr reference file
Update while_let_on_iterator tests
Fix while_let_on_iterator suggestion and make it MachineApplicable
Add lifetime test case for `new_ret_no_self`
rustup rust-lang/rust#71215
Downgrade match_bool to pedantic
Run fetch before testing if master contains beta
The beta branch update should not require a force push
Add a note to the beta sections of release.md
Remove apt-get upgrade again
Always use the deploy script and templates of the master branch
README: fix lit count line
clippy_dev: make it fatal when the regex for updating lint count does not match
`predecessors_for` will be removed soon
Rustup "Remove `BodyAndCache`"
Only run (late) internal lints, when they are warn/deny/forbid
Only run cargo lints, when they are warn/deny/forbid
span_lint_and_note now takes an Option<Span> for the note_span instead of just a span
Make lint also capture blocks and closures, adjust language to mention other mutex types
don't test the code in the lint docs
Switch to matching against full paths instead of just the last element of the path
Lint for holding locks across await points
Also mention `--fix` for nightly users
fix crash on issue-69020-assoc-const-arith-overflow.rs
Address review comments
remark fixes
Update CHANGELOG.md for Rust 1.43 and 1.44
update stderr file
util/fetch_prs_between.sh: Add Markdown formatted Link
factor ifs into function, add differing mutex test
Update the changelog update documentation
Apply suggestions from PR review
update span_lint_and_help call to six args
test for mutex eq, add another test case
use if chain
cargo dev fmt
fix map import to rustc_middle
dev update_lints
fix internal clippy warnings
change visitor name to OppVisitor
use Visitor api to find Mutex::lock calls
add note about update-all-refs script, revert redundant pat to master
move closures to seperate fns, remove known problems
use span_lint_and_help, cargo dev fmt
creating suggestion
progress work on suggestion for auto fix
Implement unsafe_derive_deserialize lint
Update empty_enum.stderr
Formatting and naming
Formatting and naming
Cleanup: `node_id` -> `hir_id`
Fix issue rust-lang#2907.
Don't trigger toplevel_ref_arg for `for` loops
Cleanup: future_not_send: use `return_ty` method
Remove badge FIXME from Cargo.toml
Change note_span argument for span_lint_and_note.
Add an Option<Span> argument to span_lint_and_help.
Fixes internal lint warning in code base.
Implement collapsible_span_lint_calls lint.
````

Fixes #71453
@ebroto
Copy link
Member

ebroto commented May 15, 2020

I think this issue can be closed given that #5491 was merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied
Projects
None yet
Development

No branches or pull requests

5 participants