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

Add lint against function pointer comparisons #118833

Merged
merged 4 commits into from
Dec 5, 2024

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Dec 11, 2023

This is kind of a follow-up to #117758 where we added a lint against wide pointer comparisons for being ambiguous and unreliable; well function pointer comparisons are also unreliable. We should IMO follow a similar logic and warn people about it.


unpredictable_function_pointer_comparisons

warn-by-default

The unpredictable_function_pointer_comparisons lint checks comparison of function pointer as the operands.

Example

fn foo() {}
let a = foo as fn();

let _ = a == foo;

Explanation

Function pointers comparisons do not produce meaningful result since they are never guaranteed to be unique and could vary between different code generation units. Furthermore different function could have the same address after being merged together.


This PR also uplift the very similar clippy::fn_address_comparisons lint, which only linted on if one of the operand was an ty::FnDef while this PR lints proposes to lint on all ty::FnPtr and ty::FnDef.

@rustbot labels +I-lang-nominated

Edit: Blocked on rust-lang/libs-team#323 being accepted and it's follow-up pr

@rustbot
Copy link
Collaborator

rustbot commented Dec 11, 2023

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@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. labels Dec 11, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 11, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

The Miri subtree was changed

cc @rust-lang/miri

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 11, 2023
@rust-log-analyzer

This comment has been minimized.

@Noratrieb
Copy link
Member

The lint name is very long, it would be nice if we could come up with a shorter name. I don't have a suggestion though.

@Urgau
Copy link
Member Author

Urgau commented Dec 12, 2023

The lint name is very long, it would be nice if we could come up with a shorter name. I don't have a suggestion though.

Sure, but I don't think it matter that much, I'm not expecting anyone to write the lint name by hand (since it's warn-by-default, people can just copy/paste in the diagnostic output). We also already have some pretty long lint name, like unknown_or_malformed_diagnostic_attributes.
I also think it's better to have a long and descriptive lint name than a short one that is less descriptive.

But if someone comes up with a name as descriptive but shorter I can integrate it, I'm just not sure it's worth anyone time.

@Urgau Urgau force-pushed the lint_function_pointer_comparisons branch from e0a6772 to 4798319 Compare December 12, 2023 11:38
@rust-log-analyzer

This comment has been minimized.

@Urgau Urgau force-pushed the lint_function_pointer_comparisons branch from 444a0b9 to 23214cb Compare December 13, 2023 10:22
@compiler-errors
Copy link
Member

If it's not too much work, could we do a crater run here to find out what kind of places are using this kind of comparison in the wild?

@Urgau
Copy link
Member Author

Urgau commented Dec 13, 2023

If it's not too much work, could we do a crater run here to find out what kind of places are using this kind of comparison in the wild?

Sure, I just pushed a commit making the lint deny-by-default. I will let you do the bors+craterbot commands.

@compiler-errors
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Dec 13, 2023

⌛ Trying commit 6c5c546 with merge 2959657...

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 13, 2023
…ons, r=<try>

Add lint against function pointer comparisons

This is kind of a follow-up to rust-lang#117758 where we added a lint against wide pointer comparisons for being ambiguous and unreliable; well function pointer comparisons are also unreliable. We should IMO follow a similar logic and warn people about it.

-----

## `unpredictable_function_pointer_comparisons`

*warn-by-default*

The `unpredictable_function_pointer_comparisons` lint checks comparison of function pointer as the operands.

### Example

```rust
fn foo() {}
let a = foo as fn();

let _ = a == foo;
```

### Explanation

Function pointers comparisons do not produce meaningful result since they are never guaranteed to be unique and could vary between different code generation units. Furthermore different function could have the same address after being merged together.

----

This PR also uplift the very similar `clippy::fn_address_comparisons` lint, which only linted on if one of the operand was an `ty::FnDef` while this PR lints proposes to lint on all `ty::FnPtr` and `ty::FnDef`.

`@rustbot` labels +I-lang-nominated
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Dec 13, 2023

☀️ Try build successful - checks-actions
Build commit: 2959657 (29596578cb5035fa2b7ac7fbdfea23fab38836e7)

@traviscross
Copy link
Contributor

traviscross commented Dec 13, 2023

@rustbot labels -I-lang-nominated

We discussed this in the T-lang meeting today. There was some support for this, but people were concerned about whether there may be legitimate use cases and what we would be suggesting that those people do instead. For wide pointer comparisons, we suggest that people use ptr::addr_eq even though that has the same problems as comparison with == as it at least makes the intent of the user clear. What would be the comparable thing we would do here?

The consensus was that seeing use cases would help, and that a crater run would help to find those use cases. That's now happening in the comments above. Once the crater run comes back and has been analyzed, please renominate for T-lang discussion.

For the analysis, we're looking to find 1) use cases of this that are correct in the sense that they rely only on the actual semantics, and 2) the prevalence of bugs where people are using this in ways that rely on semantics that do not actually hold.

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 13, 2023
@RalfJung
Copy link
Member

What would be the comparable thing we would do here?

There's no such thing, comparing Rust functions for equality is in general just inherently meaningless. That doesn't make code using == any less buggy though. If we want to support that usecase we need a fundamental language change, likely removing the LLVM flag which tells the backend that function addresses do not matter.

The flip side of this is that the standard library itself actually relies on such a comparison in the format-args machinery... it's for a special case (a monomorphic function) where the current implementation AFAIK does not generate duplicates (but I don't know how multi-CGU handling works so I am not sure). It's certainly not guaranteed by the language.

@compiler-errors
Copy link
Member

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-118833 created and queued.
🤖 Automatically detected try build 2959657
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 14, 2023
@traviscross
Copy link
Contributor

What would be the comparable thing we would do here?

There's no such thing, comparing Rust functions for equality is in general just inherently meaningless.

One thought expressed in the meeting was as follows:

Comparing functions for equality via pointers may yield false negatives but not false positives. The fact that two functions may compare unequal (based on pointers to them) but in fact do the same thing is a rather fundamental property of not just Rust, but of any language. In general, it's impossible to know whether two different functions may in fact do the same thing.

In this light, maybe it's OK that these comparisons produce false negatives, and maybe there exist valid use cases that only rely on the property that we will not return false positives.

@RalfJung
Copy link
Member

Comparing functions for equality via pointers may yield false negatives but not false positives.

That's not true. There are both false negatives and false positives. That's exactly why I wanted the lint description to be clear about this.

False positives arise when LLVM merges two functions because they optimized to the same code.

@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

@bors

This comment was marked as resolved.

@Urgau Urgau force-pushed the lint_function_pointer_comparisons branch from 33e1745 to 2ed43d9 Compare November 9, 2024 12:22
@apiraino apiraino added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed finished-final-comment-period The final comment period is finished for this PR / Issue. labels Nov 11, 2024
@Urgau Urgau force-pushed the lint_function_pointer_comparisons branch from 2ed43d9 to 7b06fcf Compare December 2, 2024 17:44
@Urgau
Copy link
Member Author

Urgau commented Dec 2, 2024

ptr::ptr_addr_eq was stabilized. I've rebased and CI is green. Let's move on.

@rustbot labels -S-blocked -S-waiting-on-fcp +finished-final-comment-period
@bors r=@cjgillot

@rustbot rustbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. labels Dec 2, 2024
@Urgau Urgau closed this Dec 2, 2024
@Urgau Urgau reopened this Dec 2, 2024
@Urgau
Copy link
Member Author

Urgau commented Dec 2, 2024

@bors r=@cjgillot

@bors
Copy link
Contributor

bors commented Dec 2, 2024

📌 Commit 7b06fcf has been approved by cjgillot

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 2, 2024
fmease added a commit to fmease/rust that referenced this pull request Dec 4, 2024
…isons, r=cjgillot

Add lint against function pointer comparisons

This is kind of a follow-up to rust-lang#117758 where we added a lint against wide pointer comparisons for being ambiguous and unreliable; well function pointer comparisons are also unreliable. We should IMO follow a similar logic and warn people about it.

-----

## `unpredictable_function_pointer_comparisons`

*warn-by-default*

The `unpredictable_function_pointer_comparisons` lint checks comparison of function pointer as the operands.

### Example

```rust
fn foo() {}
let a = foo as fn();

let _ = a == foo;
```

### Explanation

Function pointers comparisons do not produce meaningful result since they are never guaranteed to be unique and could vary between different code generation units. Furthermore different function could have the same address after being merged together.

----

This PR also uplift the very similar `clippy::fn_address_comparisons` lint, which only linted on if one of the operand was an `ty::FnDef` while this PR lints proposes to lint on all `ty::FnPtr` and `ty::FnDef`.

`@rustbot` labels +I-lang-nominated

~~Edit: Blocked on rust-lang/libs-team#323 being accepted and it's follow-up pr~~
fmease added a commit to fmease/rust that referenced this pull request Dec 4, 2024
…isons, r=cjgillot

Add lint against function pointer comparisons

This is kind of a follow-up to rust-lang#117758 where we added a lint against wide pointer comparisons for being ambiguous and unreliable; well function pointer comparisons are also unreliable. We should IMO follow a similar logic and warn people about it.

-----

## `unpredictable_function_pointer_comparisons`

*warn-by-default*

The `unpredictable_function_pointer_comparisons` lint checks comparison of function pointer as the operands.

### Example

```rust
fn foo() {}
let a = foo as fn();

let _ = a == foo;
```

### Explanation

Function pointers comparisons do not produce meaningful result since they are never guaranteed to be unique and could vary between different code generation units. Furthermore different function could have the same address after being merged together.

----

This PR also uplift the very similar `clippy::fn_address_comparisons` lint, which only linted on if one of the operand was an `ty::FnDef` while this PR lints proposes to lint on all `ty::FnPtr` and `ty::FnDef`.

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

~~Edit: Blocked on rust-lang/libs-team#323 being accepted and it's follow-up pr~~
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 4, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#118833 (Add lint against function pointer comparisons)
 - rust-lang#122161 (Fix suggestion when shorthand `self` has erroneous type)
 - rust-lang#133233 (Add context to "const in pattern" errors)
 - rust-lang#133843 (Do not emit empty suggestion)
 - rust-lang#133863 (Rename `core_pattern_type` and `core_pattern_types` lib feature  gates to `pattern_type_macro`)
 - rust-lang#133872 (No need to create placeholders for GAT args in confirm_object_candidate)
 - rust-lang#133874 (`fn_sig_for_fn_abi` should return a `ty::FnSig`, no need for a binder)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 5, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#118833 (Add lint against function pointer comparisons)
 - rust-lang#122161 (Fix suggestion when shorthand `self` has erroneous type)
 - rust-lang#133233 (Add context to "const in pattern" errors)
 - rust-lang#133843 (Do not emit empty suggestion)
 - rust-lang#133863 (Rename `core_pattern_type` and `core_pattern_types` lib feature  gates to `pattern_type_macro`)
 - rust-lang#133872 (No need to create placeholders for GAT args in confirm_object_candidate)
 - rust-lang#133874 (`fn_sig_for_fn_abi` should return a `ty::FnSig`, no need for a binder)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 5, 2024
Rollup of 10 pull requests

Successful merges:

 - rust-lang#118833 (Add lint against function pointer comparisons)
 - rust-lang#122161 (Fix suggestion when shorthand `self` has erroneous type)
 - rust-lang#133233 (Add context to "const in pattern" errors)
 - rust-lang#133761 (Update books)
 - rust-lang#133843 (Do not emit empty suggestion)
 - rust-lang#133863 (Rename `core_pattern_type` and `core_pattern_types` lib feature  gates to `pattern_type_macro`)
 - rust-lang#133872 (No need to create placeholders for GAT args in confirm_object_candidate)
 - rust-lang#133874 (`fn_sig_for_fn_abi` should return a `ty::FnSig`, no need for a binder)
 - rust-lang#133890 (Add a new test ui/incoherent-inherent-impls/no-other-unrelated-errors to check E0116 does not cause unrelated errors)
 - rust-lang#133892 (Revert rust-lang#133817)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 35ea48d into rust-lang:master Dec 5, 2024
12 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 5, 2024
Rollup merge of rust-lang#118833 - Urgau:lint_function_pointer_comparisons, r=cjgillot

Add lint against function pointer comparisons

This is kind of a follow-up to rust-lang#117758 where we added a lint against wide pointer comparisons for being ambiguous and unreliable; well function pointer comparisons are also unreliable. We should IMO follow a similar logic and warn people about it.

-----

## `unpredictable_function_pointer_comparisons`

*warn-by-default*

The `unpredictable_function_pointer_comparisons` lint checks comparison of function pointer as the operands.

### Example

```rust
fn foo() {}
let a = foo as fn();

let _ = a == foo;
```

### Explanation

Function pointers comparisons do not produce meaningful result since they are never guaranteed to be unique and could vary between different code generation units. Furthermore different function could have the same address after being merged together.

----

This PR also uplift the very similar `clippy::fn_address_comparisons` lint, which only linted on if one of the operand was an `ty::FnDef` while this PR lints proposes to lint on all `ty::FnPtr` and `ty::FnDef`.

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

~~Edit: Blocked on rust-lang/libs-team#323 being accepted and it's follow-up pr~~
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.