New lint const_is_empty

[samueltardieu]

This lint detects calls to .is_empty() on an entity initialized from a string literal and flag them as suspicious. To avoid triggering on macros called from generated code, it checks that the .is_empty() receiver, the call itself and the initialization come from the same context.

Fixes #12307

changelog: [const_is_empty]: new lint

[rustbot]

r? @xFrednet

rustbot has assigned @xFrednet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[bors]

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

[samueltardieu]

Updated to account for the $DIR replacement policy change in tests (1d107ab).

[xFrednet]

The implementation looks good to me. I would like to see a few more tests. Once they are added, we can merge this :D

[samueltardieu]

The implementation looks good to me. I would like to see a few more tests. Once they are added, we can merge this :D

Done. I've also added a third commit which uses constant folding to identify more cases, including those involving const, slices, and arrays. I've kept it separated as it requires modifications to the clippy_utils::consts module.

[samueltardieu]

A lintcheck run on the top 500 packages from crates.io reveals one hit in the cc crate:

fn which(tool: &Path, path_entries: Option<OsString>) -> Option<PathBuf> {
    fn check_exe(exe: &mut PathBuf) -> bool {
        let exe_ext = std::env::consts::EXE_EXTENSION;
        exe.exists() || (!exe_ext.is_empty() && exe.set_extension(exe_ext) && exe.exists())
    }

    […]
}

A #[allow(clippy::const_is_empty)] will be required here (although it appears that Clippy is rarely used on this crate). I would say that this is a consequence of a missing add_extension() method in the standard library which forces to play this kind of game with executables whose name may contain a dot on an extensionless system (as exe.set_extension("") would remove the suffix starting at the latest dot).

[bors]

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

[samueltardieu]

Rebased on master

[xFrednet]

Thank you for the update and also running this on 500 crates :)

[xFrednet]

I have the feeling using miri to check if the value is constant can cause annoying false positives. Though, you ran it in 500 crates, and it only generated a single warning, which indicates that this fear might be overly cautions.

I would suggest adding a restriction at this position, to only evaluate the path, if it's a local path, meaning it comes from the same crate. Constant values of dependencies are not always in the control of the user and should therefore be seen as a black box IMO. (Maybe this should be configurable)

[samueltardieu]

I've run lintcheck on the 1000 most popular crates: no new hit compared to the 1 on 500 crates. But you're right, we might want to err on the side of caution, I've added such a check in an additional commit so that you can have a look at it separately. Tell me if it requires stashing, even though the changes are logically separated.

[xFrednet]

I would guess that the likelihood of a crate using external constants highly depends on the type of crate. Crates.io mainly hosts libraries, they would probably provide such constant values but use them rarly

[samueltardieu]

Yes, except constants such as the path separator, but this is now taken care of, so we get no hit in popular libraries.

[xFrednet]

It looks like most is_empty functions in rustc's std are already static. Could we maybe make our lives easier and throw the whole <recv>.is_empty() expression into the constant evaluation? That seems easier than this entire thing.

(I haven't worked with const too much, so excuse me, if this suggestion is invalid)

[samueltardieu]

Yes, probably, if those methods were const they would not trigger a warning but the use of their result might.

Those methods are const already, it is surprising that the use of their result does not trigger a warning. But as far as I can see, no warning is triggered on this code even for the if true:

fn main() {
    if true {
        println!("True");
    }

    if [0u8; 0].is_empty() {
        println!("Is empty");
    }
}

[samueltardieu]

r? @matthiaskrgr
(solliciting another reviewer as @xFrednet seems to be busy right now)

[xFrednet]

LGTM, thank you for addressing my comments.

The last few weeks have been a bit busy (And I also genuinely needed a break), sorry for delaying the review.

Either way, let's get this merged :D

@bors r+

[bors]

📌 Commit 898ed88 has been approved by xFrednet

It is now in the queue for this repository.

[bors]

⌛ Testing commit 898ed88 with merge 453242c...

[samueltardieu]

The last few weeks have been a bit busy (And I also genuinely needed a break)

I can relate, no problem, that's why I requested another reviewer instead of bothering you.

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 453242c to master...