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

Regression 1.66 "some_fn::{opaque#0}<'_> does not live long enough" #107426

Closed
Dav1dde opened this issue Jan 28, 2023 · 14 comments · Fixed by #108691
Closed

Regression 1.66 "some_fn::{opaque#0}<'_> does not live long enough" #107426

Dav1dde opened this issue Jan 28, 2023 · 14 comments · Fixed by #108691
Labels
A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Dav1dde
Copy link
Contributor

Dav1dde commented Jan 28, 2023

Code

Unfortunately I couldn't minimize it further without using a dependency (code can be executed online here):

/*
[dependencies]
sycamore = "0.8.2"
web-sys = "0.3"

*/

use sycamore::prelude::{Scope, View, Html, DomNode, ReadSignal, GenericNode};

pub fn create_toggle_bool<'a>(cx: Scope<'a>, initial: bool) -> (&'a ReadSignal<bool>, impl Fn() + Copy + 'a) {
    // let open = sycamore::prelude::create_signal(cx, false);
    // let open_toggle = || open.set(!*open.get());
    // (open, open_toggle)
    (todo!(), || {})
}

pub fn Foo<G: Html>(cx: Scope) -> View<G> {
    let (open, open_toggle) = create_toggle_bool(cx, false);

    // Uncomment these 2 lines to make it compile (moved out of the function)
    // let open = sycamore::prelude::create_signal(cx, false);
    // let open_toggle = || open.set(!*open.get());

    let a = move |_: web_sys::Event| open_toggle();

    move || {
        let __el = DomNode::text_node("test");
        ::sycamore::generic_node::GenericNode::event(&__el, cx, "click", a);
        __el;
    };

    todo!()
}

fn main() {
}

The code compiles and runs with 1.66 but fails with 1.67:

error: `create_toggle_bool::{opaque#0}<'_>` does not live long enough
  --> src/main.rs:28:9
   |
28 |         ::sycamore::generic_node::GenericNode::event(&__el, cx, "click", a);
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I expect this code to still compile on 1.67

Version it worked on

1.66

Version with regression

1.67

rustc 1.67.0 (fc594f156 2023-01-24)
binary: rustc
commit-hash: fc594f15669680fa70d255faec3ca3fb507c3405
commit-date: 2023-01-24
host: x86_64-unknown-linux-gnu
release: 1.67.0
LLVM version: 15.0.6
@Dav1dde Dav1dde added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Jan 28, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jan 28, 2023
@compiler-errors compiler-errors added E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example labels Jan 28, 2023
@Teapot4195
Copy link
Contributor

Teapot4195 commented Jan 28, 2023

searched nightlies: from nightly-2022-09-15 to nightly-2022-10-15
regressed nightly: nightly-2022-09-26
searched commit range: 3f83906...f5193a9
regressed commit: f5193a9

bisected with cargo-bisect-rustc v0.6.5

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

cargo bisect-rustc --end=2022-10-15 

@rustbot label -E-needs-bisection

@rustbot rustbot removed the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Jan 28, 2023
@clubby789
Copy link
Contributor

Minimized to:

use std::marker::PhantomData;
#[derive(Clone, Copy)]
pub struct Scope<'a>(&'a PhantomData<&'a mut &'a ()>);
fn event<'a, F: FnMut() + 'a>(_: Scope<'a>, _: F) {}
fn make_fn<'a>(_: Scope<'a>) -> impl Fn() + Copy + 'a {
    || {}
}

fn foo(cx: Scope) {
    let open_toggle = make_fn(cx);

    || event(cx, open_toggle);
}

fn main() {}

@rustbot label -E-needs-mcve

@rustbot rustbot removed the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Jan 28, 2023
@clubby789
Copy link
Contributor

clubby789 commented Jan 28, 2023

This is a breaking change but it is a necessary soundness fix

#95474

@Noratrieb
Copy link
Member

Since the crater run there found no regressions I'm gonna @oli-obk anyways but I don't think there's anything to do here.

@oli-obk oli-obk added A-diagnostics Area: Messages for errors, warnings, and lints A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. and removed C-bug Category: This is a bug. I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-untriaged Untriaged performance or correctness regression. labels Jan 29, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Jan 29, 2023

Yea, this is expected breakage. We should improve the diagnostic though.

@Dav1dde
Copy link
Contributor Author

Dav1dde commented Jan 29, 2023

Is the reduced example unsound?

I would expect let open_toggle = make_fn(cx); to behave the same as let open_toggle = || {};, is this assumption wrong?

@Noratrieb
Copy link
Member

The minimized example is not really unsound as in it doesn't contain UB - it doesn't do anything. But the feature that it uses is unsound. Such is the fate of a safe language, it often has "false positives".

@Dav1dde
Copy link
Contributor Author

Dav1dde commented Jan 29, 2023

Thanks, but this is a bit advanced for me still, but if this is expected I can close the issue or do you want me to keep it open for better diagnostics?

@steffahn
Copy link
Member

On my machine the original post as well as the reduced example regress in 1.66, not 1.67. Can someone confirm or deny this? If that’s true, than the title here would be misleading.

@oli-obk oli-obk changed the title Regression 1.67 "some_fn::{opaque#0}<'_> does not live long enough" Regression 1.66 "some_fn::{opaque#0}<'_> does not live long enough" Jan 31, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Jan 31, 2023

Godbolt agrees, this regressed in 1.66

@steffahn
Copy link
Member

I only looked at this issue, because there’s been a thread on URLO on a similarly-looking issue that appears to actually regress on 1.67: #107516

Mentioning it here, because it might be of interest to readers of this thread :-)

@Dav1dde
Copy link
Contributor Author

Dav1dde commented Jan 31, 2023

This is weird, this only broke on my CI because it was pinned to stable (-> 1.67), my local machine was still compiling and running the code fine on 1.66. Maybe the reduced example already regressed in 1.66 but my corner case only regresses in 1.67?

@cjgillot cjgillot assigned cjgillot and unassigned cjgillot Jan 31, 2023
@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 24, 2023
@aliemjay
Copy link
Member

aliemjay commented Mar 3, 2023

This regression can indeed be mitigated (cc #108691), at least for the MCVE we have.
@rustbot label -A-diagonstics regression-from-stable-to-stable

@rustbot rustbot added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 3, 2023
@workingjubilee workingjubilee removed the A-diagnostics Area: Messages for errors, warnings, and lints label Mar 3, 2023
@bors bors closed this as completed in 8824994 Mar 7, 2023
@apiraino
Copy link
Contributor

apiraino commented Mar 7, 2023

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.