Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New AsyncFn* traits being #[fundamental] might come with significant forward-compatibility issues #136723

Closed
steffahn opened this issue Feb 8, 2025 · 10 comments
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. F-async_closure `#![feature(async_closure)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@steffahn
Copy link
Member

steffahn commented Feb 8, 2025

As far as I understand the current design of AsyncFn* traits allows them to become somewhat of a “sugar” for a more generalized approach in the future. Quoting from the RFC:

Changing the underlying definition to use LendingFn*

As mentioned above, async Fn*() trait bounds can be adjusted to desugar to LendingFn* + FnOnce trait bounds, using associated-type-bounds like:

where F: async Fn() -> i32

// desugars to

where F: for<'s> LendingFn<LendingOutput<'s>: Future<Output = i32>> + FnOnce<Output: Future<Output = i32>>

This should be doable in a way that does not affect existing code, but remain blocked on improvements to higher-ranked trait bounds around GATs. Any changes along these lines remain implementation details unless we decide separately to stabilize more user-observable aspects of the AsyncFn* trait, which is not likely to happen soon.

In particular, the case for AsyncFnOnce seems pretty clear. It’s a future possibility (which is IMO very desirable) that AsyncFnOnce(Args…) -> R might be automatically implemented for any implementor of FnOnce(Args…) -> F that returns some F: Future<Output = R>; either by turning it into a sort of “alias” (in a way that avoids the shortcomings mentioned in the RFC) or by finding a way to re-structure the blanket impls.

In particular, IMO it’s a very relevant future possibility that Box<dyn FnOnce(Args…) -> Pin<Box<dyn Future<Output = R> [+ Send] [+Sync]>> could become an implementor of AsyncFnOnce(Args…) -> R in the future.

But #[fundamental] kills this possibility, as the following code compiles successfully, powered by the negative reasoning that #[fundamental] provides:

use std::pin::Pin;
use std::future::Future;

type BoxedFnOnceReturningBoxFuture = Box<dyn FnOnce() -> Pin<Box<dyn Future<Output = ()>>>>;

trait MyTraitNoOverlap {}
impl MyTraitNoOverlap for BoxedFnOnceReturningBoxFuture {}
impl<F: AsyncFnOnce()> MyTraitNoOverlap for F {}

(playground)


I was unable to find any prior discussion about the value of why these traits (AsyncFn, AsyncFnMut, AsyncFnOnce) are marked #[fundamental] in the first place. I can imagine it’s perhaps by analogy to Fn, FnMut, FnOnce being marked as such. But this decision is an important trade-off that should be considered.

As far as I can tell, there should be no issue in simply removing the #[fundamental] markers from AsyncFn, AsyncFnMut, AsyncFnOnce before they’re stabilized in 1.85. Testing this locally, std still compiles find, and UI tests pass.

It should always be backwards-compatible to add back the #[fundamental] marker later.

cc @compiler-errors

@rustbot label T-compiler, T-libs-api, F-async_closure, C-discussion

@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. F-async_closure `#![feature(async_closure)]` I-prioritize Issue: Indicates that prioritization has been requested for this issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Feb 8, 2025
@compiler-errors
Copy link
Member

Yeah there's no reason for them to need to be fundamental; it's not like we need it for coherence in core today. Probably just an oversight from when I initially mirrored the Fn* traits. I'll remove it.

@compiler-errors
Copy link
Member

There's no reason to prioritize this issue tho.

@rustbot label -I-prioritize

@rustbot rustbot removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Feb 8, 2025
@steffahn
Copy link
Member Author

steffahn commented Feb 8, 2025

There's no reason to prioritize this issue tho.

That’s alright if there’s no need for it. I thought a future-compatibility issue on beta is a bit like a regression on beta 🤷🏻

@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 8, 2025
@cuviper
Copy link
Member

cuviper commented Feb 8, 2025

It should always be backwards-compatible to add back the #[fundamental] marker later.

Is it? At least for types, adding #[fundamental] can definitely break downstream code, like when we discussed tuples:
https://rust-lang.zulipchat.com/#narrow/channel/144729-t-types/topic/.5BCoherence.5D.20Fundamental.20tuples

... but I don't know if there's a similar risk for traits.

@compiler-errors
Copy link
Member

compiler-errors commented Feb 8, 2025

For traits it's backwards-compatible to add fundamental, since it means something completely different. For types, it means that we ignore the outermost "type constructor" for the orphan check (e.g. we allow you to impl non-local traits for Box<MyLocalType>) -- adding that would open up the type for more overlap from downstream crates. For traits, it means that we assume the trait will have no new implementations added for existing types -- adding it is essentially "sealing" the trait at that point.

@cuviper
Copy link
Member

cuviper commented Feb 8, 2025

Does that reasoning hold for the type dyn Trait as well?
(but I see AsyncFn* aren't dyn-compatible anyway)

@compiler-errors
Copy link
Member

compiler-errors commented Feb 8, 2025

Only ADTs (struct/enum/union) can be fundamental (for types).

@theemathas
Copy link
Contributor

theemathas commented Feb 8, 2025

If it were possible for users to define their own types in stable rust that implements the AsyncFn trait, then it would be hypothetically technically possible that rust later changing the AsyncFn trait to be fundamental could cause previously semver-compatible versions of user crates to become semver-incompatible. I don't know if this counts as a breaking change in rust.

For example, consider the following scenario (assuming that users can implement AsyncFn themselves):

  • Rust stabilizes a non-fundamental AsyncFn trait.
  • Crate foo version 1.0.0 is published, containing a type named Thing that doesn't do much.
  • Crate foo version 1.0.1 is published. It adds an impl AsyncFn for Thing. This is allowed because AsyncFn is not fundamental at this point.
  • Rust changes the AsyncFn trait to be fundamental.
  • Crate bar is published. It depends on crate foo version ^1.0.0, and was tested against foo version 1.0.0. This crate bar unintentionally depends on the fact that Thing doesn't implement AsyncFn. The compiler allows this because AsyncFn is now fundamental.
  • A user uses crate foo version 1.0.1 alongside crate bar. Things break, and the user complains.

I can't figure out a way for this scenario to happen with the current feature set of stable rust though...

@steffahn
Copy link
Member Author

steffahn commented Feb 8, 2025

Of course, the issues of meta-semver-breakage… those are always easy to miss.

I guess, it’s a good thing you can’t (stably) implement AsyncFn* traits?

workingjubilee added a commit to workingjubilee/rustc that referenced this issue Feb 8, 2025
…r=compiler-errors

Make `AsyncFnOnce`, `AsyncFnMut`, `AsyncFn` non-`#[fundamental]`

Address the issue rust-lang#136723 on nightly (the issue will only *actually* be fixed with a beta backport).
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Feb 9, 2025
…r=compiler-errors

Make `AsyncFnOnce`, `AsyncFnMut`, `AsyncFn` non-`#[fundamental]`

Address the issue rust-lang#136723 on nightly (the issue will only *actually* be fixed with a beta backport).
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Feb 9, 2025
…r=compiler-errors

Make `AsyncFnOnce`, `AsyncFnMut`, `AsyncFn` non-`#[fundamental]`

Address the issue rust-lang#136723 on nightly (the issue will only *actually* be fixed with a beta backport).
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 9, 2025
Rollup merge of rust-lang#136724 - steffahn:asyncfn-non-fundamental, r=compiler-errors

Make `AsyncFnOnce`, `AsyncFnMut`, `AsyncFn` non-`#[fundamental]`

Address the issue rust-lang#136723 on nightly (the issue will only *actually* be fixed with a beta backport).
@steffahn
Copy link
Member Author

steffahn commented Feb 14, 2025

backport landed today 🚀 #136980

@rustbot rustbot added C-discussion Category: Discussion or questions that doesn't represent real issues. I-prioritize Issue: Indicates that prioritization has been requested for this issue. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. F-async_closure `#![feature(async_closure)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants