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

warn(must_not_suspend) started being raised incorrectly when moving from stable to nightly #90459

Closed
finnbear opened this issue Oct 31, 2021 · 6 comments
Labels
C-bug Category: This is a bug. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta.
Milestone

Comments

@finnbear
Copy link
Contributor

finnbear commented Oct 31, 2021

Code

I tried this code:

use std::cell::RefCell;

fn main() {
    
}

async fn bug(ref_cell: &RefCell<Option<bool>>) {
    let borrow = ref_cell.borrow();
    if let Some(_) = borrow.as_ref() {
        drop(borrow);
        
        async {
            
        }.await;
    }
}

Note: The if let Some(_) = borrow.as_ref() { } block is not necessary to reproduce (only its contents are needed), but this is how my actual code is structured, so I want to ensure it is considered

Playground link: here

I expected to see this compile without errors.

Instead, this happened:

warning: `Ref` held across a suspend point, but should not be
  --> src/main.rs:9:9
   |
9  |       let borrow = ref_cell.borrow();
   |           ^^^^^^
...
13 | /         async {
14 | |             
15 | |         }.await;
   | |_______________- the value is held across this suspend point
   |
   = note: `#[warn(must_not_suspend)]` on by default
note: holding a Ref across suspend points can cause BorrowErrors
  --> src/main.rs:9:9
   |
9  |     let borrow = ref_cell.borrow();
   |         ^^^^^^
help: consider using a block (`{ ... }`) to shrink the value's scope, ending before the suspend point
  --> src/main.rs:9:9
   |
9  |     let borrow = ref_cell.borrow();
   |         ^^^^^^

Version it worked on

It most recently worked on: 1.56.0 stable

Version with regression

rustc --version --verbose:

1.58.0-nightly 2021-10-29 e99963c554e4d12010c2

@rustbot modify labels: +regression-from-stable-to-nightly -regression-untriaged

@finnbear finnbear added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Oct 31, 2021
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-untriaged Untriaged performance or correctness regression. labels Oct 31, 2021
@apiraino
Copy link
Contributor

apiraino commented Nov 1, 2021

Bisection seems to point to commit 598d89b

searched nightlies: from nightly-2021-01-01 to nightly-2021-10-29
regressed nightly: nightly-2021-10-02
searched commits: from aa7aca3 to c02371c
regressed commit: 598d89b

bisected with cargo-bisect-rustc v0.6.0

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start=2021-01-01 --end=2021-10-29 --script=./script.sh 

@Mark-Simulacrum Mark-Simulacrum added this to the 1.58.0 milestone Nov 1, 2021
@Mark-Simulacrum Mark-Simulacrum 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 Nov 1, 2021
@Mark-Simulacrum
Copy link
Member

Regression is actually in 1.57 based on that bisection, but we're planning to switch the lint to allow by default for now regardless.

@camelid
Copy link
Member

camelid commented Nov 1, 2021

This issue is marked with the 1.58.0 milestone, but beta is currently 1.57.0, so should the milestone be updated?

@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.58.0, 1.57.0 Nov 1, 2021
@Mark-Simulacrum
Copy link
Member

Yeah, I think I misclicked.

@camelid camelid added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Nov 1, 2021
@camelid
Copy link
Member

camelid commented Nov 1, 2021

#89826 has landed on master now (not beta yet).

@pnkfelix
Copy link
Member

PR #89826 is in beta now. Closing as fixed.

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. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta.
Projects
None yet
Development

No branches or pull requests

6 participants