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

Return value from const_trait_impl did not promote to constant? #69574

Open
crlf0710 opened this issue Feb 29, 2020 · 8 comments
Open

Return value from const_trait_impl did not promote to constant? #69574

crlf0710 opened this issue Feb 29, 2020 · 8 comments
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) A-const-prop Area: Constant propagation A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@crlf0710
Copy link
Member

I tried this code:

#![feature(const_trait_impl)]
#![feature(const_fn)]
use std::marker::PhantomData;

trait ConstDefault {
    fn const_default() -> Self;
}

impl const ConstDefault for u8 {
    fn const_default() -> Self { 0 }
}

const fn foo() -> &'static u8 {
    &u8::const_default()
}

I expected to see this happen: This should pass compile if i'm not mistaken.

Instead, this happened: The const value promotion did not happen.

cc @ecstatic-morse @oli-obk

@crlf0710 crlf0710 added the C-bug Category: This is a bug. label Feb 29, 2020
@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Feb 29, 2020

The result of a const fn call does not get promoted. See rust-lang/const-eval#19 for background.

@crlf0710
Copy link
Member Author

@ecstatic-morse
Thank you for the pointer! Yes after reading that, it seems reasonable to prohibit this promotion here.
I believe for the above case, it would be nice to have a separate error code and explain this to the user. Also instruction user to add a constant item there to workaround this limitation, e.g.

 const V: u8 = u8::const_default();
 &V

@estebank

@ecstatic-morse
Copy link
Contributor

@crlf0710 I totally agree regarding the diagnostics. I think it will be a bit tricky to implement however. Perhaps it is better to have a dedicated issue for this? I only found #66541 which seems to have arrived at a similar conclusion.

@crlf0710
Copy link
Member Author

Sure, feel free to edit the text of this issue or open another issue etc, just whatever is helpful to the teams and work groups. (i don't really know what works the best.) It's not urgent at all since the workaround is so easy after the rule is learnt (just explicitly write an constant item, instead of expecting automatic promotion).

@oli-obk
Copy link
Contributor

oli-obk commented Feb 29, 2020

We'd have to register code that can be promoted via an explicit constant somewhere where borrowck can access it again. All the ways that I can think of would make this either resource consuming in the happy path or very convoluted in the error reporting path.

While I do agree it would be nice to have a better diagnostic, that seems to be little gain for a lot of effort for now.

But going back to the original issue. This should indeed compile I think because you are promoting from inside a const fn where all the const rules need to apply anyway. I though we were promoting anything const inside const fn. Or are we just doing that inside constants?

@ecstatic-morse
Copy link
Contributor

@oli-obk Just inside constants, since they always execute at compile-time.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 13, 2020

Ok, so basically, when a lifetime error occurs, we should rerun the promotion check with const rules enabled, and if they succeed, suggest to wrap it in a const block.

@fmease fmease added A-diagnostics Area: Messages for errors, warnings, and lints A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-const-prop Area: Constant propagation and removed C-bug Category: This is a bug. needs-triage-legacy labels Sep 6, 2023
@estebank
Copy link
Contributor

The above comment is still needed and likely the best course of action.

Current output:

error[E0515]: cannot return reference to temporary value
  --> src/lib.rs:13:5
   |
13 |     &u8::const_default()
   |     ^-------------------
   |     ||
   |     |temporary value created here
   |     returns a reference to data owned by the current function

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, covers all const contexts (static, const fn, ...) A-const-prop Area: Constant propagation A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. 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

6 participants