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

or_fun_call doesn't trigger on unsafe blocks #6675

Closed
jyn514 opened this issue Feb 4, 2021 · 4 comments · Fixed by #6928
Closed

or_fun_call doesn't trigger on unsafe blocks #6675

jyn514 opened this issue Feb 4, 2021 · 4 comments · Fixed by #6928
Assignees
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-false-negative Issue: The lint should have been triggered on code, but wasn't I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied

Comments

@jyn514
Copy link
Member

jyn514 commented Feb 4, 2021

I tried this code (playground):

unsafe fn f() {
    None.unwrap_or(String::new().as_mut_vec());
}

fn g() {
    None.unwrap_or(unsafe { String::new().as_mut_vec() });
}

I expected to see this happen: Clippy warns on both f and g.

Instead, this happened: Clippy only warns on f.

Meta

  • cargo clippy -V: 0.1.51 (2021-02-03 e708cbd)
@jyn514 jyn514 added the C-bug Category: Clippy is not doing the correct thing label Feb 4, 2021
@jyn514
Copy link
Member Author

jyn514 commented Feb 4, 2021

Also the suggestion causes a borrowck error:

error[E0500]: closure requires unique access to `this` but it is already borrowed
    --> src/simple_api/mod.rs:1150:55
     |
1147 |         unsafe fn get_last_buffer(this: &mut Key) -> &mut Vec<u8> {
     |                                         - let's call the lifetime of this reference `'1`
...
1150 |             this.subscripts.last_mut().unwrap_or_else(|| this.variable.as_mut_vec())
     |             ------------------------------------------^^----------------------------
     |             |                                         |  |
     |             |                                         |  second borrow occurs due to use of `this` in closure
     |             |                                         closure construction occurs here
     |             borrow occurs here
     |             returning this value requires that `this.subscripts` is borrowed for `'1`

@giraffate giraffate added I-false-negative Issue: The lint should have been triggered on code, but wasn't I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied labels Feb 4, 2021
@camsteffen
Copy link
Contributor

@jyn514 Can I ask a favor? In the future, it would be nice to have multiple issues reported in separate github issues, even if they are related. It makes it easier to track as they can be fixed separately. Also each bug may have a different difficulty level (which I believe is the case here) and it's nice to have "good first issues".

@camsteffen camsteffen added the good-first-issue These issues are a good way to get started with Clippy label Feb 18, 2021
@jyn514
Copy link
Member Author

jyn514 commented Feb 18, 2021

Sure thing, I can open a new issue.

@mgacek8
Copy link
Contributor

mgacek8 commented Mar 17, 2021

@rustbot claim

@bors bors closed this as completed in 36aee1c Mar 18, 2021
nars1 pushed a commit to YottaDB/YDBRust that referenced this issue Jun 9, 2021
This is a complicated fix. Here are the things it does:

- Calculate the `ydb_buffer_t` references for the variable and
  subscripts on every loop iteration, not once per call. This is what
  avoids the unsoundness in #40, since the `buf_addr` will be updated if
  the variable is reallocated.
- Drop `t` before calling `ydb_subscript_next`. This avoids the following borrow-check errors:

  ```
  error[E0502]: cannot borrow `*self` as immutable because it is also borrowed as mutable
      --> src/simple_api/mod.rs:1163:41
       |
  1149 |             let t = self.subscripts.last_mut().unwrap_or(unsafe { self.variable.as_mut_vec() });
       |                     --------------- mutable borrow occurs here
  ...
  1163 |             let (varname, subscripts) = self.get_buffers();
       |                                         ^^^^ immutable borrow occurs here
  ...
  1183 |                 t.reserve(last_self_buffer.len_used as usize - t.len());
       |                 - mutable borrow later used here
  ```

  See code comments for details.

  It's possible in theory that this could be avoided by using raw
  pointers instead of a `&mut`, but I don't feel confident enough in my
  knowledge of unsafe Rust to do that. I would feel more confident if
  [Miri](https://github.com/rust-lang/miri/) worked on YDBRust, but
  unfortunately it [doesn't support external FFI
  calls](rust-lang/miri#11).
- Add a test for the previous undefined behavior.
- Make `get_last_buffer` an unsafe function

  It is used correctly, but knowing that requires non-local reasoning.

  This found a clippy bug: rust-lang/rust-clippy#6675
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-false-negative Issue: The lint should have been triggered on code, but wasn't I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants