( )⚙ D9071 rust-matchers: make `Matcher` trait object-safe

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9071

rust-matchers: make `Matcher` trait object-safe
ClosedPublic

Authored by Alphare on Sep 23 2020, 4:32 AM.

Details

Reviewers
marmoute
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG75f785888a7b: rust-matchers: make `Matcher` trait object-safe
Summary

Before this patch, it is not possible to create a Matcher trait-object (like
Box<dyn Matcher>), because of the use of a generic parameters in some methods,
namely impl AsRef<HgPath>.

While this makes the interface less flexible for callers in theory, it does not
change anything in the current codebase.

Until something like [1] is implemented, this is a "tradeoff" that we need to
make anyway.

[1] https://internals.rust-lang.org/t/pre-rfc-expand-object-safety/12693

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

Alphare created this revision.Sep 23 2020, 4:32 AM
marmoute accepted this revision.Sep 30 2020, 10:06 AM
marmoute added a subscriber: marmoute.

That seems fine to me. We can battle with rustc to re-introducing the genericity when we actually need it.

Alphare added a comment.Sep 30 2020, 10:08 AM
In D9071#136983, @marmoute wrote:

That seems fine to me. We can battle with rustc to re-introducing the genericity when we actually need it.

I don't think we ever will need to make it generic. However one of my (not so distant) future patches already needs to store a dyn Matcher in a struct.

martinvonz accepted this revision.Oct 1 2020, 12:50 AM
This revision is now accepted and ready to land.Oct 1 2020, 12:50 AM
Alphare added a commit: rHG75f785888a7b: rust-matchers: make `Matcher` trait object-safe.Oct 1 2020, 12:57 AM
Closed by commit rHG75f785888a7b: rust-matchers: make `Matcher` trait object-safe (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/matchers.rs (56 lines)

Diff 22775

rust/hg-core/src/matchers.rs

/// Visit this directory and all subdirectories /// Visit this directory and all subdirectories
Recursive, Recursive,
} }
pub trait Matcher { pub trait Matcher {
/// Explicitly listed files /// Explicitly listed files
fn file_set(&self) -> Option<&HashSet<&HgPath>>; fn file_set(&self) -> Option<&HashSet<&HgPath>>;
/// Returns whether `filename` is in `file_set` /// Returns whether `filename` is in `file_set`
fn exact_match(&self, filename: impl AsRef<HgPath>) -> bool; fn exact_match(&self, filename: &HgPath) -> bool;
/// Returns whether `filename` is matched by this matcher /// Returns whether `filename` is matched by this matcher
fn matches(&self, filename: impl AsRef<HgPath>) -> bool; fn matches(&self, filename: &HgPath) -> bool;
/// Decides whether a directory should be visited based on whether it /// Decides whether a directory should be visited based on whether it
/// has potential matches in it or one of its subdirectories, and /// has potential matches in it or one of its subdirectories, and
/// potentially lists which subdirectories of that directory should be /// potentially lists which subdirectories of that directory should be
/// visited. This is based on the match's primary, included, and excluded /// visited. This is based on the match's primary, included, and excluded
/// patterns. /// patterns.
/// ///
/// # Example /// # Example
/// ///
/// file or a directory, so `visit_children_set('dir')` for most matchers /// file or a directory, so `visit_children_set('dir')` for most matchers
/// will return `HashSet{ HgPath { "f" } }`, but if the matcher knows it's /// will return `HashSet{ HgPath { "f" } }`, but if the matcher knows it's
/// a file (like the yet to be implemented in Rust `ExactMatcher` does), /// a file (like the yet to be implemented in Rust `ExactMatcher` does),
/// it may return `VisitChildrenSet::This`. /// it may return `VisitChildrenSet::This`.
/// Do not rely on the return being a `HashSet` indicating that there are /// Do not rely on the return being a `HashSet` indicating that there are
/// no files in this dir to investigate (or equivalently that if there are /// no files in this dir to investigate (or equivalently that if there are
/// files to investigate in 'dir' that it will always return /// files to investigate in 'dir' that it will always return
/// `VisitChildrenSet::This`). /// `VisitChildrenSet::This`).
fn visit_children_set( fn visit_children_set(&self, directory: &HgPath) -> VisitChildrenSet;
&self,
directory: impl AsRef<HgPath>,
) -> VisitChildrenSet;
/// Matcher will match everything and `files_set()` will be empty: /// Matcher will match everything and `files_set()` will be empty:
/// optimization might be possible. /// optimization might be possible.
fn matches_everything(&self) -> bool; fn matches_everything(&self) -> bool;
/// Matcher will match exactly the files in `files_set()`: optimization /// Matcher will match exactly the files in `files_set()`: optimization
/// might be possible. /// might be possible.
fn is_exact(&self) -> bool; fn is_exact(&self) -> bool;
} }
/// ``` /// ```
#[derive(Debug)] #[derive(Debug)]
pub struct AlwaysMatcher; pub struct AlwaysMatcher;
impl Matcher for AlwaysMatcher { impl Matcher for AlwaysMatcher {
fn file_set(&self) -> Option<&HashSet<&HgPath>> { fn file_set(&self) -> Option<&HashSet<&HgPath>> {
None None
} }
fn exact_match(&self, _filename: impl AsRef<HgPath>) -> bool { fn exact_match(&self, _filename: &HgPath) -> bool {
false false
} }
fn matches(&self, _filename: impl AsRef<HgPath>) -> bool { fn matches(&self, _filename: &HgPath) -> bool {
true true
} }
fn visit_children_set( fn visit_children_set(&self, _directory: &HgPath) -> VisitChildrenSet {
&self,
_directory: impl AsRef<HgPath>,
) -> VisitChildrenSet {
VisitChildrenSet::Recursive VisitChildrenSet::Recursive
} }
fn matches_everything(&self) -> bool { fn matches_everything(&self) -> bool {
true true
} }
fn is_exact(&self) -> bool { fn is_exact(&self) -> bool {
false false
} }
} }
/// Matches the input files exactly. They are interpreted as paths, not /// Matches the input files exactly. They are interpreted as paths, not
/// patterns. /// patterns.
/// ///
///``` ///```
/// use hg::{ matchers::{Matcher, FileMatcher}, utils::hg_path::HgPath }; /// use hg::{ matchers::{Matcher, FileMatcher}, utils::hg_path::{HgPath, HgPathBuf} };
/// ///
/// let files = [HgPath::new(b"a.txt"), HgPath::new(br"re:.*\.c$")]; /// let files = [HgPathBuf::from_bytes(b"a.txt"), HgPathBuf::from_bytes(br"re:.*\.c$")];
/// let matcher = FileMatcher::new(&files).unwrap(); /// let matcher = FileMatcher::new(&files).unwrap();
/// ///
/// assert_eq!(matcher.matches(HgPath::new(b"a.txt")), true); /// assert_eq!(matcher.matches(HgPath::new(b"a.txt")), true);
/// assert_eq!(matcher.matches(HgPath::new(b"b.txt")), false); /// assert_eq!(matcher.matches(HgPath::new(b"b.txt")), false);
/// assert_eq!(matcher.matches(HgPath::new(b"main.c")), false); /// assert_eq!(matcher.matches(HgPath::new(b"main.c")), false);
/// assert_eq!(matcher.matches(HgPath::new(br"re:.*\.c$")), true); /// assert_eq!(matcher.matches(HgPath::new(br"re:.*\.c$")), true);
/// ``` /// ```
#[derive(Debug)] #[derive(Debug)]
pub struct FileMatcher<'a> { pub struct FileMatcher<'a> {
files: HashSet<&'a HgPath>, files: HashSet<&'a HgPath>,
dirs: DirsMultiset, dirs: DirsMultiset,
} }
impl<'a> FileMatcher<'a> { impl<'a> FileMatcher<'a> {
pub fn new( pub fn new(files: &'a [HgPathBuf]) -> Result<Self, DirstateMapError> {
files: &'a [impl AsRef<HgPath>],
) -> Result<Self, DirstateMapError> {
Ok(Self { Ok(Self {
files: HashSet::from_iter(files.iter().map(AsRef::as_ref)), files: HashSet::from_iter(files.iter().map(AsRef::as_ref)),
dirs: DirsMultiset::from_manifest(files)?, dirs: DirsMultiset::from_manifest(files)?,
}) })
} }
fn inner_matches(&self, filename: impl AsRef<HgPath>) -> bool { fn inner_matches(&self, filename: &HgPath) -> bool {
self.files.contains(filename.as_ref()) self.files.contains(filename.as_ref())
} }
} }
impl<'a> Matcher for FileMatcher<'a> { impl<'a> Matcher for FileMatcher<'a> {
fn file_set(&self) -> Option<&HashSet<&HgPath>> { fn file_set(&self) -> Option<&HashSet<&HgPath>> {
Some(&self.files) Some(&self.files)
} }
fn exact_match(&self, filename: impl AsRef<HgPath>) -> bool { fn exact_match(&self, filename: &HgPath) -> bool {
self.inner_matches(filename) self.inner_matches(filename)
} }
fn matches(&self, filename: impl AsRef<HgPath>) -> bool { fn matches(&self, filename: &HgPath) -> bool {
self.inner_matches(filename) self.inner_matches(filename)
} }
fn visit_children_set( fn visit_children_set(&self, directory: &HgPath) -> VisitChildrenSet {
&self,
directory: impl AsRef<HgPath>,
) -> VisitChildrenSet {
if self.files.is_empty() || !self.dirs.contains(&directory) { if self.files.is_empty() || !self.dirs.contains(&directory) {
return VisitChildrenSet::Empty; return VisitChildrenSet::Empty;
} }
let dirs_as_set = self.dirs.iter().map(Deref::deref).collect(); let dirs_as_set = self.dirs.iter().map(Deref::deref).collect();
let mut candidates: HashSet<&HgPath> = let mut candidates: HashSet<&HgPath> =
self.files.union(&dirs_as_set).cloned().collect(); self.files.union(&dirs_as_set).cloned().collect();
candidates.remove(HgPath::new(b"")); candidates.remove(HgPath::new(b""));
parents: HashSet<HgPathBuf>, parents: HashSet<HgPathBuf>,
} }
impl<'a> Matcher for IncludeMatcher<'a> { impl<'a> Matcher for IncludeMatcher<'a> {
fn file_set(&self) -> Option<&HashSet<&HgPath>> { fn file_set(&self) -> Option<&HashSet<&HgPath>> {
None None
} }
fn exact_match(&self, _filename: impl AsRef<HgPath>) -> bool { fn exact_match(&self, _filename: &HgPath) -> bool {
false false
} }
fn matches(&self, filename: impl AsRef<HgPath>) -> bool { fn matches(&self, filename: &HgPath) -> bool {
(self.match_fn)(filename.as_ref()) (self.match_fn)(filename.as_ref())
} }
fn visit_children_set( fn visit_children_set(&self, directory: &HgPath) -> VisitChildrenSet {
&self,
directory: impl AsRef<HgPath>,
) -> VisitChildrenSet {
let dir = directory.as_ref(); let dir = directory.as_ref();
if self.prefix && self.roots.contains(dir) { if self.prefix && self.roots.contains(dir) {
return VisitChildrenSet::Recursive; return VisitChildrenSet::Recursive;
} }
if self.roots.contains(HgPath::new(b"")) if self.roots.contains(HgPath::new(b""))
|| self.roots.contains(dir) || self.roots.contains(dir)
|| self.dirs.contains(dir) || self.dirs.contains(dir)
|| find_dirs(dir).any(|parent_dir| self.roots.contains(parent_dir)) || find_dirs(dir).any(|parent_dir| self.roots.contains(parent_dir))
parents parents
} }
); );
} }
#[test] #[test]
fn test_filematcher_visit_children_set() { fn test_filematcher_visit_children_set() {
// Visitchildrenset // Visitchildrenset
let files = vec![HgPath::new(b"dir/subdir/foo.txt")]; let files = vec![HgPathBuf::from_bytes(b"dir/subdir/foo.txt")];
let matcher = FileMatcher::new(&files).unwrap(); let matcher = FileMatcher::new(&files).unwrap();
let mut set = HashSet::new(); let mut set = HashSet::new();
set.insert(HgPath::new(b"dir")); set.insert(HgPath::new(b"dir"));
assert_eq!( assert_eq!(
matcher.visit_children_set(HgPath::new(b"")), matcher.visit_children_set(HgPath::new(b"")),
VisitChildrenSet::Set(set) VisitChildrenSet::Set(set)
); );
matcher.visit_children_set(HgPath::new(b"folder")), matcher.visit_children_set(HgPath::new(b"folder")),
VisitChildrenSet::Empty VisitChildrenSet::Empty
); );
} }
#[test] #[test]
fn test_filematcher_visit_children_set_files_and_dirs() { fn test_filematcher_visit_children_set_files_and_dirs() {
let files = vec![ let files = vec![
HgPath::new(b"rootfile.txt"), HgPathBuf::from_bytes(b"rootfile.txt"),
HgPath::new(b"a/file1.txt"), HgPathBuf::from_bytes(b"a/file1.txt"),
HgPath::new(b"a/b/file2.txt"), HgPathBuf::from_bytes(b"a/b/file2.txt"),
// No file in a/b/c // No file in a/b/c
HgPath::new(b"a/b/c/d/file4.txt"), HgPathBuf::from_bytes(b"a/b/c/d/file4.txt"),
]; ];
let matcher = FileMatcher::new(&files).unwrap(); let matcher = FileMatcher::new(&files).unwrap();
let mut set = HashSet::new(); let mut set = HashSet::new();
set.insert(HgPath::new(b"a")); set.insert(HgPath::new(b"a"));
set.insert(HgPath::new(b"rootfile.txt")); set.insert(HgPath::new(b"rootfile.txt"));
assert_eq!( assert_eq!(
matcher.visit_children_set(HgPath::new(b"")), matcher.visit_children_set(HgPath::new(b"")),