Skip to content

Commit

Permalink
tree: remove unsafe with ouroboros for self-referential iterators
Browse files Browse the repository at this point in the history
  • Loading branch information
@arxanas
arxanas committed Nov 10, 2023
1 parent 6ff3a4f commit a60733f
Show file tree
Hide file tree
Showing 5 changed files with 106 additions and 86 deletions.
38 changes: 38 additions & 0 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ libc = { version = "0.2.150" }
maplit = "1.0.2"
num_cpus = "1.16.0"
once_cell = "1.18.0"
ouroboros = "0.18.0"
pest = "2.7.5"
pest_derive = "2.7.5"
pollster = "0.3.0"
Expand Down
1 change: 1 addition & 0 deletions lib/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ hex = { workspace = true }
itertools = { workspace = true }
maplit = { workspace = true }
once_cell = { workspace = true }
ouroboros = { workspace = true }
pest = { workspace = true }
pest_derive = { workspace = true }
pollster = { workspace = true }
Expand Down
121 changes: 53 additions & 68 deletions lib/src/merged_tree.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -575,30 +575,25 @@ pub struct TreeEntriesIterator<'matcher> {
matcher: &'matcher dyn Matcher,
}

#[ouroboros::self_referencing]
struct TreeEntriesDirItem {
entry_iterator: TreeEntriesNonRecursiveIterator<'static>,
// On drop, tree must outlive entry_iterator
tree: Box<MergedTree>,
tree: MergedTree,
#[borrows(tree)]
#[not_covariant]
entry_iterator: TreeEntriesNonRecursiveIterator<'this>,
}

impl TreeEntriesDirItem {
fn new(tree: MergedTree) -> Self {
let tree = Box::new(tree);
let entry_iterator = tree.entries_non_recursive();
let entry_iterator: TreeEntriesNonRecursiveIterator<'static> =
unsafe { std::mem::transmute(entry_iterator) };
Self {
entry_iterator,
tree,
}
impl From<MergedTree> for TreeEntriesDirItem {
fn from(tree: MergedTree) -> Self {
TreeEntriesDirItem::new(tree, |tree| tree.entries_non_recursive())
}
}

impl<'matcher> TreeEntriesIterator<'matcher> {
fn new(tree: MergedTree, matcher: &'matcher dyn Matcher) -> Self {
// TODO: Restrict walk according to Matcher::visit()
Self {
stack: vec![TreeEntriesDirItem::new(tree)],
stack: vec![TreeEntriesDirItem::from(tree)],
matcher,
}
}
Expand All @@ -609,20 +604,18 @@ impl Iterator for TreeEntriesIterator<'_> {

fn next(&mut self) -> Option<Self::Item> {
while let Some(top) = self.stack.last_mut() {
if let Some((name, value)) = top.entry_iterator.next() {
let path = top.tree.dir().join(name);
if let (tree, Some((name, value))) = top.with_mut(|x| (x.tree, x.entry_iterator.next()))
{
let path = tree.dir().join(name);
let value = value.to_merge();
if value.is_tree() {
// TODO: Handle the other cases (specific files and trees)
if self.matcher.visit(&path).is_nothing() {
continue;
}
let tree_merge = value
.to_tree_merge(top.tree.store(), &path)
.unwrap()
.unwrap();
let tree_merge = value.to_tree_merge(tree.store(), &path).unwrap().unwrap();
let merged_tree = MergedTree::Merge(tree_merge);
self.stack.push(TreeEntriesDirItem::new(merged_tree));
self.stack.push(TreeEntriesDirItem::from(merged_tree));
} else if self.matcher.matches(&path) {
return Some((path, value));
}
Expand Down Expand Up @@ -681,23 +674,17 @@ impl<'a> Iterator for ConflictEntriesNonRecursiveIterator<'a> {

/// The state for the non-recursive iteration over the conflicted entries in a
/// single directory.
#[ouroboros::self_referencing]
struct ConflictsDirItem {
entry_iterator: ConflictEntriesNonRecursiveIterator<'static>,
// On drop, tree must outlive entry_iterator
tree: Box<MergedTree>,
tree: MergedTree,
#[borrows(tree)]
#[not_covariant]
entry_iterator: ConflictEntriesNonRecursiveIterator<'this>,
}

impl ConflictsDirItem {
fn new(tree: MergedTree) -> Self {
// Put the tree in a box so it doesn't move if `ConflictsDirItem` moves.
let tree = Box::new(tree);
let entry_iterator = ConflictEntriesNonRecursiveIterator::new(&tree);
let entry_iterator: ConflictEntriesNonRecursiveIterator<'static> =
unsafe { std::mem::transmute(entry_iterator) };
Self {
entry_iterator,
tree,
}
impl From<MergedTree> for ConflictsDirItem {
fn from(tree: MergedTree) -> Self {
Self::new(tree, |tree| ConflictEntriesNonRecursiveIterator::new(tree))
}
}

Expand All @@ -719,7 +706,7 @@ impl ConflictIterator {
conflicts_iter: tree.conflicts().into_iter(),
},
MergedTree::Merge(_) => ConflictIterator::Merge {
stack: vec![ConflictsDirItem::new(tree)],
stack: vec![ConflictsDirItem::from(tree)],
},
}
}
Expand All @@ -744,14 +731,15 @@ impl Iterator for ConflictIterator {
}
ConflictIterator::Merge { stack } => {
while let Some(top) = stack.last_mut() {
if let Some((basename, tree_values)) = top.entry_iterator.next() {
let path = top.tree.dir().join(basename);
if let (tree, Some((basename, tree_values))) =
top.with_mut(|x| (x.tree, x.entry_iterator.next()))
{
let path = tree.dir().join(basename);
// TODO: propagate errors
if let Some(trees) =
tree_values.to_tree_merge(top.tree.store(), &path).unwrap()
if let Some(trees) = tree_values.to_tree_merge(tree.store(), &path).unwrap()
{
// If all sides are trees or missing, descend into the merged tree
stack.push(ConflictsDirItem::new(MergedTree::Merge(trees)));
stack.push(ConflictsDirItem::from(MergedTree::Merge(trees)));
} else {
// Otherwise this is a conflict between files, trees, etc. If they could
// be automatically resolved, they should have been when the top-level
Expand Down Expand Up @@ -806,13 +794,14 @@ pub struct TreeDiffIterator<'matcher> {
matcher: &'matcher dyn Matcher,
}

#[ouroboros::self_referencing]
struct TreeDiffDirItem {
path: RepoPath,
// Iterator over the diffs between tree1 and tree2
entry_iterator: TreeEntryDiffIterator<'static>,
// On drop, tree1 and tree2 must outlive entry_iterator
tree1: Box<MergedTree>,
tree2: Box<MergedTree>,
tree1: MergedTree,
tree2: MergedTree,
#[borrows(tree1, tree2)]
#[not_covariant]
entry_iterator: TreeEntryDiffIterator<'this>,
}

enum TreeDiffItem {
Expand All @@ -830,7 +819,7 @@ impl<'matcher> TreeDiffIterator<'matcher> {
let root_dir = RepoPath::root();
let mut stack = Vec::new();
if !matcher.visit(&root_dir).is_nothing() {
stack.push(TreeDiffItem::Dir(TreeDiffDirItem::new(
stack.push(TreeDiffItem::Dir(TreeDiffDirItem::from_trees(
root_dir, tree1, tree2,
)));
};
Expand Down Expand Up @@ -873,17 +862,10 @@ impl<'matcher> TreeDiffIterator<'matcher> {
}

impl TreeDiffDirItem {
fn new(path: RepoPath, tree1: MergedTree, tree2: MergedTree) -> Self {
let tree1 = Box::new(tree1);
let tree2 = Box::new(tree2);
let iter: TreeEntryDiffIterator = TreeEntryDiffIterator::new(&tree1, &tree2);
let iter: TreeEntryDiffIterator<'static> = unsafe { std::mem::transmute(iter) };
Self {
path,
entry_iterator: iter,
tree1,
tree2,
}
fn from_trees(path: RepoPath, tree1: MergedTree, tree2: MergedTree) -> Self {
Self::new(path, tree1, tree2, |tree1, tree2| {
TreeEntryDiffIterator::new(tree1, tree2)
})
}
}

Expand All @@ -892,13 +874,16 @@ impl Iterator for TreeDiffIterator<'_> {

fn next(&mut self) -> Option<Self::Item> {
while let Some(top) = self.stack.last_mut() {
let (dir, (name, before, after)) = match top {
let ((path, tree1, tree2), (name, before, after)) = match top {
TreeDiffItem::Dir(dir) => {
if let Some((name, before, after)) = dir.entry_iterator.next() {
(dir, (name, before.to_merge(), after.to_merge()))
} else {
self.stack.pop().unwrap();
continue;
match dir.with_mut(|x| ((x.path, x.tree1, x.tree2), x.entry_iterator.next())) {
(dir, Some((name, before, after))) => {
(dir, (name, before.to_merge(), after.to_merge()))
}
_ => {
self.stack.pop().unwrap();
continue;
}
}
}
TreeDiffItem::File(..) => {
Expand All @@ -910,14 +895,14 @@ impl Iterator for TreeDiffIterator<'_> {
}
};

let path = dir.path.join(name);
let path = path.join(name);
let tree_before = before.is_tree();
let tree_after = after.is_tree();
let post_subdir =
if (tree_before || tree_after) && !self.matcher.visit(&path).is_nothing() {
let (before_tree, after_tree) = async {
let before_tree = Self::tree(dir.tree1.as_ref(), &path, &before);
let after_tree = Self::tree(dir.tree2.as_ref(), &path, &after);
let before_tree = Self::tree(tree1, &path, &before);
let after_tree = Self::tree(tree2, &path, &after);
futures::join!(before_tree, after_tree)
}
.block_on();
Expand All @@ -933,7 +918,7 @@ impl Iterator for TreeDiffIterator<'_> {
return Some((path, Err(err)));
}
};
let subdir = TreeDiffDirItem::new(path.clone(), before_tree, after_tree);
let subdir = TreeDiffDirItem::from_trees(path.clone(), before_tree, after_tree);
self.stack.push(TreeDiffItem::Dir(subdir));
self.stack.len() - 1
} else {
Expand Down
31 changes: 13 additions & 18 deletions lib/src/tree.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,30 +196,25 @@ pub struct TreeEntriesIterator<'matcher> {
matcher: &'matcher dyn Matcher,
}

#[ouroboros::self_referencing]
struct TreeEntriesDirItem {
entry_iterator: TreeEntriesNonRecursiveIterator<'static>,
// On drop, tree must outlive entry_iterator
tree: Box<Tree>,
tree: Tree,
#[borrows(tree)]
#[not_covariant]
entry_iterator: TreeEntriesNonRecursiveIterator<'this>,
}

impl TreeEntriesDirItem {
fn new(tree: Tree) -> Self {
let tree = Box::new(tree);
let entry_iterator = tree.entries_non_recursive();
let entry_iterator: TreeEntriesNonRecursiveIterator<'static> =
unsafe { std::mem::transmute(entry_iterator) };
Self {
entry_iterator,
tree,
}
impl From<Tree> for TreeEntriesDirItem {
fn from(tree: Tree) -> Self {
Self::new(tree, |tree| tree.entries_non_recursive())
}
}

impl<'matcher> TreeEntriesIterator<'matcher> {
fn new(tree: Tree, matcher: &'matcher dyn Matcher) -> Self {
// TODO: Restrict walk according to Matcher::visit()
Self {
stack: vec![TreeEntriesDirItem::new(tree)],
stack: vec![TreeEntriesDirItem::from(tree)],
matcher,
}
}
Expand All @@ -230,16 +225,16 @@ impl Iterator for TreeEntriesIterator<'_> {

fn next(&mut self) -> Option<Self::Item> {
while let Some(top) = self.stack.last_mut() {
if let Some(entry) = top.entry_iterator.next() {
let path = top.tree.dir().join(entry.name());
if let (tree, Some(entry)) = top.with_mut(|x| (x.tree, x.entry_iterator.next())) {
let path = tree.dir().join(entry.name());
match entry.value() {
TreeValue::Tree(id) => {
// TODO: Handle the other cases (specific files and trees)
if self.matcher.visit(&path).is_nothing() {
continue;
}
let subtree = top.tree.known_sub_tree(&path, id);
self.stack.push(TreeEntriesDirItem::new(subtree));
let subtree = tree.known_sub_tree(&path, id);
self.stack.push(TreeEntriesDirItem::from(subtree));
}
value => {
if self.matcher.matches(&path) {
Expand Down

0 comments on commit a60733f

Please sign in to comment.