From 6f1f41371789e938e0f1fe1e3dfc879835351989 Mon Sep 17 00:00:00 2001 From: Jason Olson Date: Thu, 12 Sep 2019 17:30:58 -0700 Subject: [PATCH 1/4] Changes cast-lossless to a pedantic lint Fixes #4528 --- clippy_lints/src/lib.rs | 3 +-- clippy_lints/src/types.rs | 2 +- src/lintlist/mod.rs | 2 +- tests/ui/types.fixed | 1 + tests/ui/types.rs | 1 + tests/ui/types.stderr | 2 +- 6 files changed, 6 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 9d0a91c53182..719b414c56f0 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -667,6 +667,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con shadow::SHADOW_UNRELATED, strings::STRING_ADD_ASSIGN, trait_bounds::TYPE_REPETITION_IN_BOUNDS, + types::CAST_LOSSLESS, types::CAST_POSSIBLE_TRUNCATION, types::CAST_POSSIBLE_WRAP, types::CAST_PRECISION_LOSS, @@ -890,7 +891,6 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con types::ABSURD_EXTREME_COMPARISONS, types::BORROWED_BOX, types::BOX_VEC, - types::CAST_LOSSLESS, types::CAST_PTR_ALIGNMENT, types::CAST_REF_TO_MUT, types::CHAR_LIT_AS_U8, @@ -1072,7 +1072,6 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con transmute::TRANSMUTE_PTR_TO_REF, transmute::USELESS_TRANSMUTE, types::BORROWED_BOX, - types::CAST_LOSSLESS, types::CHAR_LIT_AS_U8, types::OPTION_OPTION, types::TYPE_COMPLEXITY, diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 2e4128ccca63..a1d54ff3f0e9 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -765,7 +765,7 @@ declare_clippy_lint! { /// } /// ``` pub CAST_LOSSLESS, - complexity, + pedantic, "casts using `as` that are known to be lossless, e.g., `x as u64` where `x: u8`" } diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 223e6aa9acd7..d0be8c1d3410 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -121,7 +121,7 @@ pub const ALL_LINTS: [Lint; 313] = [ }, Lint { name: "cast_lossless", - group: "complexity", + group: "pedantic", desc: "casts using `as` that are known to be lossless, e.g., `x as u64` where `x: u8`", deprecation: None, module: "types", diff --git a/tests/ui/types.fixed b/tests/ui/types.fixed index a71a9ec81247..b1622e45f3b3 100644 --- a/tests/ui/types.fixed +++ b/tests/ui/types.fixed @@ -1,6 +1,7 @@ // run-rustfix #![allow(dead_code, unused_variables)] +#![warn(clippy::all, clippy::pedantic)] // should not warn on lossy casting in constant types // because not supported yet diff --git a/tests/ui/types.rs b/tests/ui/types.rs index 6f48080ceddb..30463f9e2a17 100644 --- a/tests/ui/types.rs +++ b/tests/ui/types.rs @@ -1,6 +1,7 @@ // run-rustfix #![allow(dead_code, unused_variables)] +#![warn(clippy::all, clippy::pedantic)] // should not warn on lossy casting in constant types // because not supported yet diff --git a/tests/ui/types.stderr b/tests/ui/types.stderr index 3b4f57a7a431..daba766856df 100644 --- a/tests/ui/types.stderr +++ b/tests/ui/types.stderr @@ -1,5 +1,5 @@ error: casting i32 to i64 may become silently lossy if you later change the type - --> $DIR/types.rs:13:22 + --> $DIR/types.rs:14:22 | LL | let c_i64: i64 = c as i64; | ^^^^^^^^ help: try: `i64::from(c)` From 24ec9940017a228bf9a8eaa36c8c9f6a284aa576 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sun, 15 Sep 2019 10:33:20 -0700 Subject: [PATCH 2/4] remove machine applicable suggestion explicit_write format #4542 --- clippy_lints/src/explicit_write.rs | 3 ++- tests/ui/explicit_write_non_rustfix.rs | 8 ++++++++ tests/ui/explicit_write_non_rustfix.stderr | 10 ++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) create mode 100644 tests/ui/explicit_write_non_rustfix.rs create mode 100644 tests/ui/explicit_write_non_rustfix.stderr diff --git a/clippy_lints/src/explicit_write.rs b/clippy_lints/src/explicit_write.rs index a2edb4855b64..477f0e760258 100644 --- a/clippy_lints/src/explicit_write.rs +++ b/clippy_lints/src/explicit_write.rs @@ -140,7 +140,8 @@ fn write_output_string(write_args: &HirVec) -> Option { if output_args.len() > 0; if let ExprKind::AddrOf(_, ref output_string_expr) = output_args[0].node; if let ExprKind::Array(ref string_exprs) = output_string_expr.node; - if string_exprs.len() > 0; + // we only want to provide an automatic suggestion for simple (non-format) strings + if string_exprs.len() == 1; if let ExprKind::Lit(ref lit) = string_exprs[0].node; if let LitKind::Str(ref write_output, _) = lit.node; then { diff --git a/tests/ui/explicit_write_non_rustfix.rs b/tests/ui/explicit_write_non_rustfix.rs new file mode 100644 index 000000000000..f21e8ef935bd --- /dev/null +++ b/tests/ui/explicit_write_non_rustfix.rs @@ -0,0 +1,8 @@ +#![allow(unused_imports, clippy::blacklisted_name)] +#![warn(clippy::explicit_write)] + +fn main() { + use std::io::Write; + let bar = "bar"; + writeln!(std::io::stderr(), "foo {}", bar).unwrap(); +} diff --git a/tests/ui/explicit_write_non_rustfix.stderr b/tests/ui/explicit_write_non_rustfix.stderr new file mode 100644 index 000000000000..77cadb99bb55 --- /dev/null +++ b/tests/ui/explicit_write_non_rustfix.stderr @@ -0,0 +1,10 @@ +error: use of `writeln!(stderr(), ...).unwrap()`. Consider using `eprintln!` instead + --> $DIR/explicit_write_non_rustfix.rs:7:5 + | +LL | writeln!(std::io::stderr(), "foo {}", bar).unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::explicit-write` implied by `-D warnings` + +error: aborting due to previous error + From ceeffcb1f2b50e0a3edbd58a8e03ccb9f86af0d3 Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Fri, 13 Sep 2019 18:39:14 +0200 Subject: [PATCH 3/4] new lint: mem-replace-with-uninit --- CHANGELOG.md | 1 + README.md | 2 +- clippy_lints/src/lib.rs | 2 + clippy_lints/src/mem_replace.rs | 126 ++++++++++++++++++++++++-------- clippy_lints/src/utils/paths.rs | 2 + src/lintlist/mod.rs | 9 ++- tests/ui/repl_uninit.rs | 35 +++++++++ tests/ui/repl_uninit.stderr | 27 +++++++ 8 files changed, 171 insertions(+), 33 deletions(-) create mode 100644 tests/ui/repl_uninit.rs create mode 100644 tests/ui/repl_uninit.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index dbdf3df4ddc3..1865c39c664b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1050,6 +1050,7 @@ Released 2018-09-13 [`mem_discriminant_non_enum`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_discriminant_non_enum [`mem_forget`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_forget [`mem_replace_option_with_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_option_with_none +[`mem_replace_with_uninit`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_with_uninit [`min_max`]: https://rust-lang.github.io/rust-clippy/master/index.html#min_max [`misaligned_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#misaligned_transmute [`misrefactored_assign_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#misrefactored_assign_op diff --git a/README.md b/README.md index dd315fd397b0..4541af9c844e 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 313 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 314 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 9d0a91c53182..abd314c6be5e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -781,6 +781,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con matches::SINGLE_MATCH, mem_discriminant::MEM_DISCRIMINANT_NON_ENUM, mem_replace::MEM_REPLACE_OPTION_WITH_NONE, + mem_replace::MEM_REPLACE_WITH_UNINIT, methods::CHARS_LAST_CMP, methods::CHARS_NEXT_CMP, methods::CLONE_DOUBLE_REF, @@ -1116,6 +1117,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con loops::REVERSE_RANGE_LOOP, loops::WHILE_IMMUTABLE_CONDITION, mem_discriminant::MEM_DISCRIMINANT_NON_ENUM, + mem_replace::MEM_REPLACE_WITH_UNINIT, methods::CLONE_DOUBLE_REF, methods::INTO_ITER_ON_ARRAY, methods::TEMPORARY_CSTRING_AS_PTR, diff --git a/clippy_lints/src/mem_replace.rs b/clippy_lints/src/mem_replace.rs index 7e83e836b856..3e1155806b9e 100644 --- a/clippy_lints/src/mem_replace.rs +++ b/clippy_lints/src/mem_replace.rs @@ -1,4 +1,6 @@ -use crate::utils::{match_def_path, match_qpath, paths, snippet_with_applicability, span_lint_and_sugg}; +use crate::utils::{ + match_def_path, match_qpath, paths, snippet_with_applicability, span_help_and_lint, span_lint_and_sugg, +}; use if_chain::if_chain; use rustc::hir::{Expr, ExprKind, MutMutable, QPath}; use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; @@ -32,7 +34,40 @@ declare_clippy_lint! { "replacing an `Option` with `None` instead of `take()`" } -declare_lint_pass!(MemReplace => [MEM_REPLACE_OPTION_WITH_NONE]); +declare_clippy_lint! { + /// **What it does:** Checks for `mem::replace(&mut _, mem::uninitialized())` + /// and `mem::replace(&mut _, mem::zeroed())`. + /// + /// **Why is this bad?** This will lead to undefined behavior even if the + /// value is overwritten later, because the uninitialized value may be + /// observed in the case of a panic. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ``` + /// use std::mem; + ///# fn may_panic(v: Vec) -> Vec { v } + /// + /// #[allow(deprecated, invalid_value)] + /// fn myfunc (v: &mut Vec) { + /// let taken_v = unsafe { mem::replace(v, mem::uninitialized()) }; + /// let new_v = may_panic(taken_v); // undefined behavior on panic + /// mem::forget(mem::replace(v, new_v)); + /// } + /// ``` + /// + /// The [take_mut](https://docs.rs/take_mut) crate offers a sound solution, + /// at the cost of either lazily creating a replacement value or aborting + /// on panic, to ensure that the uninitialized value cannot be observed. + pub MEM_REPLACE_WITH_UNINIT, + correctness, + "`mem::replace(&mut _, mem::uninitialized())` or `mem::replace(&mut _, mem::zeroed())`" +} + +declare_lint_pass!(MemReplace => + [MEM_REPLACE_OPTION_WITH_NONE, MEM_REPLACE_WITH_UNINIT]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MemReplace { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { @@ -45,37 +80,66 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MemReplace { if match_def_path(cx, def_id, &paths::MEM_REPLACE); // Check that second argument is `Option::None` - if let ExprKind::Path(ref replacement_qpath) = func_args[1].node; - if match_qpath(replacement_qpath, &paths::OPTION_NONE); - then { - // Since this is a late pass (already type-checked), - // and we already know that the second argument is an - // `Option`, we do not need to check the first - // argument's type. All that's left is to get - // replacee's path. - let replaced_path = match func_args[0].node { - ExprKind::AddrOf(MutMutable, ref replaced) => { - if let ExprKind::Path(QPath::Resolved(None, ref replaced_path)) = replaced.node { - replaced_path - } else { - return - } - }, - ExprKind::Path(QPath::Resolved(None, ref replaced_path)) => replaced_path, - _ => return, - }; + if let ExprKind::Path(ref replacement_qpath) = func_args[1].node { + if match_qpath(replacement_qpath, &paths::OPTION_NONE) { - let mut applicability = Applicability::MachineApplicable; - span_lint_and_sugg( - cx, - MEM_REPLACE_OPTION_WITH_NONE, - expr.span, - "replacing an `Option` with `None`", - "consider `Option::take()` instead", - format!("{}.take()", snippet_with_applicability(cx, replaced_path.span, "", &mut applicability)), - applicability, - ); + // Since this is a late pass (already type-checked), + // and we already know that the second argument is an + // `Option`, we do not need to check the first + // argument's type. All that's left is to get + // replacee's path. + let replaced_path = match func_args[0].node { + ExprKind::AddrOf(MutMutable, ref replaced) => { + if let ExprKind::Path(QPath::Resolved(None, ref replaced_path)) = replaced.node { + replaced_path + } else { + return + } + }, + ExprKind::Path(QPath::Resolved(None, ref replaced_path)) => replaced_path, + _ => return, + }; + + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + MEM_REPLACE_OPTION_WITH_NONE, + expr.span, + "replacing an `Option` with `None`", + "consider `Option::take()` instead", + format!("{}.take()", snippet_with_applicability(cx, replaced_path.span, "", &mut applicability)), + applicability, + ); + } + } + if let ExprKind::Call(ref repl_func, ref repl_args) = func_args[1].node { + if_chain! { + if repl_args.is_empty(); + if let ExprKind::Path(ref repl_func_qpath) = repl_func.node; + if let Some(repl_def_id) = cx.tables.qpath_res(repl_func_qpath, repl_func.hir_id).opt_def_id(); + then { + if match_def_path(cx, repl_def_id, &paths::MEM_UNINITIALIZED) { + span_help_and_lint( + cx, + MEM_REPLACE_WITH_UNINIT, + expr.span, + "replacing with `mem::uninitialized()`", + "consider using the `take_mut` crate instead", + ); + } else if match_def_path(cx, repl_def_id, &paths::MEM_ZEROED) && + !cx.tables.expr_ty(&func_args[1]).is_primitive() { + span_help_and_lint( + cx, + MEM_REPLACE_WITH_UNINIT, + expr.span, + "replacing with `mem::zeroed()`", + "consider using a default value or the `take_mut` crate instead", + ); + } + } + } + } } } } diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 9b88a0d3089b..cda3d2024d2e 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -52,6 +52,8 @@ pub const MEM_FORGET: [&str; 3] = ["core", "mem", "forget"]; pub const MEM_MAYBEUNINIT: [&str; 4] = ["core", "mem", "maybe_uninit", "MaybeUninit"]; pub const MEM_MAYBEUNINIT_UNINIT: [&str; 5] = ["core", "mem", "maybe_uninit", "MaybeUninit", "uninit"]; pub const MEM_REPLACE: [&str; 3] = ["core", "mem", "replace"]; +pub const MEM_UNINITIALIZED: [&str; 3] = ["core", "mem", "uninitialized"]; +pub const MEM_ZEROED: [&str; 3] = ["core", "mem", "zeroed"]; pub const MUTEX: [&str; 4] = ["std", "sync", "mutex", "Mutex"]; pub const OPEN_OPTIONS: [&str; 3] = ["std", "fs", "OpenOptions"]; pub const OPS_MODULE: [&str; 2] = ["core", "ops"]; diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 223e6aa9acd7..847d5e2aa011 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::Lint; pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 313] = [ +pub const ALL_LINTS: [Lint; 314] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -1043,6 +1043,13 @@ pub const ALL_LINTS: [Lint; 313] = [ deprecation: None, module: "mem_replace", }, + Lint { + name: "mem_replace_with_uninit", + group: "correctness", + desc: "`mem::replace(&mut _, mem::uninitialized())` or `mem::zeroed()`", + deprecation: None, + module: "mem_replace", + }, Lint { name: "min_max", group: "correctness", diff --git a/tests/ui/repl_uninit.rs b/tests/ui/repl_uninit.rs new file mode 100644 index 000000000000..346972b7bb4e --- /dev/null +++ b/tests/ui/repl_uninit.rs @@ -0,0 +1,35 @@ +#![allow(deprecated, invalid_value)] +#![warn(clippy::all)] + +use std::mem; + +fn might_panic(x: X) -> X { + // in practice this would be a possibly-panicky operation + x +} + +fn main() { + let mut v = vec![0i32; 4]; + // the following is UB if `might_panic` panics + unsafe { + let taken_v = mem::replace(&mut v, mem::uninitialized()); + let new_v = might_panic(taken_v); + std::mem::forget(mem::replace(&mut v, new_v)); + } + + unsafe { + let taken_v = mem::replace(&mut v, mem::zeroed()); + let new_v = might_panic(taken_v); + std::mem::forget(mem::replace(&mut v, new_v)); + } + + // this is silly but OK, because usize is a primitive type + let mut u: usize = 42; + let uref = &mut u; + let taken_u = unsafe { mem::replace(uref, mem::zeroed()) }; + *uref = taken_u + 1; + + // this is still not OK, because uninit + let taken_u = unsafe { mem::replace(uref, mem::uninitialized()) }; + *uref = taken_u + 1; +} diff --git a/tests/ui/repl_uninit.stderr b/tests/ui/repl_uninit.stderr new file mode 100644 index 000000000000..c1f55d7601e5 --- /dev/null +++ b/tests/ui/repl_uninit.stderr @@ -0,0 +1,27 @@ +error: replacing with `mem::uninitialized()` + --> $DIR/repl_uninit.rs:15:23 + | +LL | let taken_v = mem::replace(&mut v, mem::uninitialized()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::mem-replace-with-uninit` implied by `-D warnings` + = help: consider using the `take_mut` crate instead + +error: replacing with `mem::zeroed()` + --> $DIR/repl_uninit.rs:21:23 + | +LL | let taken_v = mem::replace(&mut v, mem::zeroed()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using a default value or the `take_mut` crate instead + +error: replacing with `mem::uninitialized()` + --> $DIR/repl_uninit.rs:33:28 + | +LL | let taken_u = unsafe { mem::replace(uref, mem::uninitialized()) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using the `take_mut` crate instead + +error: aborting due to 3 previous errors + From 70a7dab773b5d97fd46541a46bdb6de7abf77b2f Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Wed, 11 Sep 2019 18:39:02 +0200 Subject: [PATCH 4/4] New lint: Require `# Safety` section in pub unsafe fn docs --- CHANGELOG.md | 1 + README.md | 2 +- clippy_lints/Cargo.toml | 2 +- clippy_lints/src/doc.rs | 111 +++++++++++++++++++++++++++---------- clippy_lints/src/lib.rs | 2 + src/lintlist/mod.rs | 9 ++- tests/ui/doc_unsafe.rs | 25 +++++++++ tests/ui/doc_unsafe.stderr | 12 ++++ 8 files changed, 133 insertions(+), 31 deletions(-) create mode 100644 tests/ui/doc_unsafe.rs create mode 100644 tests/ui/doc_unsafe.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index dbdf3df4ddc3..c39b2e69df91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1056,6 +1056,7 @@ Released 2018-09-13 [`missing_const_for_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn [`missing_docs_in_private_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_docs_in_private_items [`missing_inline_in_public_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_inline_in_public_items +[`missing_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_safety_doc [`mistyped_literal_suffixes`]: https://rust-lang.github.io/rust-clippy/master/index.html#mistyped_literal_suffixes [`mixed_case_hex_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#mixed_case_hex_literals [`module_inception`]: https://rust-lang.github.io/rust-clippy/master/index.html#module_inception diff --git a/README.md b/README.md index dd315fd397b0..4541af9c844e 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 313 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 314 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/Cargo.toml b/clippy_lints/Cargo.toml index 48133cc5b02b..423e13e9dd5a 100644 --- a/clippy_lints/Cargo.toml +++ b/clippy_lints/Cargo.toml @@ -27,7 +27,7 @@ semver = "0.9.0" serde = { version = "1.0", features = ["derive"] } toml = "0.5.3" unicode-normalization = "0.1" -pulldown-cmark = "0.5.3" +pulldown-cmark = "0.6.0" url = { version = "2.1.0", features = ["serde"] } # cargo requires serde feat in its url dep # see https://github.com/rust-lang/rust/pull/63587#issuecomment-522343864 if_chain = "1.0.0" diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index 4950212a4b5d..86d83bf9602e 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -34,6 +34,40 @@ declare_clippy_lint! { "presence of `_`, `::` or camel-case outside backticks in documentation" } +declare_clippy_lint! { + /// **What it does:** Checks for the doc comments of publicly visible + /// unsafe functions and warns if there is no `# Safety` section. + /// + /// **Why is this bad?** Unsafe functions should document their safety + /// preconditions, so that users can be sure they are using them safely. + /// + /// **Known problems:** None. + /// + /// **Examples**: + /// ```rust + ///# type Universe = (); + /// /// This function should really be documented + /// pub unsafe fn start_apocalypse(u: &mut Universe) { + /// unimplemented!(); + /// } + /// ``` + /// + /// At least write a line about safety: + /// + /// ```rust + ///# type Universe = (); + /// /// # Safety + /// /// + /// /// This function should not be called before the horsemen are ready. + /// pub unsafe fn start_apocalypse(u: &mut Universe) { + /// unimplemented!(); + /// } + /// ``` + pub MISSING_SAFETY_DOC, + style, + "`pub unsafe fn` without `# Safety` docs" +} + #[allow(clippy::module_name_repetitions)] #[derive(Clone)] pub struct DocMarkdown { @@ -46,7 +80,7 @@ impl DocMarkdown { } } -impl_lint_pass!(DocMarkdown => [DOC_MARKDOWN]); +impl_lint_pass!(DocMarkdown => [DOC_MARKDOWN, MISSING_SAFETY_DOC]); impl EarlyLintPass for DocMarkdown { fn check_crate(&mut self, cx: &EarlyContext<'_>, krate: &ast::Crate) { @@ -54,7 +88,20 @@ impl EarlyLintPass for DocMarkdown { } fn check_item(&mut self, cx: &EarlyContext<'_>, item: &ast::Item) { - check_attrs(cx, &self.valid_idents, &item.attrs); + if check_attrs(cx, &self.valid_idents, &item.attrs) { + return; + } + // no safety header + if let ast::ItemKind::Fn(_, ref header, ..) = item.node { + if item.vis.node.is_pub() && header.unsafety == ast::Unsafety::Unsafe { + span_lint( + cx, + MISSING_SAFETY_DOC, + item.span, + "unsafe function's docs miss `# Safety` section", + ); + } + } } } @@ -115,7 +162,7 @@ pub fn strip_doc_comment_decoration(comment: &str, span: Span) -> (String, Vec<( panic!("not a doc-comment: {}", comment); } -pub fn check_attrs<'a>(cx: &EarlyContext<'_>, valid_idents: &FxHashSet, attrs: &'a [ast::Attribute]) { +pub fn check_attrs<'a>(cx: &EarlyContext<'_>, valid_idents: &FxHashSet, attrs: &'a [ast::Attribute]) -> bool { let mut doc = String::new(); let mut spans = vec![]; @@ -129,7 +176,7 @@ pub fn check_attrs<'a>(cx: &EarlyContext<'_>, valid_idents: &FxHashSet, } } else if attr.check_name(sym!(doc)) { // ignore mix of sugared and non-sugared doc - return; + return true; // don't trigger the safety check } } @@ -140,26 +187,28 @@ pub fn check_attrs<'a>(cx: &EarlyContext<'_>, valid_idents: &FxHashSet, current += offset_copy; } - if !doc.is_empty() { - let parser = pulldown_cmark::Parser::new(&doc).into_offset_iter(); - // Iterate over all `Events` and combine consecutive events into one - let events = parser.coalesce(|previous, current| { - use pulldown_cmark::Event::*; - - let previous_range = previous.1; - let current_range = current.1; - - match (previous.0, current.0) { - (Text(previous), Text(current)) => { - let mut previous = previous.to_string(); - previous.push_str(¤t); - Ok((Text(previous.into()), previous_range)) - }, - (previous, current) => Err(((previous, previous_range), (current, current_range))), - } - }); - check_doc(cx, valid_idents, events, &spans); + if doc.is_empty() { + return false; } + + let parser = pulldown_cmark::Parser::new(&doc).into_offset_iter(); + // Iterate over all `Events` and combine consecutive events into one + let events = parser.coalesce(|previous, current| { + use pulldown_cmark::Event::*; + + let previous_range = previous.1; + let current_range = current.1; + + match (previous.0, current.0) { + (Text(previous), Text(current)) => { + let mut previous = previous.to_string(); + previous.push_str(¤t); + Ok((Text(previous.into()), previous_range)) + }, + (previous, current) => Err(((previous, previous_range), (current, current_range))), + } + }); + check_doc(cx, valid_idents, events, &spans) } fn check_doc<'a, Events: Iterator, Range)>>( @@ -167,12 +216,15 @@ fn check_doc<'a, Events: Iterator, Range, events: Events, spans: &[(usize, Span)], -) { +) -> bool { + // true if a safety header was found use pulldown_cmark::Event::*; use pulldown_cmark::Tag::*; + let mut safety_header = false; let mut in_code = false; let mut in_link = None; + let mut in_heading = false; for (event, range) in events { match event { @@ -180,9 +232,11 @@ fn check_doc<'a, Events: Iterator, Range in_code = false, Start(Link(_, url, _)) => in_link = Some(url), End(Link(..)) => in_link = None, - Start(_tag) | End(_tag) => (), // We don't care about other tags - Html(_html) | InlineHtml(_html) => (), // HTML is weird, just ignore it - SoftBreak | HardBreak | TaskListMarker(_) | Code(_) => (), + Start(Heading(_)) => in_heading = true, + End(Heading(_)) => in_heading = false, + Start(_tag) | End(_tag) => (), // We don't care about other tags + Html(_html) => (), // HTML is weird, just ignore it + SoftBreak | HardBreak | TaskListMarker(_) | Code(_) | Rule => (), FootnoteReference(text) | Text(text) => { if Some(&text) == in_link.as_ref() { // Probably a link of the form `` @@ -190,7 +244,7 @@ fn check_doc<'a, Events: Iterator, Range o, @@ -207,6 +261,7 @@ fn check_doc<'a, Events: Iterator, Range, valid_idents: &FxHashSet, text: &str, span: Span) { diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 9d0a91c53182..2892c6c417db 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -708,6 +708,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con copies::IFS_SAME_COND, copies::IF_SAME_THEN_ELSE, derive::DERIVE_HASH_XOR_EQ, + doc::MISSING_SAFETY_DOC, double_comparison::DOUBLE_COMPARISONS, double_parens::DOUBLE_PARENS, drop_bounds::DROP_BOUNDS, @@ -929,6 +930,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con block_in_if_condition::BLOCK_IN_IF_CONDITION_EXPR, block_in_if_condition::BLOCK_IN_IF_CONDITION_STMT, collapsible_if::COLLAPSIBLE_IF, + doc::MISSING_SAFETY_DOC, enum_variants::ENUM_VARIANT_NAMES, enum_variants::MODULE_INCEPTION, eq_op::OP_REF, diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 223e6aa9acd7..f45c240014fd 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::Lint; pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 313] = [ +pub const ALL_LINTS: [Lint; 314] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -1078,6 +1078,13 @@ pub const ALL_LINTS: [Lint; 313] = [ deprecation: None, module: "missing_inline", }, + Lint { + name: "missing_safety_doc", + group: "style", + desc: "`pub unsafe fn` without `# Safety` docs", + deprecation: None, + module: "doc", + }, Lint { name: "mistyped_literal_suffixes", group: "correctness", diff --git a/tests/ui/doc_unsafe.rs b/tests/ui/doc_unsafe.rs new file mode 100644 index 000000000000..7b26e86b40b8 --- /dev/null +++ b/tests/ui/doc_unsafe.rs @@ -0,0 +1,25 @@ +/// This is not sufficiently documented +pub unsafe fn destroy_the_planet() { + unimplemented!(); +} + +/// This one is +/// +/// # Safety +/// +/// This function shouldn't be called unless the horsemen are ready +pub unsafe fn apocalypse(universe: &mut ()) { + unimplemented!(); +} + +/// This is a private function, so docs aren't necessary +unsafe fn you_dont_see_me() { + unimplemented!(); +} + +fn main() { + you_dont_see_me(); + destroy_the_planet(); + let mut universe = (); + apocalypse(&mut universe); +} diff --git a/tests/ui/doc_unsafe.stderr b/tests/ui/doc_unsafe.stderr new file mode 100644 index 000000000000..d6d1cd2d4faf --- /dev/null +++ b/tests/ui/doc_unsafe.stderr @@ -0,0 +1,12 @@ +error: unsafe function's docs miss `# Safety` section + --> $DIR/doc_unsafe.rs:2:1 + | +LL | / pub unsafe fn destroy_the_planet() { +LL | | unimplemented!(); +LL | | } + | |_^ + | + = note: `-D clippy::missing-safety-doc` implied by `-D warnings` + +error: aborting due to previous error +