Resolve inherent associated functions & constants defined on function pointer types

[fmease]

Fixes #108270.

[rustbot]

r? @TaKO8Ki

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

[compiler-errors]

r? @compiler-errors

@bors try @rust-timer queue

[bors]

⌛ Trying commit ffad636 with merge 192617ada336ebe7e790566a33fd3b414937e71b...

[compiler-errors]

Nothing's wrong with the impl, just two things:

1. Core doesn't currently use this, so we're kinda doing extra work for nothing?
2. Since we're doing extra work, there's a tiny risk that we're gonna cause a perf regression.

Let's see what rust-timer says, then go from there.

[bors]

☀️ Try build successful - checks-actions
Build commit: 192617ada336ebe7e790566a33fd3b414937e71b (192617ada336ebe7e790566a33fd3b414937e71b)

[rust-timer]

Finished benchmarking commit (192617ada336ebe7e790566a33fd3b414937e71b): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.1%	[1.0%, 1.2%]	2
Regressions ❌ (secondary)	2.2%	[2.0%, 2.6%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[1.0%, 1.2%]	2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.7%	[3.7%, 3.7%]	1
Regressions ❌ (secondary)	2.7%	[2.7%, 2.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.7%	[3.7%, 3.7%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

[fmease]

Closing for now due to the perf regression. Might re-open at some point once I have a vague idea how to speed things up.

[fmease]

Ah, I just saw #103042 (comment), so potentially keccak and cranelift-codegen are false positives after all. Let's wait and see and I might open this PR at some point w/o any adjustments.

[fmease]

It seems like #108815 fixed the keccak & cranelift-codegen performance ‘bimodalities’. See also keccak bimodility (Zulip).

If the above is true and if I understood the situation correctly, it's worth rechecking the perf of this PR.
@compiler-errors Could you kick off a timing check? That'd be great. Thanks in advance! :)

[compiler-errors]

@bors try @rust-timer queue

[bors]

⌛ Trying commit ffad636 with merge e322b72bfaf72c243e2cebc027cfbc2e6b050d00...

[bors]

☀️ Try build successful - checks-actions
Build commit: e322b72bfaf72c243e2cebc027cfbc2e6b050d00 (e322b72bfaf72c243e2cebc027cfbc2e6b050d00)

[rust-timer]

Finished benchmarking commit (e322b72bfaf72c243e2cebc027cfbc2e6b050d00): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.3%	[-0.3%, -0.3%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.8%	[0.9%, 2.6%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-1.4%	[-1.4%, -1.4%]	1
All ❌✅ (primary)	0.8%	[-1.0%, 2.6%]	3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.6%	[-0.6%, -0.6%]	1

[compiler-errors]

Gonna FCP this for T-types -- The question here is whether or not we want to be able to write incoherent/core impls for fn ptrs. I think it's good for consistency, but not particularly useful (since we can't be generic over argument arity), but also I see no harm in allowing it -- seems along the same lines that we allow incoherent impls for tuples. cc #94963

@rfcbot fcp merge

[rfcbot]

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

• @aliemjay
• @compiler-errors
• @jackh726
• @lcnr
• @nikomatsakis
• @oli-obk
• @spastorino

Concerns:

• fnptr (Resolve inherent associated functions & constants defined on function pointer types #108347 (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!

See this document for info about what commands tagged team members can give me.

[lcnr]

I cannot imagine us ever using this. Any impl for function pointers should go through the FnPtr trait instead 🤔 I guess this is fairly trivially correct so it doesn't matter too much

[oli-obk]

We could reject it with an error mentioning the FnPtr trait

[nikomatsakis]

I'm confused here -- we are allowing incoherent impls? I would be more comfortable using FnPtr, are there methods where that doesn't suffice?

[nikomatsakis]

@rfcbot concern fnptr

Nothing against this PR per se, but I'd prefer that we use the FnPtr trait and disallow incoherent fn-based inherent impls, and I want to better understand that option.

[fmease]

we are allowing incoherent impls?

Not outright, only with #![rustc_coherence_is_core], so effectively only in the core library.

FnPtr is definitely superior. This PR is mostly motivated by a drive towards consistency in the area of inherent impls.

[compiler-errors]

Nothing against this PR per se, but I'd prefer that we use the FnPtr trait and disallow incoherent fn-based inherent impls, and I want to better understand that option.

I don't mind that approach either necessarily, but in that case, we probably should also reject inherent impls on tuples and maybe others...

[lcnr]

@rfcbot cancel

[rfcbot]

@lcnr proposal cancelled.

[lcnr]

talked about this in the planning meeting today https://rust-lang.zulipchat.com/#narrow/stream/326132-t-types.2Fmeetings/topic/2023-08-07.20planning.20meeting/near/382646902

We would prefer to completely forbid inherent impls on function pointers with a suggestion to use the FnPtr trait instead.

We are open to adding this once there is a valid use case here, but this feels quite unlikely as adding an impl for just one kind of function pointer is far from ideal.

[fmease]

Makes sense. I'm gonna close this PR then & create a new one for forbidding inherent impls on function pointer types.