rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.36k stars 1.53k forks source link

"impractical" const fn #11192

Open aatifsyed opened 1 year ago

aatifsyed commented 1 year ago

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>;
Alexendoo commented 1 year ago

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 commented 1 year ago

Here's a couple :)

https://github.com/multiformats/rust-multihash/issues/330 (addressing in https://github.com/multiformats/rust-multihash/pull/331) https://github.com/multiformats/rust-cid/issues/138

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

Centri3 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Related issue for the aforementioned lint:

aatifsyed commented 1 year ago
(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?