From e912d6507f38b98ce56fa1f44d1f52c0433fe8e6 Mon Sep 17 00:00:00 2001 From: Basti Ortiz <39114273+Some-Dood@users.noreply.github.com> Date: Tue, 10 Aug 2021 15:43:51 +0800 Subject: [PATCH 1/8] Chore: use short-circuiting for file ID getter --- crates/vfs/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/vfs/src/lib.rs b/crates/vfs/src/lib.rs index e075d752b7f..7828e51be93 100644 --- a/crates/vfs/src/lib.rs +++ b/crates/vfs/src/lib.rs @@ -111,7 +111,9 @@ pub fn len(&self) -> usize { /// Id of the given path if it exists in the `Vfs` and is not deleted. pub fn file_id(&self, path: &VfsPath) -> Option { - self.interner.get(path).filter(|&it| self.get(it).is_some()) + let it = self.interner.get(path)?; + let _ = self.get(it).as_ref()?; + Some(it) } /// File path corresponding to the given `file_id`. From 0765c9713b75230c643ca386b24080c8dba00a6e Mon Sep 17 00:00:00 2001 From: Basti Ortiz <39114273+Some-Dood@users.noreply.github.com> Date: Tue, 10 Aug 2021 15:56:16 +0800 Subject: [PATCH 2/8] Refactor: use `filter_map` when iterating over stored IDs and --- crates/vfs/src/lib.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/crates/vfs/src/lib.rs b/crates/vfs/src/lib.rs index 7828e51be93..8404f1635f8 100644 --- a/crates/vfs/src/lib.rs +++ b/crates/vfs/src/lib.rs @@ -139,13 +139,12 @@ pub fn file_contents(&self, file_id: FileId) -> &[u8] { /// /// This will skip deleted files. pub fn iter(&self) -> impl Iterator + '_ { - (0..self.data.len()) - .map(|it| FileId(it as u32)) - .filter(move |&file_id| self.get(file_id).is_some()) - .map(move |file_id| { - let path = self.interner.lookup(file_id); - (file_id, path) - }) + (0..self.data.len()).filter_map(move |it| { + let file_id = FileId(it as u32); + let _ = self.get(file_id).as_ref()?; + let path = self.interner.lookup(file_id); + Some((file_id, path)) + }) } /// Update the `path` with the given `contents`. `None` means the file was deleted. From 6d0336b2e41703254b31f5d9a1358eb7e15cee3f Mon Sep 17 00:00:00 2001 From: Basti Ortiz <39114273+Some-Dood@users.noreply.github.com> Date: Tue, 10 Aug 2021 16:32:00 +0800 Subject: [PATCH 3/8] Refactor: simplify logic for `Directories::contains_file` The logic was simply inverted to accomodate for the short-circuiting `&&` operator. --- crates/vfs/src/loader.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/vfs/src/loader.rs b/crates/vfs/src/loader.rs index 473b29fcb32..514644723f8 100644 --- a/crates/vfs/src/loader.rs +++ b/crates/vfs/src/loader.rs @@ -137,10 +137,7 @@ impl Directories { /// Returns `true` if `path` is included in `self`. pub fn contains_file(&self, path: &AbsPath) -> bool { let ext = path.extension().unwrap_or_default(); - if self.extensions.iter().all(|it| it.as_str() != ext) { - return false; - } - self.includes_path(path) + self.extensions.iter().any(|it| it.as_str() == ext) && self.includes_path(path) } /// Returns `true` if `path` is included in `self`. From 7ee6cca3d7fe4d73d8cc4ef9d5c0753cc132b7e2 Mon Sep 17 00:00:00 2001 From: Basti Ortiz <39114273+Some-Dood@users.noreply.github.com> Date: Tue, 10 Aug 2021 17:51:11 +0800 Subject: [PATCH 4/8] Refactor: use iterator methods over `for` loops --- crates/vfs/src/loader.rs | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/crates/vfs/src/loader.rs b/crates/vfs/src/loader.rs index 514644723f8..b1d3918cb93 100644 --- a/crates/vfs/src/loader.rs +++ b/crates/vfs/src/loader.rs @@ -155,25 +155,23 @@ pub fn contains_dir(&self, path: &AbsPath) -> bool { /// - This path is longer than any element in `self.exclude` that is a prefix /// of `path`. In case of equality, exclusion wins. fn includes_path(&self, path: &AbsPath) -> bool { - let mut include: Option<&AbsPathBuf> = None; - for incl in &self.include { - if path.starts_with(incl) { - include = Some(match include { - Some(prev) if prev.starts_with(incl) => prev, - _ => incl, - }) + let include = self.include.iter().fold(None::<&AbsPathBuf>, |include, incl| { + if !path.starts_with(incl) { + return include; } - } + + Some(match include { + Some(prev) if prev.starts_with(incl) => prev, + _ => incl, + }) + }); + let include = match include { Some(it) => it, None => return false, }; - for excl in &self.exclude { - if path.starts_with(excl) && excl.starts_with(include) { - return false; - } - } - true + + !self.exclude.iter().any(|excl| path.starts_with(excl) && excl.starts_with(include)) } } From ae1288eeed72c2b2a1ad638c1b9d99fa35035f91 Mon Sep 17 00:00:00 2001 From: Basti Ortiz <39114273+Some-Dood@users.noreply.github.com> Date: Tue, 10 Aug 2021 22:25:43 +0800 Subject: [PATCH 5/8] Fix: revert complex conditional in `Directories::contains_file` This reverts commit 6d0336b2e41703254b31f5d9a1358eb7e15cee3f. --- crates/vfs/src/loader.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/vfs/src/loader.rs b/crates/vfs/src/loader.rs index b1d3918cb93..211d4f7c782 100644 --- a/crates/vfs/src/loader.rs +++ b/crates/vfs/src/loader.rs @@ -137,7 +137,10 @@ impl Directories { /// Returns `true` if `path` is included in `self`. pub fn contains_file(&self, path: &AbsPath) -> bool { let ext = path.extension().unwrap_or_default(); - self.extensions.iter().any(|it| it.as_str() == ext) && self.includes_path(path) + if self.extensions.iter().all(|it| it.as_str() != ext) { + return false; + } + self.includes_path(path) } /// Returns `true` if `path` is included in `self`. From 0e480a6e9c9fcb24d42011a031444e0446c07353 Mon Sep 17 00:00:00 2001 From: Basti Ortiz <39114273+Some-Dood@users.noreply.github.com> Date: Tue, 10 Aug 2021 22:30:08 +0800 Subject: [PATCH 6/8] Chore: add comments to explicitly express two-step check See https://github.com/rust-analyzer/rust-analyzer/pull/9836#discussion_r685953381 --- crates/vfs/src/loader.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/vfs/src/loader.rs b/crates/vfs/src/loader.rs index 211d4f7c782..5959ce2ce79 100644 --- a/crates/vfs/src/loader.rs +++ b/crates/vfs/src/loader.rs @@ -136,10 +136,13 @@ pub fn contains_dir(&self, path: &AbsPath) -> bool { impl Directories { /// Returns `true` if `path` is included in `self`. pub fn contains_file(&self, path: &AbsPath) -> bool { + // First, check the file extension... let ext = path.extension().unwrap_or_default(); if self.extensions.iter().all(|it| it.as_str() != ext) { return false; } + + // Then, check for path inclusion... self.includes_path(path) } From 27605b402e5009d03bd1c961c6272b8d2dbe674d Mon Sep 17 00:00:00 2001 From: Basti Ortiz <39114273+Some-Dood@users.noreply.github.com> Date: Tue, 10 Aug 2021 22:40:04 +0800 Subject: [PATCH 7/8] Fix: prefer the usage of `for` loops over `fold` --- crates/vfs/src/loader.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/crates/vfs/src/loader.rs b/crates/vfs/src/loader.rs index 5959ce2ce79..c341b974a04 100644 --- a/crates/vfs/src/loader.rs +++ b/crates/vfs/src/loader.rs @@ -161,16 +161,15 @@ pub fn contains_dir(&self, path: &AbsPath) -> bool { /// - This path is longer than any element in `self.exclude` that is a prefix /// of `path`. In case of equality, exclusion wins. fn includes_path(&self, path: &AbsPath) -> bool { - let include = self.include.iter().fold(None::<&AbsPathBuf>, |include, incl| { - if !path.starts_with(incl) { - return include; + let mut include = None::<&AbsPathBuf>; + for incl in &self.include { + if path.starts_with(incl) { + include = Some(match include { + Some(prev) if prev.starts_with(incl) => prev, + _ => incl, + }); } - - Some(match include { - Some(prev) if prev.starts_with(incl) => prev, - _ => incl, - }) - }); + } let include = match include { Some(it) => it, From 0986632c040e562826ab0dfc13e183d34a3c3a34 Mon Sep 17 00:00:00 2001 From: Basti Ortiz <39114273+Some-Dood@users.noreply.github.com> Date: Tue, 10 Aug 2021 22:44:15 +0800 Subject: [PATCH 8/8] Fix: revert strange usage of `?` operator --- crates/vfs/src/lib.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/crates/vfs/src/lib.rs b/crates/vfs/src/lib.rs index 8404f1635f8..e075d752b7f 100644 --- a/crates/vfs/src/lib.rs +++ b/crates/vfs/src/lib.rs @@ -111,9 +111,7 @@ pub fn len(&self) -> usize { /// Id of the given path if it exists in the `Vfs` and is not deleted. pub fn file_id(&self, path: &VfsPath) -> Option { - let it = self.interner.get(path)?; - let _ = self.get(it).as_ref()?; - Some(it) + self.interner.get(path).filter(|&it| self.get(it).is_some()) } /// File path corresponding to the given `file_id`. @@ -139,12 +137,13 @@ pub fn file_contents(&self, file_id: FileId) -> &[u8] { /// /// This will skip deleted files. pub fn iter(&self) -> impl Iterator + '_ { - (0..self.data.len()).filter_map(move |it| { - let file_id = FileId(it as u32); - let _ = self.get(file_id).as_ref()?; - let path = self.interner.lookup(file_id); - Some((file_id, path)) - }) + (0..self.data.len()) + .map(|it| FileId(it as u32)) + .filter(move |&file_id| self.get(file_id).is_some()) + .map(move |file_id| { + let path = self.interner.lookup(file_id); + (file_id, path) + }) } /// Update the `path` with the given `contents`. `None` means the file was deleted.