r? @cjgillot

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

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

The Miri subtree was changed

cc @rust-lang/miri

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.

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.

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?

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.

@bors try

⌛ Trying commit 6c5c546 with merge 2959657...

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

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

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.

@craterbot check

👌 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

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.

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.

Crater Analysis Result

Summary

...

Note: I stopped at the nes-rs for now, I may continue later but given the trends I don't think anything new will come.

Highlights

ShivaBhattacharjee/Synthia

code

VM / Interpreter, very similar pattern in many other projects.
Unable to determine if it's true-positive or false-positive.

impl PartialEq for Object {
    fn eq(&self, other: &Self) -> bool {
        match (self, other) {
            (Object::Number(a), Object::Number(b)) => a == b,
            (Object::String(a), Object::String(b)) => a == b,
            ...
            (Object::Fn(a, b, c), Object::Fn(d, e, f)) => a == d && b == e && c == f,
            (Object::Inbuilt(a), Object::Inbuilt(b)) => a == b,
            ...
            _ => false,
        }
    }
}

ZachClayburn/CompilerDesign-Project2

logs code

Rely on function NOT being duplicated.

pub fn get_reduction_name(function_pointer: &ReductionOp) -> &'static str {
    match *function_pointer {
        x if x == reduce_value => "reduce_value()",
        x if x == reduce_binary_op => "reduce_binary_op()",
        x if x == reduce_parenthetical => "reduce_parenthetical()",
        x if x == reduce_unary_operator => "reduce_unary_operator()",
        x if x == reduce_program => "reduce_program()",
        x if x == reduce_statement_list => "reduce_statement_list()",
        x if x == reduce_declaration => "reduce_declaration()",
        x if x == reduce_param_spec => "reduce_param_spec()",
        x if x == reduce_return_statement => "reduce_return_statement()",
        x if x == reduce_procedure_declaration => "reduce_procedure_declaration()",
        x if x == reduce_procedure_call => "reduce_procedure_call()",
        _ => panic!("unknown reduction"),
    }
}

HugoBelotDeloro/rlox

logs code

Compare functions pointers derived from the same list.

                *p = p.iter().filter(|&&f2| f2 != f).copied().collect::<Vec<&OpFn>>();

Observation

1. Some false positives, mainly when filtering fn-ptrs within a vector.
2. Many true-positives, the vast majority when comparing a fn-ptr with a fn-def.

Requiring that one of the operands is an fn-def may be a good compromise.

Note: I think I remember Josh mentioning a caching pattern that involved fn-ptrs, but I haven't seen any evidence that this pattern is used and affected by this lint.

@rustbot label I-lang-nominated -S-waiting-on-review S-waiting-on-team T-lang

The standard library also uses function pointer comparison to implement Waker::will_wake.

I'm concerned about this if there isn't a way to say "I know, I want to do it anyway" other than allowing the lint.

I consider the fat pointer lint to be about "you should be clearer about whether you intended to compare the metadata or not", not "comparing metadata is bad".

It would be entirely correct to memoize based on function pointer equality, for example. If that means that x x and 2 * x share the same cache, then great. And if two different instantiations aren't merged, oh well, it's not the end of the world.

To me a "be sure you know what this does" lint like this is a clippy lint, not a rustc lint, if we can't suggest a better alternative.

I'm concerned about this if there isn't a way to say "I know, I want to do it anyway" other than allowing the lint.

The T-lang triage meeting consensus was to suggest a method (in the same way that was done for thin ptr comparisons lint with ptr::addr_eq) and proposed ptr::fn_addr_eq ACP.

The fact that the result can never be meaningful beyond "definitely runs the same code if true" makes comparing function pointers a lot like needs_drop or similar where you can't rely on it for anything besides optimization and need to be correct in case of spurious incorrect results, and I think that needs to be much more obvious.

The standard library also uses function pointer comparison to implement Waker::will_wake.

It also compares them here, in dubious ways:

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

For the record the T-lib-api ACP#323 about adding ptr::fn_addr_eq (with 2 generic arguments) was accepted and implemented in #129323.

@Urgau this is no longer blocked so if you can resolve the conflicts we can push this forward for a review. Thanks

Looks like on the lang side we still need to make a decision about whether we want this. That's easier now that the libs-api prerequisite happened (on nightly). This is more actionable now, so we'll bump it up the agenda.