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

Improve UI/UX of post-monomorphization errors #1268

Merged
merged 1 commit into from
May 16, 2024
Merged

Conversation

jswrenn
Copy link
Collaborator

@jswrenn jswrenn commented May 15, 2024

By asserting in a macro, rather than a helper function, rustc's diagnostics point to the user's callsite, rather than our own.

Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

Can we add UI tests for these? We honestly should have had them all along.

src/macros.rs Outdated
Comment on lines 724 to 726
/// Asserts at compile time that `$condition` is true for `Self` or the given
/// `$tyvar`s.
macro_rules! ct_assert {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to name this (and phrase the doc comment) so that it's clear why it's different from const_assert (immediately above in this file)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that this name will appear in compiler errors, so we want it to be both externally intelligible and succinct.

I can definitely improve the documentation, but would welcome suggestions of alternative names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I suppose static_assert might be fitting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

src/macros.rs Outdated
Comment on lines 727 to 754
(Self $(: $(? $optbound:ident $(+)?)* $($bound:ident $(+)?)* )? => $condition:expr) => {
trait ConstAssert {
const ASSERT: bool;
}

impl<T $(: $(? $optbound +)* $($bound +)*)?> ConstAssert for T {
const ASSERT: bool = {
const_assert!($condition);
$condition
};
}

const_assert!(<Self as ConstAssert>::ASSERT);
};
($($tyvar:ident $(: $(? $optbound:ident $(+)?)* $($bound:ident $(+)?)* )?),* => $condition:expr) => {
trait ConstAssert {
const ASSERT: bool;
}

impl<$($tyvar $(: $(? $optbound +)* $($bound +)*)?,)*> ConstAssert for ($($tyvar,)*) {
const ASSERT: bool = {
const_assert!($condition);
$condition
};
}

const_assert!(<($($tyvar,)*) as ConstAssert>::ASSERT);
};
Copy link
Member

Choose a reason for hiding this comment

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

Wrap these bodies in an extra set of curly braces? Otherwise ConstAssert is introduced into the caller's namespace.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

src/macros.rs Outdated
Comment on lines 760 to 771
($tyvar:ident) => {
use crate::KnownLayout;
ct_assert!($tyvar: ?Sized + KnownLayout => {
let dst_is_zst = match $tyvar::LAYOUT.size_info {
crate::SizeInfo::Sized { .. } => false,
crate::SizeInfo::SliceDst(TrailingSliceLayout { elem_size, .. }) => {
elem_size == 0
}
};
!dst_is_zst
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Wrap this in an extra set of curly braces? Otherwise, use crate::KnownLayout; is introduced into the caller's namespace.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jswrenn
Copy link
Collaborator Author

jswrenn commented May 15, 2024

We cannot, because trybuild only does a cargo check, not a cargo build.

@joshlf
Copy link
Member

joshlf commented May 15, 2024

We cannot, because trybuild only does a cargo check, not a cargo build.

Oh look, not the first time we've run into this issue. Bummer.

@jswrenn jswrenn force-pushed the zsty-dsty-ui-ux branch 2 times, most recently from 2a433d4 to b2cb6b7 Compare May 15, 2024 17:31
@jswrenn jswrenn enabled auto-merge May 16, 2024 18:26
@jswrenn jswrenn requested a review from joshlf May 16, 2024 19:08
@joshlf
Copy link
Member

joshlf commented May 16, 2024

Still need a resolution on https://github.com/google/zerocopy/pull/1268/files#r1601997671.

@jswrenn jswrenn force-pushed the zsty-dsty-ui-ux branch 2 times, most recently from bdba67b to 6cfde91 Compare May 16, 2024 19:39
src/macros.rs Outdated

/// Assert at compile time that `tyvar` does not have a zero-sized DST
/// component.
macro_rules! ct_assert_dst_is_not_zst {
Copy link
Member

Choose a reason for hiding this comment

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

Also update this name?

src/macros.rs Outdated
Comment on lines 724 to 727
/// Asserts at compile time that `$condition` is true for `Self` or the given
/// `$tyvar`s. Unlike `const_assert`, this is *strictly* a compile-time check.
/// The condition is checked after monomorphization and, upon failure, emits a
/// compile error.
Copy link
Member

Choose a reason for hiding this comment

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

const_assert! is also strictly a compile-time check so long as it's always called from const code; its fanciness has to do with its behavior on older toolchains that don't support explicitly panicking in const code. Maybe I should update the docs to be more clear about that.

The distinction is actually that static_assert uses post-monomorphization errors only, and so its errors are generated during a different compiler phase. Arguably we could just inline static_assert into ct_assert_dst_is_not_zst below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made additional clarifications both in the docs of const_assert and static_assert.

By asserting in a macro, rather than a helper function, rustc's
diagnostics point to the user's callsite, rather than our own.
@jswrenn jswrenn enabled auto-merge May 16, 2024 22:37
@jswrenn jswrenn added this pull request to the merge queue May 16, 2024
Merged via the queue into main with commit 264619b May 16, 2024
210 checks passed
@jswrenn jswrenn deleted the zsty-dsty-ui-ux branch May 16, 2024 23:09
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.

None yet

2 participants