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.

Sign up for GitHub

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

BTree: evaluate static type-related check at compile time #95005

Merged
merged 1 commit into from
Aug 26, 2022
Merged

BTree: evaluate static type-related check at compile time #95005

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions library/alloc/src/collections/btree/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ impl<BorrowType: marker::BorrowType, K, V, Type> NodeRef<BorrowType, K, V, Type>
pub fn ascend(
self,
) -> Result<Handle<NodeRef<BorrowType, K, V, marker::Internal>, marker::Edge>, Self> {
assert!(BorrowType::PERMITS_TRAVERSAL);
let _ = BorrowType::TRAVERSAL_PERMIT;
// We need to use raw pointers to nodes because, if BorrowType is marker::ValMut,
// there might be outstanding mutable references to values that we must not invalidate.
let leaf_ptr: *const _ = Self::as_leaf_ptr(&self);
Expand Down Expand Up @@ -986,7 +986,7 @@ impl<BorrowType: marker::BorrowType, K, V>
/// `edge.descend().ascend().unwrap()` and `node.ascend().unwrap().descend()` should
/// both, upon success, do nothing.
pub fn descend(self) -> NodeRef<BorrowType, K, V, marker::LeafOrInternal> {
assert!(BorrowType::PERMITS_TRAVERSAL);
let _ = BorrowType::TRAVERSAL_PERMIT;
// We need to use raw pointers to nodes because, if BorrowType is
// marker::ValMut, there might be outstanding mutable references to
// values that we must not invalidate. There's no worry accessing the
Expand Down Expand Up @@ -1637,15 +1637,17 @@ pub mod marker {
pub struct ValMut<'a>(PhantomData<&'a mut ()>);

pub trait BorrowType {
// Whether node references of this borrow type allow traversing
// to other nodes in the tree.
const PERMITS_TRAVERSAL: bool = true;
// If node references of this borrow type allow traversing to other
// nodes in the tree, this constant can be evaluated. Thus reading it
// serves as a compile-time assertion.
const TRAVERSAL_PERMIT: () = ();
}
impl BorrowType for Owned {
// Traversal isn't needed, it happens using the result of `borrow_mut`.
// Reject evaluation, because traversal isn't needed. Instead traversal
// happens using the result of `borrow_mut`.
// By disabling traversal, and only creating new references to roots,
// we know that every reference of the `Owned` type is to a root node.
const PERMITS_TRAVERSAL: bool = false;
const TRAVERSAL_PERMIT: () = panic!();
}
Comment on lines 1645 to 1651
Copy link
Member

Choose a reason for hiding this comment

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

Can you add some UI tests to show the cases where this does and doesn't panic and to let us review the panic output?

Copy link
Contributor Author

@ssomers ssomers Apr 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think I can. These static asserts guard the private internals of node.rs. We have unit test of those internals, we can do positive compile tests, but a negative compile test of internals, I doubt the test framework caters for that. And surely we don't want to make this stuff public.

The way I tested this is to try to address the remaining FIXME on reborrow_mutin node.rs: disable the permit for Mut<'+> like it is disabled for Owned, and build errors point out the first place where a Mut handle is traversed. While without this PR, you have to run tests to see it, and you have to assume tests offer complete coverage. I've edited the PR description a bit.

impl BorrowType for Dying {}
impl<'a> BorrowType for Immut<'a> {}
Expand Down