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 triggers on const fn #5658

Closed
mahkoh opened this issue May 28, 2020 · 11 comments
Closed

or_fun_call triggers on const fn #5658

mahkoh opened this issue May 28, 2020 · 11 comments
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages E-medium Call for participation: Medium difficulty level problem and requires some initial experience.

Comments

@mahkoh
Copy link

mahkoh commented May 28, 2020

const fn f(v: i32) -> Option<i32> {
    Some(v)
}

fn main() {
    None.or(f(1));  // try this: `or_else(|| f(1))`
}
@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages E-medium Call for participation: Medium difficulty level problem and requires some initial experience. and removed good-first-issue These issues are a good way to get started with Clippy labels May 28, 2020
@tesuji
Copy link
Contributor

tesuji commented Jun 2, 2020

cc #1609

@ebroto
Copy link
Member

ebroto commented Aug 10, 2020

Linking this relevant comment so that it does not get buried in a closed PR: #5682 (comment)

bors added a commit that referenced this issue Aug 10, 2020
…arth

Avoid or_fun_call for const_fn with no args

Based on #5682 by @lzutao

This avoids a subset of false positives, specifically those related to `const fn`s that take no arguments.
For the rest, a much more involved fix would be needed, see #5682 (comment).

So this does *not* solve #5658

changelog: Avoid triggering [`or_fun_call`] with `const fn`s that take no arguments.

Fixes #5886
@tnielens
Copy link
Contributor

Are const fn calls with const arguments guaranteed to be evaluated at compile-time?

From the unstable book I understand that they can be called in a const context, but nothing about evaluation guarantees.

@flip1995
Copy link
Member

Are const fn calls with const arguments guaranteed to be evaluated at compile-time?

Good question. @oli-obk can you give some insights here?

@tesuji
Copy link
Contributor

tesuji commented Aug 31, 2020

@montrivo No, it is only guaranteed to evaluate at compile time in const item or static item.

@tnielens
Copy link
Contributor

tnielens commented Aug 31, 2020

@montrivo No, it is only guaranteed to evaluate at compile time in const item or static item.

If so, then I don't think clippy should handle const fns the way proposed in this issue's description, unless the const fn call is used in an expression assigned to a const variable.

Continuing on @oli-obk 's example:

const fn compute_nth_prime(n: usize) -> Option<usize> {
    // super heavy logic here
}

// here recommending `unwrap_or_else` is questionable since there is no runtime performance improvement 
// (although it might improve compilation time if `compute_nth_prime` is really heavy)
const a_prime_number: i32 = Some(13).unwrap_or(comput_nth_prime(42));

// here the lint is applicable, as unwrap_or will cause an unnecessary runtime cost 
// given compute_nth_prime(42) is not evaluated during compilation
fn main() {
  println!("{}", Some(42).unwrap_or(compute_nth_prime(42)));
}

@ebroto
Copy link
Member

ebroto commented Sep 1, 2020

Are const fn calls with const arguments guaranteed to be evaluated at compile-time?

That's a good question, indeed, but I would say Clippy should be interested in if it could be evaluated at compile time.

If it can (even though not guaranteed), suggesting the or_else variants can be considered a false positive. In cases it's not evaluated at compile time, it's just a false negative.

(I was mistaken)

@ebroto
Copy link
Member

ebroto commented Sep 2, 2020

Relevant Zulip conversation, IIUC @montrivo is on the right track. On my side, I was confused between constant evaluation and constant propagation, the latter has nothing to do with const fn.

@tnielens
Copy link
Contributor

tnielens commented Sep 13, 2020

@ebroto I think nullary const fn should not be treated specifically in or_fun_call. We should revert #5889 and #5984.

@ebroto
Copy link
Member

ebroto commented Sep 22, 2020

@montrivo I think you're right, I've opened #6077 to revert them.

Continuing with your example, IIUC none of the methods linted by or_fun_call can be called in a const or static initializer because they are not const fns (so the first case will not compile) and the lint should treat const fns like any other function, because it will never lint in const contexts (well, at least until a potential definition of those methods as const fn).

@ebroto
Copy link
Member

ebroto commented Sep 22, 2020

@mahkoh I'm going to close this issue as I think this is not a false positive, feel free to reopen in case you think otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages 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