make intern_const_alloc_recursive return error #78742
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @oli-obk (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
vn-ki
force-pushed
the
fix-issue-78655
branch
from
379c243
to
bd7229d
Compare
|
@bors r+ |
|
📌 Commit a15ee4d has been approved by |
bors
added
S-waiting-on-bors
make intern_const_alloc_recursive return error fix rust-lang#78655 r? `@oli-obk`
Rollup of 15 pull requests Successful merges: - rust-lang#76718 (Move Vec UI tests to unit tests when possible) - rust-lang#78093 (Clean up docs for 'as' keyword) - rust-lang#78425 (Move f64::NAN ui tests into `library`) - rust-lang#78465 (Change as_str → to_string in proc_macro::Ident::span() docs) - rust-lang#78584 (Add keyboard handling to the theme picker menu) - rust-lang#78716 (Array trait impl comment/doc fixes) - rust-lang#78727 ((rustdoc) fix test for trait impl display) - rust-lang#78733 (fix a couple of clippy warnings:) - rust-lang#78735 (Simplify the implementation of `get_mut` (no unsafe)) - rust-lang#78738 (Move range in ui test to ops test in library/core) - rust-lang#78739 (Fix ICE on type error in async function) - rust-lang#78742 (make intern_const_alloc_recursive return error) - rust-lang#78756 (Update cargo) - rust-lang#78757 (Improve and clean up some intra-doc links) - rust-lang#78758 (Fixed typo in comment) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
| @@ -129,7 +135,7 @@ impl fmt::Display for InvalidProgramInfo<'_> { | |||
| match self { | |||
| TooGeneric => write!(f, "encountered overly generic constant"), | |||
| ReferencedConstant => write!(f, "referenced constant has errors"), | |||
| TypeckError(ErrorReported) => { | |||
| AlreadyReported(ErrorReported) => { | |||
| write!(f, "encountered constants with type errors, stopping evaluation") | |||
There was a problem hiding this comment.
The error message text does not match the new variant name.
| pub fn intern_const_alloc_recursive<M: CompileTimeMachine<'mir, 'tcx>>( | ||
| ecx: &mut InterpCx<'mir, 'tcx, M>, | ||
| intern_kind: InternKind, | ||
| ret: MPlaceTy<'tcx>, | ||
| ) where | ||
| ) -> Result<(), ErrorReported> |
There was a problem hiding this comment.
Hm, I had quite deliberately made interning infallible a while ago.^^ Is this really necessary or just yet another hack to work around #76064 ?
There was a problem hiding this comment.
well... infallible or not, it was reporting errors, so it wasn't really infallible, it just didn't report its failures.
The problem isn't #76064 in this case, even if the repro test would also be fixed by a fix for #76064 . This issue also occurs for
const FOO: *const u32 = {
let x = 42;
&x
};which typechecks just fine. Basically any dangling pointer was causing this.
As you noted in #78655 (comment) that error should stop compilation. We could change the interning error to a delay_span_bug and have it get reported by validation instead, but then validation needs to have a look at relocations within padding, which it currently does not.
There was a problem hiding this comment.
well... infallible or not, it was reporting errors, so it wasn't really infallible, it just didn't report its failures.
That's fair.
Basically any dangling pointer was causing this.
It was causing an error, but usually not an ICE -- IIRC we even had a testcase for that.
But I guess what I really care about is that interning does not return an InterpError, and that is still the case. Actually telling the outside world whether we did span_err seems reasonable and seems to be a more general pattern.
So, LGTM.
There was a problem hiding this comment.
then validation needs to have a look at relocations within padding, which it currently does not.
Interning is in a much better place to do this, so unless we really want the error to point to where in the value the problem occurs, I think the current approach works better.
fix #78655
r? @oli-obk