diff --git a/Cargo.lock b/Cargo.lock index 21ba8c02..91d36336 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -569,7 +569,9 @@ version = "0.4.5" dependencies = [ "cargo-geiger-serde", "proc-macro2", + "rstest", "syn", + "tempfile", ] [[package]] diff --git a/cargo-geiger/src/scan/find.rs b/cargo-geiger/src/scan/find.rs index bad189c2..8222f615 100644 --- a/cargo-geiger/src/scan/find.rs +++ b/cargo-geiger/src/scan/find.rs @@ -11,7 +11,8 @@ use super::{GeigerContext, ScanMode}; use cargo::util::CargoResult; use cargo::{CliError, Config}; use cargo_metadata::PackageId; -use geiger::{find_unsafe_in_file, IncludeTests, RsFileMetrics, ScanFileError}; +use geiger::find::find_unsafe_in_file; +use geiger::{IncludeTests, RsFileMetrics, ScanFileError}; use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; diff --git a/geiger/Cargo.toml b/geiger/Cargo.toml index abaf3159..8c2a4d06 100644 --- a/geiger/Cargo.toml +++ b/geiger/Cargo.toml @@ -17,3 +17,7 @@ maintenance = { status = "experimental" } cargo-geiger-serde = { path = "../cargo-geiger-serde", version = "0.1.0" } syn = { version = "1.0.34", features = ["parsing", "printing", "clone-impls", "full", "extra-traits", "visit"] } proc-macro2 = "1.0.18" + +[dev-dependencies] +rstest = "0.6.4" +tempfile = "3.1.0" diff --git a/geiger/src/find.rs b/geiger/src/find.rs new file mode 100644 index 00000000..4e85635d --- /dev/null +++ b/geiger/src/find.rs @@ -0,0 +1,222 @@ +use super::{IncludeTests, RsFileMetrics, ScanFileError}; + +use crate::geiger_syn_visitor::GeigerSynVisitor; + +use std::fs::File; +use std::io::Read; +use std::path::Path; + +/// Scan a single file for `unsafe` usage. +pub fn find_unsafe_in_file( + path: &Path, + include_tests: IncludeTests, +) -> Result { + let mut file = File::open(path) + .map_err(|e| ScanFileError::Io(e, path.to_path_buf()))?; + let mut src = vec![]; + file.read_to_end(&mut src) + .map_err(|e| ScanFileError::Io(e, path.to_path_buf()))?; + let src = String::from_utf8(src) + .map_err(|e| ScanFileError::Utf8(e, path.to_path_buf()))?; + find_unsafe_in_string(&src, include_tests) + .map_err(|e| ScanFileError::Syn(e, path.to_path_buf())) +} + +pub fn find_unsafe_in_string( + src: &str, + include_tests: IncludeTests, +) -> Result { + use syn::visit::Visit; + let syntax = syn::parse_file(&src)?; + let mut vis = GeigerSynVisitor::new(include_tests); + vis.visit_file(&syntax); + Ok(vis.metrics) +} + +#[cfg(test)] +mod find_tests { + use super::*; + + use cargo_geiger_serde::{Count, CounterBlock}; + use rstest::*; + use std::io::Write; + use tempfile::tempdir; + + const FILE_CONTENT_STRING: &str = "use std::io::Write; + +pub unsafe fn f() { + unimplemented!() +} + +pub fn g() { + std::io::stdout().write_all(unsafe { + std::str::from_utf8_unchecked(b\"binarystring\") + }.as_bytes()).unwrap(); +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_1() { + unsafe { + println!(\"Inside unsafe\"); + } + } +} +"; + + #[rstest( + input_include_tests, + expected_rs_file_metrics, + case( + IncludeTests::Yes, + RsFileMetrics { + counters: CounterBlock { + functions: Count { + safe: 2, + unsafe_: 1 + }, + exprs: Count { + safe: 4, + unsafe_: 3 + }, + item_impls: Count { + safe: 0, + unsafe_: 0 + }, + item_traits: Count { + safe: 0, + unsafe_: 0 + }, + methods: Count { + safe: 0, + unsafe_: 0 + } + }, + forbids_unsafe: false + } + ), + case( + IncludeTests::No, + RsFileMetrics { + counters: CounterBlock { + functions: Count { + safe: 1, + unsafe_: 1 + }, + exprs: Count { + safe: 4, + unsafe_: 2 + }, + item_impls: Count { + safe: 0, + unsafe_: 0 + }, + item_traits: Count { + safe: 0, + unsafe_: 0 + }, + methods: Count { + safe: 0, + unsafe_: 0 + } + }, + forbids_unsafe: false + } + ) + )] + fn find_unsafe_in_file_test_no_errors( + input_include_tests: IncludeTests, + expected_rs_file_metrics: RsFileMetrics, + ) { + let temp_dir = tempdir().unwrap(); + let lib_file_path = temp_dir.path().join("lib.rs"); + let mut file = File::create(lib_file_path.clone()).unwrap(); + + writeln!(file, "{}", FILE_CONTENT_STRING).unwrap(); + + let unsafe_in_file_result = + find_unsafe_in_file(&lib_file_path, input_include_tests); + + assert!(unsafe_in_file_result.is_ok()); + + let unsafe_in_file = unsafe_in_file_result.unwrap(); + + assert_eq!(unsafe_in_file, expected_rs_file_metrics); + } + + #[rstest( + input_include_tests, + expected_rs_file_metrics, + case( + IncludeTests::Yes, + RsFileMetrics { + counters: CounterBlock { + functions: Count { + safe: 2, + unsafe_: 1 + }, + exprs: Count { + safe: 4, + unsafe_: 3 + }, + item_impls: Count { + safe: 0, + unsafe_: 0 + }, + item_traits: Count { + safe: 0, + unsafe_: 0 + }, + methods: Count { + safe: 0, + unsafe_: 0 + } + }, + forbids_unsafe: false + } + ), + case( + IncludeTests::No, + RsFileMetrics { + counters: CounterBlock { + functions: Count { + safe: 1, + unsafe_: 1 + }, + exprs: Count { + safe: 4, + unsafe_: 2 + }, + item_impls: Count { + safe: 0, + unsafe_: 0 + }, + item_traits: Count { + safe: 0, + unsafe_: 0 + }, + methods: Count { + safe: 0, + unsafe_: 0 + } + }, + forbids_unsafe: false + } + ) + )] + fn find_unsafe_in_string_test( + input_include_tests: IncludeTests, + expected_rs_file_metrics: RsFileMetrics, + ) { + let unsafe_in_string_result = + find_unsafe_in_string(FILE_CONTENT_STRING, input_include_tests); + + assert!(unsafe_in_string_result.is_ok()); + let unsafe_in_string = unsafe_in_string_result.unwrap(); + + assert_eq!(unsafe_in_string, expected_rs_file_metrics); + } +} diff --git a/geiger/src/geiger_syn_visitor.rs b/geiger/src/geiger_syn_visitor.rs new file mode 100644 index 00000000..e2ebb254 --- /dev/null +++ b/geiger/src/geiger_syn_visitor.rs @@ -0,0 +1,130 @@ +use super::{ + file_forbids_unsafe, is_test_fn, is_test_mod, IncludeTests, RsFileMetrics, +}; + +use syn::{visit, Expr, ImplItemMethod, ItemFn, ItemImpl, ItemMod, ItemTrait}; + +pub struct GeigerSynVisitor { + /// Count unsafe usage inside tests + include_tests: IncludeTests, + + /// The resulting data from a single file scan. + pub metrics: RsFileMetrics, + + /// The number of nested unsafe scopes that the GeigerSynVisitor are + /// currently in. For example, if the visitor is inside an unsafe function + /// and inside an unnecessary unsafe block inside that function, then this + /// number should be 2. If the visitor is outside unsafe scopes, in a safe + /// scope, this number should be 0. + /// This is needed since unsafe scopes can be nested and we need to know + /// when we leave the outmost unsafe scope and get back into a safe scope. + unsafe_scopes: u32, +} + +impl GeigerSynVisitor { + pub fn new(include_tests: IncludeTests) -> Self { + GeigerSynVisitor { + include_tests, + metrics: Default::default(), + unsafe_scopes: 0, + } + } + + pub fn enter_unsafe_scope(&mut self) { + self.unsafe_scopes += 1; + } + + pub fn exit_unsafe_scope(&mut self) { + self.unsafe_scopes -= 1; + } +} + +impl<'ast> visit::Visit<'ast> for GeigerSynVisitor { + fn visit_file(&mut self, i: &'ast syn::File) { + self.metrics.forbids_unsafe = file_forbids_unsafe(i); + syn::visit::visit_file(self, i); + } + + /// Free-standing functions + fn visit_item_fn(&mut self, item_fn: &ItemFn) { + if IncludeTests::No == self.include_tests && is_test_fn(item_fn) { + return; + } + if item_fn.sig.unsafety.is_some() { + self.enter_unsafe_scope() + } + self.metrics + .counters + .functions + .count(item_fn.sig.unsafety.is_some()); + visit::visit_item_fn(self, item_fn); + if item_fn.sig.unsafety.is_some() { + self.exit_unsafe_scope() + } + } + + fn visit_expr(&mut self, i: &Expr) { + // Total number of expressions of any type + match i { + Expr::Unsafe(i) => { + self.enter_unsafe_scope(); + visit::visit_expr_unsafe(self, i); + self.exit_unsafe_scope(); + } + Expr::Path(_) | Expr::Lit(_) => { + // Do not count. The expression `f(x)` should count as one + // expression, not three. + } + other => { + // TODO: Print something pretty here or gather the data for later + // printing. + // if self.verbosity == Verbosity::Verbose && self.unsafe_scopes > 0 { + // println!("{:#?}", other); + // } + self.metrics.counters.exprs.count(self.unsafe_scopes > 0); + visit::visit_expr(self, other); + } + } + } + + fn visit_item_mod(&mut self, i: &ItemMod) { + if IncludeTests::No == self.include_tests && is_test_mod(i) { + return; + } + visit::visit_item_mod(self, i); + } + + fn visit_item_impl(&mut self, i: &ItemImpl) { + // unsafe trait impl's + self.metrics.counters.item_impls.count(i.unsafety.is_some()); + visit::visit_item_impl(self, i); + } + + fn visit_item_trait(&mut self, i: &ItemTrait) { + // Unsafe traits + self.metrics + .counters + .item_traits + .count(i.unsafety.is_some()); + visit::visit_item_trait(self, i); + } + + fn visit_impl_item_method(&mut self, i: &ImplItemMethod) { + if i.sig.unsafety.is_some() { + self.enter_unsafe_scope() + } + self.metrics + .counters + .methods + .count(i.sig.unsafety.is_some()); + visit::visit_impl_item_method(self, i); + if i.sig.unsafety.is_some() { + self.exit_unsafe_scope() + } + } + + // TODO: Visit macros. + // + // TODO: Figure out if there are other visit methods that should be + // implemented here. +} diff --git a/geiger/src/lib.rs b/geiger/src/lib.rs index dff964ed..b889b1e3 100644 --- a/geiger/src/lib.rs +++ b/geiger/src/lib.rs @@ -7,16 +7,32 @@ #![forbid(unsafe_code)] #![forbid(warnings)] +pub mod find; +mod geiger_syn_visitor; + use cargo_geiger_serde::CounterBlock; use std::error::Error; use std::fmt; -use std::fs::File; use std::io; -use std::io::Read; -use std::path::Path; use std::path::PathBuf; use std::string::FromUtf8Error; -use syn::{visit, Expr, ImplItemMethod, ItemFn, ItemImpl, ItemMod, ItemTrait}; +use syn::{ItemFn, ItemMod}; + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum IncludeTests { + Yes, + No, +} + +/// Scan result for a single `.rs` file. +#[derive(Clone, Debug, Default, PartialEq)] +pub struct RsFileMetrics { + /// Metrics storage. + pub counters: CounterBlock, + + /// This file is decorated with `#![forbid(unsafe_code)]` + pub forbids_unsafe: bool, +} #[derive(Debug)] pub enum ScanFileError { @@ -35,58 +51,47 @@ impl fmt::Display for ScanFileError { } } -/// Scan result for a single `.rs` file. -#[derive(Clone, Debug, Default, PartialEq)] -pub struct RsFileMetrics { - /// Metrics storage. - pub counters: CounterBlock, - - /// This file is decorated with `#![forbid(unsafe_code)]` - pub forbids_unsafe: bool, -} - -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub enum IncludeTests { - Yes, - No, -} - -struct GeigerSynVisitor { - /// Count unsafe usage inside tests - include_tests: IncludeTests, - - /// The resulting data from a single file scan. - metrics: RsFileMetrics, - - /// The number of nested unsafe scopes that the GeigerSynVisitor are - /// currently in. For example, if the visitor is inside an unsafe function - /// and inside an unnecessary unsafe block inside that function, then this - /// number should be 2. If the visitor is outside unsafe scopes, in a safe - /// scope, this number should be 0. - /// This is needed since unsafe scopes can be nested and we need to know - /// when we leave the outmost unsafe scope and get back into a safe scope. - unsafe_scopes: u32, +fn file_forbids_unsafe(f: &syn::File) -> bool { + use syn::AttrStyle; + use syn::Meta; + use syn::MetaList; + use syn::NestedMeta; + f.attrs + .iter() + .filter(|a| matches!(a.style, AttrStyle::Inner(_))) + .filter_map(|a| a.parse_meta().ok()) + .filter(|meta| match meta { + Meta::List(MetaList { + path, + paren_token: _paren, + nested, + }) => { + if !path.is_ident("forbid") { + return false; + } + nested.iter().any(|n| match n { + NestedMeta::Meta(Meta::Path(p)) => { + p.is_ident("unsafe_code") + } + _ => false, + }) + } + _ => false, + }) + .count() + > 0 } -impl GeigerSynVisitor { - fn new(include_tests: IncludeTests) -> Self { - GeigerSynVisitor { - include_tests, - metrics: Default::default(), - unsafe_scopes: 0, - } - } - - fn enter_unsafe_scope(&mut self) { - self.unsafe_scopes += 1; - } - - fn exit_unsafe_scope(&mut self) { - self.unsafe_scopes -= 1; - } +fn is_test_fn(item_fn: &ItemFn) -> bool { + use syn::Attribute; + item_fn + .attrs + .iter() + .flat_map(Attribute::parse_meta) + .any(|m| meta_is_word_test(&m)) } -/// Will return true for #[cfg(test)] decodated modules. +/// Will return true for #[cfg(test)] decorated modules. /// /// This function is a somewhat of a hack and will probably misinterpret more /// advanced cfg expressions. A better way to do this would be to let rustc emit @@ -105,6 +110,14 @@ fn is_test_mod(i: &ItemMod) -> bool { }) } +fn meta_is_word_test(m: &syn::Meta) -> bool { + use syn::Meta; + match m { + Meta::Path(p) => p.is_ident("test"), + _ => false, + } +} + // MetaList { // ident: Ident( // cfg @@ -120,177 +133,13 @@ fn is_test_mod(i: &ItemMod) -> bool { // ) // ] // } -fn meta_list_is_cfg_test(ml: &syn::MetaList) -> bool { +fn meta_list_is_cfg_test(meta_list: &syn::MetaList) -> bool { use syn::NestedMeta; - if !ml.path.is_ident("cfg") { + if !meta_list.path.is_ident("cfg") { return false; } - ml.nested.iter().any(|n| match n { + meta_list.nested.iter().any(|n| match n { NestedMeta::Meta(meta) => meta_is_word_test(meta), _ => false, }) } - -fn meta_is_word_test(m: &syn::Meta) -> bool { - use syn::Meta; - match m { - Meta::Path(p) => p.is_ident("test"), - _ => false, - } -} - -fn is_test_fn(i: &ItemFn) -> bool { - use syn::Attribute; - i.attrs - .iter() - .flat_map(Attribute::parse_meta) - .any(|m| meta_is_word_test(&m)) -} - -fn file_forbids_unsafe(f: &syn::File) -> bool { - use syn::AttrStyle; - use syn::Meta; - use syn::MetaList; - use syn::NestedMeta; - f.attrs - .iter() - .filter(|a| matches!(a.style, AttrStyle::Inner(_))) - .filter_map(|a| a.parse_meta().ok()) - .filter(|meta| match meta { - Meta::List(MetaList { - path, - paren_token: _paren, - nested, - }) => { - if !path.is_ident("forbid") { - return false; - } - nested.iter().any(|n| match n { - NestedMeta::Meta(Meta::Path(p)) => { - p.is_ident("unsafe_code") - } - _ => false, - }) - } - _ => false, - }) - .count() - > 0 -} - -impl<'ast> visit::Visit<'ast> for GeigerSynVisitor { - fn visit_file(&mut self, i: &'ast syn::File) { - self.metrics.forbids_unsafe = file_forbids_unsafe(i); - syn::visit::visit_file(self, i); - } - - /// Free-standing functions - fn visit_item_fn(&mut self, i: &ItemFn) { - if IncludeTests::No == self.include_tests && is_test_fn(i) { - return; - } - if i.sig.unsafety.is_some() { - self.enter_unsafe_scope() - } - self.metrics - .counters - .functions - .count(i.sig.unsafety.is_some()); - visit::visit_item_fn(self, i); - if i.sig.unsafety.is_some() { - self.exit_unsafe_scope() - } - } - - fn visit_expr(&mut self, i: &Expr) { - // Total number of expressions of any type - match i { - Expr::Unsafe(i) => { - self.enter_unsafe_scope(); - visit::visit_expr_unsafe(self, i); - self.exit_unsafe_scope(); - } - Expr::Path(_) | Expr::Lit(_) => { - // Do not count. The expression `f(x)` should count as one - // expression, not three. - } - other => { - // TODO: Print something pretty here or gather the data for later - // printing. - // if self.verbosity == Verbosity::Verbose && self.unsafe_scopes > 0 { - // println!("{:#?}", other); - // } - self.metrics.counters.exprs.count(self.unsafe_scopes > 0); - visit::visit_expr(self, other); - } - } - } - - fn visit_item_mod(&mut self, i: &ItemMod) { - if IncludeTests::No == self.include_tests && is_test_mod(i) { - return; - } - visit::visit_item_mod(self, i); - } - - fn visit_item_impl(&mut self, i: &ItemImpl) { - // unsafe trait impl's - self.metrics.counters.item_impls.count(i.unsafety.is_some()); - visit::visit_item_impl(self, i); - } - - fn visit_item_trait(&mut self, i: &ItemTrait) { - // Unsafe traits - self.metrics - .counters - .item_traits - .count(i.unsafety.is_some()); - visit::visit_item_trait(self, i); - } - - fn visit_impl_item_method(&mut self, i: &ImplItemMethod) { - if i.sig.unsafety.is_some() { - self.enter_unsafe_scope() - } - self.metrics - .counters - .methods - .count(i.sig.unsafety.is_some()); - visit::visit_impl_item_method(self, i); - if i.sig.unsafety.is_some() { - self.exit_unsafe_scope() - } - } - - // TODO: Visit macros. - // - // TODO: Figure out if there are other visit methods that should be - // implemented here. -} - -pub fn find_unsafe_in_string( - src: &str, - include_tests: IncludeTests, -) -> Result { - use syn::visit::Visit; - let syntax = syn::parse_file(&src)?; - let mut vis = GeigerSynVisitor::new(include_tests); - vis.visit_file(&syntax); - Ok(vis.metrics) -} - -/// Scan a single file for `unsafe` usage. -pub fn find_unsafe_in_file( - p: &Path, - include_tests: IncludeTests, -) -> Result { - let mut file = - File::open(p).map_err(|e| ScanFileError::Io(e, p.to_path_buf()))?; - let mut src = vec![]; - file.read_to_end(&mut src) - .map_err(|e| ScanFileError::Io(e, p.to_path_buf()))?; - let src = String::from_utf8(src) - .map_err(|e| ScanFileError::Utf8(e, p.to_path_buf()))?; - find_unsafe_in_string(&src, include_tests) - .map_err(|e| ScanFileError::Syn(e, p.to_path_buf())) -}