Merge pull request #158 from jmcconnell26/NOISSUE-RefactorGeigerLib

NOISSUE - Refactoring geiger lib and adding further testing

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;

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"

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<RsFileMetrics, ScanFileError> {
+    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<RsFileMetrics, syn::Error> {
+    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);
+    }
+}

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.
+}