clippy::question_mark lint suggests code that does not compile

tdyas commented 2 years ago

Summary

As part of upgrading pantsbuild to Rust v1.58.0, we had to disable the clippy::question_mark lint on one segment of code because it suggested code that (a) is semantically different and (b) does not actually compile.

Lint Name

clippy::question_mark

Reproducer

Code at issue: https://github.com/pantsbuild/pants/blob/6759bd7ef7b792f26391e70b6b40556020365e81/src/rust/engine/async_value/src/lib.rs#L102-L105

Clippy error:

error: this block may be rewritten with the `?` operator
   --> async_value/src/lib.rs:102:7
    |
102 | /       if item_receiver.changed().await.is_err() {
103 | |         return None;
104 | |       }
    | |_______^ help: replace it with: `item_receiver.changed().await.as_ref()?;`
    |
note: the lint level is defined here
   --> async_value/src/lib.rs:7:3
    |
7   |   clippy::all,
    |   ^^^^^^^^^^^
    = note: `#[deny(clippy::question_mark)]` implied by `#[deny(clippy::all)]`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#question_mark

Applied this diff:

diff --git a/src/rust/engine/async_value/src/lib.rs b/src/rust/engine/async_value/src/lib.rs
index 1133bdf0d..3fb168839 100644
--- a/src/rust/engine/async_value/src/lib.rs
+++ b/src/rust/engine/async_value/src/lib.rs
@@ -99,10 +99,7 @@ impl<T: Clone + Send + Sync + 'static> AsyncValueReceiver<T> {
         return Some(value.clone());
       }

-      #[allow(clippy::question_mark)]
-      if item_receiver.changed().await.is_err() {
-        return None;
-      }
+      item_receiver.changed().await.as_ref()?;
     }
   }
 }

Following clippy's suggestion results in this error:

error[E0277]: the `?` operator can only be used on `Option`s, not `Result`s, in an async function that returns `Option`
   --> async_value/src/lib.rs:102:45
    |
95  |     pub async fn recv(&self) -> Option<T> {
    |  _________________________________________-
96  | |     let mut item_receiver = (*self.item_receiver).clone();
97  | |     loop {
98  | |       if let Some(ref value) = *item_receiver.borrow() {
...   |
102 | |       item_receiver.changed().await.as_ref()?;
    | |                                             ^ use `.ok()?` if you want to discard the `std::result::Result<std::convert::Infallible, &tokio::sync::watch::error::RecvError>` error information
103 | |     }
104 | |   }
    | |___- this function returns an `Option`
    |
    = help: the trait `std::ops::FromResidual<std::result::Result<std::convert::Infallible, &tokio::sync::watch::error::RecvError>>` is not implemented for `std::option::Option<T>`

Version

rustc 1.58.0 (02072b482 2022-01-11)
binary: rustc
commit-hash: 02072b482a8b5357f7fb5e5637444ae30e423c40
commit-date: 2022-01-11
host: x86_64-apple-darwin
release: 1.58.0
LLVM version: 13.0.0

Additional Labels

No response

tdyas commented 2 years ago

lopopolo commented 2 years ago

I couldn't get a minimal reproduction but this also triggered on some of my code:

warning: this block may be rewritten with the `?` operator
   --> artichoke-backend/src/load_path/memory.rs:381:9
    |
381 | /         if path.strip_prefix(RUBY_LOAD_PATH).is_err() {
382 | |             return None;
383 | |         }
    | |_________^ help: replace it with: `path.strip_prefix(RUBY_LOAD_PATH)?;`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#question_mark

warning: this block may be rewritten with the `?` operator
   --> artichoke-backend/src/load_path/memory.rs:427:9
    |
427 | /         if path.strip_prefix(RUBY_LOAD_PATH).is_err() {
428 | |             return None;
429 | |         }
    | |_________^ help: replace it with: `path.strip_prefix(RUBY_LOAD_PATH)?;`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#question_mark

https://github.com/artichoke/artichoke/blob/f713a32ce43547b45f76d16cf55aac5338c2fb12/artichoke-backend/src/load_path/memory.rs#L378-L390

  #[must_use]
  pub fn get_extension(&self, path: &Path) -> Option<ExtensionHook> {
      let path = absolutize_relative_to(path, &self.cwd);
      if path.strip_prefix(RUBY_LOAD_PATH).is_err() {
          return None;
      }
      let path = normalize_slashes(path).ok()?;
      if let Some(entry) = self.fs.get(path.as_bstr()) {
          entry.extension()
      } else {
          None
      }
  }

https://github.com/artichoke/artichoke/blob/f713a32ce43547b45f76d16cf55aac5338c2fb12/artichoke-backend/src/load_path/memory.rs#L424-L435

  #[must_use]
  pub fn is_required(&self, path: &Path) -> Option<bool> {
      let path = absolutize_relative_to(path, &self.cwd);
      if path.strip_prefix(RUBY_LOAD_PATH).is_err() {
          return None;
      }
      if let Ok(path) = normalize_slashes(path) {
          Some(self.loaded_features.contains(path.as_bstr()))
      } else {
          None
      }
  }

clippy's suggestion does not compile because Path::strip_prefix returns Result but the functions in question return Option. I happen to think this if result.is_err { return None; } construction is clearer than result.ok()?;.

dswij commented 2 years ago

This is fixed in nightly in #8080 (playground), but I can confirm this is a problem in 1.58.0

darnuria commented 2 years ago

Still present on:

Clippy version

clippy 0.1.64 (29e4a9e 2022-08-10)

Rustc version

rustc 1.65.0-nightly (29e4a9ee0 2022-08-10)
binary: rustc
commit-hash: 29e4a9ee0253cd39e552a77f51f11f9a5f1c41e6
commit-date: 2022-08-10
host: x86_64-unknown-linux-gnu
release: 1.65.0-nightly
LLVM version: 14.0.6

The actual issue

Got a suggestion like:

error: this block may be rewritten with the `?` operator
   --> crates/client/src/client/async_client.rs:857:21
    |
857 | /                     if let Err(e) = self.state.process(ok.answers()) {
858 | |                         Err(e)
859 | |                     } else {
860 | |                         Ok(ok)
861 | |                     }
    | |_____________________^ help: replace it with: `self.state.process(ok.answers())?`
    |
    = note: `-D clippy::question-mark` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#question_mark

Code impacted: https://github.com/bluejekyll/trust-dns/blob/main/crates/client/src/client/async_client.rs#L854-L868

impl<R> Stream for ClientStreamXfr<R>
where
    R: Stream<Item = Result<DnsResponse, ProtoError>> + Send + Unpin + 'static,
{
    type Item = Result<DnsResponse, ClientError>;

    fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
        use ClientStreamXfrState::*;

        if matches!(self.state, Ended) {
            return Poll::Ready(None);
        }

        let message = if let Some(response) = ready!(self.state.inner().poll_next_unpin(cx)) {
            Some(match response {
                Ok(ok) => {
                    if let Err(e) = self.state.process(ok.answers()) {
                        Err(e)
                    } else {
                        Ok(ok)
                    }
                }
                Err(e) => Err(e.into()),
            })
        } else {
            None
        };
        Poll::Ready(message)
    }
}

the Ok(ok) returned is from the upper match, the ?-able function process work by side-effect returning Result<(), ClientError>,

?ing would return a Result<(), _> here. (also break the return type of the function).

Running a --fix ended up with:

warning: failed to automatically apply fixes suggested by rustc to crate `trust_dns_client`

after fixes were automatically applied the compiler reported errors within these files:

  * crates/client/src/client/async_client.rs

This likely indicates a bug in either rustc or cargo itself,
and we would appreciate a bug report! You're likely to see 
a number of compiler warnings after this message which cargo
attempted to fix but failed. If you could open an issue at
https://github.com/rust-lang/rust/issues
quoting the full output of this command we'd be very appreciative!
Note that you may be able to make some more progress in the near-term
fixing code with the `--broken-code` flag

The following errors were reported:
error[E0308]: `match` arms have incompatible types
   --> crates/client/src/client/async_client.rs:858:27
    |
854 |               Some(match response {
    |  __________________-
855 | |                 Ok(ok) => {
856 | |                     self.state.process(ok.answers())?
    | |                     --------------------------------- this is found to be of type `()`
857 | |                 }
858 | |                 Err(e) => Err(e.into()),
    | |                           ^^^^^^^^^^^^^ expected `()`, found enum `std::result::Result`
859 | |             })
    | |_____________- `match` arms have incompatible types
    |
    = note: expected unit type `()`
                    found enum `std::result::Result<_, _>`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
Original diagnostics will follow.

A possible change is doing, but it's questionable.

        let message = if let Some(response) = ready!(self.state.inner().poll_next_unpin(cx)) {
            Some(match response {
                Ok(ok) => self.state.process(ok.answers()).map(|_| ok),
                Err(e) => Err(e.into()),
            })
        } else {
            None
        };

SpecificProtagonist commented 1 year ago

Found another instance of the suggested fix not compiling (playground link):

let foo = &Some(());
let Some(_) = foo else { return None };

The fix (let _ = foo?;) doesn't compile because the option is behind a reference.

torfsen commented 6 months ago

This might have been fixed by #11983.

Natr1x commented 1 month ago

This might have been fixed by #11983.

It has not been completely fixed. This will not compile for example:

#[derive(Debug, Eq, PartialEq)]
enum B { C, D }

#[derive(Debug)]
struct A(Option<B>);

fn take_c(maybe_b: &mut A) -> Option<B> {
    // Take a reference to Some value if it exists or return None
    // let Some(ref maybe_c) = maybe_b.0 else { return None; };

    // Clippy suggests this instead:
    let ref maybe_c = maybe_b.0?;

    // Which won't compile because it would move the value..

    if *maybe_c == B::C {
        maybe_b.0.take()
    } else {
        None
    }
}

playground link

rust-lang / rust-clippy