diff --git a/CHANGELOG.md b/CHANGELOG.md index c04735a495a7..b4b879f2dc9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6867,6 +6867,7 @@ Released 2018-09-13 [`unchecked_duration_subtraction`]: https://rust-lang.github.io/rust-clippy/master/index.html#unchecked_duration_subtraction [`unchecked_time_subtraction`]: https://rust-lang.github.io/rust-clippy/master/index.html#unchecked_time_subtraction [`unconditional_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#unconditional_recursion +[`undocumented_may_panic_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_may_panic_call [`undocumented_unsafe_blocks`]: https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks [`undropped_manually_drops`]: https://rust-lang.github.io/rust-clippy/master/index.html#undropped_manually_drops [`unicode_not_nfc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unicode_not_nfc @@ -7047,6 +7048,7 @@ Released 2018-09-13 [`max-struct-bools`]: https://doc.rust-lang.org/clippy/lint_configuration.html#max-struct-bools [`max-suggested-slice-pattern-length`]: https://doc.rust-lang.org/clippy/lint_configuration.html#max-suggested-slice-pattern-length [`max-trait-bounds`]: https://doc.rust-lang.org/clippy/lint_configuration.html#max-trait-bounds +[`may-panic-functions`]: https://doc.rust-lang.org/clippy/lint_configuration.html#may-panic-functions [`min-ident-chars-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#min-ident-chars-threshold [`missing-docs-allow-unused`]: https://doc.rust-lang.org/clippy/lint_configuration.html#missing-docs-allow-unused [`missing-docs-in-crate-items`]: https://doc.rust-lang.org/clippy/lint_configuration.html#missing-docs-in-crate-items diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index 6569bdabf115..3fa91e22e478 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -776,6 +776,17 @@ The maximum number of bounds a trait can have to be linted * [`type_repetition_in_bounds`](https://rust-lang.github.io/rust-clippy/master/index.html#type_repetition_in_bounds) +## `may-panic-functions` +List of function/method paths that may panic and should be documented with a `// Panic:` comment +at call sites. + +**Default Value:** `[]` + +--- +**Affected lints:** +* [`undocumented_may_panic_call`](https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_may_panic_call) + + ## `min-ident-chars-threshold` Minimum chars an ident can have, anything below or equal to this will be linted. diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 2a042e6c3d85..d731f87c2eb1 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -703,6 +703,10 @@ define_Conf! { /// The maximum number of bounds a trait can have to be linted #[lints(type_repetition_in_bounds)] max_trait_bounds: u64 = 3, + /// List of function/method paths that may panic and should be documented with a `// Panic:` comment + /// at call sites. + #[lints(undocumented_may_panic_call)] + may_panic_functions: Vec = Vec::new(), /// Minimum chars an ident can have, anything below or equal to this will be linted. #[lints(min_ident_chars)] min_ident_chars_threshold: u64 = 1, diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 67e5b1de17a7..995459b6b01c 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -739,6 +739,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::types::TYPE_COMPLEXITY_INFO, crate::types::VEC_BOX_INFO, crate::unconditional_recursion::UNCONDITIONAL_RECURSION_INFO, + crate::undocumented_may_panic_call::UNDOCUMENTED_MAY_PANIC_CALL_INFO, crate::undocumented_unsafe_blocks::UNDOCUMENTED_UNSAFE_BLOCKS_INFO, crate::undocumented_unsafe_blocks::UNNECESSARY_SAFETY_COMMENT_INFO, crate::unicode::INVISIBLE_CHARACTERS_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 1d0ebff26e69..4b67c074b92d 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -358,6 +358,7 @@ mod transmute; mod tuple_array_conversions; mod types; mod unconditional_recursion; +mod undocumented_may_panic_call; mod undocumented_unsafe_blocks; mod unicode; mod uninhabited_references; @@ -825,5 +826,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co store.register_late_pass(|_| Box::new(toplevel_ref_arg::ToplevelRefArg)); store.register_late_pass(|_| Box::new(volatile_composites::VolatileComposites)); store.register_late_pass(|_| Box::new(replace_box::ReplaceBox)); + store + .register_late_pass(move |tcx| Box::new(undocumented_may_panic_call::UndocumentedMayPanicCall::new(tcx, conf))); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/undocumented_may_panic_call.rs b/clippy_lints/src/undocumented_may_panic_call.rs new file mode 100644 index 000000000000..9ab3eef4e387 --- /dev/null +++ b/clippy_lints/src/undocumented_may_panic_call.rs @@ -0,0 +1,172 @@ +use clippy_config::Conf; +use clippy_utils::diagnostics::span_lint; +use clippy_utils::paths::{PathNS, lookup_path_str}; +use clippy_utils::{get_unique_attr, sym}; +use rustc_data_structures::fx::FxHashSet; +use rustc_hir as hir; +use rustc_hir::def_id::DefId; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::ty::TyCtxt; +use rustc_session::impl_lint_pass; +use rustc_span::Pos; + +declare_clippy_lint! { + /// ### What it does + /// Checks for calls to functions marked with `#[clippy::may_panic]` or configured in + /// `may-panic-functions` that don't have a `// Panic:` comment on the line above. + /// + /// ### Why is this bad? + /// Functions that may panic should be documented at their call sites to explain why the + /// panic is acceptable or impossible in that context. + /// + /// ### Example + /// ```rust,ignore + /// #[clippy::may_panic] + /// fn my_panicable_func(n: u32) { + /// if n % 2 == 0 { + /// panic!("even number not allowed") + /// } + /// } + /// + /// fn main() { + /// // Missing documentation - will lint + /// my_panicable_func(1); + /// } + /// ``` + /// Use instead: + /// ```rust,ignore + /// #[clippy::may_panic] + /// fn my_panicable_func(n: u32) { + /// if n % 2 == 0 { + /// panic!("even number not allowed") + /// } + /// } + /// + /// fn main() { + /// // Panic: This is safe, it's an odd number + /// my_panicable_func(1); + /// } + /// ``` + /// + /// ### Configuration + /// This lint can be configured to check calls to external functions that may panic: + /// ```toml + /// # clippy.toml + /// may-panic-functions = [ + /// "alloc::vec::Vec::push", # Can panic on allocation failure + /// "std::fs::File::open", # Can panic in some configurations + /// ] + /// ``` + #[clippy::version = "1.92.0"] + pub UNDOCUMENTED_MAY_PANIC_CALL, + pedantic, + "missing `// Panic:` documentation on calls to functions that may panic" +} + +pub struct UndocumentedMayPanicCall { + may_panic_def_ids: FxHashSet, +} + +impl_lint_pass!(UndocumentedMayPanicCall => [UNDOCUMENTED_MAY_PANIC_CALL]); + +impl UndocumentedMayPanicCall { + pub fn new(tcx: TyCtxt<'_>, conf: &'static Conf) -> Self { + let may_panic_def_ids = conf + .may_panic_functions + .iter() + .flat_map(|path| lookup_path_str(tcx, PathNS::Value, path)) + .collect(); + + Self { may_panic_def_ids } + } + + // A function is a may_panic_function if it has the may_panic attribute + // or is in the may-panic-functions configuration + fn is_may_panic_function(&self, cx: &LateContext<'_>, def_id: DefId) -> bool { + if get_unique_attr(cx.sess(), cx.tcx.get_all_attrs(def_id), sym::may_panic).is_some() { + return true; + } + + self.may_panic_def_ids.contains(&def_id) + } +} + +impl LateLintPass<'_> for UndocumentedMayPanicCall { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ rustc_hir::Expr<'_>) { + let def_id = match &expr.kind { + hir::ExprKind::Call(func, _args) => { + if let hir::ExprKind::Path(qpath) = &func.kind { + cx.qpath_res(qpath, func.hir_id).opt_def_id() + } else { + None + } + }, + hir::ExprKind::MethodCall(_path, _receiver, _args, _span) => { + cx.typeck_results().type_dependent_def_id(expr.hir_id) + }, + _ => None, + }; + + if let Some(def_id) = def_id + && self.is_may_panic_function(cx, def_id) + && !has_panic_comment_above(cx, expr.span) + { + span_lint( + cx, + UNDOCUMENTED_MAY_PANIC_CALL, + expr.span, + "call to a function that may panic is not documented with a `// Panic:` comment", + ); + } + } +} + +/// Checks if the lines immediately preceding the call contain a "Panic:" comment. +fn has_panic_comment_above(cx: &LateContext<'_>, call_span: rustc_span::Span) -> bool { + let source_map = cx.sess().source_map(); + + if let Ok(call_line) = source_map.lookup_line(call_span.lo()) + && call_line.line > 0 + && let Some(src) = call_line.sf.src.as_deref() + { + let lines = call_line.sf.lines(); + let line_starts = &lines[..=call_line.line]; + + has_panic_comment_in_text(src, line_starts) + } else { + false + } +} + +fn has_panic_comment_in_text(src: &str, line_starts: &[rustc_span::RelativeBytePos]) -> bool { + let mut lines = line_starts + .array_windows::<2>() + .rev() + .map_while(|[start, end]| { + let start = start.to_usize(); + let end = end.to_usize(); + let text = src.get(start..end)?; + let trimmed = text.trim_start(); + Some((trimmed, text.len() - trimmed.len())) + }) + .filter(|(text, _)| !text.is_empty()); + + let Some((line, _)) = lines.next() else { + return false; + }; + + if line.starts_with("//") { + let mut current_line = line; + loop { + if current_line.to_ascii_uppercase().contains("PANIC:") { + return true; + } + match lines.next() { + Some((text, _)) if text.starts_with("//") => current_line = text, + _ => return false, + } + } + } + + false +} diff --git a/clippy_utils/src/attrs.rs b/clippy_utils/src/attrs.rs index 2d42e76dcbc9..011756c4e0d4 100644 --- a/clippy_utils/src/attrs.rs +++ b/clippy_utils/src/attrs.rs @@ -33,6 +33,7 @@ pub const BUILTIN_ATTRIBUTES: &[(Symbol, DeprecationStatus)] = &[ // See book/src/attribs.md (sym::has_significant_drop, DeprecationStatus::None), (sym::format_args, DeprecationStatus::None), + (sym::may_panic, DeprecationStatus::None), ]; pub struct LimitStack { diff --git a/clippy_utils/src/sym.rs b/clippy_utils/src/sym.rs index 8e8a80a6a9c9..bc3775437439 100644 --- a/clippy_utils/src/sym.rs +++ b/clippy_utils/src/sym.rs @@ -227,6 +227,7 @@ generate! { max_by_key, max_value, maximum, + may_panic, min, min_by, min_by_key, diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index 2d9503c5ac53..08507b900513 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -59,6 +59,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect max-struct-bools max-suggested-slice-pattern-length max-trait-bounds + may-panic-functions min-ident-chars-threshold missing-docs-allow-unused missing-docs-in-crate-items @@ -156,6 +157,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect max-struct-bools max-suggested-slice-pattern-length max-trait-bounds + may-panic-functions min-ident-chars-threshold missing-docs-allow-unused missing-docs-in-crate-items @@ -253,6 +255,7 @@ error: error reading Clippy's configuration file: unknown field `allow_mixed_uni max-struct-bools max-suggested-slice-pattern-length max-trait-bounds + may-panic-functions min-ident-chars-threshold missing-docs-allow-unused missing-docs-in-crate-items diff --git a/tests/ui-toml/undocumented_may_panic_call/clippy.toml b/tests/ui-toml/undocumented_may_panic_call/clippy.toml new file mode 100644 index 000000000000..d3a903ad1188 --- /dev/null +++ b/tests/ui-toml/undocumented_may_panic_call/clippy.toml @@ -0,0 +1 @@ +may-panic-functions = ["alloc::vec::Vec::push"] diff --git a/tests/ui-toml/undocumented_may_panic_call/undocumented_may_panic_call.rs b/tests/ui-toml/undocumented_may_panic_call/undocumented_may_panic_call.rs new file mode 100644 index 000000000000..ef43bce9e6e4 --- /dev/null +++ b/tests/ui-toml/undocumented_may_panic_call/undocumented_may_panic_call.rs @@ -0,0 +1,11 @@ +#![warn(clippy::undocumented_may_panic_call)] + +fn main() { + let mut v = vec![1, 2, 3]; + + v.push(4); + //~^ undocumented_may_panic_call + + // Panic: The capaticy is less than isize::MAX, so we are safe! + v.push(5); +} diff --git a/tests/ui-toml/undocumented_may_panic_call/undocumented_may_panic_call.stderr b/tests/ui-toml/undocumented_may_panic_call/undocumented_may_panic_call.stderr new file mode 100644 index 000000000000..0c0ec2ede705 --- /dev/null +++ b/tests/ui-toml/undocumented_may_panic_call/undocumented_may_panic_call.stderr @@ -0,0 +1,11 @@ +error: call to a function that may panic is not documented with a `// Panic:` comment + --> tests/ui-toml/undocumented_may_panic_call/undocumented_may_panic_call.rs:6:5 + | +LL | v.push(4); + | ^^^^^^^^^ + | + = note: `-D clippy::undocumented-may-panic-call` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::undocumented_may_panic_call)]` + +error: aborting due to 1 previous error + diff --git a/tests/ui/undocumented_may_panic_call.rs b/tests/ui/undocumented_may_panic_call.rs new file mode 100644 index 000000000000..73a6b50dbc5e --- /dev/null +++ b/tests/ui/undocumented_may_panic_call.rs @@ -0,0 +1,86 @@ +#![warn(clippy::undocumented_may_panic_call)] +#![allow(clippy::short_circuit_statement)] +#![allow(clippy::manual_is_multiple_of)] + +#[clippy::may_panic] +pub fn dangerous(n: usize) -> bool { + if n % 2 == 0 { + panic!() + } + true +} + +fn main() { + let _ = dangerous(1); + //~^ undocumented_may_panic_call + + // Panic: This is safe, 1 is an odd number + let _ = dangerous(1); +} + +#[rustfmt::skip] +fn deeper() { + let v: Vec = vec![]; + !v.is_empty() + && dangerous(1); + //~^ undocumented_may_panic_call + !v.is_empty() + // Panic: This is safe, 1 is an odd number + && dangerous(1); +} + +struct MyStruct { + value: usize, +} + +impl MyStruct { + #[clippy::may_panic] + fn panic_method(&self) -> u32 { + self.value.ilog2() + } +} + +fn test_struct_methods() { + let s = MyStruct { value: 42 }; + + let _ = s.panic_method(); + //~^ undocumented_may_panic_call + + // Panic: value is non-zero, so we are safe! + let _ = s.panic_method(); +} + +trait MyTrait { + #[clippy::may_panic] + fn trait_panic_method(&self) -> u32; +} + +struct TraitImpl { + data: usize, +} + +impl MyTrait for TraitImpl { + fn trait_panic_method(&self) -> u32 { + self.data.ilog2() + } +} + +fn test_trait_methods() { + let t = TraitImpl { data: 10 }; + + let _ = t.trait_panic_method(); + //~^ undocumented_may_panic_call + + // Panic: t.data is non-zero + // We are safe! + let _ = t.trait_panic_method(); +} + +fn test_trait_object(t: &dyn MyTrait) { + // Should lint: no comment + let _ = t.trait_panic_method(); + //~^ undocumented_may_panic_call + + // Panic: This is safe, just trust me. + let _ = t.trait_panic_method(); +} diff --git a/tests/ui/undocumented_may_panic_call.stderr b/tests/ui/undocumented_may_panic_call.stderr new file mode 100644 index 000000000000..ea87f4379a55 --- /dev/null +++ b/tests/ui/undocumented_may_panic_call.stderr @@ -0,0 +1,35 @@ +error: call to a function that may panic is not documented with a `// Panic:` comment + --> tests/ui/undocumented_may_panic_call.rs:14:13 + | +LL | let _ = dangerous(1); + | ^^^^^^^^^^^^ + | + = note: `-D clippy::undocumented-may-panic-call` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::undocumented_may_panic_call)]` + +error: call to a function that may panic is not documented with a `// Panic:` comment + --> tests/ui/undocumented_may_panic_call.rs:25:12 + | +LL | && dangerous(1); + | ^^^^^^^^^^^^ + +error: call to a function that may panic is not documented with a `// Panic:` comment + --> tests/ui/undocumented_may_panic_call.rs:46:13 + | +LL | let _ = s.panic_method(); + | ^^^^^^^^^^^^^^^^ + +error: call to a function that may panic is not documented with a `// Panic:` comment + --> tests/ui/undocumented_may_panic_call.rs:71:13 + | +LL | let _ = t.trait_panic_method(); + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: call to a function that may panic is not documented with a `// Panic:` comment + --> tests/ui/undocumented_may_panic_call.rs:81:13 + | +LL | let _ = t.trait_panic_method(); + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 5 previous errors +