Add "nonsensical OpenOptions" lint

diff --git a/README.md b/README.md
index a5ab856fc21150c666fe28444cd3e1ad45ab39e5..96465d9fd35a682e27f5265f7bb5a1ed7e2e3d60 100644 (file)
--- a/README.md
+++ b/README.md
@@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
 [Jump to usage instructions](#usage)
 
 ##Lints
-There are 59 lints included in this crate:
+There are 60 lints included in this crate:
 
 name                                                                                                   | default | meaning
 -------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -42,6 +42,7 @@ name
 [needless_range_loop](https://github.com/Manishearth/rust-clippy/wiki#needless_range_loop)             | warn    | for-looping over a range of indices where an iterator over items would do
 [needless_return](https://github.com/Manishearth/rust-clippy/wiki#needless_return)                     | warn    | using a return statement like `return expr;` where an expression would suffice
 [non_ascii_literal](https://github.com/Manishearth/rust-clippy/wiki#non_ascii_literal)                 | allow   | using any literal non-ASCII chars in a string literal; suggests using the \\u escape instead
+[nonsensical_open_options](https://github.com/Manishearth/rust-clippy/wiki#nonsensical_open_options)   | warn    | The options used for opening a file are nonsensical
 [option_unwrap_used](https://github.com/Manishearth/rust-clippy/wiki#option_unwrap_used)               | allow   | using `Option.unwrap()`, which should at least get a better message using `expect()`
 [precedence](https://github.com/Manishearth/rust-clippy/wiki#precedence)                               | warn    | catches operations where precedence may be unclear. See the wiki for a list of cases caught
 [ptr_arg](https://github.com/Manishearth/rust-clippy/wiki#ptr_arg)                                     | allow   | fn arguments of the type `&Vec<...>` or `&String`, suggesting to use `&[...]` or `&str` instead, respectively

diff --git a/src/lib.rs b/src/lib.rs
index 2f71d8cc9df7e8db19d33e3654610f078a1f2594..44f8e9e5b984e543bf7513e563c5c9c3b6eae116 100755 (executable)
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -47,6 +47,7 @@
 pub mod ranges;
 pub mod matches;
 pub mod precedence;
+pub mod open_options;
 
 mod reexport {
     pub use syntax::ast::{Name, Ident, NodeId};
@@ -88,6 +89,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_late_lint_pass(box matches::MatchPass);
     reg.register_late_lint_pass(box misc::PatternPass);
     reg.register_late_lint_pass(box minmax::MinMaxPass);
+    reg.register_late_lint_pass(box open_options::NonSensicalOpenOptions);
 
     reg.register_lint_group("clippy_pedantic", vec![
         methods::OPTION_UNWRAP_USED,
@@ -142,6 +144,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         misc::TOPLEVEL_REF_ARG,
         mut_reference::UNNECESSARY_MUT_PASSED,
         needless_bool::NEEDLESS_BOOL,
+        open_options::NONSENSICAL_OPEN_OPTIONS,
         precedence::PRECEDENCE,
         ranges::RANGE_STEP_BY_ZERO,
         returns::LET_AND_RETURN,

diff --git a/src/open_options.rs b/src/open_options.rs
new file mode 100644 (file)
index 0000000..d91305c
--- /dev/null
+++ b/src/open_options.rs
@@ -0,0 +1,139 @@
+use rustc::lint::*;
+use rustc_front::hir::{Expr, ExprMethodCall, ExprLit};
+use utils::{walk_ptrs_ty_depth, match_type, span_lint, OPEN_OPTIONS_PATH};
+use syntax::codemap::{Span, Spanned};
+use syntax::ast::Lit_::LitBool;
+
+declare_lint! {
+    pub NONSENSICAL_OPEN_OPTIONS,
+    Warn,
+    "The options used for opening a file are nonsensical"
+}
+
+
+#[derive(Copy,Clone)]
+pub struct NonSensicalOpenOptions;
+
+impl LintPass for NonSensicalOpenOptions {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(NONSENSICAL_OPEN_OPTIONS)
+    }
+}
+
+impl LateLintPass for NonSensicalOpenOptions {
+    fn check_expr(&mut self, cx: &LateContext, e: &Expr) {
+        if let ExprMethodCall(ref name, _, ref arguments) = e.node {
+            let (obj_ty, _) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&arguments[0]));
+            if name.node.as_str() == "open" && match_type(cx, obj_ty, &OPEN_OPTIONS_PATH){
+                let mut options = Vec::new();
+                get_open_options(cx, &arguments[0], &mut options);
+                check_open_options(cx, &options, e.span);
+            }
+        }
+    }
+}
+
+#[derive(Debug)]
+enum Argument {
+    True,
+    False,
+    Unknown
+}
+
+#[derive(Debug)]
+enum OpenOption {
+    Write(Argument),
+    Read(Argument),
+    Truncate(Argument),
+    Create(Argument),
+    Append(Argument)
+}
+
+fn get_open_options(cx: &LateContext, argument: &Expr, options: &mut Vec<OpenOption>) {
+    if let ExprMethodCall(ref name, _, ref arguments) = argument.node {
+        let (obj_ty, _) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&arguments[0]));
+        
+        // Only proceed if this is a call on some object of type std::fs::OpenOptions
+        if match_type(cx, obj_ty, &OPEN_OPTIONS_PATH) && arguments.len() >= 2 {
+            
+            let argument_option = match arguments[1].node {
+                ExprLit(ref span) => {
+                    if let Spanned {node: LitBool(lit), span: _} = **span {
+                        if lit {Argument::True} else {Argument::False}
+                    } else {
+                        return; // The function is called with a literal
+                                // which is not a boolean literal. This is theoretically
+                                // possible, but not very likely.
+                    }
+                },
+                _ => {
+                    Argument::Unknown
+                }
+            };
+            
+            match &*name.node.as_str() {
+                "create" => {
+                    options.push(OpenOption::Create(argument_option));
+                },
+                "append" => {
+                    options.push(OpenOption::Append(argument_option));
+                },
+                "truncate" => {
+                    options.push(OpenOption::Truncate(argument_option));
+                },
+                "read" => {
+                    options.push(OpenOption::Read(argument_option));
+                },
+                "write" => {
+                    options.push(OpenOption::Write(argument_option));
+                },
+                _ => {}
+            }
+            
+            get_open_options(cx, &arguments[0], options);
+        }
+    }
+}
+
+fn check_for_duplicates(cx: &LateContext, options: &[OpenOption], span: Span) {
+    // This code is almost duplicated (oh, the irony), but I haven't found a way to unify it.
+    if options.iter().filter(|o| if let OpenOption::Create(_) = **o {true} else {false}).count() > 1 {
+        span_lint(cx, NONSENSICAL_OPEN_OPTIONS, span, "The method \"create\" \
+                                                       is called more than once");
+    }
+    if options.iter().filter(|o| if let OpenOption::Append(_) = **o {true} else {false}).count() > 1 {
+        span_lint(cx, NONSENSICAL_OPEN_OPTIONS, span, "The method \"append\" \
+                                                       is called more than once");
+    }
+    if options.iter().filter(|o| if let OpenOption::Truncate(_) = **o {true} else {false}).count() > 1 {
+        span_lint(cx, NONSENSICAL_OPEN_OPTIONS, span, "The method \"truncate\" \
+                                                       is called more than once");
+    }
+    if options.iter().filter(|o| if let OpenOption::Read(_) = **o {true} else {false}).count() > 1 {
+        span_lint(cx, NONSENSICAL_OPEN_OPTIONS, span, "The method \"read\" \
+                                                       is called more than once");
+    }
+    if options.iter().filter(|o| if let OpenOption::Write(_) = **o {true} else {false}).count() > 1 {
+        span_lint(cx, NONSENSICAL_OPEN_OPTIONS, span, "The method \"write\" \
+                                                       is called more than once");
+    }
+}
+
+fn check_for_inconsistencies(cx: &LateContext, options: &[OpenOption], span: Span) {
+    // Truncate + read makes no sense.
+    if options.iter().filter(|o| if let OpenOption::Read(Argument::True) = **o {true} else {false}).count() > 0 &&
+       options.iter().filter(|o| if let OpenOption::Truncate(Argument::True) = **o {true} else {false}).count() > 0 {
+        span_lint(cx, NONSENSICAL_OPEN_OPTIONS, span, "File opened with \"truncate\" and \"read\"");
+    }
+    
+    // Append + truncate makes no sense.
+    if options.iter().filter(|o| if let OpenOption::Append(Argument::True) = **o {true} else {false}).count() > 0 &&
+       options.iter().filter(|o| if let OpenOption::Truncate(Argument::True) = **o {true} else {false}).count() > 0 {
+        span_lint(cx, NONSENSICAL_OPEN_OPTIONS, span, "File opened with \"append\" and \"truncate\"");
+    }
+}
+
+fn check_open_options(cx: &LateContext, options: &[OpenOption], span: Span) {
+    check_for_duplicates(cx, options, span);
+    check_for_inconsistencies(cx, options, span);
+}

diff --git a/src/utils.rs b/src/utils.rs
index ac155617011bd45eb2a11dc51b95f818f18dc2ab..1f4fb2b251d80fe0dad75bf05c6de79de2af2e51 100644 (file)
--- a/src/utils.rs
+++ b/src/utils.rs
@@ -9,11 +9,12 @@
 use syntax::ast::Lit_::*;
 
 // module DefPaths for certain structs/enums we check for
-pub const OPTION_PATH: [&'static str; 3] = ["core", "option", "Option"];
-pub const RESULT_PATH: [&'static str; 3] = ["core", "result", "Result"];
-pub const STRING_PATH: [&'static str; 3] = ["collections", "string", "String"];
-pub const VEC_PATH:    [&'static str; 3] = ["collections", "vec", "Vec"];
-pub const LL_PATH:     [&'static str; 3] = ["collections", "linked_list", "LinkedList"];
+pub const OPTION_PATH:       [&'static str; 3] = ["core", "option", "Option"];
+pub const RESULT_PATH:       [&'static str; 3] = ["core", "result", "Result"];
+pub const STRING_PATH:       [&'static str; 3] = ["collections", "string", "String"];
+pub const VEC_PATH:          [&'static str; 3] = ["collections", "vec", "Vec"];
+pub const LL_PATH:           [&'static str; 3] = ["collections", "linked_list", "LinkedList"];
+pub const OPEN_OPTIONS_PATH: [&'static str; 3] = ["std", "fs", "OpenOptions"];
 
 /// returns true this expn_info was expanded by any macro
 pub fn in_macro(cx: &LateContext, span: Span) -> bool {

diff --git a/tests/compile-fail/open_options.rs b/tests/compile-fail/open_options.rs
new file mode 100644 (file)
index 0000000..35cc91c
--- /dev/null
+++ b/tests/compile-fail/open_options.rs
@@ -0,0 +1,16 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+use std::fs::OpenOptions;
+
+#[allow(unused_must_use)]
+#[deny(nonsensical_open_options)]
+fn main() {
+       OpenOptions::new().read(true).truncate(true).open("foo.txt"); //~ERROR File opened with "truncate" and "read"
+       OpenOptions::new().append(true).truncate(true).open("foo.txt"); //~ERROR File opened with "append" and "truncate"
+    
+       OpenOptions::new().read(true).read(false).open("foo.txt"); //~ERROR The method "read" is called more than once
+       OpenOptions::new().create(true).create(false).open("foo.txt"); //~ERROR The method "create" is called more than once
+       OpenOptions::new().write(true).write(false).open("foo.txt"); //~ERROR The method "write" is called more than once
+       OpenOptions::new().append(true).append(false).open("foo.txt"); //~ERROR The method "append" is called more than once
+       OpenOptions::new().truncate(true).truncate(false).open("foo.txt"); //~ERROR The method "truncate" is called more than once
+}