rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.24k stars 12.45k forks source link

1.79 regression in `const` temporary lifetime extension #126562

Closed CAD97 closed 1 week ago

CAD97 commented 2 months ago

Code

I tried this code: [playground] [godbolt]

pub struct Dir<'a>(&'a [File<'a>]);
impl<'a> Dir<'a> {
    pub const fn new(x: &'a [File<'a>]) -> Self {
        Self(x)
    }
}

pub struct File<'a>(&'a str);
impl<'a> File<'a> {
    pub const fn new(x: &'a str) -> Self {
        Self(x)
    }
}

pub static OK: Dir<'static> = Dir::new(&[File::new("")]);
pub static ERR: Dir<'static> = if true {
    Dir::new(&[File::new("")]) //~ ERROR: temporary value dropped while borrowed
} else {
    unreachable!()
};

I expected to see this happen: code compiles: temporary is promoted to &'static due to the const evaluation context.

Instead, this happened: code errors: temporary value dropped while borrowed.

Version it worked on

It most recently worked on: 1.78

Version with regression

rustc --version --verbose:

rustc 1.79.0 (129f3b996 2024-06-10)
binary: rustc
commit-hash: 129f3b9964af4d4a709d1383930ade12dfe7c081
commit-date: 2024-06-10
host: x86_64-pc-windows-msvc
release: 1.79.0
LLVM version: 18.1.7

Meta

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

Bisects to #121557

Initially reported on urlo: https://users.rust-lang.org/t/code-compiles-in-1-78-0-but-not-in-1-79-0/113038?u=cad97

CAD97 commented 2 months ago

@rustbot claim

I think I found what needs to be fixed. Just claiming the fix; I don't know how handling the regression should go.

RalfJung commented 2 months ago

We recently changed our rules for promotion of function calls, quite deliberately: https://github.com/rust-lang/rust/pull/121557. This seems to me to be expected fall-out from that change?

RalfJung commented 2 months ago

Cc @rust-lang/wg-const-eval

CAD97 commented 2 months ago

That does look to be more directly relevant than #121346. The initial report can't fix itself with const blocks though, unfortunately, since Dir::new(&[File::new()]) all shows up from a macro expansion.

@rustbot release-assignment

The thread I was pulling on didn't go anywhere and I don't know enough to determine much more on my own, plus may be intentional

RalfJung commented 2 months ago

Dir::new(&[File::new("")]) is exactly the kind of code we'd like to not compile since it moves a function call (File::new) into a promoted. However for backwards compatibility reasons, we currently still let it compile. However, once that kind of function call is inside a conditional, having it implicitly promoted becomes really sketchy, and crater showed that this pattern is very rare in the wild (crater found zero regressions). So we moved to rejecting that code, finally making tangible progress on https://github.com/rust-lang/rust/issues/80619.

The intended way to write such code is Dir::new(const { &[File::new("")] }).

CAD97 commented 2 months ago

So that it's mentioned directly here: the urlo source looks like

pub static ASSETS_DIR: Option<include_dir::Dir<'_>> = if cfg!(target_arch = "wasm32") {
    Some(include_dir::include_dir!("$CARGO_MANIFEST_DIR/assets"))
} else {
    None
};

and cannot introduce a const block to make it compile, as the correct place to introduce it is inside of the include_dir! macro. (Although in fairness OOP likely wants cfg_match! rather than if cfg! anyway. Note that include_dir! expands to Dir::new(&[File::new("file path", include_bytes!("file path")), ...]), it's not a source include.)

RalfJung commented 2 months ago

The const block should be inside that included file then. Basically, when you want &expr to be promoted to 'static lifetime, write it const { &expr }.

lqd commented 2 months ago

We recently changed our rules for promotion of function calls, quite deliberately: #121557.

(The OP does bisect to #121557)

apiraino commented 1 month ago

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

RalfJung commented 1 month ago

This is now fixed upstream in include_dir, see https://github.com/Michael-F-Bryan/include_dir/issues/99.

CAD97 commented 1 week ago

Closing as deliberate and expected breakage.