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

Unexpected build pass with a not Send future #108897

Closed
LYF1999 opened this issue Mar 8, 2023 · 8 comments · Fixed by #108901
Closed

Unexpected build pass with a not Send future #108897

LYF1999 opened this issue Mar 8, 2023 · 8 comments · Fixed by #108901
Assignees
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@LYF1999
Copy link
Contributor

LYF1999 commented Mar 8, 2023

I tried this code:

use std::{future::Future, pin::Pin};

trait Handler<R> {
    type Resp;
    type Future: Future<Output = Self::Resp> + Send + 'static;
    fn call(self) -> Self::Future;
}

impl<R, F, Res, Fut> Handler<R> for F
where
    Res: 'static,
    Fut: Future<Output = Res> + Send,
    F: FnOnce() -> Fut + Send + Clone + 'static,
{
    type Resp = Res;

    type Future = Pin<Box<dyn Future<Output = Self::Resp> + Send>>;

    fn call(self) -> Self::Future {
        Box::pin(async move {
            let res = self().await;
            res
        })
    }
}

fn require_handler<H: Handler<()>>(h: H) {}

async fn handler() -> () {
    let a = &1 as *const i32;
    async {}.await;
}

fn main() {
    require_handler(handler)
}

I expected to see this happen:
build failed, because this future doesn't implement Send, so this function shouldn't implement Handler

error: future cannot be sent between threads safely
  --> src/main.rs:35:21
   |
35 |     require_handler(handler)
   |                     ^^^^^^^ future returned by `handler` is not `Send`
   |
   = help: within `impl Future<Output = ()>`, the trait `Send` is not implemented for `*const i32`
note: future is not `Send` as this value is used across an await
  --> src/main.rs:31:13
   |
30 |     let a = &1 as *const i32;
   |         - has type `*const i32` which is not `Send`
31 |     async {}.await;
   |             ^^^^^^ await occurs here, with `a` maybe used later
32 | }
   | - `a` is later dropped here
note: required by a bound in `require_handler`
  --> src/main.rs:27:23
   |
27 | fn require_handler<H: Handler<()>>(h: H) {}
   |                       ^^^^^^^^^^^ required by this bound in `require_handler`

Instead, this happened:
build pass

Meta

rustc --version --verbose:

rustc 1.70.0-nightly (e3dfeeaa4 2023-03-07)
binary: rustc
commit-hash: e3dfeeaa45f117281b19773d67f3f253de65cee1
commit-date: 2023-03-07
host: aarch64-apple-darwin
release: 1.70.0-nightly
LLVM version: 15.0.7
Backtrace

<backtrace>

cargo-bisect-rustc show the Regression in 9bb6e60

this bug is introduced by this PR #103695.

@LYF1999 LYF1999 added the C-bug Category: This is a bug. label Mar 8, 2023
@LYF1999
Copy link
Contributor Author

LYF1999 commented Mar 8, 2023

@rustbot label: +T-types

@rustbot rustbot added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Mar 8, 2023
@LYF1999
Copy link
Contributor Author

LYF1999 commented Mar 8, 2023

I think it's a very serious bug, should we revert this change? @lcnr

@LYF1999
Copy link
Contributor Author

LYF1999 commented Mar 8, 2023

@rustbot claim

@lcnr lcnr added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness labels Mar 8, 2023
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 8, 2023
@lcnr
Copy link
Contributor

lcnr commented Mar 8, 2023

minimized

trait Handler {}
impl<F, Fut> Handler for F
where
    Fut: Send,
    F: FnOnce() -> Fut,
{}

fn require_handler<H: Handler>(h: H) {}

async fn handler() {
    let a = &1 as *const i32;
    async {}.await;
}

fn main() {
    require_handler(handler)
}

ah, figured out the issue https://github.com/rust-lang/rust/pull/103695/files#r1129308806

how did you catch this bug?

@LYF1999
Copy link
Contributor Author

LYF1999 commented Mar 8, 2023

I try to find the minimized example of #108847.
there is a ICE with a unsend future in this issue

@apiraino
Copy link
Contributor

apiraino commented Mar 8, 2023

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-critical

@rustbot rustbot added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 8, 2023
@compiler-errors
Copy link
Member

Isn't this a stable-to-beta regression since nightly just branched?

@lcnr lcnr added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Mar 8, 2023
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Mar 8, 2023
fix: evaluate with wrong obligation stack

fix rust-lang#108897
r? `@lcnr`
@wesleywiser
Copy link
Member

Just to clarify since we're in the confusing week where stable & beta are discontinuous version numbers:

Version Has this bug
1.67 (current stable) No
1.68 (stable release on Thursday) No
1.69 (beta) Yes
1.70 (nightly) Yes

@bors bors closed this as completed in 33c3036 Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants