added a lint against function item references

[ayrtonm]

this lint suggests casting function references to *const ()
closes#75239
r? @RalfJung

[jyn514]

Please add a test for this in src/test/ui. There are instructions at https://rustc-dev-guide.rust-lang.org/tests/adding.html#ui.

[jyn514]

Hmm, why is this only looking in the current crate? I'd think we'd want to link on &std::env::var or something too.

[ayrtonm]

Good point, checking references to functions in other crates is probably important. I'm not sure I understand how to check other crates though. I've been looking through the docs for hir::map::Map and ty::TyCtxt, but if someone could give me a few pointers or help out that'd be great.

[jyn514]

I don't think this needs to work on the HIR at all. Try type_of instead. type_of returns a Ty, so you could call matches!(ty.kind, TyKind::FnDef(_, _)): https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/enum.TyKind.html

[RalfJung]

Yes, I think @jyn514 is right -- going through HIR here is unexpected.

[RalfJung]

Actually, this is a design decision... do we want the lint to fire on

let fn_item = function_name;let _val = &fn_item;

If yes, this should be type-based. If no, we have to instead check if the referent is the name of a function.

[ollie27]

Shouldn't we suggest casting to a proper function pointer type rather than *const ()?

[tesuji]

Shouldn't we suggest casting to a proper function pointer type

Sometimes function pointer type is really long.

[ollie27]

Sometimes function pointer type is really long.

We could always suggest using underscores to prevent the types getting too long e.g. as fn(_, _) -> _.

[ayrtonm]

The lint breaks the following tests, so does it make sense to just add #![allow(function_references)] to not clutter them or should we add warn annotations?

custom-test-frameworks-simple.rs
enum-discriminant/actually_not_an_enum-discriminant.rs
functions-closures/fn-item-type-zero-sized.rs
issues/issue-16922-rpass.rs
issues/issue-20847.rs
issues/issue-22933-1.rs
issues/issue-25757.rs
trivial_casts-rpass.rs
unboxed-closures/unboxed-closures-extern-fn-hr.rs
unboxed-closures/unboxed-closures-extern-fn.rs

Also compiler/rustc_typeck/src/check/mod.rs:776:37 triggers the lint, so I'm guessing there should be an allow attribute on that as well?

[jyn514]

Also compiler/rustc_typeck/src/check/mod.rs:776:37 triggers the lint, so I'm guessing there should be an allow attribute on that as well?

That code looks like this:

fnadt_destructor(tcx:TyCtxt<'_>,def_id:DefId) -> Option<ty::Destructor>{
tcx.calculate_dtor(def_id,&mut dropck::check_drop_impl)}

and calculate_dtor takes &mut dyn FnMut(Self, DefId) -> Result<(), ErrorReported>. Maybe this shouldn't lint if it's being used as a dyn Fn*?

[RalfJung]

The lint breaks the following tests, so does it make sense to just add #![allow(function_references)] to not clutter them or should we add warn annotations?

custom-test-frameworks-simple.rs
enum-discriminant/actually_not_an_enum-discriminant.rs
functions-closures/fn-item-type-zero-sized.rs
issues/issue-16922-rpass.rs
issues/issue-20847.rs
issues/issue-22933-1.rs
issues/issue-25757.rs
trivial_casts-rpass.rs
unboxed-closures/unboxed-closures-extern-fn-hr.rs
unboxed-closures/unboxed-closures-extern-fn.rs

Also compiler/rustc_typeck/src/check/mod.rs:776:37 triggers the lint, so I'm guessing there should be an allow attribute on that as well?

actually_not_an_enum-discriminant.rs should have the lint allowed for the line that triggers it, that looks deliberate.

16922, 20847, 22933, 25757 all seem deliberate... and they make me wonder if we need to improve the lint. They are all of the form &fn_name as &dyn Something, but sometimes the as is implicit. Taking a reference here is crucial. The same thing is probably happening with custom test frameworks, and it also explains the rustc_typeck failure.

So given that we have at least 5 false positives in the rustc codebase and test suite, we cannot leave the lint as-is, we have to find something smarter. The key thing that happens in all these cases is that the &{fn item} gets coerced or cast to an &dyn Trait; that should be something that we can detect, I think... the only problem is that to detect this we have to look "outside" of the ExprKind::AddrOf, and I do not know how to do that.

@oli-obk@Manishearth summoning some clippy people that can hopefully help with suggestions for how to implement such a lint. :) Basically we want to detect &expr where expr satisfies some conditions (we got that part covered) and the reference is not immediately cast/coerced to &dyn Trait -- how can we implement the latter check?

[oli-obk]

We could change the lint to a MIR lint that triggers on a CastKind::Misc where the casted value's type is a reference to a function item zst. This would ensure that we only ever lint the cast and never the creation.

[RalfJung]

We don't really have MIR lints as a fixed notion though, do we? We have a few ad-hoc constructions that plug into different parts of the MIR pipeline, see #72515.

[oli-obk]

The way clippy does this is to have a normal lint that loads the mir of any function definition it sees and then analyzes that. Having a good point in the optimization pipeline is something @rust-lang/wg-mir-opt should come up with. Until we have made an informed decision, just putting it next to

rust/compiler/rustc_mir/src/transform/mod.rs

Line 386 in f76eda3

check_consts::post_drop_elaboration::check_live_drops(tcx, def.did,&body);

should be fine

[RalfJung]

Shouldn't we rather add it to one of the three places where we already have a MIR lint? E.g.

rust/compiler/rustc_mir/src/transform/mod.rs

Line 309 in f76eda3

&check_packed_ref::CheckPackedRef,

[oli-obk]

Oh there they are :D I thought we had that and didn't find it and then thought I imagined it. Yea, that's a good place to get deterministic MIR

[RalfJung]

Not sure what you mean by "deterministic".^^

[oli-obk]

Optimizations can change things in unexpected ways, we don't want to optimize out some code on the current platform and then only lint on other platforms. So we need to do it before the main optimization pipeline

[ayrtonm]

It's still very much a WIP, but I changed lint to work with MIR. It now excludes &fn_name as &dyn Trait (implicit and explicit) and (&fn_name)() (function calls). This first MIR implementation emits a lint for

let fn_item = function_name;
let _val = &fn_item;

but I think it makes more sense to exclude it in this case. I also think we should exclude it when a function reference appears alone on the rhs of a regular let binding like let zst_ref = &function_name. This broke one or two existing tests where zst_ref was just used by casting/coercion later on. It'd be nice to get a second opinion on this though.

I don't quite understand how to work with generics/ParamEnv in the compiler, so the coercion that happens in the first argument of parameterized_call_fn below doesn't exclude the lint yet.

fn parameterized_call_fn<F: Fn(u32) -> u32>(f: &F, x: u32) { f(x); }
parameterized_call_fn(&fn_name, 1);

It's also why almost all of these tests are failing.

consts/const_constructor/const-construct-call.rs#const_fn
consts/const_constructor/const-construct-call.rs#min_const_fn
enum-discriminant/actually_not_an_enum-discriminant.rs
functions-closures/fn-item-type-zero-sized.rs
issues/issue-20847.rs
lint/function-references.rs
mir/mir_calls_to_shims.rs
nll/promoted-bounds.rs
nll/user-annotations/adt-tuple-struct-calls.rs
nll/user-annotations/promoted-annotation.rs
struct-ctor-mangling.rs
unboxed-closures/unboxed-closures-extern-fn-hr.rs
unboxed-closures/unboxed-closures-extern-fn.rs

I also had another minor doubt. This implementation compares the spans of function references with function calls/casts/coercions to decide what function references to exclude. This works, but feels kinda janky... is there a better way to check if two MIR rvalues refer to the same thing in the source?

[oli-obk]

I also think we should exclude it when a function reference appears alone on the rhs of a regular let binding like let zst_ref = &function_name. This broke one or two existing tests where zst_ref was just used by casting/coercion later on. It'd be nice to get a second opinion on this though.

Good point. So, since we're only trying to avoid casts from references to fn defintions to raw pointers or ints, we may be able to simplify the logic down to just looking at Rvalue::Cast and nothing else. Not sure if that works, let me know what you think:

• get the type of the operand in https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/enum.Rvalue.html#variant.Cast
• if that type is TyKind::Ref and that inner type is TyKind::FnDef, then we have a candidate
• if the CastKind from the Rvalue::Cast is PointerCast::Unsize, then we don't lint, in all other cases we lint

Is this missing some cases that we should lint or linting in cases that we should not lint?

[RalfJung]

I also think we should exclude it when a function reference appears alone on the rhs of a regular let binding like let zst_ref = &function_name.

I guess it is reasonable to start conservative and lint less; we can crank it up later if we feel we should.

Is this missing some cases that we should lint or linting in cases that we should not lint?

Will this lint on println!("{:p}", &fun);, which explicitly came up in the original issue?

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[oli-obk]

Sorry, I did not see that force push. Please leave a message when you force push

[oli-obk]

Just a few small things, then this is ready to go.

[ayrtonm]

Sorry, I did not see that force push. Please leave a message when you force push

My bad, I didn't realize you wouldn't get a notification.

So the lint message could be something like "taking a reference to a function item does not give a function pointer" and the suggestion could be "cast ident to obtain a function pointer: ident as fn()"?

[oli-obk]

So the lint message could be something like "taking a reference to a function item does not give a function pointer" and the suggestion could be "cast ident to obtain a function pointer: ident as fn()"?

yes. that looks right.

[ayrtonm]

Just made the changes @oli-obk requested. I left the suggestion's applicability Unspecified instead of MaybeIncorrect since in cases like transmute((&foo,)), the exact substitution won't result in valid Rust code. The problem is that getting the subset of the span that should be replaced from MIR isn't straightforward, but these cases should be rare in practice.

[oli-obk]

@bors r+

Thanks!

[bors]

📌 Commit c791c64 has been approved by oli-obk

[bors]

⌛ Testing commit c791c64 with merge 07e968b...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 07e968b to master...