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

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

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> Foo for X<T> where T: 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> Foo for T where T: 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.

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

@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

Awaiting bors try build completion.

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

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

Awaiting bors try build completion.

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

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

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

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 😁

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?

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

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.

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.

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.

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

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

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

Closing in favor of #108080