Skip to content

disable type validity assertions for better error messages #4434

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

Closed
wants to merge 1 commit into from

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jul 1, 2025

Miri has pretty nice errors for type validity violations, printing which field in the type the problem occurs at and so on.

However, we don't see these errors when using e.g. mem::zeroed as that uses assert_zero_valid to bail out before Miri can detect the UB.

Similar to what we did with @saethlin's UB checks, I think we should disable such language UB checks in Miri so that we can get better error messages. If we go for this we should probably say this in the intrinsic docs as well so that people don't think they can rely on these intrinsics catching anything.

@rust-lang/miri what do you think?

@saethlin
Copy link
Member

saethlin commented Jul 1, 2025

Did you have a specific reason for putting the logic here instead of using cfgs in the standard library code?

@RalfJung
Copy link
Member Author

RalfJung commented Jul 1, 2025 via email

@@ -41,11 +42,23 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
}));
}

// Overwrite some core engine intrinsics that are used to catch UB -- we have better
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we remove them from the core engine and add them to the ctfe intrinsic handler if miri doesn't want them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's also an option, yeah. Maybe that's more clear.

So what should it be:

  • do this in the library?
  • or do it by neutering the intrinsic in Miri?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this variant as it keeps the checks in miri const eval as you said

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, because we don't do validation in normal const eval? So if we removed these checks for all interpreters, we'd miss even more UB in const eval?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, because we don't do validation in normal const eval? So if we removed these checks for all interpreters, we'd miss even more UB in const eval?

Exactly.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 2, 2025

Closing in favor of rust-lang/rust#143327.

@RalfJung RalfJung closed this Jul 2, 2025
@RalfJung RalfJung deleted the type-validity-errors branch July 2, 2025 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants