Merge pull request #305 from killercup/feature/clippy

Add [clippy](https://github.com/Manishearth/rust-clippy) and fix all of its warnings. This is

- adding the latest clippy 0.0.114 as an optional dependency with the `lint` feature
- improving a whole bunch of stuff in the various crates to make clippy real happy
- green with `#[deny(warnings)]`!
- adding a new entry in Travis' build matrix (which compiles quite fast as it is using `no-trans`)

.editorconfig

@@ -0,0 +1,25 @@
+# EditorConfig helps developers define and maintain consistent
+# coding styles between different editors and IDEs
+# editorconfig.org
+
+root = true
+
+
+[*]
+end_of_line = lf
+charset = utf-8
+trim_trailing_whitespace = true
+insert_final_newline = true
+indent_style = space
+indent_size = 4
+
+[*.rs]
+indent_style = space
+indent_size = 4
+
+[*.toml]
+indent_style = space
+indent_size = 4
+
+[*.md]
+trim_trailing_whitespace = false

.travis.yml

@@ -33,6 +33,16 @@ script:
   if [[ "$TRAVIS_RUST_VERSION" == nightly* ]]; then
     (cd diesel_compile_tests && travis-cargo test)
   fi
+matrix:
+  allow_failures:
+    - rust: nightly
+  include:
+  - rust: nightly-2017-02-09
+    env: CLIPPY=YESPLEASE
+    script:
+    - (cd diesel && cargo rustc --no-default-features --features "lint unstable chrono serde_json uuid sqlite postgres mysql" -- -Zno-trans)
+    - (cd diesel_cli && cargo rustc --no-default-features --features "lint sqlite postgres mysql" -- -Zno-trans)
+    - (cd diesel_codegen && cargo rustc --no-default-features --features "lint dotenv sqlite postgres mysql" -- -Zno-trans)
 env:
   matrix:
     - BACKEND=sqlite
@@ -66,6 +76,3 @@ notifications:
     on_success: change
     on_failure: always
     on_start: never
-matrix:
-  allow_failures:
-    - rust: nightly

clippy.toml

@@ -0,0 +1,10 @@
+cyclomatic-complexity-threshold = 30
+doc-valid-idents = [
+  "MiB", "GiB", "TiB", "PiB", "EiB",
+  "DirectX", "OpenGL", "TrueType",
+  "GPLv2", "GPLv3",
+  "GitHub",
+  "IPv4", "IPv6",
+  "JavaScript", "NaN", "OAuth",
+  "SQLite", "PostgreSQL"
+]

diesel/Cargo.toml

@@ -21,6 +21,7 @@ quickcheck = { version = "0.3.1", optional = true }
 serde_json = { version = ">=0.8.0, <0.10.0", optional = true }
 uuid = { version = ">=0.2.0, <0.5.0", optional = true, features = ["use_std"] }
 url = { version = "1.4.0", optional = true }
+clippy = { optional = true, version = "=0.0.114" }
 
 [dev-dependencies]
 cfg-if = "0.1.0"
@@ -33,6 +34,7 @@ tempdir = "^0.3.4"
 default = ["with-deprecated"]
 extras = ["chrono", "serde_json", "uuid"]
 unstable = []
+lint = ["clippy"]
 large-tables = []
 huge-tables = ["large-tables"]
 postgres = ["pq-sys"]

diesel/src/connection/transaction_manager.rs

@@ -31,18 +31,17 @@ pub trait TransactionManager<Conn: Connection> {
 
 use std::cell::Cell;
 
-/// An implementation of TransactionManager which can be used for backends
+/// An implementation of `TransactionManager` which can be used for backends
 /// which use ANSI standard syntax for savepoints such as SQLite and PostgreSQL.
 #[allow(missing_debug_implementations)]
+#[derive(Default)]
 pub struct AnsiTransactionManager {
     transaction_depth: Cell<i32>,
 }
 
 impl AnsiTransactionManager {
     pub fn new() -> Self {
-        AnsiTransactionManager {
-            transaction_depth: Cell::new(0),
-        }
+        AnsiTransactionManager::default()
     }
 
     fn change_transaction_depth(&self, by: i32, query: QueryResult<()>) -> QueryResult<()> {

diesel/src/expression/aliased.rs

@@ -34,7 +34,7 @@ impl<'a, T, DB> QueryFragment<DB> for Aliased<'a, T> where
     T: QueryFragment<DB>,
 {
     fn to_sql(&self, out: &mut DB::QueryBuilder) -> BuildQueryResult {
-        out.push_identifier(&self.alias)
+        out.push_identifier(self.alias)
     }
 
     fn collect_binds(&self, _out: &mut DB::BindCollector) -> QueryResult<()> {

diesel/src/expression/array_comparison.rs

@@ -218,7 +218,7 @@ impl<T, DB> QueryFragment<DB> for Many<T> where
     }
 
     fn collect_binds(&self, out: &mut DB::BindCollector) -> QueryResult<()> {
-        for value in self.0.iter() {
+        for value in &self.0 {
             try!(value.collect_binds(out));
         }
         Ok(())

diesel/src/expression/expression_methods/global_expression_methods.rs

@@ -20,7 +20,7 @@ pub trait ExpressionMethods: Expression + Sized {
     ///     .filter(query.matches(indexed_search_column))
     ///     .order(rank.desc())
     /// ```
-    fn aliased<'a>(self, alias: &'a str) -> Aliased<'a, Self> {
+    fn aliased(self, alias: &str) -> Aliased<Self> {
         Aliased::new(self, alias)
     }
 

diesel/src/expression/functions/date_and_time.rs

@@ -4,7 +4,7 @@ use query_builder::*;
 use result::QueryResult;
 use types::*;
 
-/// Represents the SQL CURRENT_TIMESTAMP constant. This is equivalent to the
+/// Represents the SQL `CURRENT_TIMESTAMP` constant. This is equivalent to the
 /// `NOW()` function on backends that support it.
 #[allow(non_camel_case_types)]
 #[derive(Debug, Copy, Clone)]

diesel/src/insertable.rs

@@ -70,7 +70,9 @@ impl<T, DB> InsertValues<DB> for BatchInsertValues<T> where
     DB: Backend,
 {
     fn column_names(&self, out: &mut DB::QueryBuilder) -> BuildQueryResult {
-        self.0.clone().next().unwrap().column_names(out)
+        self.0.clone()
+            .next().expect("Tried to read column names from empty list of rows")
+            .column_names(out)
     }
 
     fn values_clause(&self, out: &mut DB::QueryBuilder) -> BuildQueryResult {

diesel/src/lib.rs

@@ -1,9 +1,26 @@
 //! Diesel is an ORM and query builder designed to reduce the boilerplate for database
 //! interactions. [A getting started guide](http://diesel.rs/guides/getting-started/) can be
 //! found on our website.
-#![deny(warnings, missing_debug_implementations, missing_copy_implementations)]
 #![cfg_attr(feature = "unstable", feature(specialization))]
 
+// Built-in Lints
+#![deny(warnings, missing_debug_implementations, missing_copy_implementations)]
+
+// Clippy lints
+#![cfg_attr(feature = "clippy", allow(unstable_features))]
+#![cfg_attr(feature = "clippy", feature(plugin))]
+#![cfg_attr(feature = "clippy", plugin(clippy(conf_file="../clippy.toml")))]
+#![cfg_attr(feature = "clippy", allow(
+    option_map_unwrap_or_else, option_map_unwrap_or,
+    match_same_arms, type_complexity,
+))]
+#![cfg_attr(feature = "clippy", warn(
+    option_unwrap_used, result_unwrap_used, print_stdout, wrong_pub_self_convention,
+    mut_mut, non_ascii_literal, similar_names, unicode_not_nfc,
+    enum_glob_use, if_not_else, items_after_statements, used_underscore_binding,
+))]
+#![cfg_attr(all(test, feature = "clippy"), allow(result_unwrap_used))]
+
 extern crate byteorder;
 
 #[macro_use]

diesel/src/migrations/migration.rs

@@ -53,19 +53,27 @@ fn valid_sql_migration_directory(path: &Path) -> bool {
 }
 
 fn file_names(path: &Path) -> Result<Vec<String>, MigrationError> {
-    try!(path.read_dir()).map(|e| {
-        Ok(try!(e).file_name().into_string().unwrap())
-    }).filter(|name| match name {
-        &Ok(ref n) => !n.starts_with("."),
+    try!(path.read_dir()).map(|entry| {
+        let file_name = try!(entry).file_name();
+
+        // FIXME(killercup): Decide whether to add MigrationError variant for this
+        match file_name.into_string() {
+            Ok(utf8_file_name) => Ok(utf8_file_name),
+            Err(original_os_string) =>
+                panic!("Can't convert file name `{:?}` into UTF8 string", original_os_string)
+        }
+    }).filter(|file_name| match *file_name {
+        Ok(ref name) => !name.starts_with('.'),
         _ => true
     }).collect()
 }
 
 #[doc(hidden)]
 pub fn version_from_path(path: &Path) -> Result<String, MigrationError> {
-    path.file_name().unwrap()
+    path.file_name()
+        .expect(&format!("Can't get file name from path `{:?}`", path))
         .to_string_lossy()
-        .split("_")
+        .split('_')
         .nth(0)
         .map(|s| Ok(s.into()))
         .unwrap_or_else(|| {

diesel/src/migrations/migration_error.rs

@@ -5,8 +5,6 @@ use std::{fmt, io};
 use std::path::PathBuf;
 use std::error::Error;
 
-use self::MigrationError::*;
-
 #[derive(Debug)]
 pub enum MigrationError {
     MigrationDirectoryNotFound,
@@ -18,10 +16,15 @@ pub enum MigrationError {
 impl Error for MigrationError {
     fn description(&self) -> &str {
         match *self {
-            MigrationDirectoryNotFound => "Unable to find migrations directory in this directory or any parent directories.",
-            UnknownMigrationFormat(_) => "Invalid migration directory, the directory's name should be <timestamp>_<name_of_migration>, and it should only contain up.sql and down.sql.",
-            IoError(ref error) => error.description(),
-            UnknownMigrationVersion(_) => "Unable to find migration version to revert in the migrations directory.",
+            MigrationError::MigrationDirectoryNotFound =>
+                "Unable to find migrations directory in this directory or any parent directories.",
+            MigrationError::UnknownMigrationFormat(_) =>
+                "Invalid migration directory, the directory's name should be \
+                <timestamp>_<name_of_migration>, and it should only contain up.sql and down.sql.",
+            MigrationError::IoError(ref error) =>
+                error.description(),
+            MigrationError::UnknownMigrationVersion(_) =>
+                "Unable to find migration version to revert in the migrations directory.",
         }
     }
 }
@@ -55,6 +58,7 @@ impl From<io::Error> for MigrationError {
 }
 
 #[derive(Debug, PartialEq)]
+#[cfg_attr(feature = "clippy", allow(enum_variant_names))]
 pub enum RunMigrationsError {
     MigrationError(MigrationError),
     QueryError(result::Error),

diesel/src/migrations/mod.rs

@@ -73,7 +73,6 @@ use std::io::{stdout, Write};
 
 use expression::expression_methods::*;
 use query_dsl::*;
-use self::migration_error::MigrationError::*;
 use self::schema::__diesel_schema_migrations::dsl::*;
 use {Connection, QueryResult};
 
@@ -150,7 +149,7 @@ fn migration_with_version(ver: &str) -> Result<Box<Migration>, MigrationError> {
     });
     match migration {
         Some(m) => Ok(m),
-        None => Err(UnknownMigrationVersion(ver.into())),
+        None => Err(MigrationError::UnknownMigrationVersion(ver.into())),
     }
 }
 
@@ -177,10 +176,10 @@ pub fn migration_paths_in_directory(path: &Path) -> Result<Vec<DirEntry>, Migrat
                 Ok(e) => e,
                 Err(e) => return Some(Err(e.into())),
             };
-            if !entry.file_name().to_string_lossy().starts_with(".") {
-                Some(Ok(entry))
-            } else {
+            if entry.file_name().to_string_lossy().starts_with('.') {
                 None
+            } else {
+                Some(Ok(entry))
             }
         }).collect()
 }

diesel/src/mysql/connection/bind.rs

@@ -83,7 +83,7 @@ struct BindData {
 impl BindData {
     fn for_input(tpe: MysqlType, data: Option<Vec<u8>>) -> Self {
         let is_null = if data.is_none() { 1 } else { 0 };
-        let bytes = data.unwrap_or(Vec::new());
+        let bytes = data.unwrap_or_default();
         let length = bytes.len() as libc::c_ulong;
 
         BindData {
@@ -98,7 +98,7 @@ impl BindData {
     fn for_output(tpe: ffi::enum_field_types) -> Self {
         let bytes = known_buffer_size_for_ffi_type(tpe)
             .map(|len| vec![0; len])
-            .unwrap_or(Vec::new());
+            .unwrap_or_default();
         let length = bytes.len() as libc::c_ulong;
 
         BindData {

diesel/src/mysql/connection/raw.rs

@@ -46,10 +46,10 @@ impl RawConnection {
             // Make sure you don't use the fake one!
             ffi::mysql_real_connect(
                 self.0,
-                host.map(|x| x.as_ptr()).unwrap_or(ptr::null_mut()),
+                host.map(CStr::as_ptr).unwrap_or_else(|| ptr::null_mut()),
                 user.as_ptr(),
-                password.map(|x| x.as_ptr()).unwrap_or(ptr::null_mut()),
-                database.map(|x| x.as_ptr()).unwrap_or(ptr::null_mut()),
+                password.map(CStr::as_ptr).unwrap_or_else(|| ptr::null_mut()),
+                database.map(CStr::as_ptr).unwrap_or_else(|| ptr::null_mut()),
                 port.unwrap_or(0) as u32,
                 ptr::null_mut(),
                 0,
@@ -150,11 +150,13 @@ impl Drop for RawConnection {
     }
 }
 
-/// In a nonmulti-threaded environment, mysql_init() invokes mysql_library_init() automatically as
-/// necessary. However, mysql_library_init() is not thread-safe in a multi-threaded environment,
-/// and thus neither is mysql_init(). Before calling mysql_init(), either call mysql_library_init()
-/// prior to spawning any threads, or use a mutex to protect the mysql_library_init() call. This
-/// should be done prior to any other client library call.
+/// > In a nonmulti-threaded environment, `mysql_init()` invokes
+/// > `mysql_library_init()` automatically as necessary. However,
+/// > `mysql_library_init()` is not thread-safe in a multi-threaded environment,
+/// > and thus neither is `mysql_init()`. Before calling `mysql_init()`, either
+/// > call `mysql_library_init()` prior to spawning any threads, or use a mutex
+/// > ot protect the `mysql_library_init()` call. This should be done prior to
+/// > any other client library call.
 ///
 /// https://dev.mysql.com/doc/refman/5.7/en/mysql-init.html
 static MYSQL_THREAD_UNSAFE_INIT: Once = ONCE_INIT;

diesel/src/mysql/connection/stmt/iterator.rs

@@ -8,6 +8,7 @@ pub struct StatementIterator<'a> {
     output_binds: Binds,
 }
 
+#[cfg_attr(feature = "clippy", allow(should_implement_trait))] // don't neet `Iterator` here
 impl<'a> StatementIterator<'a> {
     pub fn new(stmt: &'a mut Statement, types: Vec<MysqlType>) -> QueryResult<Self> {
         let mut output_binds = Binds::from_output_types(types);
@@ -37,7 +38,7 @@ impl<'a> StatementIterator<'a> {
         match next_row_result as libc::c_uint {
             ffi::MYSQL_NO_DATA => return None,
             ffi::MYSQL_DATA_TRUNCATED => {
-                let res = self.output_binds.populate_dynamic_buffers(&self.stmt);
+                let res = self.output_binds.populate_dynamic_buffers(self.stmt);
                 if let Err(e) = res {
                     return Some(Err(e));
                 }

diesel/src/mysql/connection/url.rs

@@ -95,8 +95,8 @@ fn urls_must_have_zero_or_one_path_segments() {
 
 #[test]
 fn first_path_segment_is_treated_as_database() {
-    let foo_cstr = CString::new("foo".as_bytes()).unwrap();
-    let bar_cstr = CString::new("bar".as_bytes()).unwrap();
+    let foo_cstr = CString::new("foo").unwrap();
+    let bar_cstr = CString::new("bar").unwrap();
     assert_eq!(
         Some(&*foo_cstr),
         ConnectionOptions::parse("mysql://localhost/foo").unwrap().database()

diesel/src/mysql/query_builder.rs

@@ -2,15 +2,14 @@ use super::backend::Mysql;
 use query_builder::{QueryBuilder, BuildQueryResult};
 
 #[allow(missing_debug_implementations)]
+#[derive(Default)]
 pub struct MysqlQueryBuilder {
     pub sql: String,
 }
 
 impl MysqlQueryBuilder {
     pub fn new() -> Self {
-        MysqlQueryBuilder {
-            sql: String::new(),
-        }
+        MysqlQueryBuilder::default()
     }
 }
 

diesel/src/pg/connection/mod.rs

@@ -107,11 +107,12 @@ impl Connection for PgConnection {
 }
 
 impl PgConnection {
+    #[cfg_attr(feature = "clippy", allow(type_complexity))]
     fn prepare_query<T: QueryFragment<Pg> + QueryId>(&self, source: &T)
         -> QueryResult<(Rc<Query>, Vec<Option<Vec<u8>>>)>
     {
         let mut bind_collector = RawBytesBindCollector::<Pg>::new();
-        source.collect_binds(&mut bind_collector).unwrap();
+        try!(source.collect_binds(&mut bind_collector));
         let (binds, bind_types) = bind_collector.binds.into_iter()
             .map(|(meta, bind)| (bind, meta.oid)).unzip();
 
@@ -142,7 +143,9 @@ extern "C" fn noop_notice_processor(_: *mut libc::c_void, _message: *const libc:
 extern "C" fn default_notice_processor(_: *mut libc::c_void, message: *const libc::c_char) {
     use std::io::Write;
     let c_str = unsafe { CStr::from_ptr(message) };
-    ::std::io::stderr().write(c_str.to_bytes()).unwrap();
+    ::std::io::stderr()
+        .write_all(c_str.to_bytes())
+        .expect("Error writing to `stderr`");
 }
 
 #[cfg(test)]