From a60733f63213317d1be33c60624e86ea39823573 Mon Sep 17 00:00:00 2001 From: Waleed Khan Date: Thu, 9 Nov 2023 21:35:26 -0800 Subject: [PATCH] tree: remove unsafe with `ouroboros` for self-referential iterators --- Cargo.lock | 38 +++++++++++++ Cargo.toml | 1 + lib/Cargo.toml | 1 + lib/src/merged_tree.rs | 121 ++++++++++++++++++----------------------- lib/src/tree.rs | 31 +++++------ 5 files changed, 106 insertions(+), 86 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0aabeb0bd..0f91a55b7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -26,6 +26,12 @@ dependencies = [ "memchr", ] +[[package]] +name = "aliasable" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "250f629c0161ad8107cf89319e990051fae62832fd343083bea452d93e2205fd" + [[package]] name = "android-tzdata" version = "0.1.1" @@ -1673,6 +1679,7 @@ dependencies = [ "maplit", "num_cpus", "once_cell", + "ouroboros", "pest", "pest_derive", "pollster", @@ -1982,6 +1989,31 @@ version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "04744f49eae99ab78e0d5c0b603ab218f515ea8cfe5a456d7629ad883a3b6e7d" +[[package]] +name = "ouroboros" +version = "0.18.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1c86de06555b970aec45229b27291b53154f21a5743a163419f4e4c0b065dcde" +dependencies = [ + "aliasable", + "ouroboros_macro", + "static_assertions", +] + +[[package]] +name = "ouroboros_macro" +version = "0.18.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8cad0c4b129e9696e37cb712b243777b90ef489a0bfaa0ac34e7d9b860e4f134" +dependencies = [ + "heck", + "itertools 0.11.0", + "proc-macro-error", + "proc-macro2", + "quote", + "syn 2.0.37", +] + [[package]] name = "overload" version = "0.1.1" @@ -2669,6 +2701,12 @@ dependencies = [ "windows-sys 0.48.0", ] +[[package]] +name = "static_assertions" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a2eb9349b6444b326872e140eb1cf5e7c522154d69e7a0ffb0fb81c06b37543f" + [[package]] name = "strsim" version = "0.10.0" diff --git a/Cargo.toml b/Cargo.toml index 728c4c450..37fd9241d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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" diff --git a/lib/Cargo.toml b/lib/Cargo.toml index a9f719b74..2bd52db76 100644 --- a/lib/Cargo.toml +++ b/lib/Cargo.toml @@ -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 } diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs index 77b91944d..766e9bff4 100644 --- a/lib/src/merged_tree.rs +++ b/lib/src/merged_tree.rs @@ -575,22 +575,17 @@ 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: 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 for TreeEntriesDirItem { + fn from(tree: MergedTree) -> Self { + TreeEntriesDirItem::new(tree, |tree| tree.entries_non_recursive()) } } @@ -598,7 +593,7 @@ 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, } } @@ -609,20 +604,18 @@ impl Iterator for TreeEntriesIterator<'_> { fn next(&mut self) -> Option { 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)); } @@ -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, + 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 for ConflictsDirItem { + fn from(tree: MergedTree) -> Self { + Self::new(tree, |tree| ConflictEntriesNonRecursiveIterator::new(tree)) } } @@ -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)], }, } } @@ -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 @@ -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, - tree2: Box, + tree1: MergedTree, + tree2: MergedTree, + #[borrows(tree1, tree2)] + #[not_covariant] + entry_iterator: TreeEntryDiffIterator<'this>, } enum TreeDiffItem { @@ -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, ))); }; @@ -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) + }) } } @@ -892,13 +874,16 @@ impl Iterator for TreeDiffIterator<'_> { fn next(&mut self) -> Option { 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(..) => { @@ -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(); @@ -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 { diff --git a/lib/src/tree.rs b/lib/src/tree.rs index 753eb7d3b..804080c0f 100644 --- a/lib/src/tree.rs +++ b/lib/src/tree.rs @@ -196,22 +196,17 @@ 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, + #[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 for TreeEntriesDirItem { + fn from(tree: Tree) -> Self { + Self::new(tree, |tree| tree.entries_non_recursive()) } } @@ -219,7 +214,7 @@ 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, } } @@ -230,16 +225,16 @@ impl Iterator for TreeEntriesIterator<'_> { fn next(&mut self) -> Option { 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) {