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

Invalid (unstable, ill-formed, ...) const code is evaluated even after emitting error #76064

Closed
RalfJung opened this issue Aug 29, 2020 · 5 comments · Fixed by #78809
Closed
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) C-bug Category: This is a bug. glacier ICE tracked in rust-lang/glacier. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

Nightly-only ICEs are exposed on stable because array lengths are evaluated even when they are invalid:

struct Bug([u8; panic!(1)]);

This first emits an error that a feature flag is missing, but then const-evaluates the array length anyway and later leads to an ICE. This is not a stability hole (there's an error, the code will not compile), but it's an ICE on stable, so it's a bug.

While the ICE should also be fixed, the underlying problem is that we should not const-evaluate code that failed stability checking (and there are possibly other kinds of checks to which this applies as well)

@oli-obk proposed some solutions:

Cc @rust-lang/wg-const-eval

@jonas-schievink jonas-schievink added A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 29, 2020
@ecstatic-morse
Copy link
Contributor

Putting this on ConstQualifs seems reasonable. It seems better than adding more cruft to mir::Body at first glance. Currently each field of ConstQualifs is tied to an implementer of the Qualif trait but this isn't a hard requirement.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 2, 2020

Once this is fixed, we can hopefully also get rid of the TransmuteSizeDiff hack. I am not sure if we carry any other similar hacks that we still carry around.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 7, 2020

@oli-obk the solution at #78809, if I understand correctly, does not fix TransmuteSizeDiff... should that become a separate issue once this one is closed, or is there some reason that TransmuteSizeDiff cannot be fixed by adding a similar ErrorReported to the transmute size check?

@oli-obk
Copy link
Contributor

oli-obk commented Nov 9, 2020

It does indeed not fix TransmuteSizeDiff. We'll need to investigate what actually causes a transmute size check failure to not affect tainted_by_errors. We can likely fix this in a similar manner. Though we also have other bugs like { let x; &x } getting all the way to interning when it fails due to a dangling pointer. We'll be collecting more and more error flags to keep track of. I'm not sure how to address this properly. Maybe we'll need more Result<T, ErrorReported> on queries that don't return an enum which has an error variant (like ty::Err or ConstKind::Err)

vn-ki added a commit to vn-ki/rust that referenced this issue Nov 9, 2020
@RalfJung
Copy link
Member Author

I opened a new issue for the transmute problem: #79047.

bors added a commit to rust-lang-ci/rust that referenced this issue Nov 14, 2020
add error_occured field to ConstQualifs,

fix rust-lang#76064

I wasn't sure what `in_return_place` actually did and not sure why it returns `ConstQualifs` while it's sibling functions return `bool`. So I tried to make as minimal changes to the structure as possible. Please point out whether I have to refactor it or not.

r? `@oli-obk`
cc `@RalfJung`
@bors bors closed this as completed in 8bce9af Nov 14, 2020
flip1995 pushed a commit to flip1995/rust that referenced this issue Nov 20, 2020
add error_occured field to ConstQualifs,

fix rust-lang#76064

I wasn't sure what `in_return_place` actually did and not sure why it returns `ConstQualifs` while it's sibling functions return `bool`. So I tried to make as minimal changes to the structure as possible. Please point out whether I have to refactor it or not.

r? `@oli-obk`
cc `@RalfJung`
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, ...) C-bug Category: This is a bug. glacier ICE tracked in rust-lang/glacier. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ 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