Add "nonsensical OpenOptions" lint

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

src/lib.rs

@@ -47,6 +47,7 @@ pub mod loops;
 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,

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);
+}

src/utils.rs

@@ -14,6 +14,7 @@ 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 {

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