builtin trait to abstract over function pointers by lcnr · Pull Request #99531 · rust-lang/rust

lcnr · 2022-07-20T21:41:41Z

the tragedy of

rust/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs

Lines 154 to 156 in a7468c6

if candidates.len() == 1{
returnself.filter_reservation_impls(candidates.pop().unwrap(), stack.obligation);
}

until we figure out 68dea3aaadcc62837ffe7aa6a923fc9bedb6c786 i don't think we can land this. i generally have some ideas here but all of them as somewhat annoying. i think that generally the way trait errors are reported can and should be reworked. not sure what's the best short-term solution here

r? @rust-lang/types cc @RalfJung (from #92964)

rustbot · 2022-07-20T21:41:43Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

RalfJung · 2022-07-20T21:50:04Z

You are implementing expose_addr, not addr.

that means? 😅 what's the correct mir statement here, simply CastKind::Misc?

There is no MIR statement for this, https://doc.rust-lang.org/nightly/std/primitive.pointer.html#method.addr uses transmute.

I think we should just cast to *const () or some other pointer to avoid any kind of provenance worries.

RalfJung · 2022-07-20T21:51:36Z

Oh lol it wants to use FnPtr for things that have nothing to do with function pointers? :(

eddyb · 2022-07-20T23:36:40Z

Oh lol it wants to use FnPtr for things that have nothing to do with function pointers? :(

You see this a lot with certain other traits, like AFAIK you can't (easily?) get errors saying IntoIterator isn't implemented, because it finds the blanket impl of IntoIterator for any Iterator and decides that is where it draws the line. (quick example on playground - you may not have noticed it before because of how connected IntoIterator and Iterator are but it's technically wrong)

I think what needs to happen is we need to distinguish between these two:

"covered" blanket impl
```
impl<T>FooforX<T>whereT:Bar{}
```
- this is fine to continue saying that Bar isn't implemented, since this impl "peels off" an application of the X constructor
- in a sense, T: Bar is "more specific" than X<T>: Foo
"uncovered" blanket impl
```
impl<T>FooforTwhereT:Bar{}
```
- this is where I think we should stop diagnosing it as T: Bar and switch to T: Foo
- we could still potentially mention this impl with its Bar constraint in the same way we already list other examples of inapplicable impls (as a help message), though we should be careful to retain the ability to always hide some traits

@lcnr Does this sound reasonable? Maybe an issue or MCP should be opened about this.

lcnr · 2022-07-21T08:52:17Z

should move this to trait_ref_is_knowable instead

bors · 2022-07-21T20:25:45Z

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

lcnr · 2022-07-25T14:22:28Z

@lcnr Does this sound reasonable? Maybe an issue or MCP should be opened about this.

yeah, i looked into the diagnostics impact of this in #99719 and it looks promising.

fixed diagnostics now, might rework the approach before merging though 😁 (i also have to add tests)

@bors try @rust-timer queue

rust-timer · 2022-07-25T14:22:29Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-07-25T14:22:36Z

⌛ Trying commit 9d0a8e5 with merge 2e62312963f4b040a4e7aaec816772554c4baa1d...

rust-timer · 2022-07-25T14:45:16Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-07-25T15:54:30Z

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

rust-timer · 2022-07-25T15:54:33Z

Queued 2e62312963f4b040a4e7aaec816772554c4baa1d with parent 2fdbf07, future comparison URL.

rust-timer · 2022-07-25T17:11:12Z

Finished benchmarking commit (2e62312963f4b040a4e7aaec816772554c4baa1d): comparison url.

Instruction count

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	1.9%	13.5%	56
Regressions 😿 (secondary)	0.9%	2.1%	13
Improvements 🎉 (primary)	-1.5%	-3.8%	10
Improvements 🎉 (secondary)	-2.4%	-4.0%	25
All 😿🎉 (primary)	1.4%	13.5%	66

Max RSS (memory usage)

Results

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	1.3%	2.1%	2
Regressions 😿 (secondary)	3.1%	3.6%	3
Improvements 🎉 (primary)	-1.8%	-2.6%	5
Improvements 🎉 (secondary)	-2.3%	-3.3%	13
All 😿🎉 (primary)	-0.9%	-2.6%	7

Cycles

Results

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	5.3%	11.7%	12
Regressions 😿 (secondary)	2.4%	2.8%	7
Improvements 🎉 (primary)	-2.8%	-3.8%	3
Improvements 🎉 (secondary)	-3.3%	-4.3%	21
All 😿🎉 (primary)	3.7%	11.7%	15

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf +perf-regression

the arithmetic mean of the percent change↩↩² ↩³
number of relevant changes↩↩² ↩³

lcnr · 2022-07-25T18:13:40Z

so, the major issue is that the FnPtr impls are added to https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/trait_def/struct.TraitImpls.html#structfield.blanket_impls instead of a new entry in non_blanket_impls. I think it's easiest to change FunctionSimplifiedType to not include the input count and add a hack which adds all impls with a Self: FnPtr bound to that map instead of the general one 🤔

i have to admit that i don't like that too much 😁

lopopolo · 2022-08-31T18:21:15Z

Looking forward to the MCP here. I am following along on this ticket because I'm interested in seeing the c_unwind feature be stabilized. Built in trait impls on the new function types are a blocker for that. Is there a way to have a smaller change land that would unblock stabilizing the c_unwind feature?

RalfJung · 2022-08-31T21:29:14Z

Adding only the c_unwind instances without also adding a dozen other ABIs like the last PR did, might work.

lopopolo · 2022-09-01T05:22:08Z

Adding only the c_unwind instances without also adding a dozen other ABIs like the last PR did, might work.

@RalfJung I've put up a PR for this at #101263.

…pls, r=thomcc Add default trait implementations for "c-unwind" ABI function pointers Following up on rust-lang#92964, only add default trait implementations for the `c-unwind` family of function pointers. The previous attempt in rust-lang#92964 added trait implementations for many more ABIs and ran into concerns regarding the increase in size of the libcore rlib. An attempt to abstract away function pointer types behind a unified trait to reduce the duplication of trait impls is being discussed in rust-lang#99531 but this change looks to be blocked on a lang MCP. Following `@RalfJung's` suggestion in rust-lang#99531 (comment), this commit is another cut at rust-lang#92964 but it _only_ adds the impls for `extern "C-unwind" fn` and `unsafe extern "C-unwind" fn`. I am interested in landing this patch to unblock the stabilization of the `c_unwind` feature. RFC: rust-lang/rfcs#2945 Tracking Issue: rust-lang#74990

jackh726 · 2022-11-30T13:05:28Z

I think @rust-lang/lang should just chime in here. Likely we just need an FCP to land this under a feature. I think before landing it might be "important" enough to write a small RFC.

RalfJung · 2022-12-21T16:18:51Z

I'd definitely like to see this land, that would also unblock (or at least strongly simplify) a bunch of important work on the const-eval and pattern-match side (#105750 would be the start).

@lcnr is there an implementation plan that just needs someone to "do it", or are there still open conceptual questions? @eddyb mentioned a super elaborate plan but if nobody has the time to work on that, I'd rather not wait for that to materialize...

@jackh726 if the trait remains private/unstable, I am not sure that we need an RFC. But we'd definitely need T-lang FCP for the magic trait, yeah.

jackh726 · 2022-12-21T16:21:14Z

Yeah, I think I meant before stabilization we might want an RFC. For unstable, we can land with just FCP.

lcnr · 2022-12-22T08:43:46Z

@lcnr is there an implementation plan that just needs someone to "do it", or are there still open conceptual questions? @eddyb mentioned a super elaborate plan but if nobody has the time to work on that, I'd rather not wait for that to materialize...

for me the only issue was perf (and lang approval ofc 😁). have an idea how to fix that which is:

for https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/trait_def/struct.TraitImpls.html add a third field: fn_ptr_impls: Vec<DefId> which includes all impls which have a generic param T as the self type with a T: FnPtr bound
in the methods accessing TraitImpls, also iterate over these impls if they could apply. Only iterate over them if the self_ty is either an function pointer, a generic param with a T: FnPtr bound in the param_env, or something without a simplified_type (e.g. an inference var)

this approach has a similar core idea to what i've described in https://rust-lang.zulipchat.com/#narrow/stream/247081-t-compiler.2Fperformance/topic/Compile-time.20case-study.3A.20AWS.20crates/near/315550450

I myself don't have the capacity to implement myself but could (loosely) mentor.

nikomatsakis · 2022-12-24T12:34:37Z

@lcnr and I discussed this at some point. I'm in favor of having this special trait. I prefer to fix the perf solution by having the compiler be aware of T: FnPtr where-clauses that are in scope and which are required, because this solution would scale to supporting other similar traits like Tuple. I think this is what @lcnr was describing in their most recent comment, as well.

@RalfJung

Following up on #92964, only add default trait implementations for the `c-unwind` family of function pointers. The previous attempt in #92964 added trait implementations for many more ABIs and ran into concerns regarding the increase in size of the libcore rlib. An attempt to abstract away function pointer types behind a unified trait to reduce the duplication of trait impls is being discussed in #99531 but this change looks to be blocked on a lang MCP. Following @RalfJung's suggestion in rust-lang/rust#99531 (comment), this commit is another cut at #92964 but it _only_ adds the impls for `extern "C-unwind" fn` and `unsafe extern "C-unwind" fn`. I am interested in landing this patch to unblock the stabilization of the `c_unwind` feature. RFC: rust-lang/rfcs#2945 Tracking Issue: rust-lang/rust#74990

…omcc Add default trait implementations for "c-unwind" ABI function pointers Following up on #92964, only add default trait implementations for the `c-unwind` family of function pointers. The previous attempt in #92964 added trait implementations for many more ABIs and ran into concerns regarding the increase in size of the libcore rlib. An attempt to abstract away function pointer types behind a unified trait to reduce the duplication of trait impls is being discussed in #99531 but this change looks to be blocked on a lang MCP. Following `@RalfJung's` suggestion in rust-lang/rust#99531 (comment), this commit is another cut at #92964 but it _only_ adds the impls for `extern "C-unwind" fn` and `unsafe extern "C-unwind" fn`. I am interested in landing this patch to unblock the stabilization of the `c_unwind` feature. RFC: rust-lang/rfcs#2945 Tracking Issue: rust-lang/rust#74990

jackh726 · 2023-02-18T22:50:12Z

Closing in favor of #108080

Add a builtin `FnPtr` trait that is implemented for all function pointers r? `@ghost` Rebased version of rust-lang#99531 (plus adjustments mentioned in the PR). If perf is happy with this version, I would like to land it, even if the diagnostics fix in 9df8e1befb5031a5bf9d8dfe25170620642d3c59 only works for `FnPtr` specifically, and does not generally improve blanket impls.

Add a builtin `FnPtr` trait that is implemented for all function pointers r? `@ghost` Rebased version of rust-lang/rust#99531 (plus adjustments mentioned in the PR). If perf is happy with this version, I would like to land it, even if the diagnostics fix in 9df8e1befb5031a5bf9d8dfe25170620642d3c59 only works for `FnPtr` specifically, and does not generally improve blanket impls.

lcnr added T-libs-api [DEPRECATED; DO NOT USE] needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. labels Jul 20, 2022

rust-highfive assigned jackh726 Jul 20, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2022

RalfJung reviewed Jul 20, 2022
View reviewed changes

This comment has been minimized.
Sign in to view

lcnr commented Jul 21, 2022
View reviewed changes

lopopolo mentioned this pull request Jul 24, 2022
Implement mrb_raise with Rust panic!artichoke/artichoke#1904
Open

lcnr added 4 commits July 25, 2022 15:53

cool new attribute
ed4dd0c

add a builtin FnPtr trait
04accf1

use the FnPtr trait
0cdb959

update tests
9d0a8e5

lcnr force-pushed the FnPtr-trait branch from 68dea3a to 9d0a8e5Compare July 25, 2022 14:10

lcnr mentioned this pull request Jul 25, 2022
[experiment] fulfill diagnostics: do not look into T: IntoIterator#99719
Closed

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 25, 2022

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 25, 2022

lopopolo mentioned this pull request Sep 1, 2022
Add default trait implementations for "c-unwind" ABI function pointers #101263
Merged

lopopolo mentioned this pull request Sep 10, 2022
Implement standard traits on C-unwind ABI function pointers rust-lang/libs-team#100
Closed

jackh726 added T-lang Relevant to the language team and removed T-libs-api [DEPRECATED; DO NOT USE] labels Nov 30, 2022

jackh726 added S-waiting-on-team and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Nov 30, 2022

RalfJung mentioned this pull request Dec 21, 2022
Always fall back to PartialEq when a constant in a pattern is not recursively structural-eq #105750
Merged

This was referenced Feb 15, 2023
Implement PartialEq for all function pointers via a shim #108074
Closed
Add a builtin FnPtr trait that is implemented for all function pointers #108080
Merged

jackh726 closed this Feb 18, 2023

eddyb mentioned this pull request Apr 3, 2023
FnPtr blanket impls cause "multiple applicable items in scope" for unrelated types. #109892
Closed

	if candidates.len() == 1{
	returnself.filter_reservation_impls(candidates.pop().unwrap(), stack.obligation);
	}

Uh oh!

Uh oh!

Conversation

lcnr commented Jul 20, 2022

Uh oh!

rustbot commented Jul 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Jul 20, 2022

Uh oh!

This comment has been minimized.

eddyb commented Jul 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Jul 21, 2022

Uh oh!

lcnr commented Jul 25, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-timer commented Jul 25, 2022

Uh oh!

bors commented Jul 25, 2022

Uh oh!

rust-timer commented Jul 25, 2022

Uh oh!

bors commented Jul 25, 2022

Uh oh!

rust-timer commented Jul 25, 2022

Uh oh!

rust-timer commented Jul 25, 2022

Footnotes

Uh oh!

lcnr commented Jul 25, 2022

Uh oh!

lopopolo commented Aug 31, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung commented Aug 31, 2022

Uh oh!

lopopolo commented Sep 1, 2022

Uh oh!

jackh726 commented Nov 30, 2022

Uh oh!

RalfJung commented Dec 21, 2022

Uh oh!

jackh726 commented Dec 21, 2022

Uh oh!

lcnr commented Dec 22, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikomatsakis commented Dec 24, 2022

Uh oh!

jackh726 commented Feb 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

lcnr commented Jul 25, 2022•
edited
Loading

lopopolo commented Aug 31, 2022•
edited
Loading

lcnr commented Dec 22, 2022•
edited
Loading