From 15301924204d10f76e87c6d16d8ee2ef7b36c990 Mon Sep 17 00:00:00 2001 From: anderejd Date: Thu, 10 Jan 2019 20:59:56 +0100 Subject: [PATCH 1/7] Replaced a glob import. --- src/lib.rs | 5 +++++ src/main.rs | 15 ++++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index f5560603..19d57ac1 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -260,6 +260,11 @@ impl<'ast> visit::Visit<'ast> for GeigerSynVisitor { .count(i.sig.unsafety.is_some(), self.used_by_build); visit::visit_impl_item_method(self, i); } + + // TODO: Visit macros. + // + // TODO: Figure out if there are other visit methods that should be + // implemented here. } pub fn is_file_with_ext(entry: &DirEntry, file_ext: &str) -> bool { diff --git a/src/main.rs b/src/main.rs index f86dfd38..761e4061 100644 --- a/src/main.rs +++ b/src/main.rs @@ -11,7 +11,20 @@ use cargo::core::shell::Verbosity; use cargo::ops::CompileOptions; use cargo::{CliResult, Config}; use cargo_geiger::format::Pattern; -use cargo_geiger::*; +use cargo_geiger::resolve; +use cargo_geiger::print_tree; +use cargo_geiger::PrintConfig; +use cargo_geiger::UNSAFE_COUNTERS_HEADER; +use cargo_geiger::resolve_rs_file_deps; +use cargo_geiger::UTF8_SYMBOLS; +use cargo_geiger::ASCII_SYMBOLS; +use cargo_geiger::build_graph; +use cargo_geiger::Charset; +use cargo_geiger::workspace; +use cargo_geiger::registry; +use cargo_geiger::get_cfgs; +use cargo_geiger::Prefix; +use cargo_geiger::IncludeTests; use colored::*; use petgraph::EdgeDirection; use std::path::PathBuf; From d9a2eac0decf37783bea0eb587383bf23ecb4fae Mon Sep 17 00:00:00 2001 From: anderejd Date: Thu, 10 Jan 2019 21:00:36 +0100 Subject: [PATCH 2/7] cargo fmt --- src/main.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/main.rs b/src/main.rs index 761e4061..62ae102f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -10,21 +10,21 @@ use cargo::core::shell::Shell; use cargo::core::shell::Verbosity; use cargo::ops::CompileOptions; use cargo::{CliResult, Config}; +use cargo_geiger::build_graph; use cargo_geiger::format::Pattern; -use cargo_geiger::resolve; +use cargo_geiger::get_cfgs; use cargo_geiger::print_tree; -use cargo_geiger::PrintConfig; -use cargo_geiger::UNSAFE_COUNTERS_HEADER; +use cargo_geiger::registry; +use cargo_geiger::resolve; use cargo_geiger::resolve_rs_file_deps; -use cargo_geiger::UTF8_SYMBOLS; -use cargo_geiger::ASCII_SYMBOLS; -use cargo_geiger::build_graph; -use cargo_geiger::Charset; use cargo_geiger::workspace; -use cargo_geiger::registry; -use cargo_geiger::get_cfgs; -use cargo_geiger::Prefix; +use cargo_geiger::Charset; use cargo_geiger::IncludeTests; +use cargo_geiger::Prefix; +use cargo_geiger::PrintConfig; +use cargo_geiger::ASCII_SYMBOLS; +use cargo_geiger::UNSAFE_COUNTERS_HEADER; +use cargo_geiger::UTF8_SYMBOLS; use colored::*; use petgraph::EdgeDirection; use std::path::PathBuf; From 9d405c984efd900fc21e5a605c8bb1a2bce0a6e1 Mon Sep 17 00:00:00 2001 From: anderejd Date: Thu, 10 Jan 2019 21:06:31 +0100 Subject: [PATCH 3/7] Replaced some more glob imports. --- src/lib.rs | 2 +- src/main.rs | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 19d57ac1..1694f27f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -32,7 +32,7 @@ use cargo::util::paths; use cargo::util::ProcessBuilder; use cargo::util::{self, important_paths, CargoResult, Cfg}; use cargo::Config; -use colored::*; +use colored::Colorize; use petgraph::graph::NodeIndex; use petgraph::visit::EdgeRef; use petgraph::EdgeDirection; diff --git a/src/main.rs b/src/main.rs index 62ae102f..3ea269bb 100644 --- a/src/main.rs +++ b/src/main.rs @@ -9,7 +9,8 @@ use cargo::core::resolver::Method; use cargo::core::shell::Shell; use cargo::core::shell::Verbosity; use cargo::ops::CompileOptions; -use cargo::{CliResult, Config}; +use cargo::CliResult; +use cargo::Config; use cargo_geiger::build_graph; use cargo_geiger::format::Pattern; use cargo_geiger::get_cfgs; @@ -25,7 +26,7 @@ use cargo_geiger::PrintConfig; use cargo_geiger::ASCII_SYMBOLS; use cargo_geiger::UNSAFE_COUNTERS_HEADER; use cargo_geiger::UTF8_SYMBOLS; -use colored::*; +use colored::Colorize; use petgraph::EdgeDirection; use std::path::PathBuf; use structopt::clap::AppSettings; From 88c77f4ee800103f3e44b4dc7f6fb8fdac5fec17 Mon Sep 17 00:00:00 2001 From: anderejd Date: Thu, 10 Jan 2019 22:51:04 +0100 Subject: [PATCH 4/7] Removed some repetition. --- src/lib.rs | 45 +++++++++++++++++---------------------------- 1 file changed, 17 insertions(+), 28 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 1694f27f..7b902e98 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -908,38 +908,27 @@ pub fn print_dependency<'a>( Kind::Development => development.push(dep), } } - print_dependency_kind( - Kind::Normal, - normal, - graph, - visited_deps, - levels_continue, - rs_files_used, - pc, - ); - print_dependency_kind( - Kind::Build, - build, - graph, - visited_deps, - levels_continue, - rs_files_used, - pc, - ); - print_dependency_kind( - Kind::Development, - development, - graph, - visited_deps, - levels_continue, - rs_files_used, - pc, - ); + let mut kinds = [ + (Kind::Normal, normal), + (Kind::Build, build), + (Kind::Development, development), + ]; + for (kind, kind_deps) in kinds.iter_mut() { + print_dependency_kind( + *kind, + kind_deps, + graph, + visited_deps, + levels_continue, + rs_files_used, + pc, + ); + } } pub fn print_dependency_kind<'a>( kind: Kind, - mut deps: Vec<&Node<'a>>, + deps: &mut Vec<&Node<'a>>, graph: &Graph<'a>, visited_deps: &mut HashSet<&'a PackageId>, levels_continue: &mut Vec, From 60db1cf175e7651b3464d07314122b7d95748315 Mon Sep 17 00:00:00 2001 From: anderejd Date: Wed, 23 Jan 2019 00:18:36 +0100 Subject: [PATCH 5/7] Print and scan separation. --- src/lib.rs | 96 ++++++++++++++++++++++++++++++++++------------------- src/main.rs | 56 +++++++++++++++++++------------ 2 files changed, 97 insertions(+), 55 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 7b902e98..1390516b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -282,26 +282,32 @@ pub fn is_file_with_ext(entry: &DirEntry, file_ext: &str) -> bool { ext.to_string_lossy() == file_ext } -pub fn find_unsafe( - p: &Path, +fn find_rs_files_in_dir(dir: &Path) -> impl Iterator { + let walker = WalkDir::new(dir).into_iter(); + walker.filter_map(|entry| { + let entry = + entry.expect("walkdir error."); // TODO: Return result. + if !is_file_with_ext(&entry, "rs") { + return None; + } + Some(entry + .path() + .canonicalize() + .expect("Error converting to canonical path")) // TODO: Return result. + }) +} + +fn find_unsafe( + dir: &Path, rs_files_used: &mut HashMap, allow_partial_results: bool, include_tests: IncludeTests, verbosity: Verbosity, ) -> CounterBlock { let mut vis = GeigerSynVisitor::new(include_tests, verbosity); - let walker = WalkDir::new(p).into_iter(); - for entry in walker { - let entry = - entry.expect("walkdir error, TODO: Implement error handling"); - if !is_file_with_ext(&entry, "rs") { - continue; - } - let pb = entry - .path() - .canonicalize() - .expect("Error converting to canonical path"); - let p = pb.as_path(); + let file_paths = find_rs_files_in_dir(dir); + for p in file_paths { + let p = p.as_path(); let scan_counter = rs_files_used.get_mut(p); vis.used_by_build = match scan_counter { Some(c) => { @@ -446,12 +452,16 @@ impl From> for RsResolveError { } } +/// Trigger a `cargo clean` + `cargo check` and listen to the cargo/rustc +/// communication to figure out which source files were used by the build. pub fn resolve_rs_file_deps( copt: &CompileOptions, ws: &Workspace, ) -> Result, RsResolveError> { let config = ws.config(); // Need to run a cargo clean to identify all new .d deps files. + // TODO: Figure out how this can be avoided to improve performance, clean + // Rust builds are __slow__. let clean_opt = CleanOptions { config: &config, spec: vec![], @@ -718,16 +728,13 @@ pub fn resolve<'a, 'cfg>( ) -> CargoResult<(PackageSet<'a>, Resolve)> { let features = Method::split_features(&features.into_iter().collect::>()); - let (packages, resolve) = ops::resolve_ws(ws)?; - let method = Method::Required { dev_deps: true, features: &features, all_features, uses_default_features: !no_default_features, }; - let resolve = ops::resolve_with_previous( registry, ws, @@ -811,21 +818,50 @@ pub fn build_graph<'a>( Ok(graph) } +pub fn find_unsafe_recursive( + packs: &PackageSet, + mut rs_files_used: HashMap, + allow_partial_results: bool, + include_tests: IncludeTests, + verbosity: Verbosity) +-> GeigerContext { + let mut pack_id_to_counters = HashMap::new(); + let pack_ids = packs.package_ids().cloned().collect::>(); + for id in pack_ids { + let pack = packs.get_one(&id).unwrap(); // FIXME + let counters = find_unsafe( + pack.root(), + &mut rs_files_used, + allow_partial_results, + include_tests, + verbosity, + ); + pack_id_to_counters.insert(id, counters); + } + GeigerContext { pack_id_to_counters, rs_files_used } +} + +/// TODO: Write documentation. +pub struct GeigerContext { + pub pack_id_to_counters: HashMap, + pub rs_files_used: HashMap, +} + pub fn print_tree<'a>( - package: &'a PackageId, + root_pack_id: &'a PackageId, graph: &Graph<'a>, - rs_files_used: &mut HashMap, + geiger_ctx: &GeigerContext, pc: &PrintConfig, ) { let mut visited_deps = HashSet::new(); let mut levels_continue = vec![]; - let node = &graph.graph[graph.nodes[&package]]; + let node = &graph.graph[graph.nodes[&root_pack_id]]; print_dependency( node, &graph, &mut visited_deps, &mut levels_continue, - rs_files_used, + geiger_ctx, pc, ); } @@ -835,7 +871,7 @@ pub fn print_dependency<'a>( graph: &Graph<'a>, visited_deps: &mut HashSet<&'a PackageId>, levels_continue: &mut Vec, - rs_files_used: &mut HashMap, + geiger_ctx: &GeigerContext, pc: &PrintConfig, ) { let new = pc.all || visited_deps.insert(package.id); @@ -861,15 +897,7 @@ pub fn print_dependency<'a>( Prefix::None => "".into(), }; - // TODO: Find and collect unsafe stats as a separate pass over the deps - // tree before printing. - let counters = find_unsafe( - package.pack.root(), - rs_files_used, - pc.allow_partial_results, - pc.include_tests, - pc.verbosity, - ); + let counters = geiger_ctx.pack_id_to_counters.get(package.id).unwrap(); // TODO: Proper error handling. let unsafe_found = counters.has_unsafe(); let colorize = |s: String| { if unsafe_found { @@ -920,7 +948,7 @@ pub fn print_dependency<'a>( graph, visited_deps, levels_continue, - rs_files_used, + geiger_ctx, pc, ); } @@ -932,7 +960,7 @@ pub fn print_dependency_kind<'a>( graph: &Graph<'a>, visited_deps: &mut HashSet<&'a PackageId>, levels_continue: &mut Vec, - rs_files_used: &mut HashMap, + geiger_ctx: &GeigerContext, pc: &PrintConfig, ) { if deps.is_empty() { @@ -967,7 +995,7 @@ pub fn print_dependency_kind<'a>( graph, visited_deps, levels_continue, - rs_files_used, + geiger_ctx, pc, ); levels_continue.pop(); diff --git a/src/main.rs b/src/main.rs index 3ea269bb..40897509 100644 --- a/src/main.rs +++ b/src/main.rs @@ -15,6 +15,7 @@ use cargo_geiger::build_graph; use cargo_geiger::format::Pattern; use cargo_geiger::get_cfgs; use cargo_geiger::print_tree; +use cargo_geiger::find_unsafe_recursive; use cargo_geiger::registry; use cargo_geiger::resolve; use cargo_geiger::resolve_rs_file_deps; @@ -220,7 +221,7 @@ fn real_main(args: &Args, config: &mut Config) -> CliResult { let ids = packages.package_ids().cloned().collect::>(); let packages = registry.get(&ids)?; - let root = match args.package { + let root_pack_id = match args.package { Some(ref pkg) => resolve.query(pkg)?, None => package.package_id(), }; @@ -264,7 +265,7 @@ fn real_main(args: &Args, config: &mut Config) -> CliResult { }; let copt = build_compile_options(args, config); - let mut rs_files_used = resolve_rs_file_deps(&copt, &ws).unwrap(); + let rs_files_used = resolve_rs_file_deps(&copt, &ws).unwrap(); if verbosity == Verbosity::Verbose { // Print all .rs files found through the .d files, in sorted order. @@ -278,22 +279,6 @@ fn real_main(args: &Args, config: &mut Config) -> CliResult { .for_each(|p| println!("Used by build (sorted): {}", p.display())); } - println!(); - println!("Metric output format: x/y"); - println!("x = unsafe code used by the build"); - println!("y = total unsafe code found in the crate"); - println!(); - println!( - "{}", - UNSAFE_COUNTERS_HEADER - .iter() - .map(|s| s.to_owned()) - .collect::>() - .join(" ") - .bold() - ); - println!(); - // TODO: Add command line flag for this and make it default to false? let allow_partial_results = true; @@ -312,16 +297,45 @@ fn real_main(args: &Args, config: &mut Config) -> CliResult { allow_partial_results, include_tests, }; - print_tree(root, &graph, &mut rs_files_used, &pc); - rs_files_used + + eprintln!("Scanning..."); + let geiger_ctx = find_unsafe_recursive( + &packages, + rs_files_used, + pc.allow_partial_results, + pc.include_tests, + pc.verbosity); + eprintln!("Scanning...Done."); + + println!(); + println!("Metric output format: x/y"); + println!("x = unsafe code used by the build"); + println!("y = total unsafe code found in the crate"); + println!(); + println!( + "{}", + UNSAFE_COUNTERS_HEADER + .iter() + .map(|s| s.to_owned()) + .collect::>() + .join(" ") + .bold() + ); + println!(); + print_tree(root_pack_id, &graph, &geiger_ctx, &pc); + + geiger_ctx.rs_files_used .iter() .filter(|(_k, v)| **v == 0) .for_each(|(k, _v)| { - println!( + // This is likely a bug in cargo-geiger if it happens. Fail hard and + // exit instead? + eprintln!( "WARNING: Dependency file was never scanned: {}", k.display() ) }); + Ok(()) } From 8a472b75f00a93ef188b0ed75f43acec27ec59bc Mon Sep 17 00:00:00 2001 From: anderejd Date: Wed, 23 Jan 2019 00:21:36 +0100 Subject: [PATCH 6/7] cargo fmt --- src/lib.rs | 24 ++++++++++++++---------- src/main.rs | 16 +++++++++------- 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 1390516b..71999de3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -282,18 +282,19 @@ pub fn is_file_with_ext(entry: &DirEntry, file_ext: &str) -> bool { ext.to_string_lossy() == file_ext } -fn find_rs_files_in_dir(dir: &Path) -> impl Iterator { +fn find_rs_files_in_dir(dir: &Path) -> impl Iterator { let walker = WalkDir::new(dir).into_iter(); walker.filter_map(|entry| { - let entry = - entry.expect("walkdir error."); // TODO: Return result. + let entry = entry.expect("walkdir error."); // TODO: Return result. if !is_file_with_ext(&entry, "rs") { return None; } - Some(entry - .path() - .canonicalize() - .expect("Error converting to canonical path")) // TODO: Return result. + Some( + entry + .path() + .canonicalize() + .expect("Error converting to canonical path"), + ) // TODO: Return result. }) } @@ -823,8 +824,8 @@ pub fn find_unsafe_recursive( mut rs_files_used: HashMap, allow_partial_results: bool, include_tests: IncludeTests, - verbosity: Verbosity) --> GeigerContext { + verbosity: Verbosity, +) -> GeigerContext { let mut pack_id_to_counters = HashMap::new(); let pack_ids = packs.package_ids().cloned().collect::>(); for id in pack_ids { @@ -838,7 +839,10 @@ pub fn find_unsafe_recursive( ); pack_id_to_counters.insert(id, counters); } - GeigerContext { pack_id_to_counters, rs_files_used } + GeigerContext { + pack_id_to_counters, + rs_files_used, + } } /// TODO: Write documentation. diff --git a/src/main.rs b/src/main.rs index 40897509..3c38931d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -12,10 +12,10 @@ use cargo::ops::CompileOptions; use cargo::CliResult; use cargo::Config; use cargo_geiger::build_graph; +use cargo_geiger::find_unsafe_recursive; use cargo_geiger::format::Pattern; use cargo_geiger::get_cfgs; use cargo_geiger::print_tree; -use cargo_geiger::find_unsafe_recursive; use cargo_geiger::registry; use cargo_geiger::resolve; use cargo_geiger::resolve_rs_file_deps; @@ -300,11 +300,12 @@ fn real_main(args: &Args, config: &mut Config) -> CliResult { eprintln!("Scanning..."); let geiger_ctx = find_unsafe_recursive( - &packages, - rs_files_used, - pc.allow_partial_results, - pc.include_tests, - pc.verbosity); + &packages, + rs_files_used, + pc.allow_partial_results, + pc.include_tests, + pc.verbosity, + ); eprintln!("Scanning...Done."); println!(); @@ -324,7 +325,8 @@ fn real_main(args: &Args, config: &mut Config) -> CliResult { println!(); print_tree(root_pack_id, &graph, &geiger_ctx, &pc); - geiger_ctx.rs_files_used + geiger_ctx + .rs_files_used .iter() .filter(|(_k, v)| **v == 0) .for_each(|(k, _v)| { From bd43d71d52576700619a03533484b3eac9a7abd2 Mon Sep 17 00:00:00 2001 From: anderejd Date: Wed, 23 Jan 2019 00:26:16 +0100 Subject: [PATCH 7/7] Changed a confusing function name. --- src/lib.rs | 2 +- src/main.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 71999de3..dcb05d6a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -819,7 +819,7 @@ pub fn build_graph<'a>( Ok(graph) } -pub fn find_unsafe_recursive( +pub fn find_unsafe_in_packages( packs: &PackageSet, mut rs_files_used: HashMap, allow_partial_results: bool, diff --git a/src/main.rs b/src/main.rs index 3c38931d..dd66d426 100644 --- a/src/main.rs +++ b/src/main.rs @@ -12,7 +12,7 @@ use cargo::ops::CompileOptions; use cargo::CliResult; use cargo::Config; use cargo_geiger::build_graph; -use cargo_geiger::find_unsafe_recursive; +use cargo_geiger::find_unsafe_in_packages; use cargo_geiger::format::Pattern; use cargo_geiger::get_cfgs; use cargo_geiger::print_tree; @@ -299,7 +299,7 @@ fn real_main(args: &Args, config: &mut Config) -> CliResult { }; eprintln!("Scanning..."); - let geiger_ctx = find_unsafe_recursive( + let geiger_ctx = find_unsafe_in_packages( &packages, rs_files_used, pc.allow_partial_results,