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

APIs don't just depend on function signatures #134818

Open
swooster opened this issue Dec 27, 2024 · 3 comments
Open

APIs don't just depend on function signatures #134818

swooster opened this issue Dec 27, 2024 · 3 comments
Labels
A-crate-compat Area: Impacting SemVer compatibility of crates in the ecosystem C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@swooster
Copy link
Contributor

Normally Rust has the really nice property that I can focus on getting each function to compile in isolation, without worries that
refactoring the body of a function will break backwards compatibility. However, I recently stumbled across a bizarre circumstance
where that isolation appears to be violated. After spending a day pruning it down to a small-ish reproducible example:

use std::future::Future;

// Note: The original code isn't actually object-oriented; the
// object-oriented framing is just a way to give the isolated example
// more intuitive/memorable names than `Foo`, `Bar`, `Baz`, etc...

type Object = Box<dyn std::any::Any + Send>;

fn get_object() -> Object {
    unimplemented!()
}

trait AsyncFrom<T>: Sized {
    fn async_from(src: T) -> impl Future<Output = Self>;
}

impl<T, U> AsyncFrom<U> for (T, U) {
    async fn async_from(_src: U) -> Self {
        unimplemented!()
    }
}

async fn downcast<T: AsyncFrom<Object>>() -> T {
    T::async_from(get_object()).await
}

async fn get_thing() -> u32 {
    // Oddly, inlining this call fixes everything.
    downcast::<(_, _)>().await.0
    //<(_, _)>::async_from(get_object()).await.0
}

pub async fn does_compile() -> u32 {
    get_thing().await
}

// Commenting out all code below this line causes everything to work.

// Oddly, complains about an implementation of AsyncFrom here,
// even though this does not directly use that at all.
pub fn does_not_compile() -> impl AsyncFnOnce {
    move || does_compile()
}

trait AsyncFnOnce: FnOnce() -> Self::Future {
    type Future: Future + Send;
}

impl<F, Fut> AsyncFnOnce for F
where
    F: FnOnce() -> Fut,
    Fut: Future + Send,
{
    type Future = Fut;
}

I find the way Rust handles this code to be problematic in a couple of ways:

First, the error message is nonsensical:

error: implementation of `AsyncFrom` is not general enough
  --> src/lib.rs:42:5
   |
42 |     move || does_compile()
   |     ^^^^^^^^^^^^^^^^^^^^^^ implementation of `AsyncFrom` is not general enough
   |
   = note: `(u32, Box<(dyn Any + Send + '0)>)` must implement `AsyncFrom<Box<(dyn Any + Send + '1)>>`, for any two lifetimes `'0` and `'1`...
   = note: ...but it actually implements `AsyncFrom<Box<dyn Any + Send>>`

This could be an acceptable error message if it were pointing to code in downcast, but it's pointing to code in does_not_compile which doesn't even (directly) use AsyncFrom. This really threw me for a loop, especially because I encountered it while working on a large codebase.

Second, I was surprised to discover that the body of get_thing influences whether the code compiles, not just the signature of get_thing. Inlining downcast into get_thing appears to resolve the compile error without changing code in does_not_compile. Alternatively, adjusting the body of does_not_compile to avoid calling does_compile fixes the compile error. So, the compile only fails due to the interaction between two different function bodies... which feels very un-Rusty to me.

After seeking help in the community discord, one 🐸, two 🐸, tree 🐸 explained that combining closures with existential types allows you to violate the usual Rust philosophy that only function signatures matter. I feel like this is extremely unfortunate, both because it destroys local reasoning and because it makes it extra hard to be sure you haven't broken backwards compatibility when merely refactoring a function body.

Per one 🐸, two 🐸, tree 🐸's suggestion, I'm writing this issue in the hopes that providing counterintuitive example code may help push for the current behavior to be changed. I'd really like it if I could worry about getting each function to compile one at a time, without having to consider the possibility that different function bodies may interact negatively.

Thank you!

Meta

(this behavior is also exhibited on beta and nightly versions of Rust)

rustc --version --verbose:

rustc 1.80.1 (3f5fd8dd4 2024-08-06)
binary: rustc
commit-hash: 3f5fd8dd41153bc5fdca9427e9e05be2c767ba23
commit-date: 2024-08-06
host: x86_64-unknown-linux-gnu
release: 1.80.1
LLVM version: 18.1.7

(the backtrace doesn't reveal anything new:)

Backtrace

  --> src/lib.rs:42:5
   |
42 |     move || does_compile()
   |     ^^^^^^^^^^^^^^^^^^^^^^ implementation of `AsyncFrom` is not general enough
   |
   = note: `(u32, Box<(dyn Any + Send + '0)>)` must implement `AsyncFrom<Box<(dyn Any + Send + '1)>>`, for any two lifetimes `'0` and `'1`...
   = note: ...but it actually implements `AsyncFrom<Box<dyn Any + Send>>`

@swooster swooster added the C-bug Category: This is a bug. label Dec 27, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Dec 27, 2024
@jieyouxu jieyouxu added A-crate-compat Area: Impacting SemVer compatibility of crates in the ecosystem T-lang Relevant to the language team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Dec 27, 2024
@compiler-errors
Copy link
Member

This is very much not a new issue. This is just auto trait leakage, and this isn't possible to change unfortunately.

@compiler-errors
Copy link
Member

Wait, 🤔, this might just be a solver bug. I'll look into it I guess.

@compiler-errors compiler-errors self-assigned this Dec 27, 2024
@compiler-errors
Copy link
Member

I think this is just #110338.

@compiler-errors compiler-errors removed their assignment Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-crate-compat Area: Impacting SemVer compatibility of crates in the ecosystem C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants