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

Ignore const fn in or_fun_call lint #5682

Closed
wants to merge 2 commits into from
Closed

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Jun 4, 2020

cc #1609 (comment)
Closes #5658

changelog: none

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 4, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Jun 4, 2020

I don't think the constness of the function by itself will give us what we want. If the function is a const fn, but has arguments, these arguments may not be constants. An example:

const fn compute_nth_prime(n: usize) -> Option<usize> {
    // super heavy logic here
}
fn foo(n: usize) -> Option<usize> {
	// Whether the function above is `const fn` or not has no effect on the cost
    // of the work done in the `or`.
	None.or(compute_nth_prime(n))
}

Thus, we'll end up actually running the function at runtime, with all the runtime effort that comes with it. If you really want to do this, you'll need to analyze the entire expression and check if it is entirely const.

I don't know of a reliable way to do this except to do it on MIR, which would be a major undertaking.

@matthiaskrgr
Copy link
Member

This is a user-facing change, the "changelog:" line should say which false positive this fixes.

@tesuji
Copy link
Contributor Author

tesuji commented Jun 5, 2020

cc @ecstatic-morse do you know how to detect if a const fn could be evaluated at compile-time.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 5, 2020

This is not about whether it could be evaluated at compile-time. A const fn by definition can be evaluated at compile-time. It's whether your expression can be evaluated at compile time. I think the cleanest way would be to implement const fn const prop in rustc and make this lint a MIR lint that just checks whether the argument to or is a const (or one of the whitelisted kinds of functions here).

@bors
Copy link
Collaborator

bors commented Jun 7, 2020

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

@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jun 7, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Jun 21, 2020

I don't think I have enough skills to make that change.
Closing this PR in the mean time.

@tesuji tesuji closed this Jun 21, 2020
bors added a commit that referenced this pull request 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

or_fun_call triggers on const fn
6 participants