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

"impractical" const fn #11192

Open
aatifsyed opened this issue Jul 19, 2023 · 6 comments
Open

"impractical" const fn #11192

aatifsyed opened this issue Jul 19, 2023 · 6 comments
Labels
A-lint Area: New lints

Comments

@aatifsyed
Copy link

aatifsyed commented Jul 19, 2023

What it does

A couple of times, I've seen library authors create const fns that can't usefully be used in const contexts.

use std::io;
const fn foo() -> io::Result<()> { 
    Ok(())
}

// You can't do this!
const _: () = match foo() {
    Ok(()) => (),
    Err(_) => panic!("fail the compilation"),
};

// You can do this (but it's not very useful)
const _: io::Result<()> = foo();
error[E0493]: destructor of `Result<(), std::io::Error>` cannot be evaluated at compile-time
  --> src/main.rs:15:21
   |
15 | const _: () = match foo() {
   |                     ^^^^^ the destructor for this type cannot be evaluated in constants
...
18 | };
   | - value is dropped here

For more information about this error, try `rustc --explain E0493`.

This lint would catch these

Advantage

Catch API bugs

Drawbacks

It's not clear what useful means.

A candidate is "is the return type const-droppable", but that would flag Vec::new. Maybe that's okay?

Or maybe the first iteration is to catch enums that can't be dropped in a const context? Or maybe even just Results

Example

const fn foo() -> std::io::Result<()>;
const fn foo() -> Option<()>; // ???
const fn foo() -> Result<(), SomeOtherError>;
@aatifsyed aatifsyed added the A-lint Area: New lints label Jul 19, 2023
@Alexendoo
Copy link
Member

library authors create const fns that can't usefully be used in const contexts

Do you remember any specific examples? Would be good to see what we could reasonably suggest to do instead

@aatifsyed
Copy link
Author

aatifsyed commented Jul 19, 2023

Here's a couple :)

multiformats/rust-multihash#330 (addressing in multiformats/rust-multihash#331)
multiformats/rust-cid#138

In the first case, the solution is creating a new Error type.

@Centri3
Copy link
Member

Centri3 commented Jul 20, 2023

A candidate is "is the return type const-droppable", but that would flag Vec::new. Maybe that's okay?

I'd say it's fine if it lints this as well, Vec::new is essentially useless in constants as it of course doesn't allocate anything; You can't do anything with it. Of course, unless it's wrapped in a Mutex + Lazy as it can then be changed at runtime (or is static mut). There's probably other cases where that doesn't hold

But this does contradict missing_const_for_fn. Having these be const is fine imo so I think this lint should be a subtle hint to modify the API to make it useful (rather than remove the const)

@y21
Copy link
Member

y21 commented Jul 20, 2023

Vec::new is essentially useless in constants as it of course doesn't allocate anything;

A const Vec::new() used to be the only way to initialize an array of vecs using [val; n] syntax.

That is, [Vec::<u8>::new(); 10] doesn't compile because Vec<u8> isnt Copy, but this does compile because paths to constants are allowed:

const EMPTY: Vec<u8> = Vec::new();
let arr = [EMPTY; 10];

So if for some reason you can't use array::from_fn or array::map (msrv reasons?), I'd say it's quite useful for this alone.
Though maybe it's rare enough that linting is fine? Not sure

@aatifsyed
Copy link
Author

Related issue for the aforementioned lint:

@aatifsyed
Copy link
Author

(Arg) -> Ret Evaluation Clippy
impl Copy impl Drop const fn impractical_const
impl Copy impl Drop fn
impl Copy impl Copy const fn
impl Copy impl Copy fn missing_const_for_fn (if body is all const)
impl Drop

This is my current thinking for this lint - are there things I'm missing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

No branches or pull requests

4 participants