Auto merge of #3954 - andrehjr:add-lint-path-buf-overwrite, r=flip1995

Add Lint PathBufPushOverwrite

Closes #3923

This is a very simple Lint that checks if push is being called with a Root Path. Because that can make it overwrite the previous path.

I used std::path::Path to check if it's root, in order to keep it working across windows/linux environments instead of checking for '/'. Is that alright?

On the `if_chain!` block, Is there a way to make it short while getting the value of the first argument? I got the example from other lints.

Note that this is first Lint, I hope I got everything covered  🚀

CHANGELOG.md

@@ -1015,6 +1015,7 @@ All notable changes to this project will be documented in this file.
 [`panic_params`]: https://rust-lang.github.io/rust-clippy/master/index.html#panic_params
 [`panicking_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#panicking_unwrap
 [`partialeq_ne_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#partialeq_ne_impl
+[`path_buf_push_overwrite`]: https://rust-lang.github.io/rust-clippy/master/index.html#path_buf_push_overwrite
 [`possible_missing_comma`]: https://rust-lang.github.io/rust-clippy/master/index.html#possible_missing_comma
 [`precedence`]: https://rust-lang.github.io/rust-clippy/master/index.html#precedence
 [`print_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#print_literal

README.md

@@ -7,7 +7,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 298 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 299 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

clippy_lints/src/lib.rs

@@ -236,6 +236,7 @@ pub mod open_options;
 pub mod overflow_check_conditional;
 pub mod panic_unimplemented;
 pub mod partialeq_ne_impl;
+pub mod path_buf_push_overwrite;
 pub mod precedence;
 pub mod ptr;
 pub mod ptr_offset_with_cast;
@@ -572,6 +573,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
     reg.register_late_lint_pass(box assertions_on_constants::AssertionsOnConstants);
     reg.register_late_lint_pass(box missing_const_for_fn::MissingConstForFn);
     reg.register_late_lint_pass(box transmuting_null::TransmutingNull);
+    reg.register_late_lint_pass(box path_buf_push_overwrite::PathBufPushOverwrite);
 
     reg.register_lint_group("clippy::restriction", Some("clippy_restriction"), vec![
         arithmetic::FLOAT_ARITHMETIC,
@@ -805,6 +807,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL,
         panic_unimplemented::PANIC_PARAMS,
         partialeq_ne_impl::PARTIALEQ_NE_IMPL,
+        path_buf_push_overwrite::PATH_BUF_PUSH_OVERWRITE,
         precedence::PRECEDENCE,
         ptr::CMP_NULL,
         ptr::MUT_FROM_REF,
@@ -1072,6 +1075,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         non_copy_const::BORROW_INTERIOR_MUTABLE_CONST,
         non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST,
         open_options::NONSENSICAL_OPEN_OPTIONS,
+        path_buf_push_overwrite::PATH_BUF_PUSH_OVERWRITE,
         ptr::MUT_FROM_REF,
         ranges::ITERATOR_STEP_BY_ZERO,
         regex::INVALID_REGEX,

clippy_lints/src/path_buf_push_overwrite.rs

@@ -0,0 +1,71 @@
+use crate::utils::{match_type, paths, span_lint_and_sugg, walk_ptrs_ty};
+use if_chain::if_chain;
+use rustc::hir::*;
+use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
+use rustc::{declare_lint_pass, declare_tool_lint};
+use rustc_errors::Applicability;
+use std::path::{Component, Path};
+use syntax::ast::LitKind;
+
+declare_clippy_lint! {
+    /// **What it does:*** Checks for [push](https://doc.rust-lang.org/std/path/struct.PathBuf.html#method.push)
+    /// calls on `PathBuf` that can cause overwrites.
+    ///
+    /// **Why is this bad?** Calling `push` with a root path at the start can overwrite the
+    /// previous defined path.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// ```rust
+    /// use std::path::PathBuf;
+    ///
+    /// let mut x = PathBuf::from("/foo");
+    /// x.push("/bar");
+    /// assert_eq!(x, PathBuf::from("/bar"));
+    /// ```
+    /// Could be written:
+    ///
+    /// ```rust
+    /// use std::path::PathBuf;
+    ///
+    /// let mut x = PathBuf::from("/foo");
+    /// x.push("bar");
+    /// assert_eq!(x, PathBuf::from("/foo/bar"));
+    /// ```
+    pub PATH_BUF_PUSH_OVERWRITE,
+    correctness,
+    "calling `push` with file system root on `PathBuf` can overwrite it"
+}
+
+declare_lint_pass!(PathBufPushOverwrite => [PATH_BUF_PUSH_OVERWRITE]);
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for PathBufPushOverwrite {
+    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
+        if_chain! {
+            if let ExprKind::MethodCall(ref path, _, ref args) = expr.node;
+            if path.ident.name == "push";
+            if args.len() == 2;
+            if match_type(cx, walk_ptrs_ty(cx.tables.expr_ty(&args[0])), &paths::PATH_BUF);
+            if let Some(get_index_arg) = args.get(1);
+            if let ExprKind::Lit(ref lit) = get_index_arg.node;
+            if let LitKind::Str(ref path_lit, _) = lit.node;
+            if let pushed_path = Path::new(&path_lit.as_str());
+            if let Some(pushed_path_lit) = pushed_path.to_str();
+            if pushed_path.has_root();
+            if let Some(root) = pushed_path.components().next();
+            if root == Component::RootDir;
+            then {
+                span_lint_and_sugg(
+                    cx,
+                    PATH_BUF_PUSH_OVERWRITE,
+                    lit.span,
+                    "Calling `push` with '/' or '\\' (file system root) will overwrite the previous path definition",
+                    "try",
+                    format!("\"{}\"", pushed_path_lit.trim_start_matches(|c| c == '/' || c == '\\')),
+                    Applicability::MachineApplicable,
+                );
+            }
+        }
+    }
+}

tests/ui/path_buf_push_overwrite.fixed

@@ -0,0 +1,7 @@
+// run-rustfix
+use std::path::PathBuf;
+
+fn main() {
+    let mut x = PathBuf::from("/foo");
+    x.push("bar");
+}

tests/ui/path_buf_push_overwrite.rs

@@ -0,0 +1,7 @@
+// run-rustfix
+use std::path::PathBuf;
+
+fn main() {
+    let mut x = PathBuf::from("/foo");
+    x.push("/bar");
+}

tests/ui/path_buf_push_overwrite.stderr

@@ -0,0 +1,10 @@
+error: Calling `push` with '/' or '/' (file system root) will overwrite the previous path definition
+  --> $DIR/path_buf_push_overwrite.rs:6:12
+   |
+LL |     x.push("/bar");
+   |            ^^^^^^ help: try: `"bar"`
+   |
+   = note: #[deny(clippy::path_buf_push_overwrite)] on by default
+
+error: aborting due to previous error
+