From 7644f8e2a1774c426f520efc50fe3626b31e028c Mon Sep 17 00:00:00 2001 From: Pyriphlegethon Date: Wed, 7 Oct 2015 13:15:14 +0200 Subject: [PATCH] Add "nonsensical OpenOptions" lint --- README.md | 3 +- src/lib.rs | 3 + src/open_options.rs | 139 +++++++++++++++++++++++++++++ src/utils.rs | 11 +-- tests/compile-fail/open_options.rs | 16 ++++ 5 files changed, 166 insertions(+), 6 deletions(-) create mode 100644 src/open_options.rs create mode 100644 tests/compile-fail/open_options.rs diff --git a/README.md b/README.md index a5ab856fc21..96465d9fd35 100644 --- 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 2f71d8cc9df..44f8e9e5b98 100755 --- a/src/lib.rs +++ b/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, diff --git a/src/open_options.rs b/src/open_options.rs new file mode 100644 index 00000000000..d91305c36c2 --- /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) { + 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 ac155617011..1f4fb2b251d 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -9,11 +9,12 @@ use std::borrow::Cow; 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 index 00000000000..35cc91c9d0f --- /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 +}