Merged #366

src/lib.rs

@@ -48,6 +48,7 @@ pub mod ranges;
 pub mod matches;
 pub mod precedence;
 pub mod zero_div_zero;
+pub mod open_options;
 
 mod reexport {
     pub use syntax::ast::{Name, Ident, NodeId};
@@ -89,6 +90,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_late_lint_pass(box zero_div_zero::ZeroDivZeroPass);
 
     reg.register_lint_group("clippy_pedantic", vec![
@@ -145,6 +147,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,

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,
+    "nonsensical combination of options for opening a file"
+}
+
+
+#[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,
+    Read,
+    Truncate,
+    Create,
+    Append
+}
+
+fn get_open_options(cx: &LateContext, argument: &Expr, options: &mut Vec<(OpenOption, Argument)>) {
+    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, Argument)], 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, Argument)], 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, Argument)], span: Span) {
+    check_for_duplicates(cx, options, span);
+    check_for_inconsistencies(cx, options, span);
+}

src/utils.rs

@@ -10,11 +10,12 @@ use syntax::ast::Lit_::*;
 use syntax::ast;
 
 // 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"];
 
 /// Produce a nested chain of if-lets and ifs from the patterns:
 ///