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

BTreeMap: prevent tree from ever being owned by non-root node #81073

Merged
merged 1 commit into from
Jan 29, 2021

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Jan 16, 2021

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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 16, 2021
@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2021
@ssomers ssomers marked this pull request as draft January 16, 2021 16:19
@ssomers
Copy link
Contributor Author

ssomers commented Jan 17, 2021

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 descend for Owned nodes. That is actually one small step away from realizing the old "FIXME(@gereeter) consider adding yet another type parameter to NodeRef that restricts the use of navigation methods on reborrowed pointers". But I'm not taking that step, because I'm old enough to know that a small step for man usually turns out to be a giant leap.

@ssomers ssomers marked this pull request as ready for review January 17, 2021 00:25
@bors
Copy link
Contributor

bors commented Jan 18, 2021

☔ The latest upstream changes (presumably #81159) made this pull request unmergeable. Please resolve the merge conflicts.

@ssomers
Copy link
Contributor Author

ssomers commented Jan 18, 2021

@rustbot modify labels: -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 18, 2021
const PINNED: bool = false;
}
impl BorrowType for Owned {
const PINNED: bool = true; // Owned node references are always at the root node.
Copy link
Member

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).

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

@Mark-Simulacrum
Copy link
Member

Overall I think this seems fine.

@Mark-Simulacrum
Copy link
Member

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.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 24, 2021
@ssomers
Copy link
Contributor Author

ssomers commented Jan 24, 2021

It would be good to update the description of the PR to summarize the current diff

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.

@ssomers ssomers changed the title BTreeMap: stop tree from being owned by non-root node BTreeMap: prevent tree from ever being owned by non-root node Jan 25, 2021
@bors
Copy link
Contributor

bors commented Jan 26, 2021

☔ The latest upstream changes (presumably #81217) made this pull request unmergeable. Please resolve the merge conflicts.

@ssomers
Copy link
Contributor Author

ssomers commented Jan 27, 2021

@rustbot label: +S-waiting-on-review -S-waiting-on-author

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 27, 2021
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jan 28, 2021

📌 Commit 417eefe has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 28, 2021
@bors
Copy link
Contributor

bors commented Jan 29, 2021

⌛ Testing commit 417eefe with merge c6bc462...

@bors
Copy link
Contributor

bors commented Jan 29, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing c6bc462 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 29, 2021
@bors bors merged commit c6bc462 into rust-lang:master Jan 29, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 29, 2021
@ssomers ssomers deleted the btree_owned_root_vs_dying branch January 29, 2021 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants