From 6fa586f70594ce5da0e2b3b3e353f236bb1326f1 Mon Sep 17 00:00:00 2001 From: Vincent Esche Date: Sun, 8 Oct 2023 23:19:29 +0200 Subject: [PATCH] Refactor orphan detection and restrict --orphans/--no-orphans to `generate tree` command --- src/generate.rs | 6 +- src/graph.rs | 1 - src/graph/builder.rs | 10 +- src/graph/orphans.rs | 155 ---------------- src/item.rs | 4 + src/lib.rs | 1 - src/options.rs | 8 - src/orphans.rs | 80 -------- src/tree.rs | 1 + src/tree/builder.rs | 22 ++- src/tree/filter.rs | 7 +- src/tree/node.rs | 8 - src/tree/options.rs | 10 +- src/tree/orphans.rs | 172 ++++++++++++++++++ tests/generate_graph.rs | 28 --- .../generate_graph__orphans__smoke.snap | 4 - 16 files changed, 207 insertions(+), 310 deletions(-) delete mode 100644 src/graph/orphans.rs delete mode 100644 src/orphans.rs create mode 100644 src/tree/orphans.rs diff --git a/src/generate.rs b/src/generate.rs index 2c68108c..c188603c 100644 --- a/src/generate.rs +++ b/src/generate.rs @@ -101,7 +101,7 @@ impl Command { #[allow(unused_variables)] Self::Tree(options) => { let builder_options = TreeBuilderOptions { - orphans: options.selection.orphans, + orphans: options.orphans, }; let filter_options = TreeFilterOptions { focus_on: options.selection.focus_on.clone(), @@ -124,9 +124,7 @@ impl Command { } #[allow(unused_variables)] Self::Graph(options) => { - let builder_options = GraphBuilderOptions { - orphans: options.selection.orphans, - }; + let builder_options = GraphBuilderOptions {}; let filter_options = GraphFilterOptions { focus_on: options.selection.focus_on.clone(), max_depth: options.selection.max_depth, diff --git a/src/graph.rs b/src/graph.rs index ff6d387b..be915e15 100644 --- a/src/graph.rs +++ b/src/graph.rs @@ -11,7 +11,6 @@ pub(crate) mod edge; pub(super) mod filter; pub(crate) mod node; pub(crate) mod options; -pub(super) mod orphans; pub(super) mod printer; pub(crate) mod util; pub(super) mod walker; diff --git a/src/graph/builder.rs b/src/graph/builder.rs index f3fa506e..f5f64c6b 100644 --- a/src/graph/builder.rs +++ b/src/graph/builder.rs @@ -19,12 +19,8 @@ use crate::{ item::Item, }; -use super::orphans::add_orphan_nodes_to; - #[derive(Clone, PartialEq, Eq, Debug)] -pub struct Options { - pub orphans: bool, -} +pub struct Options {} #[derive(Debug)] pub struct Builder<'a> { @@ -119,10 +115,6 @@ impl<'a> Builder<'a> { self.add_edge(module_idx, declaration_idx, edge); } - if self.options.orphans { - add_orphan_nodes_to(&mut self.graph, module_idx); - } - self.add_dependencies(module_idx, self.dependencies_of_module(module_hir)); Some(module_idx) diff --git a/src/graph/orphans.rs b/src/graph/orphans.rs deleted file mode 100644 index c9027779..00000000 --- a/src/graph/orphans.rs +++ /dev/null @@ -1,155 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -use std::{ - collections::HashSet, - fs, - path::{Path, PathBuf}, -}; - -use log::{debug, trace}; -use petgraph::{graph::NodeIndex, visit::EdgeRef}; - -use crate::{ - graph::{ - edge::{Edge, EdgeKind}, - node::Node, - Graph, - }, - item::{attr::ItemAttrs, Item}, - orphans::PossibleOrphansIterator, -}; - -pub(crate) fn add_orphan_nodes_to(graph: &mut Graph, module_idx: NodeIndex) { - let module_node = graph[module_idx].clone(); - - trace!("Searching for orphans of {:?}", module_node.item.path); - - let file_path_buf = match &module_node.item.file_path { - Some(file_path) => file_path.clone(), - None => { - debug!("Module {:?} is not at file-level", module_node.item.path); - return; - } - }; - let file_path = file_path_buf.as_path(); - - let dir_path_buf = match mod_dir(file_path) { - Some(path_buf) => path_buf, - None => { - debug!( - "Could not obtain module directory path for {:?}", - module_node.item.path - ); - return; - } - }; - let dir_path = dir_path_buf.as_path(); - - let existing_modules = sub_module_nodes(graph, module_idx); - - let existing_module_names: HashSet = existing_modules - .into_iter() - .map(|module| module.display_name()) - .collect(); - - if !dir_path.exists() { - debug!("Module directory for {:?} not found", module_node.item.path); - return; - } - - let read_dir = match fs::read_dir(dir_path) { - Ok(read_dir) => read_dir, - Err(_) => { - debug!( - "Module directory for {:?} not readable", - module_node.item.path - ); - return; - } - }; - - let mut possible_orphans: Vec<_> = PossibleOrphansIterator::new(read_dir).collect(); - - // Directory traversal can be platform-dependent, so in order to make the output - // uniform we give up some performance and sort the list of possible orphans: - possible_orphans.sort_by(|lhs, rhs| lhs.name.cmp(&rhs.name)); - - for possible_orphan in possible_orphans { - let crate::orphans::PossibleOrphan { name, path } = possible_orphan; - - if existing_module_names.contains(&name) { - continue; - } - - add_orphan_node(graph, module_idx, path.as_path(), &name); - } -} - -fn add_orphan_node( - graph: &mut Graph, - module_idx: NodeIndex, - orphan_file_path: &Path, - orphan_name: &str, -) { - let module_node = &graph[module_idx]; - - let orphan_node = { - let crate_name = module_node.item.crate_name.clone(); - let path = { - let mut path = module_node.item.path.clone(); - path.push(orphan_name.to_owned()); - path - }; - let file_path = Some(orphan_file_path.to_owned()); - let hir = None; - let visibility = None; - let attrs = { - let cfgs = vec![]; - let test = None; - ItemAttrs { cfgs, test } - }; - - let item = Item { - crate_name, - path, - file_path, - hir, - visibility, - attrs, - }; - - Node::new(item) - }; - - let orphan_idx = graph.add_node(orphan_node); - - let edge = Edge { - kind: EdgeKind::Owns, - }; - graph.add_edge(module_idx, orphan_idx, edge); -} - -fn sub_module_nodes(graph: &mut Graph, module_node_idx: NodeIndex) -> Vec { - graph - .edges_directed(module_node_idx, petgraph::Direction::Outgoing) - .map(|edge_ref| { - let child = &graph[edge_ref.target()]; - child.clone() - }) - .collect() -} - -fn mod_dir(file_path: &Path) -> Option { - let file_stem = file_path.file_stem().and_then(|os_str| os_str.to_str()); - let extension = file_path.extension().and_then(|os_str| os_str.to_str()); - - match (file_stem, extension) { - (Some("lib"), Some("rs")) | (Some("main"), Some("rs")) | (Some("mod"), Some("rs")) => { - file_path.parent().map(|p| p.to_path_buf()) - } - (Some(file_stem), Some("rs")) => file_path.parent().map(|p| p.join(file_stem)), - _ => None, - } -} diff --git a/src/item.rs b/src/item.rs index 52a9c4bf..b1aba25b 100644 --- a/src/item.rs +++ b/src/item.rs @@ -152,4 +152,8 @@ impl Item { module.is_crate_root() } + + pub(crate) fn is_file(&self) -> bool { + self.file_path.is_some() + } } diff --git a/src/lib.rs b/src/lib.rs index b4a45000..49b3e858 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -8,7 +8,6 @@ pub mod options; pub(crate) mod graph; pub(crate) mod item; -pub(crate) mod orphans; pub(crate) mod target; pub(crate) mod theme; pub(crate) mod tree; diff --git a/src/options.rs b/src/options.rs index d7f249d3..39f8f675 100644 --- a/src/options.rs +++ b/src/options.rs @@ -72,14 +72,6 @@ pub mod selection { /// Exclude tests (e.g. `#[test] fn …`). [default] #[arg(long = "no-tests", action = ArgAction::SetFalse, overrides_with = "tests")] pub no_tests: (), - - /// Include orphaned modules (i.e. unused files in /src). - #[arg(long = "orphans")] - pub orphans: bool, - - /// Exclude orphaned modules (i.e. unused files in /src). [default] - #[arg(long = "no-orphans", action = ArgAction::SetFalse, overrides_with = "orphans")] - pub no_orphans: (), } } diff --git a/src/orphans.rs b/src/orphans.rs deleted file mode 100644 index ddb6b567..00000000 --- a/src/orphans.rs +++ /dev/null @@ -1,80 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -use std::{fs::ReadDir, path::PathBuf}; - -pub struct PossibleOrphan { - pub name: String, - pub path: PathBuf, -} - -pub struct PossibleOrphansIterator { - read_dir: ReadDir, -} - -impl PossibleOrphansIterator { - pub fn new(read_dir: ReadDir) -> Self { - Self { read_dir } - } - - fn is_possible_identifier(&self, name: &str) -> bool { - name.chars().all(|c| c.is_alphanumeric()) - } -} - -impl Iterator for PossibleOrphansIterator { - type Item = PossibleOrphan; - - fn next(&mut self) -> Option { - while let Some(dir_entry) = self.read_dir.next() { - let entry_path = match dir_entry { - Ok(dir_entry) => dir_entry.path(), - Err(_) => { - // If we can't retrieve the entry, then skip over it: - continue; - } - }; - let file_stem = match entry_path.file_stem().and_then(|os_str| os_str.to_str()) { - Some(name) => name, - None => { - // A rust file should have a file-stem (aka file-name) - // if the file doesn't, then skip over it: - continue; - } - }; - let extension = entry_path.extension().and_then(|os_str| os_str.to_str()); - - // Skip any files with names that could not be valid identifiers: - if !self.is_possible_identifier(file_stem) { - continue; - } - - let ignored_names = ["lib", "main", "mod"]; - let is_ignored_name = ignored_names.contains(&file_stem); - - let path = if entry_path.is_dir() { - // If it's a directory, then there might be a 'mod.rs' file within: - entry_path.join("mod.rs") - } else if extension == Some("rs") && !is_ignored_name { - // If it's a '.rs' file then we already know the path: - entry_path.clone() - } else { - // The directory can contain other arbitrary files - // as such we simply skip over them: - continue; - }; - - // Check if our file guesses actually exist on the file-system - // if they don't, simply skip over them: - if !path.exists() { - continue; - } - - let name = file_stem.to_owned(); - return Some(PossibleOrphan { name, path }); - } - - None - } -} diff --git a/src/tree.rs b/src/tree.rs index 6f4b6388..6185b42b 100644 --- a/src/tree.rs +++ b/src/tree.rs @@ -7,6 +7,7 @@ pub(super) mod command; pub(super) mod filter; pub(crate) mod node; pub(super) mod options; +pub(super) mod orphans; pub(crate) mod printer; #[derive(Clone, PartialEq, Debug)] diff --git a/src/tree/builder.rs b/src/tree/builder.rs index eff39b32..1b25f4cf 100644 --- a/src/tree/builder.rs +++ b/src/tree/builder.rs @@ -4,7 +4,6 @@ use hir::ModuleDef; use log::trace; - use ra_ap_hir::{self as hir, Crate}; use ra_ap_ide_db::RootDatabase; use ra_ap_vfs::Vfs; @@ -14,6 +13,8 @@ use crate::{ tree::{node::Node, Tree}, }; +use super::orphans::orphan_nodes_for; + // use super::orphans::add_orphan_nodes_to; #[derive(Clone, PartialEq, Eq, Debug)] @@ -80,16 +81,23 @@ impl<'a> Builder<'a> { let item = Item::new(ModuleDef::Module(module_hir), self.db, self.vfs); let mut node = Node::new(item, vec![]); - for declaration in module_hir.declarations(self.db) { - let Some(subnode) = self.process_moduledef(declaration) else { - continue; - }; + // eprintln!("node: {:?}", node.item.path); + let subnodes = module_hir + .declarations(self.db) + .into_iter() + .filter_map(|moduledef_hir| self.process_moduledef(moduledef_hir)); + + for subnode in subnodes { + // eprintln!("- subnode: {:?}", subnode.item.path); node.push_subnode(subnode); } - if self.options.orphans { - // add_orphan_nodes_to(&mut self.graph, module_idx); + if self.options.orphans && node.item.is_file() { + for subnode in orphan_nodes_for(&node) { + // eprintln!("- orphan: {:?}", subnode.item.path); + node.push_subnode(subnode); + } } Some(node) diff --git a/src/tree/filter.rs b/src/tree/filter.rs index a3ce786e..fed31f6a 100644 --- a/src/tree/filter.rs +++ b/src/tree/filter.rs @@ -3,7 +3,6 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use hir::HasAttrs; - use ra_ap_hir::{self as hir}; use ra_ap_ide_db::RootDatabase; use ra_ap_syntax::ast; @@ -80,7 +79,7 @@ impl<'a> Filter<'a> { let subnode_contains_focus_node = node .subnodes .iter() - .map(|node| self.is_or_contains_focus_node(node, focus_tree)); + .map(|node| Self::is_or_contains_focus_node(node, focus_tree)); let is_or_contains_focus_node = is_focus_node || subnode_contains_focus_node.clone().any(|flag| flag); @@ -117,14 +116,14 @@ impl<'a> Filter<'a> { Some(node) } - fn is_or_contains_focus_node(&self, node: &Node, focus_tree: &ast::UseTree) -> bool { + fn is_or_contains_focus_node(node: &Node, focus_tree: &ast::UseTree) -> bool { if util::use_tree_matches_item(focus_tree, &node.item) { return true; } node.subnodes .iter() - .any(|node| self.is_or_contains_focus_node(node, focus_tree)) + .any(|node| Self::is_or_contains_focus_node(node, focus_tree)) } fn should_retain_moduledef(&self, moduledef_hir: hir::ModuleDef) -> bool { diff --git a/src/tree/node.rs b/src/tree/node.rs index a58f0779..2564dfd2 100644 --- a/src/tree/node.rs +++ b/src/tree/node.rs @@ -27,14 +27,6 @@ impl Node { self.item.display_name() } - pub fn display_path(&self) -> String { - self.item.display_path() - } - - pub fn crate_display_name(&self) -> String { - self.item.crate_display_name() - } - pub fn kind_display_name(&self, db: &RootDatabase) -> Option { self.item.kind_display_name(db) } diff --git a/src/tree/options.rs b/src/tree/options.rs index 060aead8..e9f41740 100644 --- a/src/tree/options.rs +++ b/src/tree/options.rs @@ -2,7 +2,7 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -use clap::Parser; +use clap::{ArgAction, Parser}; use crate::options; @@ -17,4 +17,12 @@ pub struct Options { #[command(flatten)] pub selection: options::selection::Options, + + /// Include orphaned modules (i.e. unused files in /src). + #[arg(long = "orphans")] + pub orphans: bool, + + /// Exclude orphaned modules (i.e. unused files in /src). [default] + #[arg(long = "no-orphans", action = ArgAction::SetFalse, overrides_with = "orphans")] + pub no_orphans: (), } diff --git a/src/tree/orphans.rs b/src/tree/orphans.rs new file mode 100644 index 00000000..038c59e0 --- /dev/null +++ b/src/tree/orphans.rs @@ -0,0 +1,172 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +use std::{ + collections::HashSet, + fs::{self, ReadDir}, + path::{Path, PathBuf}, +}; + +use log::{debug, trace}; +use ra_ap_hir as hir; + +use crate::{ + item::{attr::ItemAttrs, Item}, + tree::node::Node, +}; + +pub(crate) fn orphan_nodes_for(node: &Node) -> Vec { + assert!(node.item.is_file()); + assert!(matches!(node.item.hir, Some(hir::ModuleDef::Module(_)))); + + trace!("Searching for orphans of {:?}", node.item.path); + + let file_path: &Path = node.item.file_path.as_ref().expect("file-level module"); + + let dir_path_buf = match mod_dir(file_path) { + Some(path_buf) => path_buf, + None => { + debug!( + "Could not obtain module directory path for {:?}", + node.item.path + ); + return vec![]; + } + }; + let dir_path = dir_path_buf.as_path(); + + if !dir_path.exists() { + debug!("Module directory for {:?} not found", node.item.path); + return vec![]; + } + + let read_dir = match fs::read_dir(dir_path) { + Ok(read_dir) => read_dir, + Err(_) => { + debug!("Module directory for {:?} not readable", node.item.path); + return vec![]; + } + }; + + let crate_name = node.item.crate_name.clone(); + let parent_path = node.item.path.clone(); + + let existing_submodule_names: HashSet = node + .subnodes + .iter() + .map(|node| node.display_name()) + .collect(); + + possible_orphans_in(read_dir) + .filter_map(|possible_orphan| { + if existing_submodule_names.contains(&possible_orphan.name) { + return None; + } + + let mut path = parent_path.clone(); + path.push(possible_orphan.name); + + Some(orphan_node( + crate_name.clone(), + path, + possible_orphan.file_path, + )) + }) + .collect() +} + +fn orphan_node(crate_name: Option, path: Vec, file_path: PathBuf) -> Node { + let file_path = Some(file_path); + let hir = None; + let visibility = None; + let attrs = { + let cfgs = vec![]; + let test = None; + ItemAttrs { cfgs, test } + }; + + let item = Item { + crate_name, + path, + file_path, + hir, + visibility, + attrs, + }; + + Node::new(item, vec![]) +} + +fn mod_dir(file_path: &Path) -> Option { + let file_stem = file_path.file_stem().and_then(|os_str| os_str.to_str()); + let extension = file_path.extension().and_then(|os_str| os_str.to_str()); + + match (file_stem, extension) { + (Some("lib"), Some("rs")) | (Some("main"), Some("rs")) | (Some("mod"), Some("rs")) => { + file_path.parent().map(|p| p.to_path_buf()) + } + (Some(file_stem), Some("rs")) => file_path.parent().map(|p| p.join(file_stem)), + _ => None, + } +} + +pub struct PossibleOrphan { + pub name: String, + pub file_path: PathBuf, +} + +fn possible_orphans_in(read_dir: ReadDir) -> impl Iterator { + fn is_possible_identifier(name: &str) -> bool { + name.chars().all(|c| c.is_alphanumeric()) + } + + read_dir.into_iter().filter_map(|dir_entry| { + let entry_path = match dir_entry { + Ok(dir_entry) => dir_entry.path(), + Err(_) => { + // If we can't retrieve the entry, then skip over it: + return None; + } + }; + let file_stem = match entry_path.file_stem().and_then(|os_str| os_str.to_str()) { + Some(name) => name, + None => { + // A rust file should have a file-stem (aka file-name) + // if the file doesn't, then skip over it: + return None; + } + }; + let extension = entry_path.extension().and_then(|os_str| os_str.to_str()); + + // Skip any files with names that could not be valid identifiers: + if !is_possible_identifier(file_stem) { + return None; + } + + let ignored_names = ["lib", "main", "mod"]; + let is_ignored_name = ignored_names.contains(&file_stem); + + let file_path = if entry_path.is_dir() { + // If it's a directory, then there might be a 'mod.rs' file within: + entry_path.join("mod.rs") + } else if extension == Some("rs") && !is_ignored_name { + // If it's a '.rs' file then we already know the path: + entry_path.clone() + } else { + // The directory can contain other arbitrary files + // as such we simply skip over them: + return None; + }; + + // Check if our file guesses actually exist on the file-system + // if they don't, simply skip over them: + if !file_path.exists() { + return None; + } + + let name = file_stem.to_owned(); + + Some(PossibleOrphan { name, file_path }) + }) +} diff --git a/tests/generate_graph.rs b/tests/generate_graph.rs index 5907af9b..0e8a14b1 100644 --- a/tests/generate_graph.rs +++ b/tests/generate_graph.rs @@ -155,24 +155,6 @@ mod negative_args { } } - #[test] - fn orphans() { - for command in ["tree", "graph"] { - for (args, expected) in args_for(command, "orphans", false) { - let app = App::parse_from(&args); - - let Generate { - cmd: Command::Graph(cmd), - } = app.command - else { - continue; - }; - - assert_eq!(cmd.selection.orphans, expected, "{:?}", args); - } - } - } - #[test] fn tests() { for command in ["tree", "graph"] { @@ -491,16 +473,6 @@ mod package_bin { } } -mod orphans { - test_cmd!( - args: "generate graph \ - --orphans", - success: true, - color_mode: ColorMode::Plain, - project: smoke - ); -} - mod tests { test_cmd!( args: "generate graph \ diff --git a/tests/snapshots/generate_graph__orphans__smoke.snap b/tests/snapshots/generate_graph__orphans__smoke.snap index 9ad9812f..ef65a550 100644 --- a/tests/snapshots/generate_graph__orphans__smoke.snap +++ b/tests/snapshots/generate_graph__orphans__smoke.snap @@ -47,8 +47,6 @@ digraph { "smoke::hierarchy::lorem::dolor::sit::amet" [label="pub(self) mod|hierarchy::lorem::dolor::sit::amet", fillcolor="#db5367"]; // "mod" node "smoke::hierarchy::lorem::ipsum" [label="pub(self) mod|hierarchy::lorem::ipsum", fillcolor="#db5367"]; // "mod" node "smoke::orphans" [label="pub(crate) mod|orphans", fillcolor="#f8c04c"]; // "mod" node - "smoke::orphans::bar" [label="orphan mod|orphans::bar", fillcolor="#ba6fa7"]; // "orphan" node - "smoke::orphans::foo" [label="orphan mod|orphans::foo", fillcolor="#ba6fa7"]; // "orphan" node "smoke::uses" [label="pub(crate) mod|uses", fillcolor="#f8c04c"]; // "mod" node "smoke::uses::cycle" [label="pub(self) mod|uses::cycle", fillcolor="#db5367"]; // "mod" node "smoke::uses::cycle::node_0" [label="pub(self) mod|uses::cycle::node_0", fillcolor="#db5367"]; // "mod" node @@ -82,8 +80,6 @@ digraph { "smoke::hierarchy::lorem::consectetur::adipiscing" -> "smoke::hierarchy::lorem::consectetur::adipiscing::elit" [label="owns", color="#000000", style="solid"] [constraint=true]; // "owns" edge "smoke::hierarchy::lorem::dolor" -> "smoke::hierarchy::lorem::dolor::sit" [label="owns", color="#000000", style="solid"] [constraint=true]; // "owns" edge "smoke::hierarchy::lorem::dolor::sit" -> "smoke::hierarchy::lorem::dolor::sit::amet" [label="owns", color="#000000", style="solid"] [constraint=true]; // "owns" edge - "smoke::orphans" -> "smoke::orphans::bar" [label="owns", color="#000000", style="solid"] [constraint=true]; // "owns" edge - "smoke::orphans" -> "smoke::orphans::foo" [label="owns", color="#000000", style="solid"] [constraint=true]; // "owns" edge "smoke::uses" -> "smoke::uses::cycle" [label="owns", color="#000000", style="solid"] [constraint=true]; // "owns" edge "smoke::uses::cycle" -> "smoke::uses::cycle::node_0" [label="owns", color="#000000", style="solid"] [constraint=true]; // "owns" edge "smoke::uses::cycle" -> "smoke::uses::cycle::node_1" [label="owns", color="#000000", style="solid"] [constraint=true]; // "owns" edge