diff options
| author | Ashley Mannix <kodraus@hey.com> | 2021-01-18 21:53:22 +1000 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2021-01-18 21:53:22 +1000 |
| commit | 090ab8c02e356f82337bbad4e2289766acc2c40b (patch) | |
| tree | e5fd918db25875c2dad03a8698b75b82e09f5bda /src/tools/clippy | |
| parent | 86e0ff47a0d1afcbe9f0c8cdb54f60bb18da20df (diff) | |
| parent | 3e236b3d1d14d0f3361742c64db37f6a430d03ef (diff) | |
| download | rust-090ab8c02e356f82337bbad4e2289766acc2c40b.tar.gz rust-090ab8c02e356f82337bbad4e2289766acc2c40b.zip | |
Rollup merge of #81038 - flip1995:clippyup, r=Manishearth
Update Clippy Biweekly Clippy update r? ``@Manishearth``
Diffstat (limited to 'src/tools/clippy')
103 files changed, 2328 insertions, 248 deletions
diff --git a/src/tools/clippy/CHANGELOG.md b/src/tools/clippy/CHANGELOG.md index de8da99cdee..64864c2e278 100644 --- a/src/tools/clippy/CHANGELOG.md +++ b/src/tools/clippy/CHANGELOG.md @@ -6,11 +6,138 @@ document. ## Unreleased / In Rust Nightly -[b20d4c1...master](https://github.com/rust-lang/rust-clippy/compare/b20d4c1...master) +[4911ab1...master](https://github.com/rust-lang/rust-clippy/compare/4911ab1...master) + +## Rust 1.50 + +Current beta, release 2021-02-11 + +[b20d4c1...4911ab1](https://github.com/rust-lang/rust-clippy/compare/b20d4c1...4911ab1) + +### New Lints + +* [`suspicious_operation_groupings`] [#6086](https://github.com/rust-lang/rust-clippy/pull/6086) +* [`size_of_in_element_count`] [#6394](https://github.com/rust-lang/rust-clippy/pull/6394) +* [`unnecessary_wraps`] [#6070](https://github.com/rust-lang/rust-clippy/pull/6070) +* [`let_underscore_drop`] [#6305](https://github.com/rust-lang/rust-clippy/pull/6305) +* [`collapsible_match`] [#6402](https://github.com/rust-lang/rust-clippy/pull/6402) +* [`redundant_else`] [#6330](https://github.com/rust-lang/rust-clippy/pull/6330) +* [`zero_sized_map_values`] [#6218](https://github.com/rust-lang/rust-clippy/pull/6218) +* [`print_stderr`] [#6367](https://github.com/rust-lang/rust-clippy/pull/6367) +* [`string_from_utf8_as_bytes`] [#6134](https://github.com/rust-lang/rust-clippy/pull/6134) + +### Moves and Deprecations + +* Previously deprecated [`str_to_string`] and [`string_to_string`] have been un-deprecated + as `restriction` lints [#6333](https://github.com/rust-lang/rust-clippy/pull/6333) +* Deprecate [`panic_params`] lint. This is now available in rustc as `panic_fmt` + [#6351](https://github.com/rust-lang/rust-clippy/pull/6351) +* Move [`map_err_ignore`] to `restriction` + [#6416](https://github.com/rust-lang/rust-clippy/pull/6416) +* Move [`await_holding_refcell_ref`] to `pedantic` + [#6354](https://github.com/rust-lang/rust-clippy/pull/6354) +* Move [`await_holding_lock`] to `pedantic` + [#6354](https://github.com/rust-lang/rust-clippy/pull/6354) + +### Enhancements + +* Add the `unreadable-literal-lint-fractions` configuration to disable + the `unreadable_literal` lint for fractions + [#6421](https://github.com/rust-lang/rust-clippy/pull/6421) +* [`clone_on_copy`]: Now shows the type in the lint message + [#6443](https://github.com/rust-lang/rust-clippy/pull/6443) +* [`redundant_pattern_matching`]: Now also lints on `std::task::Poll` + [#6339](https://github.com/rust-lang/rust-clippy/pull/6339) +* [`redundant_pattern_matching`]: Additionally also lints on `std::net::IpAddr` + [#6377](https://github.com/rust-lang/rust-clippy/pull/6377) +* [`search_is_some`]: Now suggests `contains` instead of `find(foo).is_some()` + [#6119](https://github.com/rust-lang/rust-clippy/pull/6119) +* [`clone_double_ref`]: Now prints the reference type in the lint message + [#6442](https://github.com/rust-lang/rust-clippy/pull/6442) +* [`modulo_one`]: Now also lints on -1. + [#6360](https://github.com/rust-lang/rust-clippy/pull/6360) +* [`empty_loop`]: Now lints no_std crates, too + [#6205](https://github.com/rust-lang/rust-clippy/pull/6205) +* [`or_fun_call`]: Now also lints when indexing `HashMap` or `BTreeMap` + [#6267](https://github.com/rust-lang/rust-clippy/pull/6267) +* [`wrong_self_convention`]: Now also lints in trait definitions + [#6316](https://github.com/rust-lang/rust-clippy/pull/6316) +* [`needless_borrow`]: Print the type in the lint message + [#6449](https://github.com/rust-lang/rust-clippy/pull/6449) + +[msrv_readme]: https://github.com/rust-lang/rust-clippy#specifying-the-minimum-supported-rust-version + +### False Positive Fixes + +* [`manual_range_contains`]: No longer lints in `const fn` + [#6382](https://github.com/rust-lang/rust-clippy/pull/6382) +* [`unnecessary_lazy_evaluations`]: No longer lints if closure argument is used + [#6370](https://github.com/rust-lang/rust-clippy/pull/6370) +* [`match_single_binding`]: Now ignores cases with `#[cfg()]` macros + [#6435](https://github.com/rust-lang/rust-clippy/pull/6435) +* [`match_like_matches_macro`]: No longer lints on arms with attributes + [#6290](https://github.com/rust-lang/rust-clippy/pull/6290) +* [`map_clone`]: No longer lints with deref and clone + [#6269](https://github.com/rust-lang/rust-clippy/pull/6269) +* [`map_clone`]: No longer lints in the case of &mut + [#6301](https://github.com/rust-lang/rust-clippy/pull/6301) +* [`needless_update`]: Now ignores `non_exhaustive` structs + [#6464](https://github.com/rust-lang/rust-clippy/pull/6464) +* [`needless_collect`]: No longer lints when a collect is needed multiple times + [#6313](https://github.com/rust-lang/rust-clippy/pull/6313) +* [`unnecessary_cast`] No longer lints cfg-dependent types + [#6369](https://github.com/rust-lang/rust-clippy/pull/6369) +* [`declare_interior_mutable_const`] and [`borrow_interior_mutable_const`]: + Both now ignore enums with frozen variants + [#6110](https://github.com/rust-lang/rust-clippy/pull/6110) + + +### Suggestion Fixes/Improvements + +* [`vec_box`]: Provide correct type scope suggestion + [#6271](https://github.com/rust-lang/rust-clippy/pull/6271) +* [`manual_range_contains`]: Give correct suggestion when using floats + [#6320](https://github.com/rust-lang/rust-clippy/pull/6320) +* [`unnecessary_lazy_evaluations`]: Don't always mark suggestion as MachineApplicable + [#6272](https://github.com/rust-lang/rust-clippy/pull/6272) +* [`manual_async_fn`]: Improve suggestion formatting + [#6294](https://github.com/rust-lang/rust-clippy/pull/6294) +* [`unnecessary_cast`]: Fix incorrectly formatted float literal suggestion + [#6362](https://github.com/rust-lang/rust-clippy/pull/6362) + +### ICE Fixes + +* Fix a crash in [`from_iter_instead_of_collect`] + [#6304](https://github.com/rust-lang/rust-clippy/pull/6304) +* Fix a silent crash when parsing doc comments in [`needless_doctest_main`] + [#6458](https://github.com/rust-lang/rust-clippy/pull/6458) + +### Documentation Improvements + +* The lint website search has been improved ([#6477](https://github.com/rust-lang/rust-clippy/pull/6477)): + * Searching for lints with dashes and spaces is possible now. For example + `missing-errors-doc` and `missing errors doc` are now valid aliases for lint names + * Improved fuzzy search in lint descriptions +* Various README improvements + [#6287](https://github.com/rust-lang/rust-clippy/pull/6287) +* Add known problems to [`comparison_chain`] documentation + [#6390](https://github.com/rust-lang/rust-clippy/pull/6390) +* Fix example used in [`cargo_common_metadata`] + [#6293](https://github.com/rust-lang/rust-clippy/pull/6293) +* Improve [`map_clone`] documentation + [#6340](https://github.com/rust-lang/rust-clippy/pull/6340) + +### Others + +* You can now tell Clippy about the MSRV your project supports. Please refer to + the specific README section to learn more about MSRV support [here][msrv_readme] + [#6201](https://github.com/rust-lang/rust-clippy/pull/6201) +* Add `--no-deps` option to avoid running on path dependencies in workspaces + [#6188](https://github.com/rust-lang/rust-clippy/pull/6188) ## Rust 1.49 -Current beta, release 2020-12-31 +Current stable, released 2020-12-31 [e636b88...b20d4c1](https://github.com/rust-lang/rust-clippy/compare/e636b88...b20d4c1) @@ -116,7 +243,7 @@ Current beta, release 2020-12-31 ## Rust 1.48 -Current stable, released 2020-11-19 +Released 2020-11-19 [09bd400...e636b88](https://github.com/rust-lang/rust-clippy/compare/09bd400...e636b88) @@ -1769,6 +1896,7 @@ Released 2018-09-13 [`cmp_null`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_null [`cmp_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_owned [`cognitive_complexity`]: https://rust-lang.github.io/rust-clippy/master/index.html#cognitive_complexity +[`collapsible_else_if`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_else_if [`collapsible_if`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if [`collapsible_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_match [`comparison_chain`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_chain @@ -1973,6 +2101,7 @@ Released 2018-09-13 [`needless_doctest_main`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_doctest_main [`needless_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes [`needless_pass_by_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value +[`needless_question_mark`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_question_mark [`needless_range_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_range_loop [`needless_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_return [`needless_update`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_update @@ -2012,6 +2141,7 @@ Released 2018-09-13 [`print_with_newline`]: https://rust-lang.github.io/rust-clippy/master/index.html#print_with_newline [`println_empty_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#println_empty_string [`ptr_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#ptr_arg +[`ptr_as_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#ptr_as_ptr [`ptr_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#ptr_eq [`ptr_offset_with_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#ptr_offset_with_cast [`pub_enum_variant_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#pub_enum_variant_names @@ -2152,6 +2282,7 @@ Released 2018-09-13 [`useless_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#useless_transmute [`useless_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#useless_vec [`vec_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#vec_box +[`vec_init_then_push`]: https://rust-lang.github.io/rust-clippy/master/index.html#vec_init_then_push [`vec_resize_to_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#vec_resize_to_zero [`verbose_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#verbose_bit_mask [`verbose_file_reads`]: https://rust-lang.github.io/rust-clippy/master/index.html#verbose_file_reads diff --git a/src/tools/clippy/clippy_dev/src/bless.rs b/src/tools/clippy/clippy_dev/src/bless.rs index 645098e4cfc..b877806946c 100644 --- a/src/tools/clippy/clippy_dev/src/bless.rs +++ b/src/tools/clippy/clippy_dev/src/bless.rs @@ -5,7 +5,7 @@ use std::env; use std::ffi::OsStr; use std::fs; use std::lazy::SyncLazy; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use walkdir::WalkDir; use crate::clippy_project_root; @@ -16,27 +16,41 @@ pub static CARGO_TARGET_DIR: SyncLazy<PathBuf> = SyncLazy::new(|| match env::var None => env::current_dir().unwrap().join("target"), }); -pub fn bless() { - let test_dirs = [ +static CLIPPY_BUILD_TIME: SyncLazy<Option<std::time::SystemTime>> = SyncLazy::new(|| { + let profile = env::var("PROFILE").unwrap_or_else(|_| "debug".to_string()); + let mut path = PathBuf::from(&**CARGO_TARGET_DIR); + path.push(profile); + path.push("cargo-clippy"); + fs::metadata(path).ok()?.modified().ok() +}); + +pub fn bless(ignore_timestamp: bool) { + let test_suite_dirs = [ clippy_project_root().join("tests").join("ui"), + clippy_project_root().join("tests").join("ui-internal"), clippy_project_root().join("tests").join("ui-toml"), clippy_project_root().join("tests").join("ui-cargo"), ]; - for test_dir in &test_dirs { - WalkDir::new(test_dir) + for test_suite_dir in &test_suite_dirs { + WalkDir::new(test_suite_dir) .into_iter() .filter_map(Result::ok) .filter(|f| f.path().extension() == Some(OsStr::new("rs"))) .for_each(|f| { - update_reference_file(f.path().with_extension("stdout")); - update_reference_file(f.path().with_extension("stderr")); - update_reference_file(f.path().with_extension("fixed")); + let test_name = f.path().strip_prefix(test_suite_dir).unwrap(); + for &ext in &["stdout", "stderr", "fixed"] { + update_reference_file( + f.path().with_extension(ext), + test_name.with_extension(ext), + ignore_timestamp, + ); + } }); } } -fn update_reference_file(reference_file_path: PathBuf) { - let test_output_path = build_dir().join(PathBuf::from(reference_file_path.file_name().unwrap())); +fn update_reference_file(reference_file_path: PathBuf, test_name: PathBuf, ignore_timestamp: bool) { + let test_output_path = build_dir().join(test_name); let relative_reference_file_path = reference_file_path.strip_prefix(clippy_project_root()).unwrap(); // If compiletest did not write any changes during the test run, @@ -45,6 +59,11 @@ fn update_reference_file(reference_file_path: PathBuf) { return; } + // If the test output was not updated since the last clippy build, it may be outdated + if !ignore_timestamp && !updated_since_clippy_build(&test_output_path).unwrap_or(true) { + return; + } + let test_output_file = fs::read(&test_output_path).expect("Unable to read test output file"); let reference_file = fs::read(&reference_file_path).unwrap_or_default(); @@ -64,6 +83,12 @@ fn update_reference_file(reference_file_path: PathBuf) { } } +fn updated_since_clippy_build(path: &Path) -> Option<bool> { + let clippy_build_time = (*CLIPPY_BUILD_TIME)?; + let modified = fs::metadata(path).ok()?.modified().ok()?; + Some(modified >= clippy_build_time) +} + fn build_dir() -> PathBuf { let profile = env::var("PROFILE").unwrap_or_else(|_| "debug".to_string()); let mut path = PathBuf::new(); diff --git a/src/tools/clippy/clippy_dev/src/main.rs b/src/tools/clippy/clippy_dev/src/main.rs index 4fdae38e3ab..2ea56c42faf 100644 --- a/src/tools/clippy/clippy_dev/src/main.rs +++ b/src/tools/clippy/clippy_dev/src/main.rs @@ -7,8 +7,8 @@ fn main() { let matches = get_clap_config(); match matches.subcommand() { - ("bless", Some(_)) => { - bless::bless(); + ("bless", Some(matches)) => { + bless::bless(matches.is_present("ignore-timestamp")); }, ("fmt", Some(matches)) => { fmt::run(matches.is_present("check"), matches.is_present("verbose")); @@ -47,7 +47,15 @@ fn main() { fn get_clap_config<'a>() -> ArgMatches<'a> { App::new("Clippy developer tooling") - .subcommand(SubCommand::with_name("bless").about("bless the test output changes")) + .subcommand( + SubCommand::with_name("bless") + .about("bless the test output changes") + .arg( + Arg::with_name("ignore-timestamp") + .long("ignore-timestamp") + .help("Include files updated before clippy was built"), + ), + ) .subcommand( SubCommand::with_name("fmt") .about("Run rustfmt on all projects and tests") diff --git a/src/tools/clippy/clippy_lints/src/attrs.rs b/src/tools/clippy/clippy_lints/src/attrs.rs index 7607394b2fe..652d1fa16b6 100644 --- a/src/tools/clippy/clippy_lints/src/attrs.rs +++ b/src/tools/clippy/clippy_lints/src/attrs.rs @@ -370,7 +370,7 @@ fn extract_clippy_lint(lint: &NestedMetaItem) -> Option<SymbolStr> { if let Some(meta_item) = lint.meta_item(); if meta_item.path.segments.len() > 1; if let tool_name = meta_item.path.segments[0].ident; - if tool_name.as_str() == "clippy"; + if tool_name.name == sym::clippy; let lint_name = meta_item.path.segments.last().unwrap().ident.name; then { return Some(lint_name.as_str()); diff --git a/src/tools/clippy/clippy_lints/src/collapsible_if.rs b/src/tools/clippy/clippy_lints/src/collapsible_if.rs index 42bff564de0..93ccc76d0c9 100644 --- a/src/tools/clippy/clippy_lints/src/collapsible_if.rs +++ b/src/tools/clippy/clippy_lints/src/collapsible_if.rs @@ -23,9 +23,7 @@ use rustc_errors::Applicability; declare_clippy_lint! { /// **What it does:** Checks for nested `if` statements which can be collapsed - /// by `&&`-combining their conditions and for `else { if ... }` expressions - /// that - /// can be collapsed to `else if ...`. + /// by `&&`-combining their conditions. /// /// **Why is this bad?** Each `if`-statement adds one level of nesting, which /// makes code look more complex than it really is. @@ -40,7 +38,31 @@ declare_clippy_lint! { /// } /// } /// - /// // or + /// ``` + /// + /// Should be written: + /// + /// ```rust.ignore + /// if x && y { + /// … + /// } + /// ``` + pub COLLAPSIBLE_IF, + style, + "nested `if`s that can be collapsed (e.g., `if x { if y { ... } }`" +} + +declare_clippy_lint! { + /// **What it does:** Checks for collapsible `else { if ... }` expressions + /// that can be collapsed to `else if ...`. + /// + /// **Why is this bad?** Each `if`-statement adds one level of nesting, which + /// makes code look more complex than it really is. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust,ignore /// /// if x { /// … @@ -54,24 +76,18 @@ declare_clippy_lint! { /// Should be written: /// /// ```rust.ignore - /// if x && y { - /// … - /// } - /// - /// // or - /// /// if x { /// … /// } else if y { /// … /// } /// ``` - pub COLLAPSIBLE_IF, + pub COLLAPSIBLE_ELSE_IF, style, - "`if`s that can be collapsed (e.g., `if x { if y { ... } }` and `else { if x { ... } }`)" + "nested `else`-`if` expressions that can be collapsed (e.g., `else { if x { ... } }`)" } -declare_lint_pass!(CollapsibleIf => [COLLAPSIBLE_IF]); +declare_lint_pass!(CollapsibleIf => [COLLAPSIBLE_IF, COLLAPSIBLE_ELSE_IF]); impl EarlyLintPass for CollapsibleIf { fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr) { @@ -112,7 +128,7 @@ fn check_collapsible_maybe_if_let(cx: &EarlyContext<'_>, else_: &ast::Expr) { let mut applicability = Applicability::MachineApplicable; span_lint_and_sugg( cx, - COLLAPSIBLE_IF, + COLLAPSIBLE_ELSE_IF, block.span, "this `else { if .. }` block can be collapsed", "collapse nested if block", diff --git a/src/tools/clippy/clippy_lints/src/comparison_chain.rs b/src/tools/clippy/clippy_lints/src/comparison_chain.rs index ae1143b2c50..90d31dece13 100644 --- a/src/tools/clippy/clippy_lints/src/comparison_chain.rs +++ b/src/tools/clippy/clippy_lints/src/comparison_chain.rs @@ -13,7 +13,7 @@ declare_clippy_lint! { /// repetitive /// /// **Known problems:** The match statement may be slower due to the compiler - /// not inlining the call to cmp. See issue #5354 + /// not inlining the call to cmp. See issue [#5354](https://github.com/rust-lang/rust-clippy/issues/5354) /// /// **Example:** /// ```rust,ignore diff --git a/src/tools/clippy/clippy_lints/src/copies.rs b/src/tools/clippy/clippy_lints/src/copies.rs index 6f48ffeb0e9..944aaafb46d 100644 --- a/src/tools/clippy/clippy_lints/src/copies.rs +++ b/src/tools/clippy/clippy_lints/src/copies.rs @@ -112,7 +112,8 @@ impl<'tcx> LateLintPass<'tcx> for CopyAndPaste { if let Some(&Expr { kind: ExprKind::If(_, _, Some(ref else_expr)), .. - }) = get_parent_expr(cx, expr) { + }) = get_parent_expr(cx, expr) + { if else_expr.hir_id == expr.hir_id { return; } diff --git a/src/tools/clippy/clippy_lints/src/copy_iterator.rs b/src/tools/clippy/clippy_lints/src/copy_iterator.rs index a7aa2cb35c1..48899b33899 100644 --- a/src/tools/clippy/clippy_lints/src/copy_iterator.rs +++ b/src/tools/clippy/clippy_lints/src/copy_iterator.rs @@ -1,5 +1,5 @@ use crate::utils::{is_copy, match_path, paths, span_lint_and_note}; -use rustc_hir::{Item, ItemKind, Impl}; +use rustc_hir::{Impl, Item, ItemKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; diff --git a/src/tools/clippy/clippy_lints/src/default.rs b/src/tools/clippy/clippy_lints/src/default.rs index b0d7c7b3baa..f7224811e6e 100644 --- a/src/tools/clippy/clippy_lints/src/default.rs +++ b/src/tools/clippy/clippy_lints/src/default.rs @@ -1,4 +1,6 @@ -use crate::utils::{any_parent_is_automatically_derived, contains_name, match_def_path, paths, qpath_res, snippet}; +use crate::utils::{ + any_parent_is_automatically_derived, contains_name, match_def_path, paths, qpath_res, snippet_with_macro_callsite, +}; use crate::utils::{span_lint_and_note, span_lint_and_sugg}; use if_chain::if_chain; use rustc_data_structures::fx::FxHashSet; @@ -6,6 +8,7 @@ use rustc_errors::Applicability; use rustc_hir::def::Res; use rustc_hir::{Block, Expr, ExprKind, PatKind, QPath, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::lint::in_external_macro; use rustc_middle::ty; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::symbol::{Ident, Symbol}; @@ -118,6 +121,8 @@ impl LateLintPass<'_> for Default { // only take `let ...` statements if let StmtKind::Local(local) = stmt.kind; if let Some(expr) = local.init; + if !any_parent_is_automatically_derived(cx.tcx, expr.hir_id); + if !in_external_macro(cx.tcx.sess, expr.span); // only take bindings to identifiers if let PatKind::Binding(_, binding_id, ident, _) = local.pat.kind; // only when assigning `... = Default::default()` @@ -187,7 +192,7 @@ impl LateLintPass<'_> for Default { .into_iter() .map(|(field, rhs)| { // extract and store the assigned value for help message - let value_snippet = snippet(cx, rhs.span, ".."); + let value_snippet = snippet_with_macro_callsite(cx, rhs.span, ".."); format!("{}: {}", field, value_snippet) }) .collect::<Vec<String>>() diff --git a/src/tools/clippy/clippy_lints/src/derive.rs b/src/tools/clippy/clippy_lints/src/derive.rs index b55f59f021d..b1e363663bb 100644 --- a/src/tools/clippy/clippy_lints/src/derive.rs +++ b/src/tools/clippy/clippy_lints/src/derive.rs @@ -7,7 +7,7 @@ use if_chain::if_chain; use rustc_hir::def_id::DefId; use rustc_hir::intravisit::{walk_expr, walk_fn, walk_item, FnKind, NestedVisitorMap, Visitor}; use rustc_hir::{ - BlockCheckMode, BodyId, Expr, ExprKind, FnDecl, HirId, Item, ItemKind, Impl, TraitRef, UnsafeSource, Unsafety, + BlockCheckMode, BodyId, Expr, ExprKind, FnDecl, HirId, Impl, Item, ItemKind, TraitRef, UnsafeSource, Unsafety, }; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::hir::map::Map; diff --git a/src/tools/clippy/clippy_lints/src/empty_enum.rs b/src/tools/clippy/clippy_lints/src/empty_enum.rs index a249117d182..853b3afdc3a 100644 --- a/src/tools/clippy/clippy_lints/src/empty_enum.rs +++ b/src/tools/clippy/clippy_lints/src/empty_enum.rs @@ -8,8 +8,12 @@ use rustc_session::{declare_lint_pass, declare_tool_lint}; declare_clippy_lint! { /// **What it does:** Checks for `enum`s with no variants. /// + /// As of this writing, the `never_type` is still a + /// nightly-only experimental API. Therefore, this lint is only triggered + /// if the `never_type` is enabled. + /// /// **Why is this bad?** If you want to introduce a type which - /// can't be instantiated, you should use `!` (the never type), + /// can't be instantiated, you should use `!` (the primitive type "never"), /// or a wrapper around it, because `!` has more extensive /// compiler support (type inference, etc...) and wrappers /// around it are the conventional way to define an uninhabited type. @@ -40,6 +44,11 @@ declare_lint_pass!(EmptyEnum => [EMPTY_ENUM]); impl<'tcx> LateLintPass<'tcx> for EmptyEnum { fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { + // Only suggest the `never_type` if the feature is enabled + if !cx.tcx.features().never_type { + return; + } + let did = cx.tcx.hir().local_def_id(item.hir_id); if let ItemKind::Enum(..) = item.kind { let ty = cx.tcx.type_of(did); diff --git a/src/tools/clippy/clippy_lints/src/escape.rs b/src/tools/clippy/clippy_lints/src/escape.rs index 55087697812..40e93da8dff 100644 --- a/src/tools/clippy/clippy_lints/src/escape.rs +++ b/src/tools/clippy/clippy_lints/src/escape.rs @@ -1,15 +1,16 @@ use rustc_hir::intravisit; -use rustc_hir::{self, Body, FnDecl, HirId, HirIdSet, ItemKind, Impl, Node}; +use rustc_hir::{self, AssocItemKind, Body, FnDecl, HirId, HirIdSet, Impl, ItemKind, Node}; use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty::{self, Ty}; +use rustc_middle::ty::{self, TraitRef, Ty}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::source_map::Span; +use rustc_span::symbol::kw; use rustc_target::abi::LayoutOf; use rustc_target::spec::abi::Abi; use rustc_typeck::expr_use_visitor::{ConsumeMode, Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId}; -use crate::utils::span_lint; +use crate::utils::{contains_ty, span_lint}; #[derive(Copy, Clone)] pub struct BoxedLocal { @@ -51,6 +52,7 @@ fn is_non_trait_box(ty: Ty<'_>) -> bool { struct EscapeDelegate<'a, 'tcx> { cx: &'a LateContext<'tcx>, set: HirIdSet, + trait_self_ty: Option<Ty<'a>>, too_large_for_stack: u64, } @@ -72,19 +74,34 @@ impl<'tcx> LateLintPass<'tcx> for BoxedLocal { } } - // If the method is an impl for a trait, don't warn. let parent_id = cx.tcx.hir().get_parent_item(hir_id); let parent_node = cx.tcx.hir().find(parent_id); + let mut trait_self_ty = None; if let Some(Node::Item(item)) = parent_node { + // If the method is an impl for a trait, don't warn. if let ItemKind::Impl(Impl { of_trait: Some(_), .. }) = item.kind { return; } + + // find `self` ty for this trait if relevant + if let ItemKind::Trait(_, _, _, _, items) = item.kind { + for trait_item in items { + if trait_item.id.hir_id == hir_id { + // be sure we have `self` parameter in this function + if let AssocItemKind::Fn { has_self: true } = trait_item.kind { + trait_self_ty = + Some(TraitRef::identity(cx.tcx, trait_item.id.hir_id.owner.to_def_id()).self_ty()); + } + } + } + } } let mut v = EscapeDelegate { cx, set: HirIdSet::default(), + trait_self_ty, too_large_for_stack: self.too_large_for_stack, }; @@ -153,10 +170,17 @@ impl<'a, 'tcx> Delegate<'tcx> for EscapeDelegate<'a, 'tcx> { return; } + // skip if there is a `self` parameter binding to a type + // that contains `Self` (i.e.: `self: Box<Self>`), see #4804 + if let Some(trait_self_ty) = self.trait_self_ty { + if map.name(cmt.hir_id) == kw::SelfLower && contains_ty(cmt.place.ty(), trait_self_ty) { + return; + } + } + if is_non_trait_box(cmt.place.ty()) && !self.is_large_box(cmt.place.ty()) { self.set.insert(cmt.hir_id); } - return; } } } diff --git a/src/tools/clippy/clippy_lints/src/eta_reduction.rs b/src/tools/clippy/clippy_lints/src/eta_reduction.rs index 53df3abbf54..1a722d39f73 100644 --- a/src/tools/clippy/clippy_lints/src/eta_reduction.rs +++ b/src/tools/clippy/clippy_lints/src/eta_reduction.rs @@ -22,7 +22,7 @@ declare_clippy_lint! { /// **Known problems:** If creating the closure inside the closure has a side- /// effect then moving the closure creation out will change when that side- /// effect runs. - /// See rust-lang/rust-clippy#1439 for more details. + /// See [#1439](https://github.com/rust-lang/rust-clippy/issues/1439) for more details. /// /// **Example:** /// ```rust,ignore @@ -45,8 +45,9 @@ declare_clippy_lint! { /// /// **Why is this bad?** It's unnecessary to create the closure. /// - /// **Known problems:** rust-lang/rust-clippy#3071, rust-lang/rust-clippy#4002, - /// rust-lang/rust-clippy#3942 + /// **Known problems:** [#3071](https://github.com/rust-lang/rust-clippy/issues/3071), + /// [#3942](https://github.com/rust-lang/rust-clippy/issues/3942), + /// [#4002](https://github.com/rust-lang/rust-clippy/issues/4002) /// /// /// **Example:** diff --git a/src/tools/clippy/clippy_lints/src/from_over_into.rs b/src/tools/clippy/clippy_lints/src/from_over_into.rs index 1e7e5f53cc2..b010abda24d 100644 --- a/src/tools/clippy/clippy_lints/src/from_over_into.rs +++ b/src/tools/clippy/clippy_lints/src/from_over_into.rs @@ -70,7 +70,7 @@ impl LateLintPass<'_> for FromOverInto { span_lint_and_help( cx, FROM_OVER_INTO, - item.span, + cx.tcx.sess.source_map().guess_head_span(item.span), "an implementation of `From` is preferred since it gives you `Into<_>` for free where the reverse isn't true", None, "consider to implement `From` instead", diff --git a/src/tools/clippy/clippy_lints/src/if_let_mutex.rs b/src/tools/clippy/clippy_lints/src/if_let_mutex.rs index 2e55094d90c..58511c6d57c 100644 --- a/src/tools/clippy/clippy_lints/src/if_let_mutex.rs +++ b/src/tools/clippy/clippy_lints/src/if_let_mutex.rs @@ -145,7 +145,7 @@ impl<'tcx, 'l> ArmVisitor<'tcx, 'l> { fn is_mutex_lock_call<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> { if_chain! { if let ExprKind::MethodCall(path, _span, args, _) = &expr.kind; - if path.ident.to_string() == "lock"; + if path.ident.as_str() == "lock"; let ty = cx.typeck_results().expr_ty(&args[0]); if is_type_diagnostic_item(cx, ty, sym!(mutex_type)); then { diff --git a/src/tools/clippy/clippy_lints/src/inherent_impl.rs b/src/tools/clippy/clippy_lints/src/inherent_impl.rs index e287aecb044..ea26c84cde1 100644 --- a/src/tools/clippy/clippy_lints/src/inherent_impl.rs +++ b/src/tools/clippy/clippy_lints/src/inherent_impl.rs @@ -2,7 +2,7 @@ use crate::utils::{in_macro, span_lint_and_then}; use rustc_data_structures::fx::FxHashMap; -use rustc_hir::{def_id, Crate, Item, ItemKind, Impl}; +use rustc_hir::{def_id, Crate, Impl, Item, ItemKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::Span; diff --git a/src/tools/clippy/clippy_lints/src/len_zero.rs b/src/tools/clippy/clippy_lints/src/len_zero.rs index 5474b30bdec..e95caf6a35f 100644 --- a/src/tools/clippy/clippy_lints/src/len_zero.rs +++ b/src/tools/clippy/clippy_lints/src/len_zero.rs @@ -3,7 +3,7 @@ use rustc_ast::ast::LitKind; use rustc_data_structures::fx::FxHashSet; use rustc_errors::Applicability; use rustc_hir::def_id::DefId; -use rustc_hir::{AssocItemKind, BinOpKind, Expr, ExprKind, ImplItemRef, Item, ItemKind, Impl, TraitItemRef}; +use rustc_hir::{AssocItemKind, BinOpKind, Expr, ExprKind, Impl, ImplItemRef, Item, ItemKind, TraitItemRef}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; use rustc_session::{declare_lint_pass, declare_tool_lint}; diff --git a/src/tools/clippy/clippy_lints/src/lib.rs b/src/tools/clippy/clippy_lints/src/lib.rs index aaa17561f06..781282cdfbd 100644 --- a/src/tools/clippy/clippy_lints/src/lib.rs +++ b/src/tools/clippy/clippy_lints/src/lib.rs @@ -271,6 +271,7 @@ mod needless_borrow; mod needless_borrowed_ref; mod needless_continue; mod needless_pass_by_value; +mod needless_question_mark; mod needless_update; mod neg_cmp_op_on_partial_ord; mod neg_multiply; @@ -341,6 +342,7 @@ mod unwrap_in_result; mod use_self; mod useless_conversion; mod vec; +mod vec_init_then_push; mod vec_resize_to_zero; mod verbose_file_reads; mod wildcard_dependencies; @@ -528,6 +530,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &utils::internal_lints::OUTER_EXPN_EXPN_DATA, #[cfg(feature = "internal-lints")] &utils::internal_lints::PRODUCE_ICE, + #[cfg(feature = "internal-lints")] + &utils::internal_lints::UNNECESSARY_SYMBOL_STR, &approx_const::APPROX_CONSTANT, &arithmetic::FLOAT_ARITHMETIC, &arithmetic::INTEGER_ARITHMETIC, @@ -559,6 +563,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &cargo_common_metadata::CARGO_COMMON_METADATA, &checked_conversions::CHECKED_CONVERSIONS, &cognitive_complexity::COGNITIVE_COMPLEXITY, + &collapsible_if::COLLAPSIBLE_ELSE_IF, &collapsible_if::COLLAPSIBLE_IF, &collapsible_match::COLLAPSIBLE_MATCH, &comparison_chain::COMPARISON_CHAIN, @@ -802,6 +807,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &needless_borrowed_ref::NEEDLESS_BORROWED_REFERENCE, &needless_continue::NEEDLESS_CONTINUE, &needless_pass_by_value::NEEDLESS_PASS_BY_VALUE, + &needless_question_mark::NEEDLESS_QUESTION_MARK, &needless_update::NEEDLESS_UPDATE, &neg_cmp_op_on_partial_ord::NEG_CMP_OP_ON_PARTIAL_ORD, &neg_multiply::NEG_MULTIPLY, @@ -911,6 +917,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &types::LET_UNIT_VALUE, &types::LINKEDLIST, &types::OPTION_OPTION, + &types::PTR_AS_PTR, &types::RC_BUFFER, &types::REDUNDANT_ALLOCATION, &types::TYPE_COMPLEXITY, @@ -938,6 +945,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &use_self::USE_SELF, &useless_conversion::USELESS_CONVERSION, &vec::USELESS_VEC, + &vec_init_then_push::VEC_INIT_THEN_PUSH, &vec_resize_to_zero::VEC_RESIZE_TO_ZERO, &verbose_file_reads::VERBOSE_FILE_READS, &wildcard_dependencies::WILDCARD_DEPENDENCIES, @@ -1022,6 +1030,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(move || box from_over_into::FromOverInto::new(msrv)); store.register_late_pass(move || box use_self::UseSelf::new(msrv)); store.register_late_pass(move || box missing_const_for_fn::MissingConstForFn::new(msrv)); + store.register_late_pass(move || box needless_question_mark::NeedlessQuestionMark::new(msrv)); store.register_late_pass(|| box size_of_in_element_count::SizeOfInElementCount); store.register_late_pass(|| box map_clone::MapClone); @@ -1218,6 +1227,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box strings::StrToString); store.register_late_pass(|| box strings::StringToString); store.register_late_pass(|| box zero_sized_map_values::ZeroSizedMapValues); + store.register_late_pass(|| box vec_init_then_push::VecInitThenPush::default()); + store.register_late_pass(move || box types::PtrAsPtr::new(msrv)); store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ LintId::of(&arithmetic::FLOAT_ARITHMETIC), @@ -1344,6 +1355,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&types::LET_UNIT_VALUE), LintId::of(&types::LINKEDLIST), LintId::of(&types::OPTION_OPTION), + LintId::of(&types::PTR_AS_PTR), LintId::of(&unicode::NON_ASCII_LITERAL), LintId::of(&unicode::UNICODE_NOT_NFC), LintId::of(&unnested_or_patterns::UNNESTED_OR_PATTERNS), @@ -1365,6 +1377,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&utils::internal_lints::MATCH_TYPE_ON_DIAGNOSTIC_ITEM), LintId::of(&utils::internal_lints::OUTER_EXPN_EXPN_DATA), LintId::of(&utils::internal_lints::PRODUCE_ICE), + LintId::of(&utils::internal_lints::UNNECESSARY_SYMBOL_STR), ]); store.register_group(true, "clippy::all", Some("clippy"), vec![ @@ -1386,6 +1399,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&booleans::LOGIC_BUG), LintId::of(&booleans::NONMINIMAL_BOOL), LintId::of(&bytecount::NAIVE_BYTECOUNT), + LintId::of(&collapsible_if::COLLAPSIBLE_ELSE_IF), LintId::of(&collapsible_if::COLLAPSIBLE_IF), LintId::of(&collapsible_match::COLLAPSIBLE_MATCH), LintId::of(&comparison_chain::COMPARISON_CHAIN), @@ -1547,6 +1561,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&needless_bool::BOOL_COMPARISON), LintId::of(&needless_bool::NEEDLESS_BOOL), LintId::of(&needless_borrowed_ref::NEEDLESS_BORROWED_REFERENCE), + LintId::of(&needless_question_mark::NEEDLESS_QUESTION_MARK), LintId::of(&needless_update::NEEDLESS_UPDATE), LintId::of(&neg_cmp_op_on_partial_ord::NEG_CMP_OP_ON_PARTIAL_ORD), LintId::of(&neg_multiply::NEG_MULTIPLY), @@ -1638,6 +1653,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&unwrap::UNNECESSARY_UNWRAP), LintId::of(&useless_conversion::USELESS_CONVERSION), LintId::of(&vec::USELESS_VEC), + LintId::of(&vec_init_then_push::VEC_INIT_THEN_PUSH), LintId::of(&vec_resize_to_zero::VEC_RESIZE_TO_ZERO), LintId::of(&write::PRINTLN_EMPTY_STRING), LintId::of(&write::PRINT_LITERAL), @@ -1654,6 +1670,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&attrs::BLANKET_CLIPPY_RESTRICTION_LINTS), LintId::of(&blacklisted_name::BLACKLISTED_NAME), LintId::of(&blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS), + LintId::of(&collapsible_if::COLLAPSIBLE_ELSE_IF), LintId::of(&collapsible_if::COLLAPSIBLE_IF), LintId::of(&collapsible_match::COLLAPSIBLE_MATCH), LintId::of(&comparison_chain::COMPARISON_CHAIN), @@ -1804,6 +1821,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&needless_bool::BOOL_COMPARISON), LintId::of(&needless_bool::NEEDLESS_BOOL), LintId::of(&needless_borrowed_ref::NEEDLESS_BORROWED_REFERENCE), + LintId::of(&needless_question_mark::NEEDLESS_QUESTION_MARK), LintId::of(&needless_update::NEEDLESS_UPDATE), LintId::of(&neg_cmp_op_on_partial_ord::NEG_CMP_OP_ON_PARTIAL_ORD), LintId::of(&no_effect::NO_EFFECT), @@ -1936,6 +1954,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&types::BOX_VEC), LintId::of(&types::REDUNDANT_ALLOCATION), LintId::of(&vec::USELESS_VEC), + LintId::of(&vec_init_then_push::VEC_INIT_THEN_PUSH), ]); store.register_group(true, "clippy::cargo", Some("clippy_cargo"), vec). /// /// **Example:** /// ```rust,no_run diff --git a/src/tools/clippy/clippy_lints/src/manual_async_fn.rs b/src/tools/clippy/clippy_lints/src/manual_async_fn.rs index 29439e52c48..89f5b2ff311 100644 --- a/src/tools/clippy/clippy_lints/src/manual_async_fn.rs +++ b/src/tools/clippy/clippy_lints/src/manual_async_fn.rs @@ -9,7 +9,7 @@ use rustc_hir::{ }; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::Span; +use rustc_span::{sym, Span}; declare_clippy_lint! { /// **What it does:** It checks for manual implementations of `async` functions. @@ -137,7 +137,7 @@ fn future_output_ty<'tcx>(trait_ref: &'tcx TraitRef<'tcx>) -> Option<&'tcx Ty<'t if let Some(args) = segment.args; if args.bindings.len() == 1; let binding = &args.bindings[0]; - if binding.ident.as_str() == "Output"; + if binding.ident.name == sym::Output; if let TypeBindingKind::Equality{ty: output} = binding.kind; then { return Some(output) diff --git a/src/tools/clippy/clippy_lints/src/map_clone.rs b/src/tools/clippy/clippy_lints/src/map_clone.rs index 220240acb7a..1818836d5d5 100644 --- a/src/tools/clippy/clippy_lints/src/map_clone.rs +++ b/src/tools/clippy/clippy_lints/src/map_clone.rs @@ -53,7 +53,7 @@ impl<'tcx> LateLintPass<'tcx> for MapClone { if_chain! { if let hir::ExprKind::MethodCall(ref method, _, ref args, _) = e.kind; if args.len() == 2; - if method.ident.as_str() == "map"; + if method.ident.name == sym::map; let ty = cx.typeck_results().expr_ty(&args[0]); if is_type_diagnostic_item(cx, ty, sym::option_type) || match_trait_method(cx, e, &paths::ITERATOR); if let hir::ExprKind::Closure(_, _, body_id, _, _) = args[1].kind; diff --git a/src/tools/clippy/clippy_lints/src/map_identity.rs b/src/tools/clippy/clippy_lints/src/map_identity.rs index 6b782385a38..9f9c108a85a 100644 --- a/src/tools/clippy/clippy_lints/src/map_identity.rs +++ b/src/tools/clippy/clippy_lints/src/map_identity.rs @@ -63,7 +63,7 @@ impl<'tcx> LateLintPass<'tcx> for MapIdentity { fn get_map_argument<'a>(cx: &LateContext<'_>, expr: &'a Expr<'a>) -> Option<&'a [Expr<'a>]> { if_chain! { if let ExprKind::MethodCall(ref method, _, ref args, _) = expr.kind; - if args.len() == 2 && method.ident.as_str() == "map"; + if args.len() == 2 && method.ident.name == sym::map; let caller_ty = cx.typeck_results().expr_ty(&args[0]); if match_trait_method(cx, expr, &paths::ITERATOR) || is_type_diagnostic_item(cx, caller_ty, sym::result_type) diff --git a/src/tools/clippy/clippy_lints/src/methods/mod.rs b/src/tools/clippy/clippy_lints/src/methods/mod.rs index f7231bba175..7adf6bdeac5 100644 --- a/src/tools/clippy/clippy_lints/src/methods/mod.rs +++ b/src/tools/clippy/clippy_lints/src/methods/mod.rs @@ -3096,7 +3096,7 @@ fn lint_flat_map_identity<'tcx>( if let hir::ExprKind::Path(hir::QPath::Resolved(_, ref path)) = body.value.kind; if path.segments.len() == 1; - if path.segments[0].ident.as_str() == binding_ident.as_str(); + if path.segments[0].ident.name == binding_ident.name; then { apply_lint("called `flat_map(|x| x)` on an `Iterator`"); diff --git a/src/tools/clippy/clippy_lints/src/minmax.rs b/src/tools/clippy/clippy_lints/src/minmax.rs index 004dd50a31b..8d0c3b8e0fe 100644 --- a/src/tools/clippy/clippy_lints/src/minmax.rs +++ b/src/tools/clippy/clippy_lints/src/minmax.rs @@ -89,9 +89,9 @@ fn min_max<'a>(cx: &LateContext<'_>, expr: &'a Expr<'a>) -> Option<(MinMax, Cons if let [obj, _] = args; if cx.typeck_results().expr_ty(obj).is_floating_point() || match_trait_method(cx, expr, &paths::ORD); then { - if path.ident.as_str() == sym!(max).as_str() { + if path.ident.name == sym!(max) { fetch_const(cx, args, MinMax::Max) - } else if path.ident.as_str() == sym!(min).as_str() { + } else if path.ident.name == sym!(min) { fetch_const(cx, args, MinMax::Min) } else { None diff --git a/src/tools/clippy/clippy_lints/src/missing_doc.rs b/src/tools/clippy/clippy_lints/src/missing_doc.rs index 27f1074a0dd..0e49eaab436 100644 --- a/src/tools/clippy/clippy_lints/src/missing_doc.rs +++ b/src/tools/clippy/clippy_lints/src/missing_doc.rs @@ -63,7 +63,7 @@ impl MissingDoc { if let Some(meta) = list.get(0); if let Some(name) = meta.ident(); then { - name.as_str() == "include" + name.name == sym::include } else { false } diff --git a/src/tools/clippy/clippy_lints/src/needless_bool.rs b/src/tools/clippy/clippy_lints/src/needless_bool.rs index 6b9a37b5252..d795f126457 100644 --- a/src/tools/clippy/clippy_lints/src/needless_bool.rs +++ b/src/tools/clippy/clippy_lints/src/needless_bool.rs @@ -3,9 +3,7 @@ //! This lint is **warn** by default use crate::utils::sugg::Sugg; -use crate::utils::{ - is_expn_of, parent_node_is_if_expr, snippet_with_applicability, span_lint, span_lint_and_sugg, -}; +use crate::utils::{is_expn_of, parent_node_is_if_expr, snippet_with_applicability, span_lint, span_lint_and_sugg}; use rustc_ast::ast::LitKind; use rustc_errors::Applicability; use rustc_hir::{BinOpKind, Block, Expr, ExprKind, StmtKind, UnOp}; diff --git a/src/tools/clippy/clippy_lints/src/needless_borrow.rs b/src/tools/clippy/clippy_lints/src/needless_borrow.rs index bff53eb8cca..f1c06692e30 100644 --- a/src/tools/clippy/clippy_lints/src/needless_borrow.rs +++ b/src/tools/clippy/clippy_lints/src/needless_borrow.rs @@ -2,7 +2,7 @@ //! //! This lint is **warn** by default -use crate::utils::{snippet_opt, span_lint_and_then}; +use crate::utils::{is_automatically_derived, snippet_opt, span_lint_and_then}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::{BindingAnnotation, BorrowKind, Expr, ExprKind, HirId, Item, Mutability, Pat, PatKind}; @@ -10,7 +10,6 @@ use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; use rustc_middle::ty::adjustment::{Adjust, Adjustment}; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::sym; declare_clippy_lint! { /// **What it does:** Checks for address of operations (`&`) that are going to @@ -116,7 +115,7 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrow { } fn check_item(&mut self, _: &LateContext<'tcx>, item: &'tcx Item<'_>) { - if item.attrs.iter().any(|a| a.has_name(sym::automatically_derived)) { + if is_automatically_derived(item.attrs) { debug_assert!(self.derived_item.is_none()); self.derived_item = Some(item.hir_id); } diff --git a/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs b/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs index 3b71f1b46e2..89fe2c19a57 100644 --- a/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs +++ b/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs @@ -8,11 +8,12 @@ use rustc_ast::ast::Attribute; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_errors::{Applicability, DiagnosticBuilder}; use rustc_hir::intravisit::FnKind; -use rustc_hir::{BindingAnnotation, Body, FnDecl, GenericArg, HirId, ItemKind, Impl, Node, PatKind, QPath, TyKind}; +use rustc_hir::{BindingAnnotation, Body, FnDecl, GenericArg, HirId, Impl, ItemKind, Node, PatKind, QPath, TyKind}; use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::{self, TypeFoldable}; use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::symbol::kw; use rustc_span::{sym, Span}; use rustc_target::spec::abi::Abi; use rustc_trait_selection::traits; @@ -151,7 +152,7 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByValue { // Ignore `self`s. if idx == 0 { if let PatKind::Binding(.., ident, _) = arg.pat.kind { - if ident.as_str() == "self" { + if ident.name == kw::SelfLower { continue; } } diff --git a/src/tools/clippy/clippy_lints/src/needless_question_mark.rs b/src/tools/clippy/clippy_lints/src/needless_question_mark.rs new file mode 100644 index 00000000000..9e9b79ee1cf --- /dev/null +++ b/src/tools/clippy/clippy_lints/src/needless_question_mark.rs @@ -0,0 +1,232 @@ +use rustc_errors::Applicability; +use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res}; +use rustc_hir::{Body, Expr, ExprKind, LangItem, MatchSource, QPath}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::ty::DefIdTree; +use rustc_semver::RustcVersion; +use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::sym; + +use crate::utils; +use if_chain::if_chain; + +declare_clippy_lint! { + /// **What it does:** + /// Suggests alternatives for useless applications of `?` in terminating expressions + /// + /// **Why is this bad?** There's no reason to use `?` to short-circuit when execution of the body will end there anyway. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// struct TO { + /// magic: Option<usize>, + /// } + /// + /// fn f(to: TO) -> Option<usize> { + /// Some(to.magic?) + /// } + /// + /// struct TR { + /// magic: Result<usize, bool>, + /// } + /// + /// fn g(tr: Result<TR, bool>) -> Result<usize, bool> { + /// tr.and_then(|t| Ok(t.magic?)) + /// } + /// + /// ``` + /// Use instead: + /// ```rust + /// struct TO { + /// magic: Option<usize>, + /// } + /// + /// fn f(to: TO) -> Option<usize> { + /// to.magic + /// } + /// + /// struct TR { + /// magic: Result<usize, bool>, + /// } + /// + /// fn g(tr: Result<TR, bool>) -> Result<usize, bool> { + /// tr.and_then(|t| t.magic) + /// } + /// ``` + pub NEEDLESS_QUESTION_MARK, + complexity, + "Suggest `value.inner_option` instead of `Some(value.inner_option?)`. The same goes for `Result<T, E>`." +} + +const NEEDLESS_QUESTION_MARK_RESULT_MSRV: RustcVersion = RustcVersion::new(1, 13, 0); +const NEEDLESS_QUESTION_MARK_OPTION_MSRV: RustcVersion = RustcVersion::new(1, 22, 0); + +pub struct NeedlessQuestionMark { + msrv: Option<RustcVersion>, +} + +impl NeedlessQuestionMark { + #[must_use] + pub fn new(msrv: Option<RustcVersion>) -> Self { + Self { msrv } + } +} + +impl_lint_pass!(NeedlessQuestionMark => [NEEDLESS_QUESTION_MARK]); + +#[derive(Debug)] +enum SomeOkCall<'a> { + SomeCall(&'a Expr<'a>, &'a Expr<'a>), + OkCall(&'a Expr<'a>, &'a Expr<'a>), +} + +impl LateLintPass<'_> for NeedlessQuestionMark { + /* + * The question mark operator is compatible with both Result<T, E> and Option<T>, + * from Rust 1.13 and 1.22 respectively. + */ + + /* + * What do we match: + * Expressions that look like this: + * Some(option?), Ok(result?) + * + * Where do we match: + * Last expression of a body + * Return statement + * A body's value (single line closure) + * + * What do we not match: + * Implicit calls to `from(..)` on the error value + */ + + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) { + let e = match &expr.kind { + ExprKind::Ret(Some(e)) => e, + _ => return, + }; + + if let Some(ok_some_call) = is_some_or_ok_call(self, cx, e) { + emit_lint(cx, &ok_some_call); + } + } + + fn check_body(&mut self, cx: &LateContext<'_>, body: &'_ Body<'_>) { + // Function / Closure block + let expr_opt = if let ExprKind::Block(block, _) = &body.value.kind { + block.expr + } else { + // Single line closure + Some(&body.value) + }; + + if_chain! { + if let Some(expr) = expr_opt; + if let Some(ok_some_call) = is_some_or_ok_call(self, cx, expr); + then { + emit_lint(cx, &ok_some_call); + } + }; + } + + extract_msrv_attr!(LateContext); +} + +fn emit_lint(cx: &LateContext<'_>, expr: &SomeOkCall<'_>) { + let (entire_expr, inner_expr) = match expr { + SomeOkCall::OkCall(outer, inner) | SomeOkCall::SomeCall(outer, inner) => (outer, inner), + }; + + utils::span_lint_and_sugg( + cx, + NEEDLESS_QUESTION_MARK, + entire_expr.span, + "Question mark operator is useless here", + "try", + format!("{}", utils::snippet(cx, inner_expr.span, r#""...""#)), + Applicability::MachineApplicable, + ); +} + +fn is_some_or_ok_call<'a>( + nqml: &NeedlessQuestionMark, + cx: &'a LateContext<'_>, + expr: &'a Expr<'_>, +) -> Option<SomeOkCall<'a>> { + if_chain! { + // Check outer expression matches CALL_IDENT(ARGUMENT) format + if let ExprKind::Call(path, args) = &expr.kind; + if let ExprKind::Path(QPath::Resolved(None, path)) = &path.kind; + if is_some_ctor(cx, path.res) || is_ok_ctor(cx, path.res); + + // Extract inner expression from ARGUMENT + if let ExprKind::Match(inner_expr_with_q, _, MatchSource::TryDesugar) = &args[0].kind; + if let ExprKind::Call(called, args) = &inner_expr_with_q.kind; + if args.len() == 1; + + if let ExprKind::Path(QPath::LangItem(LangItem::TryIntoResult, _)) = &called.kind; + then { + // Extract inner expr type from match argument generated by + // question mark operator + let inner_expr = &args[0]; + + let inner_ty = cx.typeck_results().expr_ty(inner_expr); + let outer_ty = cx.typeck_results().expr_ty(expr); + + // Check if outer and inner type are Option + let outer_is_some = utils::is_type_diagnostic_item(cx, outer_ty, sym::option_type); + let inner_is_some = utils::is_type_diagnostic_item(cx, inner_ty, sym::option_type); + + // Check for Option MSRV + let meets_option_msrv = utils::meets_msrv(nqml.msrv.as_ref(), &NEEDLESS_QUESTION_MARK_OPTION_MSRV); + if outer_is_some && inner_is_some && meets_option_msrv { + return Some(SomeOkCall::SomeCall(expr, inner_expr)); + } + + // Check if outer and inner type are Result + let outer_is_result = utils::is_type_diagnostic_item(cx, outer_ty, sym::result_type); + let inner_is_result = utils::is_type_diagnostic_item(cx, inner_ty, sym::result_type); + + // Additional check: if the error type of the Result can be converted + // via the From trait, then don't match + let does_not_call_from = !has_implicit_error_from(cx, expr, inner_expr); + + // Must meet Result MSRV + let meets_result_msrv = utils::meets_msrv(nqml.msrv.as_ref(), &NEEDLESS_QUESTION_MARK_RESULT_MSRV); + if outer_is_result && inner_is_result && does_not_call_from && meets_result_msrv { + return Some(SomeOkCall::OkCall(expr, inner_expr)); + } + } + } + + None +} + +fn has_implicit_error_from(cx: &LateContext<'_>, entire_expr: &Expr<'_>, inner_result_expr: &Expr<'_>) -> bool { + return cx.typeck_results().expr_ty(entire_expr) != cx.typeck_results().expr_ty(inner_result_expr); +} + +fn is_ok_ctor(cx: &LateContext<'_>, res: Res) -> bool { + if let Some(ok_id) = cx.tcx.lang_items().result_ok_variant() { + if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Fn), id) = res { + if let Some(variant_id) = cx.tcx.parent(id) { + return variant_id == ok_id; + } + } + } + false +} + +fn is_some_ctor(cx: &LateContext<'_>, res: Res) -> bool { + if let Some(some_id) = cx.tcx.lang_items().option_some_variant() { + if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Fn), id) = res { + if let Some(variant_id) = cx.tcx.parent(id) { + return variant_id == some_id; + } + } + } + false +} diff --git a/src/tools/clippy/clippy_lints/src/option_if_let_else.rs b/src/tools/clippy/clippy_lints/src/option_if_let_else.rs index 391f893ef35..7bdf975ffd4 100644 --- a/src/tools/clippy/clippy_lints/src/option_if_let_else.rs +++ b/src/tools/clippy/clippy_lints/src/option_if_let_else.rs @@ -66,7 +66,7 @@ declare_lint_pass!(OptionIfLetElse => [OPTION_IF_LET_ELSE]); /// Returns true iff the given expression is the result of calling `Result::ok` fn is_result_ok(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool { if let ExprKind::MethodCall(ref path, _, &[ref receiver], _) = &expr.kind { - path.ident.name.to_ident_string() == "ok" + path.ident.name.as_str() == "ok" && is_type_diagnostic_item(cx, &cx.typeck_results().expr_ty(&receiver), sym::result_type) } else { false @@ -110,7 +110,7 @@ fn extract_body_from_arm<'a>(arm: &'a Arm<'a>) -> Option<&'a Expr<'a>> { fn should_wrap_in_braces(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { utils::get_enclosing_block(cx, expr.hir_id).map_or(false, |parent| { let mut should_wrap = false; - + if let Some(Expr { kind: ExprKind::Match( @@ -124,7 +124,11 @@ fn should_wrap_in_braces(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { }) = parent.expr { should_wrap = expr.hir_id == arms[1].body.hir_id; - } else if let Some(Expr { kind: ExprKind::If(_, _, Some(else_clause)), .. }) = parent.expr { + } else if let Some(Expr { + kind: ExprKind::If(_, _, Some(else_clause)), + .. + }) = parent.expr + { should_wrap = expr.hir_id == else_clause.hir_id; } diff --git a/src/tools/clippy/clippy_lints/src/partialeq_ne_impl.rs b/src/tools/clippy/clippy_lints/src/partialeq_ne_impl.rs index 04b6e5d5847..ed314937ce8 100644 --- a/src/tools/clippy/clippy_lints/src/partialeq_ne_impl.rs +++ b/src/tools/clippy/clippy_lints/src/partialeq_ne_impl.rs @@ -1,6 +1,6 @@ use crate::utils::{is_automatically_derived, span_lint_hir}; use if_chain::if_chain; -use rustc_hir::{Item, ItemKind, Impl}; +use rustc_hir::{Impl, Item, ItemKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::sym; diff --git a/src/tools/clippy/clippy_lints/src/pass_by_ref_or_value.rs b/src/tools/clippy/clippy_lints/src/pass_by_ref_or_value.rs index 7814065e31a..d96a9b025f0 100644 --- a/src/tools/clippy/clippy_lints/src/pass_by_ref_or_value.rs +++ b/src/tools/clippy/clippy_lints/src/pass_by_ref_or_value.rs @@ -6,7 +6,7 @@ use rustc_ast::attr; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::intravisit::FnKind; -use rustc_hir::{BindingAnnotation, Body, FnDecl, HirId, ItemKind, MutTy, Mutability, Node, PatKind, Impl}; +use rustc_hir::{BindingAnnotation, Body, FnDecl, HirId, Impl, ItemKind, MutTy, Mutability, Node, PatKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; use rustc_session::{declare_tool_lint, impl_lint_pass}; @@ -63,7 +63,7 @@ declare_clippy_lint! { /// /// **Why is this bad?** Arguments passed by value might result in an unnecessary /// shallow copy, taking up more space in the stack and requiring a call to - /// `memcpy`, which which can be expensive. + /// `memcpy`, which can be expensive. /// /// **Example:** /// diff --git a/src/tools/clippy/clippy_lints/src/ptr.rs b/src/tools/clippy/clippy_lints/src/ptr.rs index b832add009f..c6329a1381c 100644 --- a/src/tools/clippy/clippy_lints/src/ptr.rs +++ b/src/tools/clippy/clippy_lints/src/ptr.rs @@ -8,8 +8,8 @@ use crate::utils::{ use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::{ - BinOpKind, BodyId, Expr, ExprKind, FnDecl, FnRetTy, GenericArg, HirId, ImplItem, ImplItemKind, Item, ItemKind, Impl, - Lifetime, MutTy, Mutability, Node, PathSegment, QPath, TraitFn, TraitItem, TraitItemKind, Ty, TyKind, + BinOpKind, BodyId, Expr, ExprKind, FnDecl, FnRetTy, GenericArg, HirId, Impl, ImplItem, ImplItemKind, Item, + ItemKind, Lifetime, MutTy, Mutability, Node, PathSegment, QPath, TraitFn, TraitItem, TraitItemKind, Ty, TyKind, }; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; diff --git a/src/tools/clippy/clippy_lints/src/ref_option_ref.rs b/src/tools/clippy/clippy_lints/src/ref_option_ref.rs index 803ebada54b..8cd6692ce03 100644 --- a/src/tools/clippy/clippy_lints/src/ref_option_ref.rs +++ b/src/tools/clippy/clippy_lints/src/ref_option_ref.rs @@ -13,7 +13,7 @@ declare_clippy_lint! { /// **Why is this bad?** Since `&` is Copy, it's useless to have a /// reference on `Option<&T>`. /// - /// **Known problems:** It may be irrevelent to use this lint on + /// **Known problems:** It may be irrelevant to use this lint on /// public API code as it will make a breaking change to apply it. /// /// **Example:** diff --git a/src/tools/clippy/clippy_lints/src/returns.rs b/src/tools/clippy/clippy_lints/src/returns.rs index 35827e027aa..63548d8fdb4 100644 --- a/src/tools/clippy/clippy_lints/src/returns.rs +++ b/src/tools/clippy/clippy_lints/src/returns.rs @@ -202,7 +202,7 @@ fn check_final_expr<'tcx>( check_final_expr(cx, &arm.body, Some(arm.body.span), RetReplacement::Block); } }, - | MatchSource::IfLetDesugar { + MatchSource::IfLetDesugar { contains_else_clause: true, } => { if let ExprKind::Block(ref ifblock, _) = arms[0].body.kind { @@ -217,6 +217,9 @@ fn check_final_expr<'tcx>( } fn emit_return_lint(cx: &LateContext<'_>, ret_span: Span, inner_span: Option<Span>, replacement: RetReplacement) { + if ret_span.from_expansion() { + return; + } match inner_span { Some(inner_span) => { if in_external_macro(cx.tcx.sess, inner_span) || inner_span.from_expansion() { diff --git a/src/tools/clippy/clippy_lints/src/serde_api.rs b/src/tools/clippy/clippy_lints/src/serde_api.rs index ca4fd9f3559..44e739725c8 100644 --- a/src/tools/clippy/clippy_lints/src/serde_api.rs +++ b/src/tools/clippy/clippy_lints/src/serde_api.rs @@ -1,5 +1,5 @@ use crate::utils::{get_trait_def_id, paths, span_lint}; -use rustc_hir::{Item, ItemKind, Impl}; +use rustc_hir::{Impl, Item, ItemKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; diff --git a/src/tools/clippy/clippy_lints/src/shadow.rs b/src/tools/clippy/clippy_lints/src/shadow.rs index f2f3dfa09a7..24da056770c 100644 --- a/src/tools/clippy/clippy_lints/src/shadow.rs +++ b/src/tools/clippy/clippy_lints/src/shadow.rs @@ -396,5 +396,5 @@ fn is_self_shadow(name: Symbol, expr: &Expr<'_>) -> bool { } fn path_eq_name(name: Symbol, path: &Path<'_>) -> bool { - !path.is_global() && path.segments.len() == 1 && path.segments[0].ident.as_str() == name.as_str() + !path.is_global() && path.segments.len() == 1 && path.segments[0].ident.name == name } diff --git a/src/tools/clippy/clippy_lints/src/swap.rs b/src/tools/clippy/clippy_lints/src/swap.rs index 386987eb181..699fd51ccc1 100644 --- a/src/tools/clippy/clippy_lints/src/swap.rs +++ b/src/tools/clippy/clippy_lints/src/swap.rs @@ -91,7 +91,7 @@ fn check_manual_swap(cx: &LateContext<'_>, block: &Block<'_>) { if let ExprKind::Path(QPath::Resolved(None, ref rhs2)) = rhs2.kind; if rhs2.segments.len() == 1; - if ident.as_str() == rhs2.segments[0].ident.as_str(); + if ident.name == rhs2.segments[0].ident.name; if eq_expr_value(cx, tmp_init, lhs1); if eq_expr_value(cx, rhs1, lhs2); then { diff --git a/src/tools/clippy/clippy_lints/src/to_string_in_display.rs b/src/tools/clippy/clippy_lints/src/to_string_in_display.rs index 675eaf4277a..c53727ba160 100644 --- a/src/tools/clippy/clippy_lints/src/to_string_in_display.rs +++ b/src/tools/clippy/clippy_lints/src/to_string_in_display.rs @@ -1,7 +1,7 @@ use crate::utils::{match_def_path, match_trait_method, paths, qpath_res, span_lint}; use if_chain::if_chain; use rustc_hir::def::Res; -use rustc_hir::{Expr, ExprKind, HirId, ImplItem, ImplItemKind, Item, ItemKind, Impl}; +use rustc_hir::{Expr, ExprKind, HirId, Impl, ImplItem, ImplItemKind, Item, ItemKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_tool_lint, impl_lint_pass}; diff --git a/src/tools/clippy/clippy_lints/src/types.rs b/src/tools/clippy/clippy_lints/src/types.rs index 2696c5e781a..3b5a83d2a0b 100644 --- a/src/tools/clippy/clippy_lints/src/types.rs +++ b/src/tools/clippy/clippy_lints/src/types.rs @@ -8,7 +8,6 @@ use if_chain::if_chain; use rustc_ast::{FloatTy, IntTy, LitFloatType, LitIntType, LitKind, UintTy}; use rustc_errors::{Applicability, DiagnosticBuilder}; use rustc_hir as hir; -use rustc_hir::def::Res; use rustc_hir::intravisit::{walk_body, walk_expr, walk_ty, FnKind, NestedVisitorMap, Visitor}; use rustc_hir::{ BinOpKind, Block, Body, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, GenericBounds, GenericParamKind, HirId, @@ -19,7 +18,8 @@ use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::hir::map::Map; use rustc_middle::lint::in_external_macro; use rustc_middle::ty::TypeFoldable; -use rustc_middle::ty::{self, InferTy, Ty, TyCtxt, TyS, TypeckResults}; +use rustc_middle::ty::{self, InferTy, Ty, TyCtxt, TyS, TypeAndMut, TypeckResults}; +use rustc_semver::RustcVersion; use rustc_session::{declare_lint_pass, declare_tool_lint, impl_lint_pass}; use rustc_span::hygiene::{ExpnKind, MacroKind}; use rustc_span::source_map::Span; @@ -30,11 +30,13 @@ use rustc_typeck::hir_ty_to_ty; use crate::consts::{constant, Constant}; use crate::utils::paths; +use crate::utils::sugg::Sugg; use crate::utils::{ - clip, comparisons, differing_macro_contexts, higher, in_constant, indent_of, int_bits, is_type_diagnostic_item, - last_path_segment, match_def_path, match_path, method_chain_args, multispan_sugg, numeric_literal::NumericLiteral, - qpath_res, reindent_multiline, sext, snippet, snippet_opt, snippet_with_applicability, snippet_with_macro_callsite, - span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then, unsext, + clip, comparisons, differing_macro_contexts, higher, in_constant, indent_of, int_bits, is_hir_ty_cfg_dependant, + is_type_diagnostic_item, last_path_segment, match_def_path, match_path, meets_msrv, method_chain_args, + multispan_sugg, numeric_literal::NumericLiteral, qpath_res, reindent_multiline, sext, snippet, snippet_opt, + snippet_with_applicability, snippet_with_macro_callsite, span_lint, span_lint_and_help, span_lint_and_sugg, + span_lint_and_then, unsext, }; declare_clippy_lint! { @@ -73,7 +75,7 @@ declare_clippy_lint! { /// **Why is this bad?** `Vec` already keeps its contents in a separate area on /// the heap. So if you `Box` its contents, you just add another level of indirection. /// - /// **Known problems:** Vec<Box<T: Sized>> makes sense if T is a large type (see #3530, + /// **Known problems:** Vec<Box<T: Sized>> makes sense if T is a large type (see [#3530](https://github.com/rust-lang/rust-clippy/issues/3530), /// 1st comment). /// /// **Example:** @@ -1279,8 +1281,8 @@ declare_clippy_lint! { } declare_clippy_lint! { - /// **What it does:** Checks for casts from a less-strictly-aligned pointer to a - /// more-strictly-aligned pointer + /// **What it does:** Checks for casts, using `as` or `pointer::cast`, + /// from a less-strictly-aligned pointer to a more-strictly-aligned pointer /// /// **Why is this bad?** Dereferencing the resulting pointer may be undefined /// behavior. @@ -1293,6 +1295,9 @@ declare_clippy_lint! { /// ```rust /// let _ = (&1u8 as *const u8) as *const u16; /// let _ = (&mut 1u8 as *mut u8) as *mut u16; + /// + /// (&1u8 as *const u8).cast::<u16>(); + /// (&mut 1u8 as *mut u8).cast::<u16>(); /// ``` pub CAST_PTR_ALIGNMENT, pedantic, @@ -1634,12 +1639,8 @@ impl<'tcx> LateLintPass<'tcx> for Casts { return; } if let ExprKind::Cast(ref ex, cast_to) = expr.kind { - if let TyKind::Path(QPath::Resolved(_, path)) = cast_to.kind { - if let Res::Def(_, def_id) = path.res { - if cx.tcx.has_attr(def_id, sym::cfg) || cx.tcx.has_attr(def_id, sym::cfg_attr) { - return; - } - } + if is_hir_ty_cfg_dependant(cx, cast_to) { + return; } let (cast_from, cast_to) = (cx.typeck_results().expr_ty(ex), cx.typeck_results().expr_ty(expr)); lint_fn_to_numeric_cast(cx, expr, ex, cast_from, cast_to); @@ -1689,6 +1690,19 @@ impl<'tcx> LateLintPass<'tcx> for Casts { } lint_cast_ptr_alignment(cx, expr, cast_from, cast_to); + } else if let ExprKind::MethodCall(method_path, _, args, _) = expr.kind { + if_chain! { + if method_path.ident.name == sym!(cast); + if let Some(generic_args) = method_path.args; + if let [GenericArg::Type(cast_to)] = generic_args.args; + // There probably is no obvious reason to do this, just to be consistent with `as` cases. + if !is_hir_ty_cfg_dependant(cx, cast_to); + then { + let (cast_from, cast_to) = + (cx.typeck_results().expr_ty(&args[0]), cx.typeck_results().expr_ty(expr)); + lint_cast_ptr_alignment(cx, expr, cast_from, cast_to); + } + } } } } @@ -2873,3 +2887,93 @@ impl<'tcx> LateLintPass<'tcx> for RefToMut { } } } + +const PTR_AS_PTR_MSRV: RustcVersion = RustcVersion::new(1, 38, 0); + +declare_clippy_lint! { + /// **What it does:** + /// Checks for `as` casts between raw pointers without changing its mutability, + /// namely `*const T` to `*const U` and `*mut T` to `*mut U`. + /// + /// **Why is this bad?** + /// Though `as` casts between raw pointers is not terrible, `pointer::cast` is safer because + /// it cannot accidentally change the pointer's mutability nor cast the pointer to other types like `usize`. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// let ptr: *const u32 = &42_u32; + /// let mut_ptr: *mut u32 = &mut 42_u32; + /// let _ = ptr as *const i32; + /// let _ = mut_ptr as *mut i32; + /// ``` + /// Use instead: + /// ```rust + /// let ptr: *const u32 = &42_u32; + /// let mut_ptr: *mut u32 = &mut 42_u32; + /// let _ = ptr.cast::<i32>(); + /// let _ = mut_ptr.cast::<i32>(); + /// ``` + pub PTR_AS_PTR, + pedantic, + "casting using `as` from and to raw pointers that doesn't change its mutability, where `pointer::cast` could take the place of `as`" +} + +pub struct PtrAsPtr { + msrv: Option<RustcVersion>, +} + +impl PtrAsPtr { + #[must_use] + pub fn new(msrv: Option<RustcVersion>) -> Self { + Self { msrv } + } +} + +impl_lint_pass!(PtrAsPtr => [PTR_AS_PTR]); + +impl<'tcx> LateLintPass<'tcx> for PtrAsPtr { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if !meets_msrv(self.msrv.as_ref(), &PTR_AS_PTR_MSRV) { + return; + } + + if expr.span.from_expansion() { + return; + } + + if_chain! { + if let ExprKind::Cast(cast_expr, cast_to_hir_ty) = expr.kind; + let (cast_from, cast_to) = (cx.typeck_results().expr_ty(cast_expr), cx.typeck_results().expr_ty(expr)); + if let ty::RawPtr(TypeAndMut { mutbl: from_mutbl, .. }) = cast_from.kind(); + if let ty::RawPtr(TypeAndMut { ty: to_pointee_ty, mutbl: to_mutbl }) = cast_to.kind(); + if matches!((from_mutbl, to_mutbl), + (Mutability::Not, Mutability::Not) | (Mutability::Mut, Mutability::Mut)); + // The `U` in `pointer::cast` have to be `Sized` + // as explained here: https://github.com/rust-lang/rust/issues/60602. + if to_pointee_ty.is_sized(cx.tcx.at(expr.span), cx.param_env); + then { + let mut applicability = Applicability::MachineApplicable; + let cast_expr_sugg = Sugg::hir_with_applicability(cx, cast_expr, "_", &mut applicability); + let turbofish = match &cast_to_hir_ty.kind { + TyKind::Infer => Cow::Borrowed(""), + TyKind::Ptr(mut_ty) if matches!(mut_ty.ty.kind, TyKind::Infer) => Cow::Borrowed(""), + _ => Cow::Owned(format!("::<{}>", to_pointee_ty)), + }; + span_lint_and_sugg( + cx, + PTR_AS_PTR, + expr.span, + "`as` casting between raw pointers without changing its mutability", + "try `pointer::cast`, a safer alternative", + format!("{}.cast{}()", cast_expr_sugg.maybe_par(), turbofish), + applicability, + ); + } + } + } + + extract_msrv_attr!(LateContext); +} diff --git a/src/tools/clippy/clippy_lints/src/unnecessary_sort_by.rs b/src/tools/clippy/clippy_lints/src/unnecessary_sort_by.rs index 0bccfc15678..9b45d38afd4 100644 --- a/src/tools/clippy/clippy_lints/src/unnecessary_sort_by.rs +++ b/src/tools/clippy/clippy_lints/src/unnecessary_sort_by.rs @@ -183,7 +183,7 @@ fn detect_lint(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<LintTrigger> { Param { pat: Pat { kind: PatKind::Binding(_, _, right_ident, _), .. }, .. } ] = &closure_body.params; if let ExprKind::MethodCall(method_path, _, [ref left_expr, ref right_expr], _) = &closure_body.value.kind; - if method_path.ident.name.to_ident_string() == "cmp"; + if method_path.ident.name == sym::cmp; then { let (closure_body, closure_arg, reverse) = if mirrored_exprs( &cx, diff --git a/src/tools/clippy/clippy_lints/src/unnecessary_wraps.rs b/src/tools/clippy/clippy_lints/src/unnecessary_wraps.rs index 07cd752184b..8ac5dd696b7 100644 --- a/src/tools/clippy/clippy_lints/src/unnecessary_wraps.rs +++ b/src/tools/clippy/clippy_lints/src/unnecessary_wraps.rs @@ -5,7 +5,7 @@ use crate::utils::{ use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::intravisit::FnKind; -use rustc_hir::{Body, ExprKind, FnDecl, HirId, ItemKind, Impl, Node}; +use rustc_hir::{Body, ExprKind, FnDecl, HirId, Impl, ItemKind, Node}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::subst::GenericArgKind; use rustc_session::{declare_lint_pass, declare_tool_lint}; diff --git a/src/tools/clippy/clippy_lints/src/unused_self.rs b/src/tools/clippy/clippy_lints/src/unused_self.rs index a6171794311..5349c4f7eb8 100644 --- a/src/tools/clippy/clippy_lints/src/unused_self.rs +++ b/src/tools/clippy/clippy_lints/src/unused_self.rs @@ -1,7 +1,7 @@ use if_chain::if_chain; use rustc_hir::def::Res; use rustc_hir::intravisit::{walk_path, NestedVisitorMap, Visitor}; -use rustc_hir::{HirId, ImplItem, ImplItemKind, ItemKind, Impl, Path}; +use rustc_hir::{HirId, Impl, ImplItem, ImplItemKind, ItemKind, Path}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::hir::map::Map; use rustc_session::{declare_lint_pass, declare_tool_lint}; diff --git a/src/tools/clippy/clippy_lints/src/unwrap.rs b/src/tools/clippy/clippy_lints/src/unwrap.rs index 6a87f534369..b82909eaea6 100644 --- a/src/tools/clippy/clippy_lints/src/unwrap.rs +++ b/src/tools/clippy/clippy_lints/src/unwrap.rs @@ -1,6 +1,5 @@ use crate::utils::{ - differing_macro_contexts, is_type_diagnostic_item, span_lint_and_then, - usage::is_potentially_mutated, + differing_macro_contexts, is_type_diagnostic_item, span_lint_and_then, usage::is_potentially_mutated, }; use if_chain::if_chain; use rustc_hir::intravisit::{walk_expr, walk_fn, FnKind, NestedVisitorMap, Visitor}; diff --git a/src/tools/clippy/clippy_lints/src/use_self.rs b/src/tools/clippy/clippy_lints/src/use_self.rs index b82ea66190f..72d1ca73929 100644 --- a/src/tools/clippy/clippy_lints/src/use_self.rs +++ b/src/tools/clippy/clippy_lints/src/use_self.rs @@ -28,8 +28,8 @@ declare_clippy_lint! { /// feels inconsistent. /// /// **Known problems:** - /// - False positive when using associated types (#2843) - /// - False positives in some situations when using generics (#3410) + /// - False positive when using associated types ([#2843](https://github.com/rust-lang/rust-clippy/issues/2843)) + /// - False positives in some situations when using generics ([#3410](https://github.com/rust-lang/rust-clippy/issues/3410)) /// /// **Example:** /// ```rust diff --git a/src/tools/clippy/clippy_lints/src/useless_conversion.rs b/src/tools/clippy/clippy_lints/src/useless_conversion.rs index efa9c3fab4a..c5334853986 100644 --- a/src/tools/clippy/clippy_lints/src/useless_conversion.rs +++ b/src/tools/clippy/clippy_lints/src/useless_conversion.rs @@ -80,10 +80,10 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion { ); } } - if match_trait_method(cx, e, &paths::INTO_ITERATOR) && &*name.ident.as_str() == "into_iter" { + if match_trait_method(cx, e, &paths::INTO_ITERATOR) && name.ident.name == sym::into_iter { if let Some(parent_expr) = get_parent_expr(cx, e) { if let ExprKind::MethodCall(ref parent_name, ..) = parent_expr.kind { - if &*parent_name.ident.as_str() != "into_iter" { + if parent_name.ident.name != sym::into_iter { return; } } diff --git a/src/tools/clippy/clippy_lints/src/utils/attrs.rs b/src/tools/clippy/clippy_lints/src/utils/attrs.rs index 24052a243af..8d28421d70d 100644 --- a/src/tools/clippy/clippy_lints/src/utils/attrs.rs +++ b/src/tools/clippy/clippy_lints/src/utils/attrs.rs @@ -1,6 +1,7 @@ use rustc_ast::ast; use rustc_errors::Applicability; use rustc_session::Session; +use rustc_span::sym; use std::str::FromStr; /// Deprecation status of attributes known by Clippy. @@ -64,11 +65,11 @@ pub fn get_attr<'a>( return false; }; let attr_segments = &attr.path.segments; - if attr_segments.len() == 2 && attr_segments[0].ident.to_string() == "clippy" { + if attr_segments.len() == 2 && attr_segments[0].ident.name == sym::clippy { BUILTIN_ATTRIBUTES .iter() - .find_map(|(builtin_name, deprecation_status)| { - if *builtin_name == attr_segments[1].ident.to_string() { + .find_map(|&(builtin_name, ref deprecation_status)| { + if attr_segments[1].ident.name.as_str() == builtin_name { Some(deprecation_status) } else { None @@ -99,7 +100,7 @@ pub fn get_attr<'a>( }, DeprecationStatus::None => { diag.cancel(); - attr_segments[1].ident.to_string() == name + attr_segments[1].ident.name.as_str() == name }, } }, diff --git a/src/tools/clippy/clippy_lints/src/utils/hir_utils.rs b/src/tools/clippy/clippy_lints/src/utils/hir_utils.rs index 062df68c0a4..10120a8805d 100644 --- a/src/tools/clippy/clippy_lints/src/utils/hir_utils.rs +++ b/src/tools/clippy/clippy_lints/src/utils/hir_utils.rs @@ -86,7 +86,7 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { lb == rb && l_mut == r_mut && self.eq_expr(le, re) }, (&ExprKind::Continue(li), &ExprKind::Continue(ri)) => { - both(&li.label, &ri.label, |l, r| l.ident.as_str() == r.ident.as_str()) + both(&li.label, &ri.label, |l, r| l.ident.name == r.ident.name) }, (&ExprKind::Assign(ref ll, ref lr, _), &ExprKind::Assign(ref rl, ref rr, _)) => { self.allow_side_effects && self.eq_expr(ll, rl) && self.eq_expr(lr, rr) @@ -102,7 +102,7 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { }) }, (&ExprKind::Break(li, ref le), &ExprKind::Break(ri, ref re)) => { - both(&li.label, &ri.label, |l, r| l.ident.as_str() == r.ident.as_str()) + both(&li.label, &ri.label, |l, r| l.ident.name == r.ident.name) && both(le, re, |l, r| self.eq_expr(l, r)) }, (&ExprKind::Box(ref l), &ExprKind::Box(ref r)) => self.eq_expr(l, r), @@ -124,7 +124,7 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { }, (&ExprKind::Lit(ref l), &ExprKind::Lit(ref r)) => l.node == r.node, (&ExprKind::Loop(ref lb, ref ll, ref lls), &ExprKind::Loop(ref rb, ref rl, ref rls)) => { - lls == rls && self.eq_block(lb, rb) && both(ll, rl, |l, r| l.ident.as_str() == r.ident.as_str()) + lls == rls && self.eq_block(lb, rb) && both(ll, rl, |l, r| l.ident.name == r.ident.name) }, (&ExprKind::Match(ref le, ref la, ref ls), &ExprKind::Match(ref re, ref ra, ref rs)) => { ls == rs @@ -191,7 +191,7 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { pub fn eq_fieldpat(&mut self, left: &FieldPat<'_>, right: &FieldPat<'_>) -> bool { let (FieldPat { ident: li, pat: lp, .. }, FieldPat { ident: ri, pat: rp, .. }) = (&left, &right); - li.name.as_str() == ri.name.as_str() && self.eq_pat(lp, rp) + li.name == ri.name && self.eq_pat(lp, rp) } /// Checks whether two patterns are the same. @@ -205,7 +205,7 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { self.eq_qpath(lp, rp) && over(la, ra, |l, r| self.eq_pat(l, r)) && ls == rs }, (&PatKind::Binding(ref lb, .., ref li, ref lp), &PatKind::Binding(ref rb, .., ref ri, ref rp)) => { - lb == rb && li.name.as_str() == ri.name.as_str() && both(lp, rp, |l, r| self.eq_pat(l, r)) + lb == rb && li.name == ri.name && both(lp, rp, |l, r| self.eq_pat(l, r)) }, (&PatKind::Path(ref l), &PatKind::Path(ref r)) => self.eq_qpath(l, r), (&PatKind::Lit(ref l), &PatKind::Lit(ref r)) => self.eq_expr(l, r), @@ -266,8 +266,7 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { pub fn eq_path_segment(&mut self, left: &PathSegment<'_>, right: &PathSegment<'_>) -> bool { // The == of idents doesn't work with different contexts, // we have to be explicit about hygiene - left.ident.as_str() == right.ident.as_str() - && both(&left.args, &right.args, |l, r| self.eq_path_parameters(l, r)) + left.ident.name == right.ident.name && both(&left.args, &right.args, |l, r| self.eq_path_parameters(l, r)) } pub fn eq_ty(&mut self, left: &Ty<'_>, right: &Ty<'_>) -> bool { diff --git a/src/tools/clippy/clippy_lints/src/utils/internal_lints.rs b/src/tools/clippy/clippy_lints/src/utils/internal_lints.rs index 407f06f4894..7aa17520ba7 100644 --- a/src/tools/clippy/clippy_lints/src/utils/internal_lints.rs +++ b/src/tools/clippy/clippy_lints/src/utils/internal_lints.rs @@ -10,9 +10,12 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::def::{DefKind, Res}; +use rustc_hir::def_id::DefId; use rustc_hir::hir_id::CRATE_HIR_ID; use rustc_hir::intravisit::{NestedVisitorMap, Visitor}; -use rustc_hir::{Crate, Expr, ExprKind, HirId, Item, MutTy, Mutability, Node, Path, StmtKind, Ty, TyKind}; +use rustc_hir::{ + BinOpKind, Crate, Expr, ExprKind, HirId, Item, MutTy, Mutability, Node, Path, StmtKind, Ty, TyKind, UnOp, +}; use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass}; use rustc_middle::hir::map::Map; use rustc_middle::mir::interpret::ConstValue; @@ -272,6 +275,28 @@ declare_clippy_lint! { "interning a symbol that is pre-interned and defined as a constant" } +declare_clippy_lint! { + /// **What it does:** Checks for unnecessary conversion from Symbol to a string. + /// + /// **Why is this bad?** It's faster use symbols directly intead of strings. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// Bad: + /// ```rust,ignore + /// symbol.as_str() == "clippy"; + /// ``` + /// + /// Good: + /// ```rust,ignore + /// symbol == sym::clippy; + /// ``` + pub UNNECESSARY_SYMBOL_STR, + internal, + "unnecessary conversion between Symbol and string" +} + declare_lint_pass!(ClippyLintsInternal => [CLIPPY_LINTS_INTERNAL]); impl EarlyLintPass for ClippyLintsInternal { @@ -868,11 +893,11 @@ impl<'tcx> LateLintPass<'tcx> for InvalidPaths { #[derive(Default)] pub struct InterningDefinedSymbol { - // Maps the symbol value to the constant name. - symbol_map: FxHashMap<u32, String>, + // Maps the symbol value to the constant DefId. + symbol_map: FxHashMap<u32, DefId>, } -impl_lint_pass!(InterningDefinedSymbol => [INTERNING_DEFINED_SYMBOL]); +impl_lint_pass!(InterningDefinedSymbol => [INTERNING_DEFINED_SYMBOL, UNNECESSARY_SYMBOL_STR]); impl<'tcx> LateLintPass<'tcx> for InterningDefinedSymbol { fn check_crate(&mut self, cx: &LateContext<'_>, _: &Crate<'_>) { @@ -880,16 +905,18 @@ impl<'tcx> LateLintPass<'tcx> for InterningDefinedSymbol { return; } - if let Some(Res::Def(_, def_id)) = path_to_res(cx, &paths::SYM_MODULE) { - for item in cx.tcx.item_children(def_id).iter() { - if_chain! { - if let Res::Def(DefKind::Const, item_def_id) = item.res; - let ty = cx.tcx.type_of(item_def_id); - if match_type(cx, ty, &paths::SYMBOL); - if let Ok(ConstValue::Scalar(value)) = cx.tcx.const_eval_poly(item_def_id); - if let Ok(value) = value.to_u32(); - then { - self.symbol_map.insert(value, item.ident.to_string()); + for &module in &[&paths::KW_MODULE, &paths::SYM_MODULE] { + if let Some(Res::Def(_, def_id)) = path_to_res(cx, module) { + for item in cx.tcx.item_children(def_id).iter() { + if_chain! { + if let Res::Def(DefKind::Const, item_def_id) = item.res; + let ty = cx.tcx.type_of(item_def_id); + if match_type(cx, ty, &paths::SYMBOL); + if let Ok(ConstValue::Scalar(value)) = cx.tcx.const_eval_poly(item_def_id); + if let Ok(value) = value.to_u32(); + then { + self.symbol_map.insert(value, item_def_id); + } } } } @@ -903,7 +930,7 @@ impl<'tcx> LateLintPass<'tcx> for InterningDefinedSymbol { if match_def_path(cx, *def_id, &paths::SYMBOL_INTERN); if let Some(Constant::Str(arg)) = constant_simple(cx, cx.typeck_results(), arg); let value = Symbol::intern(&arg).as_u32(); - if let Some(symbol_const) = self.symbol_map.get(&value); + if let Some(&def_id) = self.symbol_map.get(&value); then { span_lint_and_sugg( cx, @@ -911,10 +938,135 @@ impl<'tcx> LateLintPass<'tcx> for InterningDefinedSymbol { is_expn_of(expr.span, "sym").unwrap_or(expr.span), "interning a defined symbol", "try", - format!("rustc_span::symbol::sym::{}", symbol_const), + cx.tcx.def_path_str(def_id), Applicability::MachineApplicable, ); } } + if let ExprKind::Binary(op, left, right) = expr.kind { + if matches!(op.node, BinOpKind::Eq | BinOpKind::Ne) { + let data = [ + (left, self.symbol_str_expr(left, cx)), + (right, self.symbol_str_expr(right, cx)), + ]; + match data { + // both operands are a symbol string + [(_, Some(left)), (_, Some(right))] => { + span_lint_and_sugg( + cx, + UNNECESSARY_SYMBOL_STR, + expr.span, + "unnecessary `Symbol` to string conversion", + "try", + format!( + "{} {} {}", + left.as_symbol_snippet(cx), + op.node.as_str(), + right.as_symbol_snippet(cx), + ), + Applicability::MachineApplicable, + ); + }, + // one of the operands is a symbol string + [(expr, Some(symbol)), _] | [_, (expr, Some(symbol))] => { + // creating an owned string for comparison + if matches!(symbol, SymbolStrExpr::Expr { is_to_owned: true, .. }) { + span_lint_and_sugg( + cx, + UNNECESSARY_SYMBOL_STR, + expr.span, + "unnecessary string allocation", + "try", + format!("{}.as_str()", symbol.as_symbol_snippet(cx)), + Applicability::MachineApplicable, + ); + } + }, + // nothing found + [(_, None), (_, None)] => {}, + } + } + } + } +} + +impl InterningDefinedSymbol { + fn symbol_str_expr<'tcx>(&self, expr: &'tcx Expr<'tcx>, cx: &LateContext<'tcx>) -> Option<SymbolStrExpr<'tcx>> { + static IDENT_STR_PATHS: &[&[&str]] = &[&paths::IDENT_AS_STR, &paths::TO_STRING_METHOD]; + static SYMBOL_STR_PATHS: &[&[&str]] = &[ + &paths::SYMBOL_AS_STR, + &paths::SYMBOL_TO_IDENT_STRING, + &paths::TO_STRING_METHOD, + ]; + // SymbolStr might be de-referenced: `&*symbol.as_str()` + let call = if_chain! { + if let ExprKind::AddrOf(_, _, e) = expr.kind; + if let ExprKind::Unary(UnOp::UnDeref, e) = e.kind; + then { e } else { expr } + }; + if_chain! { + // is a method call + if let ExprKind::MethodCall(_, _, [item], _) = call.kind; + if let Some(did) = cx.typeck_results().type_dependent_def_id(call.hir_id); + let ty = cx.typeck_results().expr_ty(item); + // ...on either an Ident or a Symbol + if let Some(is_ident) = if match_type(cx, ty, &paths::SYMBOL) { + Some(false) + } else if match_type(cx, ty, &paths::IDENT) { + Some(true) + } else { + None + }; + // ...which converts it to a string + let paths = if is_ident { IDENT_STR_PATHS } else { SYMBOL_STR_PATHS }; + if let Some(path) = paths.iter().find(|path| match_def_path(cx, did, path)); + then { + let is_to_owned = path.last().unwrap().ends_with("string"); + return Some(SymbolStrExpr::Expr { + item, + is_ident, + is_to_owned, + }); + } + } + // is a string constant + if let Some(Constant::Str(s)) = constant_simple(cx, cx.typeck_results(), expr) { + let value = Symbol::intern(&s).as_u32(); + // ...which matches a symbol constant + if let Some(&def_id) = self.symbol_map.get(&value) { + return Some(SymbolStrExpr::Const(def_id)); + } + } + None + } +} + +enum SymbolStrExpr<'tcx> { + /// a string constant with a corresponding symbol constant + Const(DefId), + /// a "symbol to string" expression like `symbol.as_str()` + Expr { + /// part that evaluates to `Symbol` or `Ident` + item: &'tcx Expr<'tcx>, + is_ident: bool, + /// whether an owned `String` is created like `to_ident_string()` + is_to_owned: bool, + }, +} + +impl<'tcx> SymbolStrExpr<'tcx> { + /// Returns a snippet that evaluates to a `Symbol` and is const if possible + fn as_symbol_snippet(&self, cx: &LateContext<'_>) -> Cow<'tcx, str> { + match *self { + Self::Const(def_id) => cx.tcx.def_path_str(def_id).into(), + Self::Expr { item, is_ident, .. } => { + let mut snip = snippet(cx, item.span.source_callsite(), ".."); + if is_ident { + // get `Ident.name` + snip.to_mut().push_str(".name"); + } + snip + }, + } } } diff --git a/src/tools/clippy/clippy_lints/src/utils/mod.rs b/src/tools/clippy/clippy_lints/src/utils/mod.rs index 4c707c4b904..9b262517a98 100644 --- a/src/tools/clippy/clippy_lints/src/utils/mod.rs +++ b/src/tools/clippy/clippy_lints/src/utils/mod.rs @@ -1,5 +1,5 @@ #[macro_use] -pub mod sym; +pub mod sym_helper; #[allow(clippy::module_name_repetitions)] pub mod ast_utils; @@ -56,8 +56,8 @@ use rustc_semver::RustcVersion; use rustc_session::Session; use rustc_span::hygiene::{ExpnKind, MacroKind}; use rustc_span::source_map::original_sp; -use rustc_span::sym as rustc_sym; -use rustc_span::symbol::{self, kw, Symbol}; +use rustc_span::sym; +use rustc_span::symbol::{kw, Symbol}; use rustc_span::{BytePos, Pos, Span, DUMMY_SP}; use rustc_target::abi::Integer; use rustc_trait_selection::traits::query::normalize::AtExt; @@ -1121,7 +1121,7 @@ pub fn is_refutable(cx: &LateContext<'_>, pat: &Pat<'_>) -> bool { /// Checks for the `#[automatically_derived]` attribute all `#[derive]`d /// implementations have. pub fn is_automatically_derived(attrs: &[ast::Attribute]) -> bool { - attrs.iter().any(|attr| attr.has_name(rustc_sym::automatically_derived)) + attrs.iter().any(|attr| attr.has_name(sym::automatically_derived)) } /// Remove blocks around an expression. @@ -1434,12 +1434,13 @@ pub fn parent_node_is_if_expr(expr: &Expr<'_>, cx: &LateContext<'_>) -> bool { let map = cx.tcx.hir(); let parent_id = map.get_parent_node(expr.hir_id); let parent_node = map.get(parent_id); - if let Node::Expr(Expr { kind: ExprKind::If(_, _, _), .. }) = parent_node { - true - } - else { - false - } + matches!( + parent_node, + Node::Expr(Expr { + kind: ExprKind::If(_, _, _), + .. + }) + ) } // Finds the attribute with the given name, if any @@ -1514,7 +1515,7 @@ pub fn is_must_use_func_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { pub fn is_no_std_crate(krate: &Crate<'_>) -> bool { krate.item.attrs.iter().any(|attr| { if let ast::AttrKind::Normal(ref attr, _) = attr.kind { - attr.path == symbol::sym::no_std + attr.path == sym::no_std } else { false } @@ -1686,6 +1687,18 @@ macro_rules! unwrap_cargo_metadata { }}; } +pub fn is_hir_ty_cfg_dependant(cx: &LateContext<'_>, ty: &hir::Ty<'_>) -> bool { + if_chain! { + if let TyKind::Path(QPath::Resolved(_, path)) = ty.kind; + if let Res::Def(_, def_id) = path.res; + then { + cx.tcx.has_attr(def_id, sym::cfg) || cx.tcx.has_attr(def_id, sym::cfg_attr) + } else { + false + } + } +} + #[cfg(test)] mod test { use super::{reindent_multiline, without_block_comments}; diff --git a/src/tools/clippy/clippy_lints/src/utils/paths.rs b/src/tools/clippy/clippy_lints/src/utils/paths.rs index 2080a49a11c..c0b203b5388 100644 --- a/src/tools/clippy/clippy_lints/src/utils/paths.rs +++ b/src/tools/clippy/clippy_lints/src/utils/paths.rs @@ -54,6 +54,10 @@ pub const HASH: [&str; 3] = ["core", "hash", "Hash"]; pub const HASHMAP: [&str; 5] = ["std", "collections", "hash", "map", "HashMap"]; pub const HASHMAP_ENTRY: [&str; 5] = ["std", "collections", "hash", "map", "Entry"]; pub const HASHSET: [&str; 5] = ["std", "collections", "hash", "set", "HashSet"]; +#[cfg(feature = "internal-lints")] +pub const IDENT: [&str; 3] = ["rustc_span", "symbol", "Ident"]; +#[cfg(feature = "internal-lints")] +pub const IDENT_AS_STR: [&str; 4] = ["rustc_span", "symbol", "Ident", "as_str"]; pub const INDEX: [&str; 3] = ["core", "ops", "Index"]; pub const INDEX_MUT: [&str; 3] = ["core", "ops", "IndexMut"]; pub const INSERT_STR: [&str; 4] = ["alloc", "string", "String", "insert_str"]; @@ -65,6 +69,8 @@ pub const IPADDR_V4: [&str; 4] = ["std", "net", "IpAddr", "V4"]; pub const IPADDR_V6: [&str; 4] = ["std", "net", "IpAddr", "V6"]; pub const ITERATOR: [&str; 5] = ["core", "iter", "traits", "iterator", "Iterator"]; #[cfg(feature = "internal-lints")] +pub const KW_MODULE: [&str; 3] = ["rustc_span", "symbol", "kw"]; +#[cfg(feature = "internal-lints")] pub const LATE_CONTEXT: [&str; 2] = ["rustc_lint", "LateContext"]; pub const LINKED_LIST: [&str; 4] = ["alloc", "collections", "linked_list", "LinkedList"]; #[cfg(feature = "internal-lints")] @@ -148,8 +154,12 @@ pub const STR_STARTS_WITH: [&str; 4] = ["core", "str", "<impl str>", "starts_wit #[cfg(feature = "internal-lints")] pub const SYMBOL: [&str; 3] = ["rustc_span", "symbol", "Symbol"]; #[cfg(feature = "internal-lints")] +pub const SYMBOL_AS_STR: [&str; 4] = ["rustc_span", "symbol", "Symbol", "as_str"]; +#[cfg(feature = "internal-lints")] pub const SYMBOL_INTERN: [&str; 4] = ["rustc_span", "symbol", "Symbol", "intern"]; #[cfg(feature = "internal-lints")] +pub const SYMBOL_TO_IDENT_STRING: [&str; 4] = ["rustc_span", "symbol", "Symbol", "to_ident_string"]; +#[cfg(feature = "internal-lints")] pub const SYM_MODULE: [&str; 3] = ["rustc_span", "symbol", "sym"]; #[cfg(feature = "internal-lints")] pub const SYNTAX_CONTEXT: [&str; 3] = ["rustc_span", "hygiene", "SyntaxContext"]; diff --git a/src/tools/clippy/clippy_lints/src/utils/sym.rs b/src/tools/clippy/clippy_lints/src/utils/sym_helper.rs index 273288c3d52..f47dc80ebad 100644 --- a/src/tools/clippy/clippy_lints/src/utils/sym.rs +++ b/src/tools/clippy/clippy_lints/src/utils/sym_helper.rs @@ -1,4 +1,5 @@ #[macro_export] +/// Convenience wrapper around rustc's `Symbol::intern` macro_rules! sym { ($tt:tt) => { rustc_span::symbol::Symbol::intern(stringify!($tt)) diff --git a/src/tools/clippy/clippy_lints/src/utils/visitors.rs b/src/tools/clippy/clippy_lints/src/utils/visitors.rs index b769a18802b..ebf69df31ca 100644 --- a/src/tools/clippy/clippy_lints/src/utils/visitors.rs +++ b/src/tools/clippy/clippy_lints/src/utils/visitors.rs @@ -107,7 +107,7 @@ where if let Some(el) = else_opt { self.visit_expr(el); } - } + }, hir::ExprKind::Match(cond, arms, _) => { self.inside_stmt(true).visit_expr(cond); for arm in arms { diff --git a/src/tools/clippy/clippy_lints/src/vec_init_then_push.rs b/src/tools/clippy/clippy_lints/src/vec_init_then_push.rs new file mode 100644 index 00000000000..e632a7e57ee --- /dev/null +++ b/src/tools/clippy/clippy_lints/src/vec_init_then_push.rs @@ -0,0 +1,187 @@ +use crate::utils::{is_type_diagnostic_item, match_def_path, paths, snippet, span_lint_and_sugg}; +use if_chain::if_chain; +use rustc_ast::ast::LitKind; +use rustc_errors::Applicability; +use rustc_hir::{BindingAnnotation, Block, Expr, ExprKind, Local, PatKind, QPath, Stmt, StmtKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::{symbol::sym, Span, Symbol}; +use std::convert::TryInto; + +declare_clippy_lint! { + /// **What it does:** Checks for calls to `push` immediately after creating a new `Vec`. + /// + /// **Why is this bad?** The `vec![]` macro is both more performant and easier to read than + /// multiple `push` calls. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// let mut v = Vec::new(); + /// v.push(0); + /// ``` + /// Use instead: + /// ```rust + /// let v = vec![0]; + /// ``` + pub VEC_INIT_THEN_PUSH, + perf, + "`push` immediately after `Vec` creation" +} + +impl_lint_pass!(VecInitThenPush => [VEC_INIT_THEN_PUSH]); + +#[derive(Default)] +pub struct VecInitThenPush { + searcher: Option<VecPushSearcher>, +} + +#[derive(Clone, Copy)] +enum VecInitKind { + New, + WithCapacity(u64), +} +struct VecPushSearcher { + init: VecInitKind, + name: Symbol, + lhs_is_local: bool, + lhs_span: Span, + err_span: Span, + found: u64, +} +impl VecPushSearcher { + fn display_err(&self, cx: &LateContext<'_>) { + match self.init { + _ if self.found == 0 => return, + VecInitKind::WithCapacity(x) if x > self.found => return, + _ => (), + }; + + let mut s = if self.lhs_is_local { + String::from("let ") + } else { + String::new() + }; + s.push_str(&snippet(cx, self.lhs_span, "..")); + s.push_str(" = vec![..];"); + + span_lint_and_sugg( + cx, + VEC_INIT_THEN_PUSH, + self.err_span, + "calls to `push` immediately after creation", + "consider using the `vec![]` macro", + s, + Applicability::HasPlaceholders, + ); + } +} + +impl LateLintPass<'_> for VecInitThenPush { + fn check_local(&mut self, cx: &LateContext<'tcx>, local: &'tcx Local<'tcx>) { + self.searcher = None; + if_chain! { + if !in_external_macro(cx.sess(), local.span); + if let Some(init) = local.init; + if let PatKind::Binding(BindingAnnotation::Mutable, _, ident, None) = local.pat.kind; + if let Some(init_kind) = get_vec_init_kind(cx, init); + then { + self.searcher = Some(VecPushSearcher { + init: init_kind, + name: ident.name, + lhs_is_local: true, + lhs_span: local.ty.map_or(local.pat.span, |t| local.pat.span.to(t.span)), + err_span: local.span, + found: 0, + }); + } + } + } + + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if self.searcher.is_none() { + if_chain! { + if !in_external_macro(cx.sess(), expr.span); + if let ExprKind::Assign(left, right, _) = expr.kind; + if let ExprKind::Path(QPath::Resolved(_, path)) = left.kind; + if let Some(name) = path.segments.get(0); + if let Some(init_kind) = get_vec_init_kind(cx, right); + then { + self.searcher = Some(VecPushSearcher { + init: init_kind, + name: name.ident.name, + lhs_is_local: false, + lhs_span: left.span, + err_span: expr.span, + found: 0, + }); + } + } + } + } + + fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) { + if let Some(searcher) = self.searcher.take() { + if_chain! { + if let StmtKind::Expr(expr) | StmtKind::Semi(expr) = stmt.kind; + if let ExprKind::MethodCall(path, _, [self_arg, _], _) = expr.kind; + if path.ident.name.as_str() == "push"; + if let ExprKind::Path(QPath::Resolved(_, self_path)) = self_arg.kind; + if let [self_name] = self_path.segments; + if self_name.ident.name == searcher.name; + then { + self.searcher = Some(VecPushSearcher { + found: searcher.found + 1, + err_span: searcher.err_span.to(stmt.span), + .. searcher + }); + } else { + searcher.display_err(cx); + } + } + } + } + + fn check_block_post(&mut self, cx: &LateContext<'tcx>, _: &'tcx Block<'tcx>) { + if let Some(searcher) = self.searcher.take() { + searcher.display_err(cx); + } + } +} + +fn get_vec_init_kind<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<VecInitKind> { + if let ExprKind::Call(func, args) = expr.kind { + match func.kind { + ExprKind::Path(QPath::TypeRelative(ty, name)) + if is_type_diagnostic_item(cx, cx.typeck_results().node_type(ty.hir_id), sym::vec_type) => + { + if name.ident.name == sym::new { + return Some(VecInitKind::New); + } else if name.ident.name.as_str() == "with_capacity" { + return args.get(0).and_then(|arg| { + if_chain! { + if let ExprKind::Lit(lit) = &arg.kind; + if let LitKind::Int(num, _) = lit.node; + then { + Some(VecInitKind::WithCapacity(num.try_into().ok()?)) + } else { + None + } + } + }); + } + } + ExprKind::Path(QPath::Resolved(_, path)) + if match_def_path(cx, path.res.opt_def_id()?, &paths::DEFAULT_TRAIT_METHOD) + && is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::vec_type) => + { + return Some(VecInitKind::New); + } + _ => (), + } + } + None +} diff --git a/src/tools/clippy/clippy_lints/src/wildcard_imports.rs b/src/tools/clippy/clippy_lints/src/wildcard_imports.rs index 5683a71efea..10005a7fc81 100644 --- a/src/tools/clippy/clippy_lints/src/wildcard_imports.rs +++ b/src/tools/clippy/clippy_lints/src/wildcard_imports.rs @@ -7,7 +7,8 @@ use rustc_hir::{ }; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::BytePos; +use rustc_span::symbol::kw; +use rustc_span::{sym, BytePos}; declare_clippy_lint! { /// **What it does:** Checks for `use Enum::*`. @@ -198,12 +199,12 @@ impl WildcardImports { // Allow "...prelude::..::*" imports. // Many crates have a prelude, and it is imported as a glob by design. fn is_prelude_import(segments: &[PathSegment<'_>]) -> bool { - segments.iter().any(|ps| ps.ident.as_str() == "prelude") + segments.iter().any(|ps| ps.ident.name == sym::prelude) } // Allow "super::*" imports in tests. fn is_super_only_import(segments: &[PathSegment<'_>]) -> bool { - segments.len() == 1 && segments[0].ident.as_str() == "super" + segments.len() == 1 && segments[0].ident.name == kw::Super } fn is_test_module_or_function(item: &Item<'_>) -> bool { diff --git a/src/tools/clippy/clippy_lints/src/write.rs b/src/tools/clippy/clippy_lints/src/write.rs index 337f7a229b9..af324f831df 100644 --- a/src/tools/clippy/clippy_lints/src/write.rs +++ b/src/tools/clippy/clippy_lints/src/write.rs @@ -10,7 +10,8 @@ use rustc_lexer::unescape::{self, EscapeError}; use rustc_lint::{EarlyContext, EarlyLintPass}; use rustc_parse::parser; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::{sym, BytePos, Span, Symbol}; +use rustc_span::symbol::kw; +use rustc_span::{sym, BytePos, Span}; declare_clippy_lint! { /// **What it does:** This lint warns when you use `println!("")` to @@ -301,7 +302,7 @@ impl EarlyLintPass for Write { } } else if mac.path == sym!(writeln) { if let (Some(fmt_str), expr) = self.check_tts(cx, mac.args.inner_tokens(), true) { - if fmt_str.symbol == Symbol::intern("") { + if fmt_str.symbol == kw::Empty { let mut applicability = Applicability::MachineApplicable; // FIXME: remove this `#[allow(...)]` once the issue #5822 gets fixed #[allow(clippy::option_if_let_else)] @@ -484,7 +485,7 @@ impl Write { fn lint_println_empty_string(&self, cx: &EarlyContext<'_>, mac: &MacCall) { if let (Some(fmt_str), _) = self.check_tts(cx, mac.args.inner_tokens(), false) { - if fmt_str.symbol == Symbol::intern("") { + if fmt_str.symbol == kw::Empty { let name = mac.path.segments[0].ident.name; span_lint_and_sugg( cx, diff --git a/src/tools/clippy/doc/adding_lints.md b/src/tools/clippy/doc/adding_lints.md index 60dfdb76650..1a7a30c61be 100644 --- a/src/tools/clippy/doc/adding_lints.md +++ b/src/tools/clippy/doc/adding_lints.md @@ -147,10 +147,14 @@ add `// edition:2018` at the top of the test file (note that it's space-sensitiv Manually testing against an example file can be useful if you have added some `println!`s and the test suite output becomes unreadable. To try Clippy with -your local modifications, run `env CLIPPY_TESTS=true cargo run --bin -clippy-driver -- -L ./target/debug input.rs` from the working copy root. +your local modifications, run -With tests in place, let's have a look at implementing our lint now. +``` +env __CLIPPY_INTERNAL_TESTS=true cargo run --bin clippy-driver -- -L ./target/debug input.rs +``` + +from the working copy root. With tests in place, let's have a look at +implementing our lint now. ## Lint declaration diff --git a/src/tools/clippy/doc/roadmap-2021.md b/src/tools/clippy/doc/roadmap-2021.md new file mode 100644 index 00000000000..fe8b080f56f --- /dev/null +++ b/src/tools/clippy/doc/roadmap-2021.md @@ -0,0 +1,235 @@ +# Roadmap 2021 + +# Summary + +This Roadmap lays out the plans for Clippy in 2021: + +- Improving usability and reliability +- Improving experience of contributors and maintainers +- Develop and specify processes + +Members of the Clippy team will be assigned tasks from one or more of these +topics. The team member is then responsible to complete the assigned tasks. This +can either be done by implementing them or by providing mentorship to interested +contributors. + +# Motivation + +With the ongoing growth of the Rust language and with that of the whole +ecosystem, also Clippy gets more and more users and contributors. This is good +for the project, but also brings challenges along. Some of these challenges are: + +- More issues about reliability or usability are popping up +- Traffic is hard to handle for a small team +- Bigger projects don't get completed due to the lack of processes and/or time + of the team members + +Additionally, according to the [Rust Roadmap 2021], clear processes should be +defined by every team and unified across teams. This Roadmap is the first step +towards this. + +[Rust Roadmap 2021]: https://github.com/rust-lang/rfcs/pull/3037 + +# Explanation + +This section will explain the things that should be done in 2021. It is +important to note, that this document focuses on the "What?", not the "How?". +The later will be addressed in follow-up tracking issue, with an assigned team +member. + +The following is split up in two major sections. The first section covers the +user facing plans, the second section the internal plans. + +## User Facing + +Clippy should be as pleasant to use and configure as possible. This section +covers plans that should be implemented to improve the situation of Clippy in +this regard. + +### Usability + +In the following, plans to improve the usability are covered. + +#### No Output After `cargo check` + +Currently when `cargo clippy` is run after `cargo check`, it does not produce +any output. This is especially problematic since `rust-analyzer` is on the rise +and it uses `cargo check` for checking code. A fix is already implemented, but +it still has to be pushed over the finish line. This also includes the +stabilization of the `cargo clippy --fix` command or the support of multi-span +suggestions in `rustfix`. + +- [#4612](https://github.com/rust-lang/rust-clippy/issues/4612) + +#### `lints.toml` Configuration + +This is something that comes up every now and then: a reusable configuration +file, where lint levels can be defined. Discussions about this often lead to +nothing specific or to "we need an RFC for this". And this is exactly what needs +to be done. Get together with the cargo team and write an RFC and implement such +a configuration file somehow and somewhere. + +- [#3164](https://github.com/rust-lang/rust-clippy/issues/3164) +- [cargo#5034](https://github.com/rust-lang/cargo/issues/5034) +- [IRLO](https://internals.rust-lang.org/t/proposal-cargo-lint-configuration/9135/8) + +#### Lint Groups + +There are more and more issues about managing lints in Clippy popping up. Lints +are hard to implement with a guarantee of no/few false positives (FPs). One way +to address this might be to introduce more lint groups to give users the ability +to better manage lints, or improve the process of classifying lints, so that +disabling lints due to FPs becomes rare. It is important to note, that Clippy +lints are less conservative than `rustc` lints, which won't change in the +future. + +- [#5537](https://github.com/rust-lang/rust-clippy/issues/5537) +- [#6366](https://github.com/rust-lang/rust-clippy/issues/6366) + +### Reliability + +In the following, plans to improve the reliability are covered. + +#### False Positive Rate + +In the worst case, new lints are only available in nightly for 2 weeks, before +hitting beta and ultimately stable. This and the fact that fewer people use +nightly Rust nowadays makes it more probable that a lint with many FPs hits +stable. This leads to annoyed users, that will disable these new lints in the +best case and to more annoyed users, that will stop using Clippy in the worst. +A process should be developed and implemented to prevent this from happening. + +- [#6429](https://github.com/rust-lang/rust-clippy/issues/6429) + +## Internal + +(The end of) 2020 has shown, that Clippy has to think about the available +resources, especially regarding management and maintenance of the project. This +section address issues affecting team members and contributors. + +### Management + +In 2020 Clippy achieved over 1000 open issues with regularly between 25-35 open +PRs. This is simultaneously a win and a loss. More issues and PRs means more +people are interested in Clippy and in contributing to it. On the other hand, it +means for team members more work and for contributors longer wait times for +reviews. The following will describe plans how to improve the situation for both +team members and contributors. + +#### Clear Expectations for Team Members + +According to the [Rust Roadmap 2021], a document specifying what it means to be +a member of the team should be produced. This should not put more pressure on +the team members, but rather help them and interested folks to know what the +expectations are. With this it should also be easier to recruit new team members +and may encourage people to get in touch, if they're interested to join. + +#### Scaling up the Team + +More people means less work for each individual. Together with the document +about expectations for team members, a document defining the process of how to +join the team should be produced. This can also increase the stability of the +team, in case of current members dropping out (temporarily). There can also be +different roles in the team, like people triaging vs. people reviewing. + +#### Regular Meetings + +Other teams have regular meetings. Clippy is big enough that it might be worth +to also do them. Especially if more people join the team, this can be important +for sync-ups. Besides the asynchronous communication, that works well for +working on separate lints, a meeting adds a synchronous alternative at a known +time. This is especially helpful if there are bigger things that need to be +discussed (like the projects in this roadmap). For starters bi-weekly meetings +before Rust syncs might make sense. + +#### Triaging + +To get a handle on the influx of open issues, a process for triaging issues and +PRs should be developed. Officially, Clippy follows the Rust triage process, but +currently no one enforces it. This can be improved by sharing triage teams +across projects or by implementing dashboards / tools which simplify triaging. + +### Development + +Improving the developer and contributor experience is something the Clippy team +works on regularly. Though, some things might need special attention and +planing. These topics are listed in the following. + +#### Process for New and Existing Lints + +As already mentioned above, classifying new lints gets quite hard, because the +probability of a buggy lint getting into stable is quite high. A process should +be implemented on how to classify lints. In addition, a test system should be +developed to find out which lints are currently problematic in real world code +to fix or disable them. + +- [#6429 (comment)](https://github.com/rust-lang/rust-clippy/issues/6429#issuecomment-741056379) +- [#6429 (comment)](https://github.com/rust-lang/rust-clippy/issues/6429#issuecomment-741153345) + +#### Processes + +Related to the point before, a process for suggesting and discussing major +changes should be implemented. It's also not clearly defined when a lint should +be enabled or disabled by default. This can also be improved by the test system +mentioned above. + +#### Dev-Tools + +There's already `cargo dev` which makes Clippy development easier and more +pleasant. This can still be expanded, so that it covers more areas of the +development process. + +- [#5394](https://github.com/rust-lang/rust-clippy/issues/5394) + +#### Contributor Guide + +Similar to a Clippy Book, which describes how to use Clippy, a book about how to +contribute to Clippy might be helpful for new and existing contributors. There's +already the `doc` directory in the Clippy repo, this can be turned into a +`mdbook`. + +#### `rustc` integration + +Recently Clippy was integrated with `git subtree` into the `rust-lang/rust` +repository. This made syncing between the two repositories easier. A +`#[non_exhaustive]` list of things that still can be improved is: + +1. Use the same `rustfmt` version and configuration as `rustc`. +2. Make `cargo dev` work in the Rust repo, just as it works in the Clippy repo. + E.g. `cargo dev bless` or `cargo dev update_lints`. And even add more things + to it that might be useful for the Rust repo, e.g. `cargo dev deprecate`. +3. Easier sync process. The `subtree` situation is not ideal. + +## Prioritization + +The most pressing issues for users of Clippy are of course the user facing +issues. So there should be a priority on those issues, but without losing track +of the internal issues listed in this document. + +Getting the FP rate of warn/deny-by-default lints under control should have the +highest priority. Other user facing issues should also get a high priority, but +shouldn't be in the way of addressing internal issues. + +To better manage the upcoming projects, the basic internal processes, like +meetings, tracking issues and documentation, should be established as soon as +possible. They might even be necessary to properly manage the projects, +regarding the user facing issues. + +# Prior Art + +## Rust Roadmap + +Rust's roadmap process was established by [RFC 1728] in 2016. Since then every +year a roadmap was published, that defined the bigger plans for the coming +years. This years roadmap can be found [here][Rust Roadmap 2021]. + +[RFC 1728]: https://rust-lang.github.io/rfcs/1728-north-star.html + +# Drawbacks + +## Big Roadmap + +This roadmap is pretty big and not all items listed in this document might be +addressed during 2021. Because this is the first roadmap for Clippy, having open +tasks at the end of 2021 is fine, but they should be revisited in the 2022 +roadmap. diff --git a/src/tools/clippy/rust-toolchain b/src/tools/clippy/rust-toolchain index c579beeae89..72935072f8c 100644 --- a/src/tools/clippy/rust-toolchain +++ b/src/tools/clippy/rust-toolchain @@ -1,3 +1,3 @@ [toolchain] -channel = "nightly-2021-01-02" +channel = "nightly-2021-01-15" components = ["llvm-tools-preview", "rustc-dev", "rust-src", "rustfmt"] diff --git a/src/tools/clippy/src/driver.rs b/src/tools/clippy/src/driver.rs index e490ee54c0b..f5f6c09ed8e 100644 --- a/src/tools/clippy/src/driver.rs +++ b/src/tools/clippy/src/driver.rs @@ -298,7 +298,7 @@ pub fn main() { // - IF Clippy is run on the main crate, not on deps (`!cap_lints_allow`) THEN // - IF `--no-deps` is not set (`!no_deps`) OR // - IF `--no-deps` is set and Clippy is run on the specified primary package - let clippy_tests_set = env::var("CLIPPY_TESTS").map_or(false, |val| val == "true"); + let clippy_tests_set = env::var("__CLIPPY_INTERNAL_TESTS").map_or(false, |val| val == "true"); let cap_lints_allow = arg_value(&orig_args, "--cap-lints", |val| val == "allow").is_some(); let in_primary_package = env::var("CARGO_PRIMARY_PACKAGE").is_ok(); diff --git a/src/tools/clippy/tests/compile-test.rs b/src/tools/clippy/tests/compile-test.rs index ec3af94b9ca..ea800336ef5 100644 --- a/src/tools/clippy/tests/compile-test.rs +++ b/src/tools/clippy/tests/compile-test.rs @@ -254,7 +254,7 @@ fn run_ui_cargo(config: &mut compiletest::Config) { fn prepare_env() { set_var("CLIPPY_DISABLE_DOCS_LINKS", "true"); - set_var("CLIPPY_TESTS", "true"); + set_var("__CLIPPY_INTERNAL_TESTS", "true"); //set_var("RUST_BACKTRACE", "0"); } diff --git a/src/tools/clippy/tests/ui-internal/interning_defined_symbol.fixed b/src/tools/clippy/tests/ui-internal/interning_defined_symbol.fixed index c6b84d2ef65..9ab845a573a 100644 --- a/src/tools/clippy/tests/ui-internal/interning_defined_symbol.fixed +++ b/src/tools/clippy/tests/ui-internal/interning_defined_symbol.fixed @@ -14,13 +14,16 @@ macro_rules! sym { fn main() { // Direct use of Symbol::intern - let _ = rustc_span::symbol::sym::f32; + let _ = rustc_span::sym::f32; // Using a sym macro - let _ = rustc_span::symbol::sym::f32; + let _ = rustc_span::sym::f32; // Correct suggestion when symbol isn't stringified constant name - let _ = rustc_span::symbol::sym::proc_dash_macro; + let _ = rustc_span::sym::proc_dash_macro; + + // interning a keyword + let _ = rustc_span::symbol::kw::SelfLower; // Interning a symbol that is not defined let _ = Symbol::intern("xyz123"); diff --git a/src/tools/clippy/tests/ui-internal/interning_defined_symbol.rs b/src/tools/clippy/tests/ui-internal/interning_defined_symbol.rs index 9ec82d4ad0b..a58e182971d 100644 --- a/src/tools/clippy/tests/ui-internal/interning_defined_symbol.rs +++ b/src/tools/clippy/tests/ui-internal/interning_defined_symbol.rs @@ -22,6 +22,9 @@ fn main() { // Correct suggestion when symbol isn't stringified constant name let _ = Symbol::intern("proc-macro"); + // interning a keyword + let _ = Symbol::intern("self"); + // Interning a symbol that is not defined let _ = Symbol::intern("xyz123"); let _ = sym!(xyz123); diff --git a/src/tools/clippy/tests/ui-internal/interning_defined_symbol.stderr b/src/tools/clippy/tests/ui-internal/interning_defined_symbol.stderr index 74b906c8a57..50c1c268eb1 100644 --- a/src/tools/clippy/tests/ui-internal/interning_defined_symbol.stderr +++ b/src/tools/clippy/tests/ui-internal/interning_defined_symbol.stderr @@ -2,7 +2,7 @@ error: interning a defined symbol --> $DIR/interning_defined_symbol.rs:17:13 | LL | let _ = Symbol::intern("f32"); - | ^^^^^^^^^^^^^^^^^^^^^ help: try: `rustc_span::symbol::sym::f32` + | ^^^^^^^^^^^^^^^^^^^^^ help: try: `rustc_span::sym::f32` | note: the lint level is defined here --> $DIR/interning_defined_symbol.rs:2:9 @@ -15,13 +15,19 @@ error: interning a defined symbol --> $DIR/interning_defined_symbol.rs:20:13 | LL | let _ = sym!(f32); - | ^^^^^^^^^ help: try: `rustc_span::symbol::sym::f32` + | ^^^^^^^^^ help: try: `rustc_span::sym::f32` error: interning a defined symbol --> $DIR/interning_defined_symbol.rs:23:13 | LL | let _ = Symbol::intern("proc-macro"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `rustc_span::symbol::sym::proc_dash_macro` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `rustc_span::sym::proc_dash_macro` -error: aborting due to 3 previous errors +error: interning a defined symbol + --> $DIR/interning_defined_symbol.rs:26:13 + | +LL | let _ = Symbol::intern("self"); + | ^^^^^^^^^^^^^^^^^^^^^^ help: try: `rustc_span::symbol::kw::SelfLower` + +error: aborting due to 4 previous errors diff --git a/src/tools/clippy/tests/ui-internal/unnecessary_symbol_str.fixed b/src/tools/clippy/tests/ui-internal/unnecessary_symbol_str.fixed new file mode 100644 index 00000000000..2ec0efe4c10 --- /dev/null +++ b/src/tools/clippy/tests/ui-internal/unnecessary_symbol_str.fixed @@ -0,0 +1,16 @@ +// run-rustfix +#![feature(rustc_private)] +#![deny(clippy::internal)] +#![allow(clippy::unnecessary_operation, unused_must_use)] + +extern crate rustc_span; + +use rustc_span::symbol::{Ident, Symbol}; + +fn main() { + Symbol::intern("foo") == rustc_span::sym::clippy; + Symbol::intern("foo") == rustc_span::symbol::kw::SelfLower; + Symbol::intern("foo") != rustc_span::symbol::kw::SelfUpper; + Ident::invalid().name == rustc_span::sym::clippy; + rustc_span::sym::clippy == Ident::invalid().name; +} diff --git a/src/tools/clippy/tests/ui-internal/unnecessary_symbol_str.rs b/src/tools/clippy/tests/ui-internal/unnecessary_symbol_str.rs new file mode 100644 index 00000000000..87e1b3a2ee7 --- /dev/null +++ b/src/tools/clippy/tests/ui-internal/unnecessary_symbol_str.rs @@ -0,0 +1,16 @@ +// run-rustfix +#![feature(rustc_private)] +#![deny(clippy::internal)] +#![allow(clippy::unnecessary_operation, unused_must_use)] + +extern crate rustc_span; + +use rustc_span::symbol::{Ident, Symbol}; + +fn main() { + Symbol::intern("foo").as_str() == "clippy"; + Symbol::intern("foo").to_string() == "self"; + Symbol::intern("foo").to_ident_string() != "Self"; + &*Ident::invalid().as_str() == "clippy"; + "clippy" == Ident::invalid().to_string(); +} diff --git a/src/tools/clippy/tests/ui-internal/unnecessary_symbol_str.stderr b/src/tools/clippy/tests/ui-internal/unnecessary_symbol_str.stderr new file mode 100644 index 00000000000..b1284b7c8ff --- /dev/null +++ b/src/tools/clippy/tests/ui-internal/unnecessary_symbol_str.stderr @@ -0,0 +1,39 @@ +error: unnecessary `Symbol` to string conversion + --> $DIR/unnecessary_symbol_str.rs:11:5 + | +LL | Symbol::intern("foo").as_str() == "clippy"; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Symbol::intern("foo") == rustc_span::sym::clippy` + | +note: the lint level is defined here + --> $DIR/unnecessary_symbol_str.rs:3:9 + | +LL | #![deny(clippy::internal)] + | ^^^^^^^^^^^^^^^^ + = note: `#[deny(clippy::unnecessary_symbol_str)]` implied by `#[deny(clippy::internal)]` + +error: unnecessary `Symbol` to string conversion + --> $DIR/unnecessary_symbol_str.rs:12:5 + | +LL | Symbol::intern("foo").to_string() == "self"; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Symbol::intern("foo") == rustc_span::symbol::kw::SelfLower` + +error: unnecessary `Symbol` to string conversion + --> $DIR/unnecessary_symbol_str.rs:13:5 + | +LL | Symbol::intern("foo").to_ident_string() != "Self"; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Symbol::intern("foo") != rustc_span::symbol::kw::SelfUpper` + +error: unnecessary `Symbol` to string conversion + --> $DIR/unnecessary_symbol_str.rs:14:5 + | +LL | &*Ident::invalid().as_str() == "clippy"; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Ident::invalid().name == rustc_span::sym::clippy` + +error: unnecessary `Symbol` to string conversion + --> $DIR/unnecessary_symbol_str.rs:15:5 + | +LL | "clippy" == Ident::invalid().to_string(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `rustc_span::sym::clippy == Ident::invalid().name` + +error: aborting due to 5 previous errors + diff --git a/src/tools/clippy/tests/ui/auxiliary/macro_rules.rs b/src/tools/clippy/tests/ui/auxiliary/macro_rules.rs index 18324823468..d6ecd8568ce 100644 --- a/src/tools/clippy/tests/ui/auxiliary/macro_rules.rs +++ b/src/tools/clippy/tests/ui/auxiliary/macro_rules.rs @@ -94,3 +94,19 @@ macro_rules! large_enum_variant { } }; } + +#[macro_export] +macro_rules! field_reassign_with_default { + () => { + #[derive(Default)] + struct A { + pub i: i32, + pub j: i64, + } + fn lint() { + let mut a: A = Default::default(); + a.i = 42; + a; + } + }; +} diff --git a/src/tools/clippy/tests/ui/auxiliary/proc_macro_derive.rs b/src/tools/clippy/tests/ui/auxiliary/proc_macro_derive.rs index 7c4e4a14551..24891682d36 100644 --- a/src/tools/clippy/tests/ui/auxiliary/proc_macro_derive.rs +++ b/src/tools/clippy/tests/ui/auxiliary/proc_macro_derive.rs @@ -4,6 +4,7 @@ #![crate_type = "proc-macro"] #![feature(repr128, proc_macro_quote)] #![allow(incomplete_features)] +#![allow(clippy::field_reassign_with_default)] #![allow(clippy::eq_op)] extern crate proc_macro; @@ -23,3 +24,20 @@ pub fn derive(_: TokenStream) -> TokenStream { }; output } + +#[proc_macro_derive(FieldReassignWithDefault)] +pub fn derive_foo(_input: TokenStream) -> TokenStream { + quote! { + #[derive(Default)] + struct A { + pub i: i32, + pub j: i64, + } + #[automatically_derived] + fn lint() { + let mut a: A = Default::default(); + a.i = 42; + a; + } + } +} diff --git a/src/tools/clippy/tests/ui/cast_alignment.rs b/src/tools/clippy/tests/ui/cast_alignment.rs index 4c08935639f..d011e84b115 100644 --- a/src/tools/clippy/tests/ui/cast_alignment.rs +++ b/src/tools/clippy/tests/ui/cast_alignment.rs @@ -12,6 +12,10 @@ fn main() { (&1u8 as *const u8) as *const u16; (&mut 1u8 as *mut u8) as *mut u16; + // cast to more-strictly-aligned type, but with the `pointer::cast` function. + (&1u8 as *const u8).cast::<u16>(); + (&mut 1u8 as *mut u8).cast::<u16>(); + /* These should be ok */ // not a pointer type diff --git a/src/tools/clippy/tests/ui/cast_alignment.stderr b/src/tools/clippy/tests/ui/cast_alignment.stderr index 79219f86155..7998b787b91 100644 --- a/src/tools/clippy/tests/ui/cast_alignment.stderr +++ b/src/tools/clippy/tests/ui/cast_alignment.stderr @@ -12,5 +12,17 @@ error: casting from `*mut u8` to a more-strictly-aligned pointer (`*mut u16`) (1 LL | (&mut 1u8 as *mut u8) as *mut u16; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 2 previous errors +error: casting from `*const u8` to a more-strictly-aligned pointer (`*const u16`) (1 < 2 bytes) + --> $DIR/cast_alignment.rs:16:5 + | +LL | (&1u8 as *const u8).cast::<u16>(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: casting from `*mut u8` to a more-strictly-aligned pointer (`*mut u16`) (1 < 2 bytes) + --> $DIR/cast_alignment.rs:17:5 + | +LL | (&mut 1u8 as *mut u8).cast::<u16>(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 4 previous errors diff --git a/src/tools/clippy/tests/ui/clone_on_copy.fixed b/src/tools/clippy/tests/ui/clone_on_copy.fixed index 1f0ca101757..d924625132e 100644 --- a/src/tools/clippy/tests/ui/clone_on_copy.fixed +++ b/src/tools/clippy/tests/ui/clone_on_copy.fixed @@ -5,7 +5,8 @@ clippy::redundant_clone, clippy::deref_addrof, clippy::no_effect, - clippy::unnecessary_operation + clippy::unnecessary_operation, + clippy::vec_init_then_push )] use std::cell::RefCell; diff --git a/src/tools/clippy/tests/ui/clone_on_copy.rs b/src/tools/clippy/tests/ui/clone_on_copy.rs index ca39a654b4f..97f49467244 100644 --- a/src/tools/clippy/tests/ui/clone_on_copy.rs +++ b/src/tools/clippy/tests/ui/clone_on_copy.rs @@ -5,7 +5,8 @@ clippy::redundant_clone, clippy::deref_addrof, clippy::no_effect, - clippy::unnecessary_operation + clippy::unnecessary_operation, + clippy::vec_init_then_push )] use std::cell::RefCell; diff --git a/src/tools/clippy/tests/ui/clone_on_copy.stderr b/src/tools/clippy/tests/ui/clone_on_copy.stderr index 14a700886a7..7a706884fb0 100644 --- a/src/tools/clippy/tests/ui/clone_on_copy.stderr +++ b/src/tools/clippy/tests/ui/clone_on_copy.stderr @@ -1,5 +1,5 @@ error: using `clone` on type `i32` which implements the `Copy` trait - --> $DIR/clone_on_copy.rs:22:5 + --> $DIR/clone_on_copy.rs:23:5 | LL | 42.clone(); | ^^^^^^^^^^ help: try removing the `clone` call: `42` @@ -7,25 +7,25 @@ LL | 42.clone(); = note: `-D clippy::clone-on-copy` implied by `-D warnings` error: using `clone` on type `i32` which implements the `Copy` trait - --> $DIR/clone_on_copy.rs:26:5 + --> $DIR/clone_on_copy.rs:27:5 | LL | (&42).clone(); | ^^^^^^^^^^^^^ help: try dereferencing it: `*(&42)` error: using `clone` on type `i32` which implements the `Copy` trait - --> $DIR/clone_on_copy.rs:29:5 + --> $DIR/clone_on_copy.rs:30:5 | LL | rc.borrow().clone(); | ^^^^^^^^^^^^^^^^^^^ help: try dereferencing it: `*rc.borrow()` error: using `clone` on type `char` which implements the `Copy` trait - --> $DIR/clone_on_copy.rs:35:14 + --> $DIR/clone_on_copy.rs:36:14 | LL | is_ascii('z'.clone()); | ^^^^^^^^^^^ help: try removing the `clone` call: `'z'` error: using `clone` on type `i32` which implements the `Copy` trait - --> $DIR/clone_on_copy.rs:39:14 + --> $DIR/clone_on_copy.rs:40:14 | LL | vec.push(42.clone()); | ^^^^^^^^^^ help: try removing the `clone` call: `42` diff --git a/src/tools/clippy/tests/ui/collapsible_else_if.fixed b/src/tools/clippy/tests/ui/collapsible_else_if.fixed index ce2a1c28c8a..fa4bc30e933 100644 --- a/src/tools/clippy/tests/ui/collapsible_else_if.fixed +++ b/src/tools/clippy/tests/ui/collapsible_else_if.fixed @@ -3,6 +3,8 @@ #[rustfmt::skip] #[warn(clippy::collapsible_if)] +#[warn(clippy::collapsible_else_if)] + fn main() { let x = "hello"; let y = "world"; diff --git a/src/tools/clippy/tests/ui/collapsible_else_if.rs b/src/tools/clippy/tests/ui/collapsible_else_if.rs index 99c40b8d38e..bf6c1d1f894 100644 --- a/src/tools/clippy/tests/ui/collapsible_else_if.rs +++ b/src/tools/clippy/tests/ui/collapsible_else_if.rs @@ -3,6 +3,8 @@ #[rustfmt::skip] #[warn(clippy::collapsible_if)] +#[warn(clippy::collapsible_else_if)] + fn main() { let x = "hello"; let y = "world"; diff --git a/src/tools/clippy/tests/ui/collapsible_else_if.stderr b/src/tools/clippy/tests/ui/collapsible_else_if.stderr index 3d1c458879e..ee3e11ae565 100644 --- a/src/tools/clippy/tests/ui/collapsible_else_if.stderr +++ b/src/tools/clippy/tests/ui/collapsible_else_if.stderr @@ -1,5 +1,5 @@ error: this `else { if .. }` block can be collapsed - --> $DIR/collapsible_else_if.rs:12:12 + --> $DIR/collapsible_else_if.rs:14:12 | LL | } else { | ____________^ @@ -9,7 +9,7 @@ LL | | } LL | | } | |_____^ | - = note: `-D clippy::collapsible-if` implied by `-D warnings` + = note: `-D clippy::collapsible-else-if` implied by `-D warnings` help: collapse nested if block | LL | } else if y == "world" { @@ -18,7 +18,7 @@ LL | } | error: this `else { if .. }` block can be collapsed - --> $DIR/collapsible_else_if.rs:20:12 + --> $DIR/collapsible_else_if.rs:22:12 | LL | } else { | ____________^ @@ -36,7 +36,7 @@ LL | } | error: this `else { if .. }` block can be collapsed - --> $DIR/collapsible_else_if.rs:28:12 + --> $DIR/collapsible_else_if.rs:30:12 | LL | } else { | ____________^ @@ -59,7 +59,7 @@ LL | } | error: this `else { if .. }` block can be collapsed - --> $DIR/collapsible_else_if.rs:39:12 + --> $DIR/collapsible_else_if.rs:41:12 | LL | } else { | ____________^ @@ -82,7 +82,7 @@ LL | } | error: this `else { if .. }` block can be collapsed - --> $DIR/collapsible_else_if.rs:50:12 + --> $DIR/collapsible_else_if.rs:52:12 | LL | } else { | ____________^ @@ -105,7 +105,7 @@ LL | } | error: this `else { if .. }` block can be collapsed - --> $DIR/collapsible_else_if.rs:61:12 + --> $DIR/collapsible_else_if.rs:63:12 | LL | } else { | ____________^ @@ -128,7 +128,7 @@ LL | } | error: this `else { if .. }` block can be collapsed - --> $DIR/collapsible_else_if.rs:72:12 + --> $DIR/collapsible_else_if.rs:74:12 | LL | } else { | ____________^ diff --git a/src/tools/clippy/tests/ui/empty_enum.rs b/src/tools/clippy/tests/ui/empty_enum.rs index 12428f29625..a2e5c13c452 100644 --- a/src/tools/clippy/tests/ui/empty_enum.rs +++ b/src/tools/clippy/tests/ui/empty_enum.rs @@ -1,6 +1,7 @@ #![allow(dead_code)] #![warn(clippy::empty_enum)] - +// Enable never type to test empty enum lint +#![feature(never_type)] enum Empty {} fn main() {} diff --git a/src/tools/clippy/tests/ui/empty_enum.stderr b/src/tools/clippy/tests/ui/empty_enum.stderr index 466dfbe7cee..7125e5f602b 100644 --- a/src/tools/clippy/tests/ui/empty_enum.stderr +++ b/src/tools/clippy/tests/ui/empty_enum.stderr @@ -1,5 +1,5 @@ error: enum with no variants - --> $DIR/empty_enum.rs:4:1 + --> $DIR/empty_enum.rs:5:1 | LL | enum Empty {} | ^^^^^^^^^^^^^ diff --git a/src/tools/clippy/tests/ui/empty_enum_without_never_type.rs b/src/tools/clippy/tests/ui/empty_enum_without_never_type.rs new file mode 100644 index 00000000000..386677352e2 --- /dev/null +++ b/src/tools/clippy/tests/ui/empty_enum_without_never_type.rs @@ -0,0 +1,7 @@ +#![allow(dead_code)] +#![warn(clippy::empty_enum)] + +// `never_type` is not enabled; this test has no stderr file +enum Empty {} + +fn main() {} diff --git a/src/tools/clippy/tests/ui/escape_analysis.rs b/src/tools/clippy/tests/ui/escape_analysis.rs index 07004489610..d26f48fc68f 100644 --- a/src/tools/clippy/tests/ui/escape_analysis.rs +++ b/src/tools/clippy/tests/ui/escape_analysis.rs @@ -182,3 +182,23 @@ pub extern "C" fn do_not_warn_me(_c_pointer: Box<String>) -> () {} #[rustfmt::skip] // Forces rustfmt to not add ABI pub extern fn do_not_warn_me_no_abi(_c_pointer: Box<String>) -> () {} + +// Issue #4804 - default implementation in trait +mod issue4804 { + trait DefaultTraitImplTest { + // don't warn on `self` + fn default_impl(self: Box<Self>) -> u32 { + 5 + } + + // warn on `x: Box<u32>` + fn default_impl_x(self: Box<Self>, x: Box<u32>) -> u32 { + 4 + } + } + + trait WarnTrait { + // warn on `x: Box<u32>` + fn foo(x: Box<u32>) {} + } +} diff --git a/src/tools/clippy/tests/ui/escape_analysis.stderr b/src/tools/clippy/tests/ui/escape_analysis.stderr index c86a769a3da..4a82b4419f9 100644 --- a/src/tools/clippy/tests/ui/escape_analysis.stderr +++ b/src/tools/clippy/tests/ui/escape_analysis.stderr @@ -12,5 +12,17 @@ error: local variable doesn't need to be boxed here LL | pub fn new(_needs_name: Box<PeekableSeekable<&()>>) -> () {} | ^^^^^^^^^^^ -error: aborting due to 2 previous errors +error: local variable doesn't need to be boxed here + --> $DIR/escape_analysis.rs:195:44 + | +LL | fn default_impl_x(self: Box<Self>, x: Box<u32>) -> u32 { + | ^ + +error: local variable doesn't need to be boxed here + --> $DIR/escape_analysis.rs:202:16 + | +LL | fn foo(x: Box<u32>) {} + | ^ + +error: aborting due to 4 previous errors diff --git a/src/tools/clippy/tests/ui/field_reassign_with_default.rs b/src/tools/clippy/tests/ui/field_reassign_with_default.rs index 3e0921022b4..9fc208f5332 100644 --- a/src/tools/clippy/tests/ui/field_reassign_with_default.rs +++ b/src/tools/clippy/tests/ui/field_reassign_with_default.rs @@ -1,5 +1,18 @@ +// aux-build:proc_macro_derive.rs +// aux-build:macro_rules.rs + #![warn(clippy::field_reassign_with_default)] +#[macro_use] +extern crate proc_macro_derive; +#[macro_use] +extern crate macro_rules; + +// Don't lint on derives that derive `Default` +// See https://github.com/rust-lang/rust-clippy/issues/6545 +#[derive(FieldReassignWithDefault)] +struct DerivedStruct; + #[derive(Default)] struct A { i: i32, @@ -11,6 +24,11 @@ struct B { j: i64, } +#[derive(Default)] +struct C { + i: Vec<i32>, + j: i64, +} /// Implements .next() that returns a different number each time. struct SideEffect(i32); @@ -111,6 +129,13 @@ fn main() { // don't lint - some private fields let mut x = m::F::default(); x.a = 1; + + // don't expand macros in the suggestion (#6522) + let mut a: C = C::default(); + a.i = vec![1]; + + // Don't lint in external macros + field_reassign_with_default!(); } mod m { diff --git a/src/tools/clippy/tests/ui/field_reassign_with_default.stderr b/src/tools/clippy/tests/ui/field_reassign_with_default.stderr index 9a2bc778c3f..2f0f28f7bb7 100644 --- a/src/tools/clippy/tests/ui/field_reassign_with_default.stderr +++ b/src/tools/clippy/tests/ui/field_reassign_with_default.stderr @@ -1,75 +1,87 @@ error: field assignment outside of initializer for an instance created with Default::default() - --> $DIR/field_reassign_with_default.rs:30:5 + --> $DIR/field_reassign_with_default.rs:48:5 | LL | a.i = 42; | ^^^^^^^^^ | = note: `-D clippy::field-reassign-with-default` implied by `-D warnings` -note: consider initializing the variable with `A { i: 42, ..Default::default() }` and removing relevant reassignments - --> $DIR/field_reassign_with_default.rs:29:5 +note: consider initializing the variable with `main::A { i: 42, ..Default::default() }` and removing relevant reassignments + --> $DIR/field_reassign_with_default.rs:47:5 | LL | let mut a: A = Default::default(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: field assignment outside of initializer for an instance created with Default::default() - --> $DIR/field_reassign_with_default.rs:70:5 + --> $DIR/field_reassign_with_default.rs:88:5 | LL | a.j = 43; | ^^^^^^^^^ | -note: consider initializing the variable with `A { j: 43, i: 42 }` and removing relevant reassignments - --> $DIR/field_reassign_with_default.rs:69:5 +note: consider initializing the variable with `main::A { j: 43, i: 42 }` and removing relevant reassignments + --> $DIR/field_reassign_with_default.rs:87:5 | LL | let mut a: A = Default::default(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: field assignment outside of initializer for an instance created with Default::default() - --> $DIR/field_reassign_with_default.rs:75:5 + --> $DIR/field_reassign_with_default.rs:93:5 | LL | a.i = 42; | ^^^^^^^^^ | -note: consider initializing the variable with `A { i: 42, j: 44 }` and removing relevant reassignments - --> $DIR/field_reassign_with_default.rs:74:5 +note: consider initializing the variable with `main::A { i: 42, j: 44 }` and removing relevant reassignments + --> $DIR/field_reassign_with_default.rs:92:5 | LL | let mut a: A = Default::default(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: field assignment outside of initializer for an instance created with Default::default() - --> $DIR/field_reassign_with_default.rs:81:5 + --> $DIR/field_reassign_with_default.rs:99:5 | LL | a.i = 42; | ^^^^^^^^^ | -note: consider initializing the variable with `A { i: 42, ..Default::default() }` and removing relevant reassignments - --> $DIR/field_reassign_with_default.rs:80:5 +note: consider initializing the variable with `main::A { i: 42, ..Default::default() }` and removing relevant reassignments + --> $DIR/field_reassign_with_default.rs:98:5 | LL | let mut a = A::default(); | ^^^^^^^^^^^^^^^^^^^^^^^^^ error: field assignment outside of initializer for an instance created with Default::default() - --> $DIR/field_reassign_with_default.rs:91:5 + --> $DIR/field_reassign_with_default.rs:109:5 | LL | a.i = Default::default(); | ^^^^^^^^^^^^^^^^^^^^^^^^^ | -note: consider initializing the variable with `A { i: Default::default(), ..Default::default() }` and removing relevant reassignments - --> $DIR/field_reassign_with_default.rs:90:5 +note: consider initializing the variable with `main::A { i: Default::default(), ..Default::default() }` and removing relevant reassignments + --> $DIR/field_reassign_with_default.rs:108:5 | LL | let mut a: A = Default::default(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: field assignment outside of initializer for an instance created with Default::default() - --> $DIR/field_reassign_with_default.rs:95:5 + --> $DIR/field_reassign_with_default.rs:113:5 | LL | a.i = Default::default(); | ^^^^^^^^^^^^^^^^^^^^^^^^^ | -note: consider initializing the variable with `A { i: Default::default(), j: 45 }` and removing relevant reassignments - --> $DIR/field_reassign_with_default.rs:94:5 +note: consider initializing the variable with `main::A { i: Default::default(), j: 45 }` and removing relevant reassignments + --> $DIR/field_reassign_with_default.rs:112:5 | LL | let mut a: A = Default::default(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 6 previous errors +error: field assignment outside of initializer for an instance created with Default::default() + --> $DIR/field_reassign_with_default.rs:135:5 + | +LL | a.i = vec![1]; + | ^^^^^^^^^^^^^^ + | +note: consider initializing the variable with `C { i: vec![1], ..Default::default() }` and removing relevant reassignments + --> $DIR/field_reassign_with_default.rs:134:5 + | +LL | let mut a: C = C::default(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 7 previous errors diff --git a/src/tools/clippy/tests/ui/from_over_into.stderr b/src/tools/clippy/tests/ui/from_over_into.stderr index 18f56f85432..b101d2704fb 100644 --- a/src/tools/clippy/tests/ui/from_over_into.stderr +++ b/src/tools/clippy/tests/ui/from_over_into.stderr @@ -1,12 +1,8 @@ error: an implementation of `From` is preferred since it gives you `Into<_>` for free where the reverse isn't true --> $DIR/from_over_into.rs:6:1 | -LL | / impl Into<StringWrapper> for String { -LL | | fn into(self) -> StringWrapper { -LL | | StringWrapper(self) -LL | | } -LL | | } - | |_^ +LL | impl Into<StringWrapper> for String { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::from-over-into` implied by `-D warnings` = help: consider to implement `From` instead diff --git a/src/tools/clippy/tests/ui/if_same_then_else2.rs b/src/tools/clippy/tests/ui/if_same_then_else2.rs index 8d54f75b5d1..e83ce47e563 100644 --- a/src/tools/clippy/tests/ui/if_same_then_else2.rs +++ b/src/tools/clippy/tests/ui/if_same_then_else2.rs @@ -1,6 +1,7 @@ #![warn(clippy::if_same_then_else)] #![allow( clippy::blacklisted_name, + clippy::collapsible_else_if, clippy::collapsible_if, clippy::ifs_same_cond, clippy::needless_return, diff --git a/src/tools/clippy/tests/ui/if_same_then_else2.stderr b/src/tools/clippy/tests/ui/if_same_then_else2.stderr index da2be6c8aa5..f98e30fa376 100644 --- a/src/tools/clippy/tests/ui/if_same_then_else2.stderr +++ b/src/tools/clippy/tests/ui/if_same_then_else2.stderr @@ -1,5 +1,5 @@ error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:20:12 + --> $DIR/if_same_then_else2.rs:21:12 | LL | } else { | ____________^ @@ -13,7 +13,7 @@ LL | | } | = note: `-D clippy::if-same-then-else` implied by `-D warnings` note: same as this - --> $DIR/if_same_then_else2.rs:11:13 + --> $DIR/if_same_then_else2.rs:12:13 | LL | if true { | _____________^ @@ -26,7 +26,7 @@ LL | | } else { | |_____^ error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:34:12 + --> $DIR/if_same_then_else2.rs:35:12 | LL | } else { | ____________^ @@ -36,7 +36,7 @@ LL | | } | |_____^ | note: same as this - --> $DIR/if_same_then_else2.rs:32:13 + --> $DIR/if_same_then_else2.rs:33:13 | LL | if true { | _____________^ @@ -45,7 +45,7 @@ LL | | } else { | |_____^ error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:41:12 + --> $DIR/if_same_then_else2.rs:42:12 | LL | } else { | ____________^ @@ -55,7 +55,7 @@ LL | | } | |_____^ | note: same as this - --> $DIR/if_same_then_else2.rs:39:13 + --> $DIR/if_same_then_else2.rs:40:13 | LL | if true { | _____________^ @@ -64,7 +64,7 @@ LL | | } else { | |_____^ error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:91:12 + --> $DIR/if_same_then_else2.rs:92:12 | LL | } else { | ____________^ @@ -74,7 +74,7 @@ LL | | }; | |_____^ | note: same as this - --> $DIR/if_same_then_else2.rs:89:21 + --> $DIR/if_same_then_else2.rs:90:21 | LL | let _ = if true { | _____________________^ @@ -83,7 +83,7 @@ LL | | } else { | |_____^ error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:98:12 + --> $DIR/if_same_then_else2.rs:99:12 | LL | } else { | ____________^ @@ -93,7 +93,7 @@ LL | | } | |_____^ | note: same as this - --> $DIR/if_same_then_else2.rs:96:13 + --> $DIR/if_same_then_else2.rs:97:13 | LL | if true { | _____________^ @@ -102,7 +102,7 @@ LL | | } else { | |_____^ error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:123:12 + --> $DIR/if_same_then_else2.rs:124:12 | LL | } else { | ____________^ @@ -112,7 +112,7 @@ LL | | } | |_____^ | note: same as this - --> $DIR/if_same_then_else2.rs:120:20 + --> $DIR/if_same_then_else2.rs:121:20 | LL | } else if true { | ____________________^ diff --git a/src/tools/clippy/tests/ui/needless_question_mark.fixed b/src/tools/clippy/tests/ui/needless_question_mark.fixed new file mode 100644 index 00000000000..70218f3f041 --- /dev/null +++ b/src/tools/clippy/tests/ui/needless_question_mark.fixed @@ -0,0 +1,163 @@ +// run-rustfix + +#![warn(clippy::needless_question_mark)] +#![allow(clippy::needless_return, clippy::unnecessary_unwrap, dead_code, unused_must_use)] +#![feature(custom_inner_attributes)] + +struct TO { + magic: Option<usize>, +} + +struct TR { + magic: Result<usize, bool>, +} + +fn simple_option_bad1(to: TO) -> Option<usize> { + // return as a statement + return to.magic; +} + +// formatting will add a semi-colon, which would make +// this identical to the test case above +#[rustfmt::skip] +fn simple_option_bad2(to: TO) -> Option<usize> { + // return as an expression + return to.magic +} + +fn simple_option_bad3(to: TO) -> Option<usize> { + // block value "return" + to.magic +} + +fn simple_option_bad4(to: Option<TO>) -> Option<usize> { + // single line closure + to.and_then(|t| t.magic) +} + +// formatting this will remove the block brackets, making +// this test identical to the one above +#[rustfmt::skip] +fn simple_option_bad5(to: Option<TO>) -> Option<usize> { + // closure with body + to.and_then(|t| { + t.magic + }) +} + +fn simple_result_bad1(tr: TR) -> Result<usize, bool> { + return tr.magic; +} + +// formatting will add a semi-colon, which would make +// this identical to the test case above +#[rustfmt::skip] +fn simple_result_bad2(tr: TR) -> Result<usize, bool> { + return tr.magic +} + +fn simple_result_bad3(tr: TR) -> Result<usize, bool> { + tr.magic +} + +fn simple_result_bad4(tr: Result<TR, bool>) -> Result<usize, bool> { + tr.and_then(|t| t.magic) +} + +// formatting this will remove the block brackets, making +// this test identical to the one above +#[rustfmt::skip] +fn simple_result_bad5(tr: Result<TR, bool>) -> Result<usize, bool> { + tr.and_then(|t| { + t.magic + }) +} + +fn also_bad(tr: Result<TR, bool>) -> Result<usize, bool> { + if tr.is_ok() { + let t = tr.unwrap(); + return t.magic; + } + Err(false) +} + +fn false_positive_test<U, T>(x: Result<(), U>) -> Result<(), T> +where + T: From<U>, +{ + Ok(x?) +} + +fn main() {} + +mod question_mark_none { + #![clippy::msrv = "1.12.0"] + fn needless_question_mark_option() -> Option<usize> { + struct TO { + magic: Option<usize>, + } + let to = TO { magic: None }; + Some(to.magic?) // should not be triggered + } + + fn needless_question_mark_result() -> Result<usize, bool> { + struct TO { + magic: Result<usize, bool>, + } + let to = TO { magic: Ok(1_usize) }; + Ok(to.magic?) // should not be triggered + } + + fn main() { + needless_question_mark_option(); + needless_question_mark_result(); + } +} + +mod question_mark_result { + #![clippy::msrv = "1.21.0"] + fn needless_question_mark_option() -> Option<usize> { + struct TO { + magic: Option<usize>, + } + let to = TO { magic: None }; + Some(to.magic?) // should not be triggered + } + + fn needless_question_mark_result() -> Result<usize, bool> { + struct TO { + magic: Result<usize, bool>, + } + let to = TO { magic: Ok(1_usize) }; + to.magic // should be triggered + } + + fn main() { + needless_question_mark_option(); + needless_question_mark_result(); + } +} + +mod question_mark_both { + #![clippy::msrv = "1.22.0"] + fn needless_question_mark_option() -> Option<usize> { + struct TO { + magic: Option<usize>, + } + let to = TO { magic: None }; + to.magic // should be triggered + } + + fn needless_question_mark_result() -> Result<usize, bool> { + struct TO { + magic: Result<usize, bool>, + } + let to = TO { magic: Ok(1_usize) }; + to.magic // should be triggered + } + + fn main() { + needless_question_mark_option(); + needless_question_mark_result(); + } +} diff --git a/src/tools/clippy/tests/ui/needless_question_mark.rs b/src/tools/clippy/tests/ui/needless_question_mark.rs new file mode 100644 index 00000000000..60ac2c8d72e --- /dev/null +++ b/src/tools/clippy/tests/ui/needless_question_mark.rs @@ -0,0 +1,163 @@ +// run-rustfix + +#![warn(clippy::needless_question_mark)] +#![allow(clippy::needless_return, clippy::unnecessary_unwrap, dead_code, unused_must_use)] +#![feature(custom_inner_attributes)] + +struct TO { + magic: Option<usize>, +} + +struct TR { + magic: Result<usize, bool>, +} + +fn simple_option_bad1(to: TO) -> Option<usize> { + // return as a statement + return Some(to.magic?); +} + +// formatting will add a semi-colon, which would make +// this identical to the test case above +#[rustfmt::skip] +fn simple_option_bad2(to: TO) -> Option<usize> { + // return as an expression + return Some(to.magic?) +} + +fn simple_option_bad3(to: TO) -> Option<usize> { + // block value "return" + Some(to.magic?) +} + +fn simple_option_bad4(to: Option<TO>) -> Option<usize> { + // single line closure + to.and_then(|t| Some(t.magic?)) +} + +// formatting this will remove the block brackets, making +// this test identical to the one above +#[rustfmt::skip] +fn simple_option_bad5(to: Option<TO>) -> Option<usize> { + // closure with body + to.and_then(|t| { + Some(t.magic?) + }) +} + +fn simple_result_bad1(tr: TR) -> Result<usize, bool> { + return Ok(tr.magic?); +} + +// formatting will add a semi-colon, which would make +// this identical to the test case above +#[rustfmt::skip] +fn simple_result_bad2(tr: TR) -> Result<usize, bool> { + return Ok(tr.magic?) +} + +fn simple_result_bad3(tr: TR) -> Result<usize, bool> { + Ok(tr.magic?) +} + +fn simple_result_bad4(tr: Result<TR, bool>) -> Result<usize, bool> { + tr.and_then(|t| Ok(t.magic?)) +} + +// formatting this will remove the block brackets, making +// this test identical to the one above +#[rustfmt::skip] +fn simple_result_bad5(tr: Result<TR, bool>) -> Result<usize, bool> { + tr.and_then(|t| { + Ok(t.magic?) + }) +} + +fn also_bad(tr: Result<TR, bool>) -> Result<usize, bool> { + if tr.is_ok() { + let t = tr.unwrap(); + return Ok(t.magic?); + } + Err(false) +} + +fn false_positive_test<U, T>(x: Result<(), U>) -> Result<(), T> +where + T: From<U>, +{ + Ok(x?) +} + +fn main() {} + +mod question_mark_none { + #![clippy::msrv = "1.12.0"] + fn needless_question_mark_option() -> Option<usize> { + struct TO { + magic: Option<usize>, + } + let to = TO { magic: None }; + Some(to.magic?) // should not be triggered + } + + fn needless_question_mark_result() -> Result<usize, bool> { + struct TO { + magic: Result<usize, bool>, + } + let to = TO { magic: Ok(1_usize) }; + Ok(to.magic?) // should not be triggered + } + + fn main() { + needless_question_mark_option(); + needless_question_mark_result(); + } +} + +mod question_mark_result { + #![clippy::msrv = "1.21.0"] + fn needless_question_mark_option() -> Option<usize> { + struct TO { + magic: Option<usize>, + } + let to = TO { magic: None }; + Some(to.magic?) // should not be triggered + } + + fn needless_question_mark_result() -> Result<usize, bool> { + struct TO { + magic: Result<usize, bool>, + } + let to = TO { magic: Ok(1_usize) }; + Ok(to.magic?) // should be triggered + } + + fn main() { + needless_question_mark_option(); + needless_question_mark_result(); + } +} + +mod question_mark_both { + #![clippy::msrv = "1.22.0"] + fn needless_question_mark_option() -> Option<usize> { + struct TO { + magic: Option<usize>, + } + let to = TO { magic: None }; + Some(to.magic?) // should be triggered + } + + fn needless_question_mark_result() -> Result<usize, bool> { + struct TO { + magic: Result<usize, bool>, + } + let to = TO { magic: Ok(1_usize) }; + Ok(to.magic?) // should be triggered + } + + fn main() { + needless_question_mark_option(); + needless_question_mark_result(); + } +} diff --git a/src/tools/clippy/tests/ui/needless_question_mark.stderr b/src/tools/clippy/tests/ui/needless_question_mark.stderr new file mode 100644 index 00000000000..b4eb21882ec --- /dev/null +++ b/src/tools/clippy/tests/ui/needless_question_mark.stderr @@ -0,0 +1,88 @@ +error: Question mark operator is useless here + --> $DIR/needless_question_mark.rs:17:12 + | +LL | return Some(to.magic?); + | ^^^^^^^^^^^^^^^ help: try: `to.magic` + | + = note: `-D clippy::needless-question-mark` implied by `-D warnings` + +error: Question mark operator is useless here + --> $DIR/needless_question_mark.rs:25:12 + | +LL | return Some(to.magic?) + | ^^^^^^^^^^^^^^^ help: try: `to.magic` + +error: Question mark operator is useless here + --> $DIR/needless_question_mark.rs:30:5 + | +LL | Some(to.magic?) + | ^^^^^^^^^^^^^^^ help: try: `to.magic` + +error: Question mark operator is useless here + --> $DIR/needless_question_mark.rs:35:21 + | +LL | to.and_then(|t| Some(t.magic?)) + | ^^^^^^^^^^^^^^ help: try: `t.magic` + +error: Question mark operator is useless here + --> $DIR/needless_question_mark.rs:44:9 + | +LL | Some(t.magic?) + | ^^^^^^^^^^^^^^ help: try: `t.magic` + +error: Question mark operator is useless here + --> $DIR/needless_question_mark.rs:49:12 + | +LL | return Ok(tr.magic?); + | ^^^^^^^^^^^^^ help: try: `tr.magic` + +error: Question mark operator is useless here + --> $DIR/needless_question_mark.rs:56:12 + | +LL | return Ok(tr.magic?) + | ^^^^^^^^^^^^^ help: try: `tr.magic` + +error: Question mark operator is useless here + --> $DIR/needless_question_mark.rs:60:5 + | +LL | Ok(tr.magic?) + | ^^^^^^^^^^^^^ help: try: `tr.magic` + +error: Question mark operator is useless here + --> $DIR/needless_question_mark.rs:64:21 + | +LL | tr.and_then(|t| Ok(t.magic?)) + | ^^^^^^^^^^^^ help: try: `t.magic` + +error: Question mark operator is useless here + --> $DIR/needless_question_mark.rs:72:9 + | +LL | Ok(t.magic?) + | ^^^^^^^^^^^^ help: try: `t.magic` + +error: Question mark operator is useless here + --> $DIR/needless_question_mark.rs:79:16 + | +LL | return Ok(t.magic?); + | ^^^^^^^^^^^^ help: try: `t.magic` + +error: Question mark operator is useless here + --> $DIR/needless_question_mark.rs:132:9 + | +LL | Ok(to.magic?) // should be triggered + | ^^^^^^^^^^^^^ help: try: `to.magic` + +error: Question mark operator is useless here + --> $DIR/needless_question_mark.rs:148:9 + | +LL | Some(to.magic?) // should be triggered + | ^^^^^^^^^^^^^^^ help: try: `to.magic` + +error: Question mark operator is useless here + --> $DIR/needless_question_mark.rs:156:9 + | +LL | Ok(to.magic?) // should be triggered + | ^^^^^^^^^^^^^ help: try: `to.magic` + +error: aborting due to 14 previous errors + diff --git a/src/tools/clippy/tests/ui/needless_return.fixed b/src/tools/clippy/tests/ui/needless_return.fixed index d849e093da7..86bfc5b4bb2 100644 --- a/src/tools/clippy/tests/ui/needless_return.fixed +++ b/src/tools/clippy/tests/ui/needless_return.fixed @@ -86,6 +86,21 @@ fn borrows_but_not_last(value: bool) -> String { } } +macro_rules! needed_return { + ($e:expr) => { + if $e > 3 { + return; + } + }; +} + +fn test_return_in_macro() { + // This will return and the macro below won't be executed. Removing the `return` from the macro + // will change semantics. + needed_return!(10); + needed_return!(0); +} + fn main() { let _ = test_end_of_fn(); let _ = test_no_semicolon(); diff --git a/src/tools/clippy/tests/ui/needless_return.rs b/src/tools/clippy/tests/ui/needless_return.rs index 29f2bd1852a..51061370dfe 100644 --- a/src/tools/clippy/tests/ui/needless_return.rs +++ b/src/tools/clippy/tests/ui/needless_return.rs @@ -86,6 +86,21 @@ fn borrows_but_not_last(value: bool) -> String { } } +macro_rules! needed_return { + ($e:expr) => { + if $e > 3 { + return; + } + }; +} + +fn test_return_in_macro() { + // This will return and the macro below won't be executed. Removing the `return` from the macro + // will change semantics. + needed_return!(10); + needed_return!(0); +} + fn main() { let _ = test_end_of_fn(); let _ = test_no_semicolon(); diff --git a/src/tools/clippy/tests/ui/ptr_as_ptr.fixed b/src/tools/clippy/tests/ui/ptr_as_ptr.fixed new file mode 100644 index 00000000000..8346a9454f4 --- /dev/null +++ b/src/tools/clippy/tests/ui/ptr_as_ptr.fixed @@ -0,0 +1,50 @@ +// run-rustfix + +#![warn(clippy::ptr_as_ptr)] +#![feature(custom_inner_attributes)] + +fn main() { + let ptr: *const u32 = &42_u32; + let mut_ptr: *mut u32 = &mut 42_u32; + + let _ = ptr.cast::<i32>(); + let _ = mut_ptr.cast::<i32>(); + + // Make sure the lint can handle the difference in their operator precedences. + unsafe { + let ptr_ptr: *const *const u32 = &ptr; + let _ = (*ptr_ptr).cast::<i32>(); + } + + // Changes in mutability. Do not lint this. + let _ = ptr as *mut i32; + let _ = mut_ptr as *const i32; + + // `pointer::cast` cannot perform unsized coercions unlike `as`. Do not lint this. + let ptr_of_array: *const [u32; 4] = &[1, 2, 3, 4]; + let _ = ptr_of_array as *const [u32]; + let _ = ptr_of_array as *const dyn std::fmt::Debug; + + // Ensure the lint doesn't produce unnecessary turbofish for inferred types. + let _: *const i32 = ptr.cast(); + let _: *mut i32 = mut_ptr.cast(); +} + +fn _msrv_1_37() { + #![clippy::msrv = "1.37"] + let ptr: *const u32 = &42_u32; + let mut_ptr: *mut u32 = &mut 42_u32; + + // `pointer::cast` was stabilized in 1.38. Do not lint this + let _ = ptr as *const i32; + let _ = mut_ptr as *mut i32; +} + +fn _msrv_1_38() { + #![clippy::msrv = "1.38"] + let ptr: *const u32 = &42_u32; + let mut_ptr: *mut u32 = &mut 42_u32; + + let _ = ptr.cast::<i32>(); + let _ = mut_ptr.cast::<i32>(); +} diff --git a/src/tools/clippy/tests/ui/ptr_as_ptr.rs b/src/tools/clippy/tests/ui/ptr_as_ptr.rs new file mode 100644 index 00000000000..b68d4bc0aac --- /dev/null +++ b/src/tools/clippy/tests/ui/ptr_as_ptr.rs @@ -0,0 +1,50 @@ +// run-rustfix + +#![warn(clippy::ptr_as_ptr)] +#![feature(custom_inner_attributes)] + +fn main() { + let ptr: *const u32 = &42_u32; + let mut_ptr: *mut u32 = &mut 42_u32; + + let _ = ptr as *const i32; + let _ = mut_ptr as *mut i32; + + // Make sure the lint can handle the difference in their operator precedences. + unsafe { + let ptr_ptr: *const *const u32 = &ptr; + let _ = *ptr_ptr as *const i32; + } + + // Changes in mutability. Do not lint this. + let _ = ptr as *mut i32; + let _ = mut_ptr as *const i32; + + // `pointer::cast` cannot perform unsized coercions unlike `as`. Do not lint this. + let ptr_of_array: *const [u32; 4] = &[1, 2, 3, 4]; + let _ = ptr_of_array as *const [u32]; + let _ = ptr_of_array as *const dyn std::fmt::Debug; + + // Ensure the lint doesn't produce unnecessary turbofish for inferred types. + let _: *const i32 = ptr as *const _; + let _: *mut i32 = mut_ptr as _; +} + +fn _msrv_1_37() { + #![clippy::msrv = "1.37"] + let ptr: *const u32 = &42_u32; + let mut_ptr: *mut u32 = &mut 42_u32; + + // `pointer::cast` was stabilized in 1.38. Do not lint this + let _ = ptr as *const i32; + let _ = mut_ptr as *mut i32; +} + +fn _msrv_1_38() { + #![clippy::msrv = "1.38"] + let ptr: *const u32 = &42_u32; + let mut_ptr: *mut u32 = &mut 42_u32; + + let _ = ptr as *const i32; + let _ = mut_ptr as *mut i32; +} diff --git a/src/tools/clippy/tests/ui/ptr_as_ptr.stderr b/src/tools/clippy/tests/ui/ptr_as_ptr.stderr new file mode 100644 index 00000000000..854906dc111 --- /dev/null +++ b/src/tools/clippy/tests/ui/ptr_as_ptr.stderr @@ -0,0 +1,46 @@ +error: `as` casting between raw pointers without changing its mutability + --> $DIR/ptr_as_ptr.rs:10:13 + | +LL | let _ = ptr as *const i32; + | ^^^^^^^^^^^^^^^^^ help: try `pointer::cast`, a safer alternative: `ptr.cast::<i32>()` + | + = note: `-D clippy::ptr-as-ptr` implied by `-D warnings` + +error: `as` casting between raw pointers without changing its mutability + --> $DIR/ptr_as_ptr.rs:11:13 + | +LL | let _ = mut_ptr as *mut i32; + | ^^^^^^^^^^^^^^^^^^^ help: try `pointer::cast`, a safer alternative: `mut_ptr.cast::<i32>()` + +error: `as` casting between raw pointers without changing its mutability + --> $DIR/ptr_as_ptr.rs:16:17 + | +LL | let _ = *ptr_ptr as *const i32; + | ^^^^^^^^^^^^^^^^^^^^^^ help: try `pointer::cast`, a safer alternative: `(*ptr_ptr).cast::<i32>()` + +error: `as` casting between raw pointers without changing its mutability + --> $DIR/ptr_as_ptr.rs:29:25 + | +LL | let _: *const i32 = ptr as *const _; + | ^^^^^^^^^^^^^^^ help: try `pointer::cast`, a safer alternative: `ptr.cast()` + +error: `as` casting between raw pointers without changing its mutability + --> $DIR/ptr_as_ptr.rs:30:23 + | +LL | let _: *mut i32 = mut_ptr as _; + | ^^^^^^^^^^^^ help: try `pointer::cast`, a safer alternative: `mut_ptr.cast()` + +error: `as` casting between raw pointers without changing its mutability + --> $DIR/ptr_as_ptr.rs:48:13 + | +LL | let _ = ptr as *const i32; + | ^^^^^^^^^^^^^^^^^ help: try `pointer::cast`, a safer alternative: `ptr.cast::<i32>()` + +error: `as` casting between raw pointers without changing its mutability + --> $DIR/ptr_as_ptr.rs:49:13 + | +LL | let _ = mut_ptr as *mut i32; + | ^^^^^^^^^^^^^^^^^^^ help: try `pointer::cast`, a safer alternative: `mut_ptr.cast::<i32>()` + +error: aborting due to 7 previous errors + diff --git a/src/tools/clippy/tests/ui/try_err.fixed b/src/tools/clippy/tests/ui/try_err.fixed index 652b611208b..5b96bb59c5f 100644 --- a/src/tools/clippy/tests/ui/try_err.fixed +++ b/src/tools/clippy/tests/ui/try_err.fixed @@ -2,7 +2,7 @@ // aux-build:macro_rules.rs #![deny(clippy::try_err)] -#![allow(clippy::unnecessary_wraps)] +#![allow(clippy::unnecessary_wraps, clippy::needless_question_mark)] #[macro_use] extern crate macro_rules; diff --git a/src/tools/clippy/tests/ui/try_err.rs b/src/tools/clippy/tests/ui/try_err.rs index 6bd479657b7..f220d697d2c 100644 --- a/src/tools/clippy/tests/ui/try_err.rs +++ b/src/tools/clippy/tests/ui/try_err.rs @@ -2,7 +2,7 @@ // aux-build:macro_rules.rs #![deny(clippy::try_err)] -#![allow(clippy::unnecessary_wraps)] +#![allow(clippy::unnecessary_wraps, clippy::needless_question_mark)] #[macro_use] extern crate macro_rules; diff --git a/src/tools/clippy/tests/ui/unit_arg.rs b/src/tools/clippy/tests/ui/unit_arg.rs index 9ad16d36509..b6a7bc5a1cc 100644 --- a/src/tools/clippy/tests/ui/unit_arg.rs +++ b/src/tools/clippy/tests/ui/unit_arg.rs @@ -5,7 +5,8 @@ unused_variables, clippy::unused_unit, clippy::unnecessary_wraps, - clippy::or_fun_call + clippy::or_fun_call, + clippy::needless_question_mark )] use std::fmt::Debug; diff --git a/src/tools/clippy/tests/ui/unit_arg.stderr b/src/tools/clippy/tests/ui/unit_arg.stderr index c3a839a9bf8..094cff8c985 100644 --- a/src/tools/clippy/tests/ui/unit_arg.stderr +++ b/src/tools/clippy/tests/ui/unit_arg.stderr @@ -1,5 +1,5 @@ error: passing a unit value to a function - --> $DIR/unit_arg.rs:30:5 + --> $DIR/unit_arg.rs:31:5 | LL | / foo({ LL | | 1; @@ -20,7 +20,7 @@ LL | foo(()); | error: passing a unit value to a function - --> $DIR/unit_arg.rs:33:5 + --> $DIR/unit_arg.rs:34:5 | LL | foo(foo(1)); | ^^^^^^^^^^^ @@ -32,7 +32,7 @@ LL | foo(()); | error: passing a unit value to a function - --> $DIR/unit_arg.rs:34:5 + --> $DIR/unit_arg.rs:35:5 | LL | / foo({ LL | | foo(1); @@ -54,7 +54,7 @@ LL | foo(()); | error: passing a unit value to a function - --> $DIR/unit_arg.rs:39:5 + --> $DIR/unit_arg.rs:40:5 | LL | / b.bar({ LL | | 1; @@ -74,7 +74,7 @@ LL | b.bar(()); | error: passing unit values to a function - --> $DIR/unit_arg.rs:42:5 + --> $DIR/unit_arg.rs:43:5 | LL | taking_multiple_units(foo(0), foo(1)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -87,7 +87,7 @@ LL | taking_multiple_units((), ()); | error: passing unit values to a function - --> $DIR/unit_arg.rs:43:5 + --> $DIR/unit_arg.rs:44:5 | LL | / taking_multiple_units(foo(0), { LL | | foo(1); @@ -110,7 +110,7 @@ LL | taking_multiple_units((), ()); | error: passing unit values to a function - --> $DIR/unit_arg.rs:47:5 + --> $DIR/unit_arg.rs:48:5 | LL | / taking_multiple_units( LL | | { @@ -140,7 +140,7 @@ LL | foo(2); ... error: passing a unit value to a function - --> $DIR/unit_arg.rs:58:13 + --> $DIR/unit_arg.rs:59:13 | LL | None.or(Some(foo(2))); | ^^^^^^^^^^^^ @@ -154,7 +154,7 @@ LL | }); | error: passing a unit value to a function - --> $DIR/unit_arg.rs:61:5 + --> $DIR/unit_arg.rs:62:5 | LL | foo(foo(())) | ^^^^^^^^^^^^ @@ -166,7 +166,7 @@ LL | foo(()) | error: passing a unit value to a function - --> $DIR/unit_arg.rs:94:5 + --> $DIR/unit_arg.rs:95:5 | LL | Some(foo(1)) | ^^^^^^^^^^^^ diff --git a/src/tools/clippy/tests/ui/vec_init_then_push.rs b/src/tools/clippy/tests/ui/vec_init_then_push.rs new file mode 100644 index 00000000000..642ce504009 --- /dev/null +++ b/src/tools/clippy/tests/ui/vec_init_then_push.rs @@ -0,0 +1,21 @@ +#![allow(unused_variables)] +#![warn(clippy::vec_init_then_push)] + +fn main() { + let mut def_err: Vec<u32> = Default::default(); + def_err.push(0); + + let mut new_err = Vec::<u32>::new(); + new_err.push(1); + + let mut cap_err = Vec::with_capacity(2); + cap_err.push(0); + cap_err.push(1); + cap_err.push(2); + + let mut cap_ok = Vec::with_capacity(10); + cap_ok.push(0); + + new_err = Vec::new(); + new_err.push(0); +} diff --git a/src/tools/clippy/tests/ui/vec_init_then_push.stderr b/src/tools/clippy/tests/ui/vec_init_then_push.stderr new file mode 100644 index 00000000000..819ed47d099 --- /dev/null +++ b/src/tools/clippy/tests/ui/vec_init_then_push.stderr @@ -0,0 +1,34 @@ +error: calls to `push` immediately after creation + --> $DIR/vec_init_then_push.rs:5:5 + | +LL | / let mut def_err: Vec<u32> = Default::default(); +LL | | def_err.push(0); + | |____________________^ help: consider using the `vec![]` macro: `let mut def_err: Vec<u32> = vec![..];` + | + = note: `-D clippy::vec-init-then-push` implied by `-D warnings` + +error: calls to `push` immediately after creation + --> $DIR/vec_init_then_push.rs:8:5 + | +LL | / let mut new_err = Vec::<u32>::new(); +LL | | new_err.push(1); + | |____________________^ help: consider using the `vec![]` macro: `let mut new_err = vec![..];` + +error: calls to `push` immediately after creation + --> $DIR/vec_init_then_push.rs:11:5 + | +LL | / let mut cap_err = Vec::with_capacity(2); +LL | | cap_err.push(0); +LL | | cap_err.push(1); +LL | | cap_err.push(2); + | |____________________^ help: consider using the `vec![]` macro: `let mut cap_err = vec![..];` + +error: calls to `push` immediately after creation + --> $DIR/vec_init_then_push.rs:19:5 + | +LL | / new_err = Vec::new(); +LL | | new_err.push(0); + | |____________________^ help: consider using the `vec![]` macro: `new_err = vec![..];` + +error: aborting due to 4 previous errors + diff --git a/src/tools/clippy/tests/ui/wrong_self_convention.rs b/src/tools/clippy/tests/ui/wrong_self_convention.rs index 5282eba74fd..6cfc0fcb4ca 100644 --- a/src/tools/clippy/tests/ui/wrong_self_convention.rs +++ b/src/tools/clippy/tests/ui/wrong_self_convention.rs @@ -94,7 +94,8 @@ mod issue6307 { trait T: Sized { fn as_i32(self) {} fn as_u32(&self) {} - fn into_i32(&self) {} + fn into_i32(self) {} + fn into_i32_ref(&self) {} fn into_u32(self) {} fn is_i32(self) {} fn is_u32(&self) {} @@ -117,7 +118,32 @@ mod issue6307 { trait U { fn as_i32(self); fn as_u32(&self); - fn into_i32(&self); + fn into_i32(self); + fn into_i32_ref(&self); + fn into_u32(self); + fn is_i32(self); + fn is_u32(&self); + fn to_i32(self); + fn to_u32(&self); + fn from_i32(self); + // check whether the lint can be allowed at the function level + #[allow(clippy::wrong_self_convention)] + fn from_cake(self); + + // test for false positives + fn as_(self); + fn into_(&self); + fn is_(self); + fn to_(self); + fn from_(self); + fn to_mut(&mut self); + } + + trait C: Copy { + fn as_i32(self); + fn as_u32(&self); + fn into_i32(self); + fn into_i32_ref(&self); fn into_u32(self); fn is_i32(self); fn is_u32(&self); diff --git a/src/tools/clippy/tests/ui/wrong_self_convention.stderr b/src/tools/clippy/tests/ui/wrong_self_convention.stderr index 86467eb0fc7..32bd9075bd5 100644 --- a/src/tools/clippy/tests/ui/wrong_self_convention.stderr +++ b/src/tools/clippy/tests/ui/wrong_self_convention.stderr @@ -79,58 +79,70 @@ LL | fn as_i32(self) {} | ^^^^ error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name - --> $DIR/wrong_self_convention.rs:97:21 + --> $DIR/wrong_self_convention.rs:98:25 | -LL | fn into_i32(&self) {} - | ^^^^^ +LL | fn into_i32_ref(&self) {} + | ^^^^^ error: methods called `is_*` usually take self by reference or no self; consider choosing a less ambiguous name - --> $DIR/wrong_self_convention.rs:99:19 + --> $DIR/wrong_self_convention.rs:100:19 | LL | fn is_i32(self) {} | ^^^^ error: methods called `to_*` usually take self by reference; consider choosing a less ambiguous name - --> $DIR/wrong_self_convention.rs:101:19 + --> $DIR/wrong_self_convention.rs:102:19 | LL | fn to_i32(self) {} | ^^^^ error: methods called `from_*` usually take no self; consider choosing a less ambiguous name - --> $DIR/wrong_self_convention.rs:103:21 + --> $DIR/wrong_self_convention.rs:104:21 | LL | fn from_i32(self) {} | ^^^^ error: methods called `as_*` usually take self by reference or self by mutable reference; consider choosing a less ambiguous name - --> $DIR/wrong_self_convention.rs:118:19 + --> $DIR/wrong_self_convention.rs:119:19 | LL | fn as_i32(self); | ^^^^ error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name - --> $DIR/wrong_self_convention.rs:120:21 + --> $DIR/wrong_self_convention.rs:122:25 | -LL | fn into_i32(&self); - | ^^^^^ +LL | fn into_i32_ref(&self); + | ^^^^^ error: methods called `is_*` usually take self by reference or no self; consider choosing a less ambiguous name - --> $DIR/wrong_self_convention.rs:122:19 + --> $DIR/wrong_self_convention.rs:124:19 | LL | fn is_i32(self); | ^^^^ error: methods called `to_*` usually take self by reference; consider choosing a less ambiguous name - --> $DIR/wrong_self_convention.rs:124:19 + --> $DIR/wrong_self_convention.rs:126:19 | LL | fn to_i32(self); | ^^^^ error: methods called `from_*` usually take no self; consider choosing a less ambiguous name - --> $DIR/wrong_self_convention.rs:126:21 + --> $DIR/wrong_self_convention.rs:128:21 + | +LL | fn from_i32(self); + | ^^^^ + +error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name + --> $DIR/wrong_self_convention.rs:146:25 + | +LL | fn into_i32_ref(&self); + | ^^^^^ + +error: methods called `from_*` usually take no self; consider choosing a less ambiguous name + --> $DIR/wrong_self_convention.rs:152:21 | LL | fn from_i32(self); | ^^^^ -error: aborting due to 22 previous errors +error: aborting due to 24 previous errors |