From dbe9dcdcbf5614ddc25561e8c5facd6f9d40b490 Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Sun, 2 Jul 2023 11:04:36 -0500 Subject: [PATCH 1/3] New lint `bare_dos_device_names` --- CHANGELOG.md | 1 + clippy_lints/src/bare_dos_device_names.rs | 186 ++++++++++++++++++++++ clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_utils/src/paths.rs | 1 + tests/ui/bare_dos_device_names.rs | 30 ++++ tests/ui/bare_dos_device_names.stderr | 48 ++++++ 7 files changed, 269 insertions(+) create mode 100644 clippy_lints/src/bare_dos_device_names.rs create mode 100644 tests/ui/bare_dos_device_names.rs create mode 100644 tests/ui/bare_dos_device_names.stderr 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..7806d5e6358f --- /dev/null +++ b/clippy_lints/src/bare_dos_device_names.rs @@ -0,0 +1,186 @@ +use clippy_utils::{ + diagnostics::span_lint_and_then, get_parent_expr, is_from_proc_macro, match_def_path, path_res, paths::PATH_NEW, + ty::is_type_diagnostic_item, +}; +use rustc_ast::LitKind; +use rustc_errors::Applicability; +use rustc_hir::def_id::DefId; +use rustc_hir::*; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::{lint::in_external_macro, ty}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::{sym, Symbol}; + +declare_clippy_lint! { + /// ### What it does + /// TODO please do soon + /// + /// ### Why is this bad? + /// TODO please + /// + /// ### Example + /// ```rust + /// // TODO + /// ``` + /// Use instead: + /// ```rust + /// // TODO + /// ``` + #[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, _) = 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. + | "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_buf_from_or_path_new(cx, parent) || is_path_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| { + // Suggest making current behavior explicit + diag.span_suggestion_verbose( + expr.span, + "if this is intended, try", + format!(r#""\\.\{str_sym}""#), + Applicability::MaybeIncorrect, + ); + + // Suggest making the code refer to a file or folder in the current directory + diag.span_suggestion_verbose( + expr.span, + "if this was intended to point to a file or folder, try", + format!("\"./{str_sym}\""), + Applicability::MaybeIncorrect, + ); + } + ); + } + } +} + +/// Gets whether the `Expr` is an argument to `Path::new` or `PathBuf::from`. The caller must +/// provide the parent `Expr`, for performance's sake. +/// +/// TODO: We can likely refactor this like we did with `LINTED_TRAITS`. +fn is_path_buf_from_or_path_new(cx: &LateContext<'_>, parent: &Expr<'_>) -> bool { + 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() + && (match_def_path(cx, call_def_id, &PATH_NEW) + // `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. + || cx.tcx.is_diagnostic_item(sym::PathBuf, ty_def_id) && last_segment.ident.as_str() == "from") + { + return true; + } + + false +} + +/// Gets the `DefId` and arguments of `expr`, if it's a `Call` or `MethodCall` +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_ty<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, parent: &'tcx Expr<'tcx>) -> bool { + const LINTED_TRAITS: &[(Symbol, Symbol)] = &[ + (sym::AsRef, sym::Path), + (sym::Into, sym::PathBuf), + (sym::Into, sym::Path), + // TODO: Let's add more traits here. + ]; + + 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`, so `T` or `impl ` + && let [_, subst] = trit.trait_ref.substs.as_slice() + && let Some(as_ref_ty) = subst.as_type() + { + for (trait_sym, ty_sym) in LINTED_TRAITS { + if 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..f6c0940a2083 --- /dev/null +++ b/tests/ui/bare_dos_device_names.rs @@ -0,0 +1,30 @@ +//@aux-build:proc_macros.rs:proc-macro +#![allow(clippy::no_effect, unused)] +#![warn(clippy::bare_dos_device_names)] + +#[macro_use] +extern crate proc_macros; + +use std::fs::File; +use std::path::Path; +use std::path::PathBuf; + +fn a>(t: T) {} + +fn b(t: impl AsRef) {} + +fn main() { + a("con"); + b("conin$"); + File::open("conin$"); + std::path::PathBuf::from("a"); + 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..5fa189c6c231 --- /dev/null +++ b/tests/ui/bare_dos_device_names.stderr @@ -0,0 +1,48 @@ +error: this path refers to a DOS device + --> $DIR/bare_dos_device_names.rs:17:7 + | +LL | a("con"); + | ^^^^^ + | + = note: `-D clippy::bare-dos-device-names` implied by `-D warnings` +help: if this is intended, try + | +LL | a("//./con"); + | ~~~~~~~~~ +help: if this was intended to point to a file or folder, try + | +LL | a("./con"); + | ~~~~~~~ + +error: this path refers to a DOS device + --> $DIR/bare_dos_device_names.rs:18:7 + | +LL | b("conin$"); + | ^^^^^^^^ + | +help: if this is intended, try + | +LL | b("//./conin$"); + | ~~~~~~~~~~~~ +help: if this was intended to point to a file or folder, try + | +LL | b("./conin$"); + | ~~~~~~~~~~ + +error: this path refers to a DOS device + --> $DIR/bare_dos_device_names.rs:19:16 + | +LL | File::open("conin$"); + | ^^^^^^^^ + | +help: if this is intended, try + | +LL | File::open("//./conin$"); + | ~~~~~~~~~~~~ +help: if this was intended to point to a file or folder, try + | +LL | File::open("./conin$"); + | ~~~~~~~~~~ + +error: aborting due to 3 previous errors + From eb8fb50407b033070e630bde59c6e687775daa8f Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Mon, 3 Jul 2023 08:27:00 -0500 Subject: [PATCH 2/3] Refactor and add description, rewrite `is_path_constructor` --- clippy_lints/src/bare_dos_device_names.rs | 73 ++++++++++++++++------- tests/ui/bare_dos_device_names.rs | 2 + tests/ui/bare_dos_device_names.stderr | 18 +++--- 3 files changed, 64 insertions(+), 29 deletions(-) diff --git a/clippy_lints/src/bare_dos_device_names.rs b/clippy_lints/src/bare_dos_device_names.rs index 7806d5e6358f..eaf070ddfe3c 100644 --- a/clippy_lints/src/bare_dos_device_names.rs +++ b/clippy_lints/src/bare_dos_device_names.rs @@ -5,7 +5,7 @@ use clippy_utils::{ use rustc_ast::LitKind; use rustc_errors::Applicability; use rustc_hir::def_id::DefId; -use rustc_hir::*; +use rustc_hir::{Expr, ExprKind, QPath}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::{lint::in_external_macro, ty}; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -13,18 +13,22 @@ use rustc_span::{sym, Symbol}; declare_clippy_lint! { /// ### What it does - /// TODO please do soon + /// Checks for paths implicitly referring to DOS devices. /// /// ### Why is this bad? - /// TODO please + /// This will lead to unexpected path transformations on Windows. Usually, the programmer will + /// have intended to refer to a file/folder instead. /// /// ### Example - /// ```rust - /// // TODO + /// ```rust,ignore + /// let _ = PathBuf::from("CON"); /// ``` /// Use instead: - /// ```rust - /// // TODO + /// ```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, @@ -73,7 +77,7 @@ impl<'tcx> LateLintPass<'tcx> for BareDosDeviceNames { | "prn" ) && let Some(parent) = get_parent_expr(cx, expr) - && (is_path_buf_from_or_path_new(cx, parent) || is_path_ty(cx, expr, parent)) + && (is_path_constructor(cx, parent) || is_path_ty(cx, expr, parent)) && !is_from_proc_macro(cx, expr) { span_lint_and_then( @@ -86,7 +90,8 @@ impl<'tcx> LateLintPass<'tcx> for BareDosDeviceNames { diag.span_suggestion_verbose( expr.span, "if this is intended, try", - format!(r#""\\.\{str_sym}""#), + // FIXME: I have zero clue why it normalizes this. `\` -> `/` + format!(r#"r"\\.\{str_sym}"\"#), Applicability::MaybeIncorrect, ); @@ -103,22 +108,43 @@ impl<'tcx> LateLintPass<'tcx> for BareDosDeviceNames { } } -/// Gets whether the `Expr` is an argument to `Path::new` or `PathBuf::from`. The caller must -/// provide the parent `Expr`, for performance's sake. +/// Gets whether the `Expr` is an argument to path type constructors. The caller must provide the +/// parent `Expr`, for performance's sake. /// -/// TODO: We can likely refactor this like we did with `LINTED_TRAITS`. -fn is_path_buf_from_or_path_new(cx: &LateContext<'_>, parent: &Expr<'_>) -> bool { +/// We can't use `is_path_ty` as these take `AsRef` or similar. +/// +/// TODO: Should we lint `OsStr` too, in `is_path_ty`? I personally don't think so. +fn is_path_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() - && (match_def_path(cx, call_def_id, &PATH_NEW) - // `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. - || cx.tcx.is_diagnostic_item(sym::PathBuf, ty_def_id) && last_segment.ident.as_str() == "from") { - return true; + return 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 + }, + } + }); } false @@ -138,8 +164,15 @@ fn get_def_id_and_args<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> fn is_path_ty<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, parent: &'tcx Expr<'tcx>) -> bool { const LINTED_TRAITS: &[(Symbol, Symbol)] = &[ (sym::AsRef, sym::Path), - (sym::Into, sym::PathBuf), + (sym::AsMut, sym::Path), + // Basically useless, but let's lint these anyway + (sym::AsRef, sym::PathBuf), + (sym::AsMut, sym::PathBuf), (sym::Into, sym::Path), + (sym::Into, sym::PathBuf), + // Never seen `From` used in a generic context before, but let's lint these anyway + (sym::From, sym::Path), + (sym::From, sym::PathBuf), // TODO: Let's add more traits here. ]; @@ -168,7 +201,7 @@ fn is_path_ty<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, parent: &'tc { if let ty::ClauseKind::Trait(trit) = predicate && trit.trait_ref.self_ty() == arg_ty - // I believe `0` is always `Self`, so `T` or `impl ` + // 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() { diff --git a/tests/ui/bare_dos_device_names.rs b/tests/ui/bare_dos_device_names.rs index f6c0940a2083..994208b60c26 100644 --- a/tests/ui/bare_dos_device_names.rs +++ b/tests/ui/bare_dos_device_names.rs @@ -13,6 +13,8 @@ fn a>(t: T) {} fn b(t: impl AsRef) {} +// TODO: More tests for traits. + fn main() { a("con"); b("conin$"); diff --git a/tests/ui/bare_dos_device_names.stderr b/tests/ui/bare_dos_device_names.stderr index 5fa189c6c231..ab27c4d13c23 100644 --- a/tests/ui/bare_dos_device_names.stderr +++ b/tests/ui/bare_dos_device_names.stderr @@ -1,5 +1,5 @@ error: this path refers to a DOS device - --> $DIR/bare_dos_device_names.rs:17:7 + --> $DIR/bare_dos_device_names.rs:19:7 | LL | a("con"); | ^^^^^ @@ -7,38 +7,38 @@ LL | a("con"); = note: `-D clippy::bare-dos-device-names` implied by `-D warnings` help: if this is intended, try | -LL | a("//./con"); - | ~~~~~~~~~ +LL | a(r"//./con"/); + | ~~~~~~~~~~~ help: if this was intended to point to a file or folder, try | LL | a("./con"); | ~~~~~~~ error: this path refers to a DOS device - --> $DIR/bare_dos_device_names.rs:18:7 + --> $DIR/bare_dos_device_names.rs:20:7 | LL | b("conin$"); | ^^^^^^^^ | help: if this is intended, try | -LL | b("//./conin$"); - | ~~~~~~~~~~~~ +LL | b(r"//./conin$"/); + | ~~~~~~~~~~~~~~ help: if this was intended to point to a file or folder, try | LL | b("./conin$"); | ~~~~~~~~~~ error: this path refers to a DOS device - --> $DIR/bare_dos_device_names.rs:19:16 + --> $DIR/bare_dos_device_names.rs:21:16 | LL | File::open("conin$"); | ^^^^^^^^ | help: if this is intended, try | -LL | File::open("//./conin$"); - | ~~~~~~~~~~~~ +LL | File::open(r"//./conin$"/); + | ~~~~~~~~~~~~~~ help: if this was intended to point to a file or folder, try | LL | File::open("./conin$"); From 9e8e96bc28af2f956ac31156020c4147d1ae5132 Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Mon, 3 Jul 2023 12:58:40 -0500 Subject: [PATCH 3/3] Remove FIXME and make it retain old raw string hashes --- clippy_lints/src/bare_dos_device_names.rs | 82 ++++++++++++----------- tests/ui/bare_dos_device_names.rs | 8 ++- tests/ui/bare_dos_device_names.stderr | 47 ++++++++----- 3 files changed, 80 insertions(+), 57 deletions(-) diff --git a/clippy_lints/src/bare_dos_device_names.rs b/clippy_lints/src/bare_dos_device_names.rs index eaf070ddfe3c..a9cfad17ab97 100644 --- a/clippy_lints/src/bare_dos_device_names.rs +++ b/clippy_lints/src/bare_dos_device_names.rs @@ -1,15 +1,17 @@ -use clippy_utils::{ - diagnostics::span_lint_and_then, get_parent_expr, is_from_proc_macro, match_def_path, path_res, paths::PATH_NEW, - ty::is_type_diagnostic_item, -}; -use rustc_ast::LitKind; +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, ty}; +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 @@ -41,7 +43,7 @@ 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, _) = arg.node + && let LitKind::Str(str_sym, str_style) = arg.node && matches!( &*str_sym.as_str().to_ascii_lowercase(), "aux" @@ -55,6 +57,9 @@ impl<'tcx> LateLintPass<'tcx> for BareDosDeviceNames { // 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" @@ -77,7 +82,7 @@ impl<'tcx> LateLintPass<'tcx> for BareDosDeviceNames { | "prn" ) && let Some(parent) = get_parent_expr(cx, expr) - && (is_path_constructor(cx, parent) || is_path_ty(cx, expr, parent)) + && (is_path_like_constructor(cx, parent) || is_path_like_ty(cx, expr, parent)) && !is_from_proc_macro(cx, expr) { span_lint_and_then( @@ -86,20 +91,25 @@ impl<'tcx> LateLintPass<'tcx> for BareDosDeviceNames { 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, try", - // FIXME: I have zero clue why it normalizes this. `\` -> `/` - format!(r#"r"\\.\{str_sym}"\"#), + "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 - diag.span_suggestion_verbose( + .span_suggestion_verbose( expr.span, - "if this was intended to point to a file or folder, try", - format!("\"./{str_sym}\""), + "if this was intended to point to a file or folder, use", + format!(r#"{prefix}{hashes}"./{str_sym}"{hashes}"#), Applicability::MaybeIncorrect, ); } @@ -112,9 +122,7 @@ impl<'tcx> LateLintPass<'tcx> for BareDosDeviceNames { /// parent `Expr`, for performance's sake. /// /// We can't use `is_path_ty` as these take `AsRef` or similar. -/// -/// TODO: Should we lint `OsStr` too, in `is_path_ty`? I personally don't think so. -fn is_path_constructor(cx: &LateContext<'_>, parent: &Expr<'_>) -> bool { +fn is_path_like_constructor(cx: &LateContext<'_>, parent: &Expr<'_>) -> bool { enum DefPathOrTyAndName { /// Something from `clippy_utils::paths`. DefPath(&'static [&'static str]), @@ -136,21 +144,25 @@ fn is_path_constructor(cx: &LateContext<'_>, parent: &Expr<'_>) -> bool { && 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 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)), @@ -161,19 +173,16 @@ fn get_def_id_and_args<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> /// 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_ty<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, parent: &'tcx Expr<'tcx>) -> bool { +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), - // Basically useless, but let's lint these anyway (sym::AsRef, sym::PathBuf), (sym::AsMut, sym::PathBuf), (sym::Into, sym::Path), (sym::Into, sym::PathBuf), - // Never seen `From` used in a generic context before, but let's lint these anyway (sym::From, sym::Path), (sym::From, sym::PathBuf), - // TODO: Let's add more traits here. ]; let Some((callee, callee_args)) = get_def_id_and_args(cx, parent) else { @@ -204,14 +213,11 @@ fn is_path_ty<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, parent: &'tc // 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() - { - for (trait_sym, ty_sym) in LINTED_TRAITS { - if cx.tcx.is_diagnostic_item(*trait_sym, trit.trait_ref.def_id) + && 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; - } - } + }) { + return true; } } diff --git a/tests/ui/bare_dos_device_names.rs b/tests/ui/bare_dos_device_names.rs index 994208b60c26..bddae3b7b847 100644 --- a/tests/ui/bare_dos_device_names.rs +++ b/tests/ui/bare_dos_device_names.rs @@ -1,13 +1,12 @@ //@aux-build:proc_macros.rs:proc-macro -#![allow(clippy::no_effect, unused)] +#![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; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; fn a>(t: T) {} @@ -20,6 +19,9 @@ fn main() { 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! { diff --git a/tests/ui/bare_dos_device_names.stderr b/tests/ui/bare_dos_device_names.stderr index ab27c4d13c23..882e7dcc9c16 100644 --- a/tests/ui/bare_dos_device_names.stderr +++ b/tests/ui/bare_dos_device_names.stderr @@ -1,48 +1,63 @@ error: this path refers to a DOS device - --> $DIR/bare_dos_device_names.rs:19:7 + --> $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, try +help: if this is intended, use | -LL | a(r"//./con"/); - | ~~~~~~~~~~~ -help: if this was intended to point to a file or folder, try +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:20:7 + --> $DIR/bare_dos_device_names.rs:19:7 | LL | b("conin$"); | ^^^^^^^^ | -help: if this is intended, try +help: if this is intended, use | -LL | b(r"//./conin$"/); - | ~~~~~~~~~~~~~~ -help: if this was intended to point to a file or folder, try +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:21:16 + --> $DIR/bare_dos_device_names.rs:20:16 | LL | File::open("conin$"); | ^^^^^^^^ | -help: if this is intended, try +help: if this is intended, use | -LL | File::open(r"//./conin$"/); - | ~~~~~~~~~~~~~~ -help: if this was intended to point to a file or folder, try +LL | File::open(r"//./conin$"); + | ~~~~~~~~~~~~~ +help: if this was intended to point to a file or folder, use | LL | File::open("./conin$"); | ~~~~~~~~~~ -error: aborting due to 3 previous errors +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