diff --git a/CHANGELOG.md b/CHANGELOG.md index 437f08dd777a..b716a6cf3250 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4702,6 +4702,7 @@ Released 2018-09-13 [`await_holding_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_lock [`await_holding_refcell_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_refcell_ref [`bad_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#bad_bit_mask +[`bare_dos_device_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#bare_dos_device_names [`big_endian_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#big_endian_bytes [`bind_instead_of_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#bind_instead_of_map [`blacklisted_name`]: https://rust-lang.github.io/rust-clippy/master/index.html#blacklisted_name diff --git a/clippy_lints/src/bare_dos_device_names.rs b/clippy_lints/src/bare_dos_device_names.rs new file mode 100644 index 000000000000..a9cfad17ab97 --- /dev/null +++ b/clippy_lints/src/bare_dos_device_names.rs @@ -0,0 +1,225 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::paths::PATH_NEW; +use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::{get_parent_expr, is_from_proc_macro, match_def_path, path_res}; +use rustc_ast::{LitKind, StrStyle}; +use rustc_errors::Applicability; +use rustc_hir::def_id::DefId; +use rustc_hir::{Expr, ExprKind, QPath}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_middle::ty; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::{sym, Symbol}; +use std::borrow::Cow; + +declare_clippy_lint! { + /// ### What it does + /// Checks for paths implicitly referring to DOS devices. + /// + /// ### Why is this bad? + /// This will lead to unexpected path transformations on Windows. Usually, the programmer will + /// have intended to refer to a file/folder instead. + /// + /// ### Example + /// ```rust,ignore + /// let _ = PathBuf::from("CON"); + /// ``` + /// Use instead: + /// ```rust,ignore + /// // If this was unintended: + /// let _ = PathBuf::from("./CON"); + /// // To silence the lint: + /// let _ = PathBuf::from(r"\\.\CON"); + /// ``` + #[clippy::version = "1.72.0"] + pub BARE_DOS_DEVICE_NAMES, + suspicious, + "usage of paths that, on Windows, will implicitly refer to a DOS device" +} +declare_lint_pass!(BareDosDeviceNames => [BARE_DOS_DEVICE_NAMES]); + +impl<'tcx> LateLintPass<'tcx> for BareDosDeviceNames { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + if !in_external_macro(cx.sess(), expr.span) + && let ExprKind::Lit(arg) = expr.kind + && let LitKind::Str(str_sym, str_style) = arg.node + && matches!( + &*str_sym.as_str().to_ascii_lowercase(), + "aux" + | "con" + | "conin$" + // ^^^^^^ + | "conout$" + // ^^^^^^^ + // TODO: Maybe these two can be an exception. + // + // Using `CONIN$` and `CONOUT$` is common enough in other languages that it may + // trip up a couple newbies coming to rust. Besides, it's unlikely someone will + // ever use `CONIN$` as a filename. + // + // TODO: Perhaps `com10` etc. are also DOS device names? `com42` is used in + // `starship-rs` so perhaps they are. But this needs confirmation. + | "com1" + | "com2" + | "com3" + | "com4" + | "com5" + | "com6" + | "com7" + | "com8" + | "com9" + | "lpt1" + | "lpt2" + | "lpt3" + | "lpt4" + | "lpt5" + | "lpt6" + | "lpt7" + | "lpt8" + | "lpt9" + | "nul" + | "prn" + ) + && let Some(parent) = get_parent_expr(cx, expr) + && (is_path_like_constructor(cx, parent) || is_path_like_ty(cx, expr, parent)) + && !is_from_proc_macro(cx, expr) + { + span_lint_and_then( + cx, + BARE_DOS_DEVICE_NAMES, + expr.span, + "this path refers to a DOS device", + |diag| { + // Keep `r###` and `###` + let (prefix, hashes) = if let StrStyle::Raw(num) = str_style { + (Cow::Borrowed("r"), "#".repeat(num as usize).into()) + } else { + (Cow::Borrowed(""), Cow::Borrowed("")) + }; + + // Suggest making current behavior explicit + diag.span_suggestion_verbose( + expr.span, + "if this is intended, use", + format!(r#"r{hashes}"\\.\{str_sym}"{hashes}"#), + Applicability::MaybeIncorrect, + ) + // Suggest making the code refer to a file or folder in the current directory + .span_suggestion_verbose( + expr.span, + "if this was intended to point to a file or folder, use", + format!(r#"{prefix}{hashes}"./{str_sym}"{hashes}"#), + Applicability::MaybeIncorrect, + ); + } + ); + } + } +} + +/// Gets whether the `Expr` is an argument to path type constructors. The caller must provide the +/// parent `Expr`, for performance's sake. +/// +/// We can't use `is_path_ty` as these take `AsRef` or similar. +fn is_path_like_constructor(cx: &LateContext<'_>, parent: &Expr<'_>) -> bool { + enum DefPathOrTyAndName { + /// Something from `clippy_utils::paths`. + DefPath(&'static [&'static str]), + /// The type's name and the method's name. The type must be a diagnostic item and not its + /// constructor. + /// + /// Currently, this is only used for `PathBuf::from`. `PathBuf::from` is unfortunately + /// tricky, as all we end up having for `match_def_path` is `core::convert::From::from`, + /// not `std::path::PathBuf::from`. Basically useless. + TyAndName((Symbol, Symbol)), + } + // Provides no additional clarity + use DefPathOrTyAndName::{DefPath, TyAndName}; + + const LINTED_METHODS: &[DefPathOrTyAndName] = &[DefPath(&PATH_NEW), TyAndName((sym::PathBuf, sym::from))]; + + if let ExprKind::Call(path, _) = parent.kind + && let ExprKind::Path(qpath) = path.kind + && let QPath::TypeRelative(ty, last_segment) = qpath + && let Some(call_def_id) = path_res(cx, path).opt_def_id() + && let Some(ty_def_id) = path_res(cx, ty).opt_def_id() + && LINTED_METHODS.iter().any(|method| match method { + DefPath(path) => match_def_path(cx, call_def_id, path), + TyAndName((ty_name, method_name)) => { + cx.tcx.is_diagnostic_item(*ty_name, ty_def_id) && last_segment.ident.name == *method_name + }, + }) + { + return true; + } + + false +} + +/// Gets the `DefId` and arguments of `expr`, if it's a `Call` or `MethodCall` +/// +/// TODO: Move this to `clippy_utils` and extend it to give more info (not just `DefId` and +/// arguments). There are many lints that often need this sorta functionality. Most recently +/// `incorrect_partial_ord_impl_on_ord_type`, but basically all `methods` lints can use this to lint +/// `Self::method(self)` as well. +fn get_def_id_and_args<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<(DefId, &'tcx [Expr<'tcx>])> { + match expr.kind { + ExprKind::Call(path, args) => Some((path_res(cx, path).opt_def_id()?, args)), + ExprKind::MethodCall(_, _, args, _) => Some((cx.typeck_results().type_dependent_def_id(expr.hir_id)?, args)), + _ => None, + } +} + +/// Given a `Ty`, returns whether it is likely a path type, like `Path` or `PathBuf`. Also returns +/// true if it's `impl AsRef`, `T: AsRef`, etc. You get the idea. +fn is_path_like_ty<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, parent: &'tcx Expr<'tcx>) -> bool { + const LINTED_TRAITS: &[(Symbol, Symbol)] = &[ + (sym::AsRef, sym::Path), + (sym::AsMut, sym::Path), + (sym::AsRef, sym::PathBuf), + (sym::AsMut, sym::PathBuf), + (sym::Into, sym::Path), + (sym::Into, sym::PathBuf), + (sym::From, sym::Path), + (sym::From, sym::PathBuf), + ]; + + let Some((callee, callee_args)) = get_def_id_and_args(cx, parent) else { + return false; + }; + let Some(arg_index) = callee_args.iter().position(|arg| arg.hir_id == expr.hir_id) else { + return false; + }; + let arg_ty = cx.tcx.fn_sig(callee).subst_identity().inputs().skip_binder()[arg_index].peel_refs(); + + // If we find `PathBuf` or `Path`, no need to check `impl ` or `T`. + if let Some(def) = arg_ty.ty_adt_def() + && let def_id = def.did() + && (cx.tcx.is_diagnostic_item(sym::PathBuf, def_id) || cx.tcx.is_diagnostic_item(sym::Path, def_id)) + { + return true; + } + + for predicate in cx + .tcx + .param_env(callee) + .caller_bounds() + .iter() + .filter_map(|predicate| predicate.kind().no_bound_vars()) + { + if let ty::ClauseKind::Trait(trit) = predicate + && trit.trait_ref.self_ty() == arg_ty + // I believe `0` is always `Self`, i.e., `T` or `impl ` so get `1` instead + && let [_, subst] = trit.trait_ref.substs.as_slice() + && let Some(as_ref_ty) = subst.as_type() + && LINTED_TRAITS.iter().any(|(trait_sym, ty_sym)| { + cx.tcx.is_diagnostic_item(*trait_sym, trit.trait_ref.def_id) + && is_type_diagnostic_item(cx, as_ref_ty, *ty_sym) + }) { + return true; + } + } + + false +} diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index a0d181be3b18..1a3e9849931f 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -61,6 +61,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::await_holding_invalid::AWAIT_HOLDING_INVALID_TYPE_INFO, crate::await_holding_invalid::AWAIT_HOLDING_LOCK_INFO, crate::await_holding_invalid::AWAIT_HOLDING_REFCELL_REF_INFO, + crate::bare_dos_device_names::BARE_DOS_DEVICE_NAMES_INFO, crate::blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS_INFO, crate::bool_assert_comparison::BOOL_ASSERT_COMPARISON_INFO, crate::bool_to_int_with_if::BOOL_TO_INT_WITH_IF_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 1609eb396b43..1fbd288e011e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -76,6 +76,7 @@ mod assertions_on_result_states; mod async_yields_async; mod attrs; mod await_holding_invalid; +mod bare_dos_device_names; mod blocks_in_if_conditions; mod bool_assert_comparison; mod bool_to_int_with_if; @@ -1082,6 +1083,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|_| Box::new(manual_float_methods::ManualFloatMethods)); store.register_late_pass(|_| Box::new(four_forward_slashes::FourForwardSlashes)); store.register_late_pass(|_| Box::new(error_impl_error::ErrorImplError)); + store.register_late_pass(move |_| Box::new(bare_dos_device_names::BareDosDeviceNames)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index f3677e6f6141..25f1df262fdd 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -68,6 +68,7 @@ pub const PARKING_LOT_RWLOCK_READ_GUARD: [&str; 3] = ["lock_api", "rwlock", "RwL pub const PARKING_LOT_RWLOCK_WRITE_GUARD: [&str; 3] = ["lock_api", "rwlock", "RwLockWriteGuard"]; pub const PATH_BUF_AS_PATH: [&str; 4] = ["std", "path", "PathBuf", "as_path"]; pub const PATH_MAIN_SEPARATOR: [&str; 3] = ["std", "path", "MAIN_SEPARATOR"]; +pub const PATH_NEW: [&str; 4] = ["std", "path", "Path", "new"]; pub const PATH_TO_PATH_BUF: [&str; 4] = ["std", "path", "Path", "to_path_buf"]; pub const PEEKABLE: [&str; 5] = ["core", "iter", "adapters", "peekable", "Peekable"]; pub const PERMISSIONS: [&str; 3] = ["std", "fs", "Permissions"]; diff --git a/tests/ui/bare_dos_device_names.rs b/tests/ui/bare_dos_device_names.rs new file mode 100644 index 000000000000..bddae3b7b847 --- /dev/null +++ b/tests/ui/bare_dos_device_names.rs @@ -0,0 +1,34 @@ +//@aux-build:proc_macros.rs:proc-macro +#![allow(clippy::needless_raw_string_hashes, clippy::no_effect, unused)] +#![warn(clippy::bare_dos_device_names)] + +#[macro_use] +extern crate proc_macros; + +use std::fs::File; +use std::path::{Path, PathBuf}; + +fn a>(t: T) {} + +fn b(t: impl AsRef) {} + +// TODO: More tests for traits. + +fn main() { + a("con"); + b("conin$"); + File::open("conin$"); + std::path::PathBuf::from("a"); + // Keep raw string + Path::new(r##"aux"##); + // Don't lint + PathBuf::from("a"); + Path::new("a"); + external! { + a("con"); + } + with_span! { + span + a("con"); + } +} diff --git a/tests/ui/bare_dos_device_names.stderr b/tests/ui/bare_dos_device_names.stderr new file mode 100644 index 000000000000..882e7dcc9c16 --- /dev/null +++ b/tests/ui/bare_dos_device_names.stderr @@ -0,0 +1,63 @@ +error: this path refers to a DOS device + --> $DIR/bare_dos_device_names.rs:18:7 + | +LL | a("con"); + | ^^^^^ + | + = note: `-D clippy::bare-dos-device-names` implied by `-D warnings` +help: if this is intended, use + | +LL | a(r"//./con"); + | ~~~~~~~~~~ +help: if this was intended to point to a file or folder, use + | +LL | a("./con"); + | ~~~~~~~ + +error: this path refers to a DOS device + --> $DIR/bare_dos_device_names.rs:19:7 + | +LL | b("conin$"); + | ^^^^^^^^ + | +help: if this is intended, use + | +LL | b(r"//./conin$"); + | ~~~~~~~~~~~~~ +help: if this was intended to point to a file or folder, use + | +LL | b("./conin$"); + | ~~~~~~~~~~ + +error: this path refers to a DOS device + --> $DIR/bare_dos_device_names.rs:20:16 + | +LL | File::open("conin$"); + | ^^^^^^^^ + | +help: if this is intended, use + | +LL | File::open(r"//./conin$"); + | ~~~~~~~~~~~~~ +help: if this was intended to point to a file or folder, use + | +LL | File::open("./conin$"); + | ~~~~~~~~~~ + +error: this path refers to a DOS device + --> $DIR/bare_dos_device_names.rs:23:15 + | +LL | Path::new(r##"aux"##); + | ^^^^^^^^^^ + | +help: if this is intended, use + | +LL | Path::new(r##"//./aux"##); + | ~~~~~~~~~~~~~~ +help: if this was intended to point to a file or folder, use + | +LL | Path::new(r##"./aux"##); + | ~~~~~~~~~~~~ + +error: aborting due to 4 previous errors +