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

Fix issue #2907. #5491

Merged
merged 1 commit into from
Apr 19, 2020
Merged

Fix issue #2907. #5491

merged 1 commit into from
Apr 19, 2020

Conversation

smklein
Copy link
Contributor

@smklein smklein commented 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.

changelog: Don't trigger [borrow_box] lint on &mut Box references

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.
@phansch phansch added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 19, 2020
@smklein
Copy link
Contributor Author

smklein commented Apr 19, 2020

Hey all! This is my first commit to clippy - lemme know if I'm heading down the wrong direction or whatever. I'm happy to drastically change things, I just wanted to get in the flow of poking around :)

As a future improvement, it does seem like this lint could be once again enabled for mutable types if we have sufficient context to know that the object location won't change. Maybe something like:

// Might modify pointer, so clippy won't recommend &mut T.
pub fn f(&mut Box<T>) {...};

// This probably *could* be &mut T, right?
pub fn f(&mut Pin<Box<T>>) {...};

@flip1995
Copy link
Member

Thanks! Great contribution, especially the comments in the test files for context!

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 19, 2020

📌 Commit 0ef5dee has been approved by flip1995

@bors
Copy link
Collaborator

bors commented Apr 19, 2020

⌛ Testing commit 0ef5dee with merge 43b4623...

bors added a commit that referenced this pull request 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
Copy link
Collaborator

bors commented Apr 19, 2020

💔 Test failed - checks-action_test

@flip1995
Copy link
Member

@bors retry

@bors
Copy link
Collaborator

bors commented Apr 19, 2020

⌛ Testing commit 0ef5dee with merge 2efc2d6...

@bors
Copy link
Collaborator

bors commented Apr 19, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 2efc2d6 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants