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

False Positive on or_fun_call lint with enum constructor. #1338

Closed
CryZe opened this issue Nov 10, 2016 · 8 comments · Fixed by #4018
Closed

False Positive on or_fun_call lint with enum constructor. #1338

CryZe opened this issue Nov 10, 2016 · 8 comments · Fixed by #4018
Labels
C-bug Category: Clippy is not doing the correct thing E-medium Call for participation: Medium difficulty level problem and requires some initial experience.

Comments

@CryZe
Copy link

CryZe commented Nov 10, 2016

or_fn_call shouldn't warn in case the function is just an enum constructor wrapping over local variables.

Here's the code:

pub enum Error<'a> {
    Path(&'a str),
}

pub fn question_mark(path: &str) -> Result<(), Error> {
    None.ok_or(Error::Path(path))?;
    
    Ok(())
}

pub fn try(path: &str) -> Result<(), Error> {
    try!(None.ok_or(Error::Path(path)));
    
    Ok(())
}

Warning:

Compiling playground v0.0.1 (file:///playground)
warning: use of `ok_or` followed by a function call, #[warn(or_fun_call)] on by default
 --> src/main.rs:6:5
  |
6 |     None.ok_or(Error::Path(path))?;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
help: try this
  |     None.ok_or_else(|| Error::Path(path))?;
  = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#or_fun_call

Here's the link to the code in the custom playground: Playground

@Manishearth
Copy link
Member

cause it didn't do that before with the try! macro, so something is inconsistent here.

A lot of the clippy lints get turned off within macros.

@Manishearth
Copy link
Member

This isn't a false positive fwiw, since clippy has no idea how expensive that function call is.

@CryZe
Copy link
Author

CryZe commented Nov 10, 2016

Can it not check that it's just an enum constructor? I feel like this should definitely be improved, as this is something that will come up a lot more often, as you often have Options that you want to turn into proper Results before using the ? operator on them.

@Manishearth
Copy link
Member

Fair point. I'm editing the bug report.

@Manishearth Manishearth changed the title False Positive on or_fun_call lint with question mark operator False Positive on or_fun_call lint with enum constructor. Nov 10, 2016
@Manishearth Manishearth added E-medium Call for participation: Medium difficulty level problem and requires some initial experience. C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages labels Nov 10, 2016
@llogiq
Copy link
Contributor

llogiq commented Jan 17, 2017

So how could we determine something is a struct/enum constructor? fn name == return type name?

@oli-obk
Copy link
Contributor

oli-obk commented Jan 17, 2017

@ishitatsuyuki
Copy link
Contributor

ishitatsuyuki commented May 15, 2017

This also applies to Some, which should instead pointed to unwrap_or():

None.or(Some(x))

EDIT: maybe not, this yields a different type.

@oli-obk oli-obk added C-bug Category: Clippy is not doing the correct thing and removed C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages labels May 15, 2017
giodamelio added a commit to giodamelio/code-challenges that referenced this issue Sep 25, 2018
This is actually a false posative where clippy thinks an enum
constructor is a function call:
rust-lang/rust-clippy#1338
@Manishearth
Copy link
Member

This should have been fixed by #919, but wasn't, because that only deals with const promotable things.

I'll make a PR.

bors added a commit that referenced this issue Apr 23, 2019
Ignore non-const ctor expressions in or_fn_call

Fixes #1338

Should have been fixed by #919, however that focuses on const ctor expressions only, and `.or(Some(local))` isn't const.

This also automatically ignores things like `.or(Some(local.clone())` which we don't actually want to do; I need to figure out what to do here.

changelog: Fixed false positive in [`or_fn_call`] pertaining to enum variant constructors

r? @oli-obk @phansch
bors added a commit that referenced this issue Apr 23, 2019
Ignore non-const ctor expressions in or_fn_call

Fixes #1338

Should have been fixed by #919, however that focuses on const ctor expressions only, and `.or(Some(local))` isn't const.

This also automatically ignores things like `.or(Some(local.clone())` which we don't actually want to do; I need to figure out what to do here.

changelog: Fixed false positive in [`or_fn_call`] pertaining to enum variant constructors

r? @oli-obk @phansch
bors added a commit that referenced this issue Apr 23, 2019
Ignore non-const ctor expressions in or_fn_call

Fixes #1338

Should have been fixed by #919, however that focuses on const ctor expressions only, and `.or(Some(local))` isn't const.

This also automatically ignores things like `.or(Some(local.clone())` which we don't actually want to do; I need to figure out what to do here.

changelog: Fixed false positive in [`or_fn_call`] pertaining to enum variant constructors

r? @oli-obk @phansch
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 E-medium Call for participation: Medium difficulty level problem and requires some initial experience.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants