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

Lint on fn pointers comparisons in external macros #134536

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Dec 19, 2024

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
Copy link
Collaborator

rustbot commented Dec 19, 2024

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 19, 2024
@jieyouxu
Copy link
Member

r? compiler

@rustbot rustbot assigned petrochenkov and unassigned jieyouxu Dec 20, 2024
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@compiler-errors
Copy link
Member

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

@compiler-errors
Copy link
Member

Though I agree with it in theory.

@Urgau
Copy link
Member Author

Urgau commented Dec 20, 2024

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
Copy link
Contributor

traviscross commented Dec 20, 2024

@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?

@rustbot rustbot added I-lang-nominated Nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Dec 20, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 21, 2024
…er-errors

Also lint on option of function pointer comparisons

This PR is the first part of rust-lang#134536, ie. the linting on `Option<{fn ptr}>` in the `unpredictable_function_pointer_comparisons` lint, which isn't part of the lang nomination that the second part is going trough, and so should be able to be approved independently.

Related to rust-lang#134527
r? `@compiler-errors`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 21, 2024
Rollup merge of rust-lang#134586 - Urgau:fn-ptr-lint-option, r=compiler-errors

Also lint on option of function pointer comparisons

This PR is the first part of rust-lang#134536, ie. the linting on `Option<{fn ptr}>` in the `unpredictable_function_pointer_comparisons` lint, which isn't part of the lang nomination that the second part is going trough, and so should be able to be approved independently.

Related to rust-lang#134527
r? `@compiler-errors`
@bors

This comment was marked as resolved.

@Urgau
Copy link
Member Author

Urgau commented Dec 21, 2024

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

This PR is now only about linting in external macros.

@Urgau Urgau changed the title Lint on option of fn pointers and in external macros Lint on fn pointers comparisons in external macros Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-lang-nominated Nominated for discussion during a lang team meeting. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unpredictable_function_pointer_comparisons doesn't lint against Option<fn()>
7 participants