-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
BTreeMap: prevent tree from ever being owned by non-root node #81073
Conversation
5ddf49c
to
1d3e5ce
Compare
Apart from the comment, a major tweak is to have only one trait and a kind of static assert instead of a trait blocking access to |
☔ The latest upstream changes (presumably #81159) made this pull request unmergeable. Please resolve the merge conflicts. |
1d3e5ce
to
a546401
Compare
@rustbot modify labels: -S-waiting-on-author +S-waiting-on-review |
const PINNED: bool = false; | ||
} | ||
impl BorrowType for Owned { | ||
const PINNED: bool = true; // Owned node references are always at the root node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we explain this more? I'm not sure what you mean by pinned. It would probably also be good to find a different name, as Pin carries a pretty specific meaning in Rust. (Of course, if we match that meaning, then the name is fine; I am not sure we do).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link with Pin is rather loose. For the particular safety problem at hand, "descendible" would be enough, but I think it's more clear and useful to say "neither ascendable nor descendable", which implies "unmovable", "locked", "fixed", "frozen", "stuck", "constant". It seems somewhat fuzzy because it's not the borrow that is "unmovable", but the NodeRef that the borrow type qualifies. I think the language's equivalent difference is between a mut
and an immutable variable of a reference type, but "immutable" is definitely not better than "pinned" because the borrow type Immut would not have the "immutable" property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I admit that I don't really follow what you wrote. What precisely do we need? It seems like this is really just saying that you cannot descend from owned nodes? Can we call this "permits traversal" or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I mixed up value semantics with variable semantics. All NodeRef are immutable already, luckily. I like "PERMITS_TRAVERSAL", that really… pins it down.
Overall I think this seems fine. |
It would be good to update the description of the PR to summarize the current diff -- I think there was a pretty major reworking since you wrote it. |
It's still the same goal in my book. The fact it requires a distinction between dying and living ownership is., I think, useful on its own and could be another PR to jump in front. |
a546401
to
3e45f17
Compare
☔ The latest upstream changes (presumably #81217) made this pull request unmergeable. Please resolve the merge conflicts. |
3e45f17
to
417eefe
Compare
@rustbot label: +S-waiting-on-review -S-waiting-on-author |
@bors r+ rollup=never |
📌 Commit 417eefe has been approved by |
☀️ Test successful - checks-actions |
This introduces a new marker type,
Dying
, which is used to note trees which are in the process of deallocation. On such trees, some fields may be in an inconsistent state as we are deallocating the tree. Unfortunately, there's not a great way to express conditional unsafety, so the methods for traversal can cause UB if not invoked correctly, but not marked as such. This is not a regression from the previous state, but rather isolates the destructive methods to solely being called on the dying state.