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

Suggest `.as_ref()?` instead of `?` in certain circumstances. #3561

Merged
merged 7 commits into from Dec 28, 2018

Conversation

Projects
None yet
5 participants
@fuerstenau
Copy link
Contributor

fuerstenau commented Dec 18, 2018

No description provided.

@fuerstenau

This comment has been minimized.

Copy link
Contributor

fuerstenau commented Dec 18, 2018

It works^{TM} but obviously should be sanity checked by someone who knows what they are doing. I'm not even capable of not confirming the pull request by some accidental button mashing before writing a description. (=

@oli-obk
Copy link
Collaborator

oli-obk left a comment

lgtm.

r=me with rustfmt run

@mikerite

This comment has been minimized.

Copy link
Contributor

mikerite commented Dec 19, 2018

This change contains a bug. When the if expression contains an else branch but that doesn't match subject.

#![deny(clippy::all)]

pub fn func(x: Option<u32>) -> Option<u32> {

    let _ = if x.is_none() {
        return None;
    } else {
        //x
        Some(0)
    };

    Some(0)
}

fn main() {
}
error: this block may be rewritten with the `?` operator
  --> src/main.rs:12:13
   |
12 |       let _ = if x.is_none() {
   |  _____________^
13 | |         return None;
14 | |     } else {
15 | |         //x
16 | |         Some(0)
17 | |     };
   | |_____^ help: replace_it_with
@fuerstenau

This comment has been minimized.

Copy link
Contributor

fuerstenau commented Dec 19, 2018

Well the stupid “let's see if this lint should trigger but print out something in any case” bug should be fixed and even includes a test now to make it a bit harder to repeat my mistake.

The commata have been reinserted by hand.

@bors

This comment was marked as resolved.

Copy link
Contributor

bors commented Dec 28, 2018

☔️ The latest upstream changes (presumably #3587) made this pull request unmergeable. Please resolve the merge conflicts.

fuerstenau added some commits Dec 28, 2018

@fuerstenau

This comment has been minimized.

Copy link
Contributor

fuerstenau commented Dec 28, 2018

@oli-obk GitHub still shows this as you having requested changes but it does not tell me which changes that should be. Could you either spell out the changes for me or tell me where to look them up?

@oli-obk

This comment has been minimized.

Copy link
Collaborator

oli-obk commented Dec 28, 2018

It was just about the formatting failures. So everything is good now

@oli-obk

This comment has been minimized.

Copy link
Collaborator

oli-obk commented Dec 28, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 28, 2018

📌 Commit 8be7050 has been approved by oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 28, 2018

⌛️ Testing commit 8be7050 with merge 3c4abb5...

bors added a commit that referenced this pull request Dec 28, 2018

Auto merge of #3561 - fuerstenau:master, r=oli-obk
Suggest `.as_ref()?` instead of `?` in certain circumstances.
@fuerstenau

This comment has been minimized.

Copy link
Contributor

fuerstenau commented Dec 28, 2018

That's what I had assumed based on your rather cryptic comment but after a week of trying to confirm my hypothesis without finding a way to do so I elected to ask you about it.

@oli-obk

This comment has been minimized.

Copy link
Collaborator

oli-obk commented Dec 28, 2018

I am very sorry about that. I should write my comments out. I'll do my best to do that in the future.

Please never hesitate to ask for clarification or anything else for that matter.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 28, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing 3c4abb5 to master...

@bors bors merged commit 8be7050 into rust-lang:master Dec 28, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment