From 981e96008b58d34e9c52b0aa3b318974c75b9d1d Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Mon, 11 Sep 2023 22:46:15 +0200 Subject: [PATCH] new lint: `path_ends_with_ext` --- CHANGELOG.md | 2 + book/src/lint_configuration.md | 10 ++++ clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 7 +++ clippy_lints/src/methods/mod.rs | 47 ++++++++++++++++ .../src/methods/path_ends_with_ext.rs | 53 +++++++++++++++++++ clippy_lints/src/utils/conf.rs | 5 ++ .../toml_unknown_key/conf_unknown_key.stderr | 2 + tests/ui/path_ends_with_ext.fixed | 36 +++++++++++++ tests/ui/path_ends_with_ext.rs | 36 +++++++++++++ tests/ui/path_ends_with_ext.stderr | 17 ++++++ 11 files changed, 216 insertions(+) create mode 100644 clippy_lints/src/methods/path_ends_with_ext.rs create mode 100644 tests/ui/path_ends_with_ext.fixed create mode 100644 tests/ui/path_ends_with_ext.rs create mode 100644 tests/ui/path_ends_with_ext.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 9aca13a6cf64..5e01e8d07bd7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5245,6 +5245,7 @@ Released 2018-09-13 [`partialeq_ne_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#partialeq_ne_impl [`partialeq_to_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#partialeq_to_none [`path_buf_push_overwrite`]: https://rust-lang.github.io/rust-clippy/master/index.html#path_buf_push_overwrite +[`path_ends_with_ext`]: https://rust-lang.github.io/rust-clippy/master/index.html#path_ends_with_ext [`pattern_type_mismatch`]: https://rust-lang.github.io/rust-clippy/master/index.html#pattern_type_mismatch [`permissions_set_readonly_false`]: https://rust-lang.github.io/rust-clippy/master/index.html#permissions_set_readonly_false [`positional_named_format_parameters`]: https://rust-lang.github.io/rust-clippy/master/index.html#positional_named_format_parameters @@ -5575,5 +5576,6 @@ Released 2018-09-13 [`allow-one-hash-in-raw-strings`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-one-hash-in-raw-strings [`absolute-paths-max-segments`]: https://doc.rust-lang.org/clippy/lint_configuration.html#absolute-paths-max-segments [`absolute-paths-allowed-crates`]: https://doc.rust-lang.org/clippy/lint_configuration.html#absolute-paths-allowed-crates +[`allowed-dotfiles`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allowed-dotfiles [`enforce-iter-loop-reborrow`]: https://doc.rust-lang.org/clippy/lint_configuration.html#enforce-iter-loop-reborrow diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index 52c795e04fe9..3aeb6206fca9 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -751,6 +751,16 @@ Which crates to allow absolute paths from * [`absolute_paths`](https://rust-lang.github.io/rust-clippy/master/index.html#absolute_paths) +## `allowed-dotfiles` +Additional dotfiles (files or directories starting with a dot) to allow + +**Default Value:** `{}` (`rustc_data_structures::fx::FxHashSet`) + +--- +**Affected lints:** +* [`path_ends_with_ext`](https://rust-lang.github.io/rust-clippy/master/index.html#path_ends_with_ext) + + ## `enforce-iter-loop-reborrow` #### Example ``` diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index ecf44e05920a..4772f3d80371 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -399,6 +399,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::methods::OR_FUN_CALL_INFO, crate::methods::OR_THEN_UNWRAP_INFO, crate::methods::PATH_BUF_PUSH_OVERWRITE_INFO, + crate::methods::PATH_ENDS_WITH_EXT_INFO, crate::methods::RANGE_ZIP_WITH_LEN_INFO, crate::methods::READONLY_WRITE_LOCK_INFO, crate::methods::READ_LINE_WITHOUT_TRIM_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3f6bdb2b62cc..8f603d5d9881 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -662,12 +662,19 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: let allow_unwrap_in_tests = conf.allow_unwrap_in_tests; let suppress_restriction_lint_in_const = conf.suppress_restriction_lint_in_const; store.register_late_pass(move |_| Box::new(approx_const::ApproxConstant::new(msrv()))); + let allowed_dotfiles = conf + .allowed_dotfiles + .iter() + .cloned() + .chain(methods::DEFAULT_ALLOWED_DOTFILES.iter().copied().map(ToOwned::to_owned)) + .collect::>(); store.register_late_pass(move |_| { Box::new(methods::Methods::new( avoid_breaking_exported_api, msrv(), allow_expect_in_tests, allow_unwrap_in_tests, + allowed_dotfiles.clone(), )) }); store.register_late_pass(move |_| Box::new(matches::Matches::new(msrv()))); diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 81223fa8d954..f46823648068 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -74,6 +74,7 @@ mod option_map_unwrap_or; mod or_fun_call; mod or_then_unwrap; mod path_buf_push_overwrite; +mod path_ends_with_ext; mod range_zip_with_len; mod read_line_without_trim; mod readonly_write_lock; @@ -120,6 +121,8 @@ use clippy_utils::msrvs::{self, Msrv}; use clippy_utils::ty::{contains_ty_adt_constructor_opaque, implements_trait, is_copy, is_type_diagnostic_item}; use clippy_utils::{contains_return, is_bool, is_trait_method, iter_input_pats, peel_blocks, return_ty}; use if_chain::if_chain; +pub use path_ends_with_ext::DEFAULT_ALLOWED_DOTFILES; +use rustc_data_structures::fx::FxHashSet; use rustc_hir as hir; use rustc_hir::{Expr, ExprKind, Node, Stmt, StmtKind, TraitItem, TraitItemKind}; use rustc_hir_analysis::hir_ty_to_ty; @@ -3563,11 +3566,51 @@ declare_clippy_lint! { "calls to `.take()` or `.skip()` that are out of bounds" } +declare_clippy_lint! { + /// ### What it does + /// Looks for calls to `Path::ends_with` calls where the argument looks like a file extension. + /// + /// By default, Clippy has a short list of known filenames that start with a dot + /// but aren't necessarily file extensions (e.g. the `.git` folder), which are allowed by default. + /// The `allowed-dotfiles` configuration can be used to allow additional + /// file extensions that Clippy should not lint. + /// + /// ### Why is this bad? + /// This doesn't actually compare file extensions. Rather, `ends_with` compares the given argument + /// to the last **component** of the path and checks if it matches exactly. + /// + /// ### Known issues + /// File extensions are often at most three characters long, so this only lints in those cases + /// in an attempt to avoid false positives. + /// Any extension names longer than that are assumed to likely be real path components and are + /// therefore ignored. + /// + /// ### Example + /// ```rust + /// # use std::path::Path; + /// fn is_markdown(path: &Path) -> bool { + /// path.ends_with(".md") + /// } + /// ``` + /// Use instead: + /// ```rust + /// # use std::path::Path; + /// fn is_markdown(path: &Path) -> bool { + /// path.extension().is_some_and(|ext| ext == "md") + /// } + /// ``` + #[clippy::version = "1.74.0"] + pub PATH_ENDS_WITH_EXT, + suspicious, + "attempting to compare file extensions using `Path::ends_with`" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, allow_expect_in_tests: bool, allow_unwrap_in_tests: bool, + allowed_dotfiles: FxHashSet, } impl Methods { @@ -3577,12 +3620,14 @@ impl Methods { msrv: Msrv, allow_expect_in_tests: bool, allow_unwrap_in_tests: bool, + allowed_dotfiles: FxHashSet, ) -> Self { Self { avoid_breaking_exported_api, msrv, allow_expect_in_tests, allow_unwrap_in_tests, + allowed_dotfiles, } } } @@ -3703,6 +3748,7 @@ impl_lint_pass!(Methods => [ FILTER_MAP_BOOL_THEN, READONLY_WRITE_LOCK, ITER_OUT_OF_BOUNDS, + PATH_ENDS_WITH_EXT, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -3978,6 +4024,7 @@ impl Methods { if let ExprKind::MethodCall(.., span) = expr.kind { case_sensitive_file_extension_comparisons::check(cx, expr, span, recv, arg); } + path_ends_with_ext::check(cx, recv, arg, expr, &self.msrv, &self.allowed_dotfiles); }, ("expect", [_]) => { match method_call(recv) { diff --git a/clippy_lints/src/methods/path_ends_with_ext.rs b/clippy_lints/src/methods/path_ends_with_ext.rs new file mode 100644 index 000000000000..3347c8c16201 --- /dev/null +++ b/clippy_lints/src/methods/path_ends_with_ext.rs @@ -0,0 +1,53 @@ +use super::PATH_ENDS_WITH_EXT; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::msrvs; +use clippy_utils::msrvs::Msrv; +use clippy_utils::source::snippet; +use clippy_utils::ty::is_type_diagnostic_item; +use rustc_ast::{LitKind, StrStyle}; +use rustc_data_structures::fx::FxHashSet; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::LateContext; +use rustc_span::sym; +use std::fmt::Write; + +pub const DEFAULT_ALLOWED_DOTFILES: &[&str] = &[ + "git", "svn", "gem", "npm", "vim", "env", "rnd", "ssh", "vnc", "smb", "nvm", "bin", +]; + +pub(super) fn check( + cx: &LateContext<'_>, + recv: &Expr<'_>, + path: &Expr<'_>, + expr: &Expr<'_>, + msrv: &Msrv, + allowed_dotfiles: &FxHashSet, +) { + if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv).peel_refs(), sym::Path) + && !path.span.from_expansion() + && let ExprKind::Lit(lit) = path.kind + && let LitKind::Str(path, StrStyle::Cooked) = lit.node + && let Some(path) = path.as_str().strip_prefix('.') + && (1..=3).contains(&path.len()) + && !allowed_dotfiles.contains(path) + && path.chars().all(char::is_alphanumeric) + { + let mut sugg = snippet(cx, recv.span, "..").into_owned(); + if msrv.meets(msrvs::OPTION_IS_SOME_AND) { + let _ = write!(sugg, r#".extension().is_some_and(|ext| ext == "{path}")"#); + } else { + let _ = write!(sugg, r#".extension().map_or(false, |ext| ext == "{path}")"#); + }; + + span_lint_and_sugg( + cx, + PATH_ENDS_WITH_EXT, + expr.span, + "this looks like a failed attempt at checking for the file extension", + "try", + sugg, + Applicability::MaybeIncorrect + ); + } +} diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index 268894755225..e1280ef38ef7 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -561,6 +561,11 @@ define_Conf! { /// Which crates to allow absolute paths from (absolute_paths_allowed_crates: rustc_data_structures::fx::FxHashSet = rustc_data_structures::fx::FxHashSet::default()), + /// Lint: PATH_ENDS_WITH_EXT. + /// + /// Additional dotfiles (files or directories starting with a dot) to allow + (allowed_dotfiles: rustc_data_structures::fx::FxHashSet = + rustc_data_structures::fx::FxHashSet::default()), /// Lint: EXPLICIT_ITER_LOOP /// /// Whether to recommend using implicit into iter for reborrowed values. diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index b97bb144468b..4bed5c149f51 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -10,6 +10,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect allow-print-in-tests allow-private-module-inception allow-unwrap-in-tests + allowed-dotfiles allowed-idents-below-min-chars allowed-scripts arithmetic-side-effects-allowed @@ -82,6 +83,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect allow-print-in-tests allow-private-module-inception allow-unwrap-in-tests + allowed-dotfiles allowed-idents-below-min-chars allowed-scripts arithmetic-side-effects-allowed diff --git a/tests/ui/path_ends_with_ext.fixed b/tests/ui/path_ends_with_ext.fixed new file mode 100644 index 000000000000..49767e242cee --- /dev/null +++ b/tests/ui/path_ends_with_ext.fixed @@ -0,0 +1,36 @@ +#![warn(clippy::path_ends_with_ext)] +use std::path::Path; + +macro_rules! arg { + () => { + ".md" + }; +} + +fn test(path: &Path) { + path.extension().is_some_and(|ext| ext == "md"); + //~^ ERROR: this looks like a failed attempt at checking for the file extension + + // some "extensions" are allowed by default + path.ends_with(".git"); + + // most legitimate "dotfiles" are longer than 3 chars, so we allow them as well + path.ends_with(".bashrc"); + + // argument from expn shouldn't trigger + path.ends_with(arg!()); + + path.ends_with(".."); + path.ends_with("./a"); + path.ends_with("."); + path.ends_with(""); +} + +// is_some_and was stabilized in 1.70, so suggest map_or(false, ..) if under that +#[clippy::msrv = "1.69"] +fn under_msv(path: &Path) -> bool { + path.extension().map_or(false, |ext| ext == "md") + //~^ ERROR: this looks like a failed attempt at checking for the file extension +} + +fn main() {} diff --git a/tests/ui/path_ends_with_ext.rs b/tests/ui/path_ends_with_ext.rs new file mode 100644 index 000000000000..2dfd046218af --- /dev/null +++ b/tests/ui/path_ends_with_ext.rs @@ -0,0 +1,36 @@ +#![warn(clippy::path_ends_with_ext)] +use std::path::Path; + +macro_rules! arg { + () => { + ".md" + }; +} + +fn test(path: &Path) { + path.ends_with(".md"); + //~^ ERROR: this looks like a failed attempt at checking for the file extension + + // some "extensions" are allowed by default + path.ends_with(".git"); + + // most legitimate "dotfiles" are longer than 3 chars, so we allow them as well + path.ends_with(".bashrc"); + + // argument from expn shouldn't trigger + path.ends_with(arg!()); + + path.ends_with(".."); + path.ends_with("./a"); + path.ends_with("."); + path.ends_with(""); +} + +// is_some_and was stabilized in 1.70, so suggest map_or(false, ..) if under that +#[clippy::msrv = "1.69"] +fn under_msv(path: &Path) -> bool { + path.ends_with(".md") + //~^ ERROR: this looks like a failed attempt at checking for the file extension +} + +fn main() {} diff --git a/tests/ui/path_ends_with_ext.stderr b/tests/ui/path_ends_with_ext.stderr new file mode 100644 index 000000000000..a73ab4d08e9c --- /dev/null +++ b/tests/ui/path_ends_with_ext.stderr @@ -0,0 +1,17 @@ +error: this looks like a failed attempt at checking for the file extension + --> $DIR/path_ends_with_ext.rs:11:5 + | +LL | path.ends_with(".md"); + | ^^^^^^^^^^^^^^^^^^^^^ help: try: `path.extension().is_some_and(|ext| ext == "md")` + | + = note: `-D clippy::path-ends-with-ext` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::path_ends_with_ext)]` + +error: this looks like a failed attempt at checking for the file extension + --> $DIR/path_ends_with_ext.rs:32:5 + | +LL | path.ends_with(".md") + | ^^^^^^^^^^^^^^^^^^^^^ help: try: `path.extension().map_or(false, |ext| ext == "md")` + +error: aborting due to 2 previous errors +