Lint on fn pointers comparisons in external macros

[Urgau]

This PR extends the recently stabilized unpredictable_function_pointer_comparisons lint to also lint on Option<{function pointer}> and as well as linting in external macros (as to catch assert_eq! and others).

assert_eq!(Some::<FnPtr>(func), Some(func as unsafe extern "C" fn()));
//~^ WARN function pointer comparisons

#[derive(PartialEq, Eq)]
struct A {
    f: fn(),
    //~^ WARN function pointer comparisons
}

Fixes #134527

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
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

[jieyouxu]

r? compiler

[compiler-errors]

This seems like it could be more generalized, but fine for now.

[compiler-errors]

Specifically linting in macros seems like a much larger extension of the lint, though. Was this ever discussed by T-lang?

[compiler-errors]

Though I agree with it in theory.

[Urgau]

Specifically linting in macros seems like a much larger extension of the lint, though. Was this ever discussed by T-lang?

Yeah, linting on external macros made me hesitate to put it in the same PR as linting on Option of fn ptrs given that it's a much bigger extension, but it's still in line with the goal of the lint as FCP-ed by T-lang (so it's fine? 🤷).

Anyway, cc @traviscross

[traviscross]

@rustbot labels +I-lang-nominated +T-lang

We did talk about the derive(PartialEq) case, and we had left that undecided, so let's nominate to discuss.

Here are some notes on that unanswered question, from an earlier set of minutes:

TC: Separately, on the 2024-01-10 call, we discussed some interesting use cases for function pointer comparison, especially when it's indirected through PartialEq. We had earlier said we didn't want to lint when such comparisons were indirected through generics, but we didn't address the non-generic case of simply composing such comparisons.

One example of how this is used is in the standard library, in Waker::will_wake:

https://doc.rust-lang.org/core/task/struct.Waker.html#method.will_wake

It's comparing multiple function pointers via a #[derive(PartialEq)] on the RawWakerVTable.

We decided on 2024-01-01 that this case was interesting and we wanted to think about it further. We opened a discussion thread about this:

https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Function.20pointer.20comparison.20and.20.60PartialEq.60

Since then, another interesting use case in the standard library was raised, in the formatting machinery:

https://doc.rust-lang.org/src/core/fmt/rt.rs.html

What do we think about these, and would we lint on derived PartialEq cases like these or no?

[Urgau]

I have extracted the Option<{fn ptr}> part into #134586 (which was merged).

This PR is now only about linting in external macros.

[compiler-errors]

This is still waiting on T-lang

@rustbot team

[traviscross]

@rfcbot fcp merge

We talked about this in lang triage today. Personally, I've warmed to the idea that if we've decided to lint against these comparisons then, yes, we should lint against them everywhere, and people can add the allow if needed on the derive. So I propose that we do this.

[rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

Concerns:

• explain to the user where to put the allow attribute on derives resolved by Lint on fn pointers comparisons in external macros #134536 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[tmandry]

@rfcbot reviewed

@rfcbot concern explain to the user where to put the allow attribute on derives

From the test, it looks like you have to place the #[allow()] below the derive attribute, which makes sense from an implementation perspective, but is surprising. I would like to support the allow going above the derive (or even in front of the PartialEq word, though that probably requires an RFC).

If we can't do either of those, can we guide the user by telling them where to put the allow attribute they aren't confused?

Otherwise I'm in favor of this change.

[Urgau]

The #[allow(unpredictable_function_pointer_comparisons)] can already be placed before or after the #[derive], in both cases it will silence the lint. I have added them as tests to verify it won't regress.

[tmandry]

Okay, thanks for adding the test @Urgau!

@rfcbot resolve explain to the user where to put the allow attribute on derives

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔