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

Const in trait impl block not evaluated unless used, skipping panics. #91877

Closed
XrXr opened this issue Dec 13, 2021 · 9 comments
Closed

Const in trait impl block not evaluated unless used, skipping panics. #91877

XrXr opened this issue Dec 13, 2021 · 9 comments
Labels
C-bug Category: This is a bug.

Comments

@XrXr
Copy link
Contributor

XrXr commented Dec 13, 2021

I found a surprising behavior of constant evaluation and I'm not sure if it's intended.
I feel this is a small usability issue with panics in const context for implementing
compile time assertions.

I tried this code(playground):

trait ConstPanic {
    const EVAL: ();
}

impl ConstPanic for i32 {
    // Does not seem to be evaluated unless used.
    // Program compiles, unexpectedly.
    const EVAL: () = panic!("oh no!");
}

fn main() {
    // Uncommenting the following gives the compile time panic
    // I expect to see. It also gives "warning: path statement with no effect"
    // In this case, it clearly has a compile time effect.
    //i32::EVAL;
}

I expected to see this happen: The program fails to compile

Instead, this happened: The program compiles and runs.

Meta

rustc --version --verbose:

rustc 1.57.0 (f1edd0429 2021-11-29)
binary: rustc
commit-hash: f1edd0429582dd29cccacaf50fd134b05593bd9c
commit-date: 2021-11-29
host: x86_64-apple-darwin
release: 1.57.0
LLVM version: 13.0.0
@XrXr XrXr added the C-bug Category: This is a bug. label Dec 13, 2021
@est31
Copy link
Member

est31 commented Dec 14, 2021

I wonder why the compiler evaluates unused consts at all?

const EVAL: () = panic!("oh no!"); // ERROR: evaluation of constant value failed

There is an inconsistency to consts in trait impl blocks, but I wonder if the inconsistency could be resolved the other way, by not evaluating any unused consts.

@XrXr
Copy link
Contributor Author

XrXr commented Dec 14, 2021

I wonder why the compiler evaluates unused consts at all?

I use it to do check things at compile time, as suggested by the 1.57.0 release note.

I think if "no use, no evaluation" becomes a general rule in the language, it hurts this use case. I want to have confidence that the compile time assert is evaluated when I write it so I can focus on the content of the assertion, not whether it is evaluated.

@Aaron1011
Copy link
Member

cc @RalfJung - I remember a similar case being discussed before

@RalfJung
Copy link
Member

RalfJung commented Dec 15, 2021

Cc @rust-lang/wg-const-eval

For const that are syntactically referenced inside a function we guarantee that if that function is codegen'd, they are all evaluated even if the only reference is in dead code. And for top-level const we also guarantee that, since it is easy to do and needed for things like size_assert!. But we don't have such a guarantee for anything else. And very quickly we reach territory where we cannot have such a guarantee:

impl<T> ConstPanic for Vec<T> {
    const EVAL: () = panic!("oh no!");
}

EVAL can only be computed once T is known (it could, in principle, use T).

So currently we stay on the consistent side that associated consts in traits are not eagerly evaluated. I am not sure if eagerly evaluating some associated consts in traits would be better, since it could be hard to predict when the rule does or does not apply. Eagerly evaluating all of them is simply not possible.

@est31
Copy link
Member

est31 commented Dec 15, 2021

needed for things like size_assert!

Which btw was a bad example: https://www.reddit.com/r/rust/comments/r799fs/announcing_rust_1570/hn05zpf/

But otherwise I think you make good points.

@XrXr
Copy link
Contributor Author

XrXr commented Dec 16, 2021

Assuming no docs for the current rules are already available, do you folks think it would make sense to add to the language reference? Might be worth mentioning in Constant Items and/or Constant Evaluation.

@RalfJung
Copy link
Member

Yes this would definitely be good to document properly if that is not already the case.

@RalfJung
Copy link
Member

RalfJung commented Dec 16, 2021

Also I skipped over associated consts in regular (non-trait) impl blocks -- but I think we treat them like associated consts in traits, since impl blocks can be generic, so the same problem applies.

So all top-level consts are always evaluated, but associated consts are only evaluated when they are used, where "used" means "referenced (possibly transitively through other consts) in a function that actually has its code generated, even if it is in dead code inside that function". Note that if the entire function is dead, then code might not be generated so its consts can again go unevaluated.

Also, errors in unused top-level consts are currently just a deny-by-default future-compat lint, but not (yet) a hard error. See #71800.

XrXr added a commit to XrXr/reference that referenced this issue Dec 24, 2021
Since const panics are now [stabilized], I figure it would be good to
document some of its gotchas. I couldn't really find documents for
the specifics I ran into. I am no const eval expert, and what I
wrote is basically a paraphrase of Ralf's comments[^1][^2],
but I hope to get the ball rolling for adding some docs.

I deliberately chose to not mention the guarantee about syntactically
referenced constants in functions that require code generation as it
seems hard to explain completely without some ambiguity. It also seems
to me that the rules are not completely stable[^3][^4] yet.

[stabilized]: rust-lang/rust#89006
[^1]: rust-lang/rust#91877 (comment)
[^2]: rust-lang/rust#91877 (comment)
[^3]: rust-lang/rust#71800
[^4]: rust-lang/rust#51999 (comment)
XrXr added a commit to XrXr/reference that referenced this issue Dec 24, 2021
Since const panics are now [stabilized], I figure it would be good to
document some of its gotchas. I couldn't really find documents for
the specifics I ran into. I am no const eval expert, and what I
wrote is basically a paraphrase of Ralf's comments[^1][^2],
but I hope to get the ball rolling for adding some docs.

I deliberately chose to not mention the guarantee about syntactically
referenced constants in functions that require code generation as it
seems hard to explain completely without some ambiguity. It also seems
to me that the rules are not completely stable[^3][^4] yet.

[stabilized]: rust-lang/rust#89006
[^1]: rust-lang/rust#91877 (comment)
[^2]: rust-lang/rust#91877 (comment)
[^3]: rust-lang/rust#71800
[^4]: rust-lang/rust#51999 (comment)
@XrXr
Copy link
Contributor Author

XrXr commented Jan 25, 2022

I'm going to close this for now since this behavior looks to be by design and is now documented in the reference through rust-lang/reference#1120. It's not a bug and changing this might need to go through an RFC.

@XrXr XrXr closed this as completed Jan 25, 2022
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.
Projects
None yet
Development

No branches or pull requests

4 participants