unconditional_recursion lint is confused by (at least) blanket PartialEq impls

[HadrienG2]

Summary

So, I am writing an hwloc binding and in various circumstances hwloc hands me over C allocations that I am supposed to free using their routine of choice.

Our concern today is a C allocation that contains an XML string, which I model using an RAII type that has an explicit conversion to Rust string, and can be compared with either itself or any other type that can be compared with a Rust string using a pair of PartialEq impls based on the aforementioned explicit conversion.

The second PartialEq impl (which, as it turns out, did not quite do what I want and is going to be fixed) is wrongly flagged as unconditionally recursive by the unconditional_recursion lint, when actually it is fine.

I suspect that happens because the unconditional_recursion lint somehow does not understand that I'm calling the PartialEq impl of &str, not recursing into the PartialEq impl of XML<'_>. If so, the bug may not lie in PartialEq-specific logic, but in more general trait bound processing. Another thing that fuels my suspicion that this isn't a problem with PartialEq specific code is that the lint is not triggered if I replace the explicit .eq() call with an equivalent == operator expression.

Lint Name

unconditional_recursion

Reproducer

This simplified reproducer...

struct Foo;

impl Foo {
    fn as_str(&self) -> &str {
        "Foo"
    }
}

impl PartialEq for Foo {
    fn eq(&self, other: &Self) -> bool {
        self.as_str().eq(other.as_str())
    }
}

impl<T> PartialEq<T> for Foo
where
    for<'a> &'a str: PartialEq<T>
{
    fn eq(&self, other: &T) -> bool {
        (&self.as_str()).eq(other)
    }
}

fn main() {
    println!("{}", Foo == "Hello");
}

...triggers the unconditional_recursion lint:

warning: function cannot return without recursing
  --> src/main.rs:10:5
   |
10 | /     fn eq(&self, other: &Self) -> bool {
11 | |         self.as_str().eq(other.as_str())
12 | |     }
   | |_____^
   |
note: recursive call site
  --> src/main.rs:11:9
   |
11 |         self.as_str().eq(other.as_str())
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unconditional_recursion
   = note: `#[warn(clippy::unconditional_recursion)]` on by default

Because a different PartialEq implementation is called, the program is actually not unconditionally recursive, as you can check by running it. So I would expect clippy not to trigger this lint, which is incorrect in this particular situation.

Version

rustc 1.77.0-nightly (88189a71e 2024-01-19)
binary: rustc
commit-hash: 88189a71e4e4376eea82ac61db6a539612eb200a
commit-date: 2024-01-19
host: x86_64-unknown-linux-gnu
release: 1.77.0-nightly
LLVM version: 17.0.6

Additional Labels

@rustbot label +C-bug +I-false-positive +L-suspicious

[y21]

This looks like the same underlying issue as #12154, so this will be fixed by #12177.

For .eq() method calls, the lint tries to extract the DefId of the receiver to early return if the type doesn't match with self, but &str does not have a DefId, so it doesn't actually get to the early return.

Another thing that fuels my suspicion that this isn't a problem with PartialEq specific code is that the lint is not triggered if I replace the explicit .eq() call with an equivalent == operator expression.

This is because the code for == checking is written in a different way that does not early-return, but has the DefId extracting part of the if chain, so if that extracting fails it won't lint.

Adding this as a test case to the PR.

[kpreid]

A user observed this in rustc 1.77.0-beta.7 (339fb6965 2024-03-06) and I can confirm that the reproducer above still has the false warning on the Playground.

@rustbot label +beta-nominated

[kpreid]

...wait, that belongs on the PR, not the issue, sorry.

@rustbot label -beta-nominated