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

1.79 regression in const temporary lifetime extension #126562

Closed
CAD97 opened this issue Jun 16, 2024 · 11 comments
Closed

1.79 regression in const temporary lifetime extension #126562

CAD97 opened this issue Jun 16, 2024 · 11 comments
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-lifetimes Area: Lifetimes / regions C-bug Category: This is a bug. 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

@CAD97
Copy link
Contributor

CAD97 commented Jun 16, 2024

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 CAD97 added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Jun 16, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-untriaged Untriaged performance or correctness regression. labels Jun 16, 2024
@CAD97
Copy link
Contributor Author

CAD97 commented Jun 16, 2024

@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
Copy link
Member

We recently changed our rules for promotion of function calls, quite deliberately: #121557. This seems to me to be expected fall-out from that change?

@RalfJung
Copy link
Member

Cc @rust-lang/wg-const-eval

@CAD97
Copy link
Contributor Author

CAD97 commented Jun 16, 2024

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
Copy link
Member

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 #80619.

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

@CAD97
Copy link
Contributor Author

CAD97 commented Jun 16, 2024

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
Copy link
Member

RalfJung commented Jun 16, 2024

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
Copy link
Member

lqd commented Jun 16, 2024

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

(The OP does bisect to #121557)

@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-lifetimes Area: Lifetimes / regions A-const-eval Area: Constant evaluation (MIR interpretation) labels Jun 16, 2024
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jun 16, 2024
@apiraino
Copy link
Contributor

apiraino commented Jul 11, 2024

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 Jul 11, 2024
@RalfJung
Copy link
Member

This is now fixed upstream in include_dir, see Michael-F-Bryan/include_dir#99.

@CAD97
Copy link
Contributor Author

CAD97 commented Aug 16, 2024

Closing as deliberate and expected breakage.

@CAD97 CAD97 closed this as completed Aug 16, 2024
@CAD97 CAD97 reopened this Aug 16, 2024
@CAD97 CAD97 closed this as not planned Won't fix, can't repro, duplicate, stale Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-lifetimes Area: Lifetimes / regions C-bug Category: This is a bug. 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

No branches or pull requests

7 participants