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

An unusable constant should at least warn #47054

Closed
scottmcm opened this issue Dec 29, 2017 · 6 comments · Fixed by #50110
Closed

An unusable constant should at least warn #47054

scottmcm opened this issue Dec 29, 2017 · 6 comments · Fixed by #50110
Assignees
Labels
A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@scottmcm
Copy link
Member

The following produces no warnings:

pub const A: [i32;3] = [1,2,3];
pub const FOO: i32 = A[100000];

fn main() { println!("Verified"); }

But any attempt to use the FOO const will give error[E0080]: constant evaluation error.

It would be nice for it to at least warn. and perhaps start erring once the "error on use" case can be handled with a stable const fn.

@est31
Copy link
Member

est31 commented Dec 29, 2017

There is a trade-off here with compile times. In order to find out whether a constant can be used or not, you do have to const-evaluate it. This is a possibly expensive operation. If it is expensive, it will worsen compile times. IMO the best way of tackling this is to count the MIR steps spent, and after 1000 steps or so the evaluation stops and it doesn't emit anything. Not sure if miri has a feature for this...

@ranma42
Copy link
Contributor

ranma42 commented Dec 29, 2017

It might also break existing code, especially auto-generated code. EDIT: just warning would be fine, assuming the code does not rely on deny.

Example: the autogenerated code always contains a bunch of constants; depending on the parameters some of them are unusable, but it does not matter as they are not actually used in the code that is emitted for those parameters.
This could be fixed by not emitting the constants in the first place, but it would make the code generator more complex.

@est31
Copy link
Member

est31 commented Dec 29, 2017

Are constants being emitted? I've thought they only get evaluated at their usage site.

@scottmcm
Copy link
Member Author

@est31 You're absolutely correct in everything you said. However, to me this has the same "I compiled and it was fine; what do you mean there's a compilation error on use that's not the user's fault?!" vibe as monomorphization errors, which are also checked for eagerly. And, at least as long as it's a lint, presumably the expense wouldn't happen if the lint is allowed, giving a workaround.

@est31
Copy link
Member

est31 commented Dec 29, 2017

@scottmcm the lint is definitely a great idea, for the reasons you state. If an unusable const is being declared in a library, your code will fail to compile even though the library is at fault, that's no good experience.

@pietroalbini pietroalbini added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. labels Jan 30, 2018
@oli-obk oli-obk self-assigned this Mar 6, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Apr 20, 2018

I created a PR (#50110), anyone feel like reviewing 50 test changes across 15 files?

bors added a commit that referenced this issue Apr 25, 2018
Warn on all erroneous constants

fixes #49791
fixes #47054

@Zoxc this PR triggers the nondeterministic errors of #49950 (comment) really often (at least on stage1).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. C-enhancement Category: An issue proposing an enhancement or a PR with one. 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.

5 participants