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

missing_const_for_fn misses some cases with generics involved #12677

Open
EdJoPaTo opened this issue Apr 16, 2024 · 1 comment · May be fixed by #12681
Open

missing_const_for_fn misses some cases with generics involved #12677

EdJoPaTo opened this issue Apr 16, 2024 · 1 comment · May be fixed by #12681
Labels
C-bug Category: Clippy is not doing the correct thing I-false-negative Issue: The lint should have been triggered on code, but wasn't

Comments

@EdJoPaTo
Copy link

Summary

As soon as there is some type complexity involved the lint seems to fail to see the possible const.
At first, I suspected generics and lifetimes on the struct, but they don't seem to be the main reason as its also reproducible without. Having generic things like Vec<Generic> seems to be part of the issue.
Interestingly it's not the entire issue as my second example highlights.

I'm not sure if I can further pinpoint the root cause. When more is known please update the issue title to reflect this.

Lint Name

missing_const_for_fn

Reproducer

pub struct TabsState {
    pub titles: Vec<String>,
}

impl TabsState {
    #[must_use]
    pub fn new(titles: Vec<String>) -> Self {
        Self { titles }
    }

    #[must_use]
    pub fn empty() -> Self {
        Self { titles: Vec::new() }
    }
}

I expected to see this happen:

The lint should hint both methods to be possible as const.

Instead, this happened:

Only the second method is marked to be possible as const.

Another way where a Vec seems to prevent this lint from working is this:

pub struct Other {
    pub text: String,
    pub vec: Vec<String>,
}

impl Other {
    pub fn new(text: String) -> Self {
        let vec = Vec::new();
        Self { text, vec }
    }
}

The lint should also detect this one, but it doesn't. Removing the vec from the struct and new function and this is correctly suggested. Using text: usize instead of text: String also works. So somehow this combination results in not detecting this possible const.

Version

rustc 1.77.2 (25ef9e3d8 2024-04-09)
binary: rustc
commit-hash: 25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04
commit-date: 2024-04-09
host: x86_64-unknown-linux-gnu
release: 1.77.2
LLVM version: 17.0.6
@EdJoPaTo EdJoPaTo added C-bug Category: Clippy is not doing the correct thing I-false-negative Issue: The lint should have been triggered on code, but wasn't labels Apr 16, 2024
@y21 y21 linked a pull request Apr 16, 2024 that will close this issue
@y21
Copy link
Member

y21 commented Apr 16, 2024

Currently the lint conservatively assumes that it can't lint if any of the parameter's types implement Drop, because dropping things isn't supported in const fn (ignoring ~const Destruct, which I'm not sure even exists anymore?). Vec<String> has Drop glue, so that's why it wouldn't emit a warning there.

As for why this bug didn't happen when the parameter was a String, I'm guessing that was a bug in itself. String doesn't implement Drop directly, but it still has Drop glue, inherited from its inner Vec<u8>, which it probably didn't account for and so it managed to get past that check.

#12681 tries to make this Drop tracking a bit more precise, by only not linting if the parameter is truly dropped: in your examples, none of the vecs or strings are dropped (but moved via return), so it should continue to emit a warning again for all of the cases here.

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 I-false-negative Issue: The lint should have been triggered on code, but wasn't
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants