From 3789d2b887adac1eefd8a4b9970d9cfaec9d9de7 Mon Sep 17 00:00:00 2001 From: Radbuglet <10578060+Radbuglet@users.noreply.github.com> Date: Sun, 1 Oct 2023 15:40:37 -0600 Subject: [PATCH 01/16] Implement MVP of `danger_not_accepted` --- CHANGELOG.md | 1 + clippy_lints/src/danger_not_accepted.rs | 181 ++++++++++++++++++++++++ clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_utils/src/attrs.rs | 2 + tests/ui/danger_not_accepted.rs | 16 +++ tests/ui/danger_not_accepted.stderr | 12 ++ 7 files changed, 215 insertions(+) create mode 100644 clippy_lints/src/danger_not_accepted.rs create mode 100644 tests/ui/danger_not_accepted.rs create mode 100644 tests/ui/danger_not_accepted.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 25230e46e889..9cdf041084af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4854,6 +4854,7 @@ Released 2018-09-13 [`create_dir`]: https://rust-lang.github.io/rust-clippy/master/index.html#create_dir [`crosspointer_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#crosspointer_transmute [`cyclomatic_complexity`]: https://rust-lang.github.io/rust-clippy/master/index.html#cyclomatic_complexity +[`danger_not_accepted`]: https://rust-lang.github.io/rust-clippy/master/index.html#danger_not_accepted [`dbg_macro`]: https://rust-lang.github.io/rust-clippy/master/index.html#dbg_macro [`debug_assert_with_mut_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#debug_assert_with_mut_call [`decimal_literal_representation`]: https://rust-lang.github.io/rust-clippy/master/index.html#decimal_literal_representation diff --git a/clippy_lints/src/danger_not_accepted.rs b/clippy_lints/src/danger_not_accepted.rs new file mode 100644 index 000000000000..22e196bc05b6 --- /dev/null +++ b/clippy_lints/src/danger_not_accepted.rs @@ -0,0 +1,181 @@ +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::get_attr; +use rustc_ast::{ast, token, tokenstream}; +use rustc_data_structures::fx::{FxHashMap, StdEntry}; +use rustc_hir::*; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::Symbol; + +// TODO: Ensure that our attributes are being used properly +// TODO: Improve lint messages + +declare_clippy_lint! { + /// ### What it does + /// + /// ### Why is this bad? + /// + /// ### Example + /// ```rust + /// // example code where clippy issues a warning + /// ``` + /// Use instead: + /// ```rust + /// // example code which does not raise clippy warning + /// ``` + #[clippy::version = "1.74.0"] + pub DANGER_NOT_ACCEPTED, + nursery, + "default lint description" +} + +#[derive(Default)] +pub struct DangerNotAccepted { + accepted_dangers: FxHashMap, +} + +impl_lint_pass!(DangerNotAccepted => [DANGER_NOT_ACCEPTED]); + +impl LateLintPass<'_> for DangerNotAccepted { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) { + // If we're calling a method... + if let ExprKind::MethodCall(path, _, _self_arg, ..) = &expr.kind + && let def::Res::Def(def::DefKind::Fn, fn_id) = path.res + // And that function is dangerous to us... + && let Some(dangers) = self.get_unaccepted_dangers(cx, fn_id) + { + // Raise a lint + emit_dangerous_call_lint(cx, expr, &dangers); + return; + } + + // If we're referencing a function... + if let ExprKind::Path(path) = &expr.kind + && let QPath::Resolved(_, path) = path + && let def::Res::Def(def::DefKind::Fn, fn_id) = path.res + // And that function is dangerous to us... + && let Some(dangers) = self.get_unaccepted_dangers(cx, fn_id) + { + // Raise a lint + emit_dangerous_call_lint(cx, expr, &dangers); + return; + } + } + + fn enter_lint_attrs(&mut self, cx: &LateContext<'_>, attrs: &'_ [rustc_ast::Attribute]) { + for attr in get_attr(cx.sess(), attrs, "accept_danger") { + let dangers = parse_dangers_attr(cx, attr); + for danger in dangers { + *self.accepted_dangers.entry(danger).or_default() += 1; + } + } + } + + fn exit_lint_attrs(&mut self, cx: &LateContext<'_>, attrs: &'_ [rustc_ast::Attribute]) { + for attr in get_attr(cx.sess(), attrs, "accept_danger") { + let dangers = parse_dangers_attr(cx, attr); + for danger in dangers { + match self.accepted_dangers.entry(danger) { + StdEntry::Occupied(mut entry) => { + *entry.get_mut() -= 1; + if *entry.get() == 0 { + entry.remove(); + } + }, + StdEntry::Vacant(_) => unreachable!(), + } + } + } + } +} + +impl DangerNotAccepted { + fn get_unaccepted_dangers(&self, cx: &LateContext<'_>, item_id: def_id::DefId) -> Option> { + let mut unaccepted_dangers = Vec::new(); + let mut item_iter = Some(item_id); + + while let Some(item_id) = item_iter { + for attr in cx.tcx.get_attrs_by_path(item_id, &[sym!(clippy), sym!(dangerous)]) { + for danger in parse_dangers_attr(cx, attr) { + if self.accepted_dangers.contains_key(&danger) { + continue; + } + + unaccepted_dangers.push(danger); + } + } + + item_iter = cx.tcx.opt_parent(item_id); + } + + (!unaccepted_dangers.is_empty()).then_some(unaccepted_dangers) + } +} + +fn emit_dangerous_call_lint(cx: &LateContext<'_>, expr: &'_ Expr<'_>, dangers: &[Symbol]) { + span_lint_and_help( + cx, + DANGER_NOT_ACCEPTED, + expr.span, + "Called a method marked with `#[clippy::dangerous]` without blessing the calling module with `#![clippy::accept_danger]`.", + None, + format!("This method poses the following unaccepted dangers: {dangers:?}").as_str(), + ); +} + +fn parse_dangers_attr(cx: &LateContext<'_>, attr: &ast::Attribute) -> Vec { + const EXPECTATION: &str = "Expected a delimited attribute with a list of danger identifiers."; + + let span = attr.span; + + let rustc_ast::AttrKind::Normal(attr) = &attr.kind else { + cx.sess().span_err(span, EXPECTATION); + return Vec::new(); + }; + + let ast::AttrArgs::Delimited(attr) = &attr.item.args else { + cx.sess().span_err(span, EXPECTATION); + return Vec::new(); + }; + + if attr.delim != token::Delimiter::Parenthesis { + cx.sess().span_err(span, EXPECTATION); + return Vec::new(); + } + + let mut stream = attr.tokens.trees(); + let mut dangers = Vec::new(); + + loop { + let sym = match stream.next() { + Some(tokenstream::TokenTree::Token(sym, _)) => sym, + None => break, + _ => { + cx.sess().span_err(span, EXPECTATION); + return Vec::new(); + }, + }; + + let Some((sym, _)) = sym.ident() else { + cx.sess().span_err(span, EXPECTATION); + return Vec::new(); + }; + + dangers.push(sym.name); + + match stream.next() { + Some(tokenstream::TokenTree::Token(sym, _)) if sym.kind == token::TokenKind::Comma => { + continue; + }, + None => { + break; + }, + _ => { + cx.sess().span_err(span, EXPECTATION); + return Vec::new(); + }, + } + } + + dangers +} diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 481c44031cf7..475826add3b9 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -111,6 +111,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::copy_iterator::COPY_ITERATOR_INFO, crate::crate_in_macro_def::CRATE_IN_MACRO_DEF_INFO, crate::create_dir::CREATE_DIR_INFO, + crate::danger_not_accepted::DANGER_NOT_ACCEPTED_INFO, crate::dbg_macro::DBG_MACRO_INFO, crate::default::DEFAULT_TRAIT_ACCESS_INFO, crate::default::FIELD_REASSIGN_WITH_DEFAULT_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 0f35ec276657..14515830dfe8 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -96,6 +96,7 @@ mod copies; mod copy_iterator; mod crate_in_macro_def; mod create_dir; +mod danger_not_accepted; mod dbg_macro; mod default; mod default_constructed_unit_structs; @@ -1123,6 +1124,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: }); store.register_late_pass(move |_| Box::new(manual_hash_one::ManualHashOne::new(msrv()))); store.register_late_pass(|_| Box::new(iter_without_into_iter::IterWithoutIntoIter)); + store.register_late_pass(|_| Box::::default()); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_utils/src/attrs.rs b/clippy_utils/src/attrs.rs index 51771f78d4ff..fcceb45c6c1e 100644 --- a/clippy_utils/src/attrs.rs +++ b/clippy_utils/src/attrs.rs @@ -22,6 +22,8 @@ pub const BUILTIN_ATTRIBUTES: &[(&str, DeprecationStatus)] = &[ ("dump", DeprecationStatus::None), ("msrv", DeprecationStatus::None), ("has_significant_drop", DeprecationStatus::None), + ("dangerous", DeprecationStatus::None), + ("accept_danger", DeprecationStatus::None), ]; pub struct LimitStack { diff --git a/tests/ui/danger_not_accepted.rs b/tests/ui/danger_not_accepted.rs new file mode 100644 index 000000000000..27e9869e6928 --- /dev/null +++ b/tests/ui/danger_not_accepted.rs @@ -0,0 +1,16 @@ +#![warn(clippy::danger_not_accepted)] + +fn main() { + wee(); + waz::woo(); + + #[clippy::accept_danger(may_deadlock)] + waz::woo(); +} + +fn wee() {} + +#[clippy::dangerous(may_deadlock)] +mod waz { + pub fn woo() {} +} diff --git a/tests/ui/danger_not_accepted.stderr b/tests/ui/danger_not_accepted.stderr new file mode 100644 index 000000000000..c90251193d5e --- /dev/null +++ b/tests/ui/danger_not_accepted.stderr @@ -0,0 +1,12 @@ +error: Called a method marked with `#[clippy::dangerous]` without blessing the calling module with `#![clippy::accept_danger]`. + --> $DIR/danger_not_accepted.rs:5:5 + | +LL | waz::woo(); + | ^^^^^^^^ + | + = help: This method poses the following unaccepted dangers: ["may_deadlock"] + = note: `-D clippy::danger-not-accepted` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::danger_not_accepted)]` + +error: aborting due to previous error + From 639515b6af6061ade4ff015ae1db91d43b9e1255 Mon Sep 17 00:00:00 2001 From: Radbuglet <10578060+Radbuglet@users.noreply.github.com> Date: Sun, 1 Oct 2023 16:03:35 -0600 Subject: [PATCH 02/16] Fix method resolution while checking for danger --- clippy_lints/src/danger_not_accepted.rs | 4 +-- tests/ui/danger_not_accepted.rs | 32 +++++++++++++++++++++++ tests/ui/danger_not_accepted.stderr | 34 ++++++++++++++++++++++++- 3 files changed, 67 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/danger_not_accepted.rs b/clippy_lints/src/danger_not_accepted.rs index 22e196bc05b6..3ccacd8672fe 100644 --- a/clippy_lints/src/danger_not_accepted.rs +++ b/clippy_lints/src/danger_not_accepted.rs @@ -39,8 +39,8 @@ impl_lint_pass!(DangerNotAccepted => [DANGER_NOT_ACCEPTED]); impl LateLintPass<'_> for DangerNotAccepted { fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) { // If we're calling a method... - if let ExprKind::MethodCall(path, _, _self_arg, ..) = &expr.kind - && let def::Res::Def(def::DefKind::Fn, fn_id) = path.res + if let ExprKind::MethodCall(_path, _, _self_arg, ..) = &expr.kind + && let Some(fn_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) // And that function is dangerous to us... && let Some(dangers) = self.get_unaccepted_dangers(cx, fn_id) { diff --git a/tests/ui/danger_not_accepted.rs b/tests/ui/danger_not_accepted.rs index 27e9869e6928..add0f09a8464 100644 --- a/tests/ui/danger_not_accepted.rs +++ b/tests/ui/danger_not_accepted.rs @@ -6,11 +6,43 @@ fn main() { #[clippy::accept_danger(may_deadlock)] waz::woo(); + + Maz.faz(); + + #[clippy::accept_danger(may_deadlock)] + Maz.faz(); + + #[clippy::accept_danger(may_deadlock, may_delete_system)] + Maz.faz(); + + Maz.faz2(); + + #[clippy::accept_danger(may_deadlock)] + Maz.faz2(); + + #[clippy::accept_danger(may_deadlock, may_delete_system)] + Maz.faz2(); } fn wee() {} +struct Maz; + #[clippy::dangerous(may_deadlock)] mod waz { pub fn woo() {} + + impl super::Maz { + #[clippy::dangerous(may_delete_system)] + pub fn faz(&self) {} + } + + impl super::FazTrait for super::Maz { + fn faz2(&self) {} + } +} + +trait FazTrait { + #[clippy::dangerous(may_delete_system)] + fn faz2(&self); } diff --git a/tests/ui/danger_not_accepted.stderr b/tests/ui/danger_not_accepted.stderr index c90251193d5e..7ab7460ec20b 100644 --- a/tests/ui/danger_not_accepted.stderr +++ b/tests/ui/danger_not_accepted.stderr @@ -8,5 +8,37 @@ LL | waz::woo(); = note: `-D clippy::danger-not-accepted` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::danger_not_accepted)]` -error: aborting due to previous error +error: Called a method marked with `#[clippy::dangerous]` without blessing the calling module with `#![clippy::accept_danger]`. + --> $DIR/danger_not_accepted.rs:10:5 + | +LL | Maz.faz(); + | ^^^^^^^^^ + | + = help: This method poses the following unaccepted dangers: ["may_delete_system", "may_deadlock"] + +error: Called a method marked with `#[clippy::dangerous]` without blessing the calling module with `#![clippy::accept_danger]`. + --> $DIR/danger_not_accepted.rs:13:5 + | +LL | Maz.faz(); + | ^^^^^^^^^ + | + = help: This method poses the following unaccepted dangers: ["may_delete_system"] + +error: Called a method marked with `#[clippy::dangerous]` without blessing the calling module with `#![clippy::accept_danger]`. + --> $DIR/danger_not_accepted.rs:18:5 + | +LL | Maz.faz2(); + | ^^^^^^^^^^ + | + = help: This method poses the following unaccepted dangers: ["may_delete_system"] + +error: Called a method marked with `#[clippy::dangerous]` without blessing the calling module with `#![clippy::accept_danger]`. + --> $DIR/danger_not_accepted.rs:21:5 + | +LL | Maz.faz2(); + | ^^^^^^^^^^ + | + = help: This method poses the following unaccepted dangers: ["may_delete_system"] + +error: aborting due to 5 previous errors From 9036b6ead23edd57c98b960514774c20664b5c8b Mon Sep 17 00:00:00 2001 From: Radbuglet <10578060+Radbuglet@users.noreply.github.com> Date: Sun, 1 Oct 2023 20:03:42 -0600 Subject: [PATCH 03/16] Make `dangerous` accept its own dangers --- clippy_lints/src/danger_not_accepted.rs | 36 ++++++++++++++----------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/danger_not_accepted.rs b/clippy_lints/src/danger_not_accepted.rs index 3ccacd8672fe..ce3b4f26286d 100644 --- a/clippy_lints/src/danger_not_accepted.rs +++ b/clippy_lints/src/danger_not_accepted.rs @@ -63,26 +63,32 @@ impl LateLintPass<'_> for DangerNotAccepted { } fn enter_lint_attrs(&mut self, cx: &LateContext<'_>, attrs: &'_ [rustc_ast::Attribute]) { - for attr in get_attr(cx.sess(), attrs, "accept_danger") { - let dangers = parse_dangers_attr(cx, attr); - for danger in dangers { - *self.accepted_dangers.entry(danger).or_default() += 1; + // Both `accept_danger` and `dangerous` contribute to the accepted danger map. + for attr_name in ["accept_danger", "dangerous"] { + for attr in get_attr(cx.sess(), attrs, attr_name) { + let dangers = parse_dangers_attr(cx, attr); + for danger in dangers { + *self.accepted_dangers.entry(danger).or_default() += 1; + } } } } fn exit_lint_attrs(&mut self, cx: &LateContext<'_>, attrs: &'_ [rustc_ast::Attribute]) { - for attr in get_attr(cx.sess(), attrs, "accept_danger") { - let dangers = parse_dangers_attr(cx, attr); - for danger in dangers { - match self.accepted_dangers.entry(danger) { - StdEntry::Occupied(mut entry) => { - *entry.get_mut() -= 1; - if *entry.get() == 0 { - entry.remove(); - } - }, - StdEntry::Vacant(_) => unreachable!(), + // Both `accept_danger` and `dangerous` contribute to the accepted danger map. + for attr_name in ["accept_danger", "dangerous"] { + for attr in get_attr(cx.sess(), attrs, attr_name) { + let dangers = parse_dangers_attr(cx, attr); + for danger in dangers { + match self.accepted_dangers.entry(danger) { + StdEntry::Occupied(mut entry) => { + *entry.get_mut() -= 1; + if *entry.get() == 0 { + entry.remove(); + } + }, + StdEntry::Vacant(_) => unreachable!(), + } } } } From 67afb0c2198906c5f709fb035f050f0843b1b7b8 Mon Sep 17 00:00:00 2001 From: Radbuglet <10578060+Radbuglet@users.noreply.github.com> Date: Sun, 1 Oct 2023 21:24:06 -0600 Subject: [PATCH 04/16] Fix ICE when analyzing `extern` block --- clippy_lints/src/danger_not_accepted.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/danger_not_accepted.rs b/clippy_lints/src/danger_not_accepted.rs index ce3b4f26286d..0e8a7823ba33 100644 --- a/clippy_lints/src/danger_not_accepted.rs +++ b/clippy_lints/src/danger_not_accepted.rs @@ -101,7 +101,15 @@ impl DangerNotAccepted { let mut item_iter = Some(item_id); while let Some(item_id) = item_iter { - for attr in cx.tcx.get_attrs_by_path(item_id, &[sym!(clippy), sym!(dangerous)]) { + item_iter = cx.tcx.opt_parent(item_id); + + // HACK: Ensure that this is not an intrinsic because calling `get_attrs_unchecked` on + // a foreign module breaks everything. + if cx.tcx.def_kind(item_id) == def::DefKind::ForeignMod { + continue; + } + + for attr in get_attr(cx.sess(), cx.tcx.get_attrs_unchecked(item_id), "dangerous") { for danger in parse_dangers_attr(cx, attr) { if self.accepted_dangers.contains_key(&danger) { continue; @@ -110,8 +118,6 @@ impl DangerNotAccepted { unaccepted_dangers.push(danger); } } - - item_iter = cx.tcx.opt_parent(item_id); } (!unaccepted_dangers.is_empty()).then_some(unaccepted_dangers) From a00124672f984d38591423edf8ebc7f00c04cfdc Mon Sep 17 00:00:00 2001 From: Radbuglet <10578060+Radbuglet@users.noreply.github.com> Date: Mon, 2 Oct 2023 13:42:32 -0600 Subject: [PATCH 05/16] Improve lint diagnostics --- clippy_lints/src/danger_not_accepted.rs | 139 +++++++++++++++++++----- tests/ui/danger_not_accepted.rs | 8 ++ tests/ui/danger_not_accepted.stderr | 64 +++++++++-- 3 files changed, 174 insertions(+), 37 deletions(-) diff --git a/clippy_lints/src/danger_not_accepted.rs b/clippy_lints/src/danger_not_accepted.rs index 0e8a7823ba33..730623b8dfed 100644 --- a/clippy_lints/src/danger_not_accepted.rs +++ b/clippy_lints/src/danger_not_accepted.rs @@ -1,32 +1,79 @@ -use clippy_utils::diagnostics::span_lint_and_help; +use std::fmt; + +use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::get_attr; use rustc_ast::{ast, token, tokenstream}; -use rustc_data_structures::fx::{FxHashMap, StdEntry}; +use rustc_data_structures::fx::{FxHashMap, FxHashSet, StdEntry}; use rustc_hir::*; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::Symbol; +use rustc_span::{Span, Symbol}; -// TODO: Ensure that our attributes are being used properly -// TODO: Improve lint messages +// TODO: Safety override +// TODO: Config declare_clippy_lint! { /// ### What it does /// + /// Checks for uses of functions, inherent methods, and trait methods which have been marked as + /// dangerous with the `#[clippy::dangerous(...)]` attribute. + /// + /// Each `#[clippy::dangerous(reason_1, reason_2, ...)]` attribute specifies a list of dangers + /// that the user must accept using the `#[clippy::accept_danger(reason_1, reason_2, ...)]` + /// attribute before using the dangerous item. + /// /// ### Why is this bad? /// + /// Some functionality in a project may be dangerous to use without giving it the appropriate + /// caution, even if its misuse does not cause undefined behavior—for example, the method could + /// be the source of tricky logic bugs. Other functionality may be dangerous in some contexts + /// but not others. This lint helps ensure that users do not unknowingly call into these + /// dangerous functions while still allowing users who know what they're doing to call these + /// functions without issue. + /// /// ### Example /// ```rust - /// // example code where clippy issues a warning + /// #[clippy::dangerous(use_of_lib_1_dangerous_module)] + /// mod dangerous_module { + /// # fn break_the_program() {} + /// #[clippy::dangerous(may_break_program)] + /// pub fn do_something_innocuous_looking() { + /// break_the_program(); + /// } + /// } + /// + /// use unsuspecting_module { + /// fn do_something() { + /// // This method call causes clippy to issue a warning + /// dangerous_module::do_something_innocuous_looking(); + /// } + /// } /// ``` /// Use instead: /// ```rust - /// // example code which does not raise clippy warning + /// #[clippy::dangerous(use_of_lib_1_dangerous_module)] + /// mod dangerous_module { + /// # fn break_the_program() {} + /// #[clippy::dangerous(may_break_program)] + /// pub fn do_something_innocuous_looking() { + /// break_the_program(); + /// } + /// } + /// + /// // This entire module can use functions with the danger `use_of_lib_1_dangerous_module`. + /// #[clippy::accept_danger(use_of_lib_1_dangerous_module)] + /// use unsuspecting_module { + /// fn do_something() { + /// // Only this statement can call functions with the danger `may_break_program`. + /// #[clippy::accept_danger(may_break_program)] + /// dangerous_module::do_something_innocuous_looking(); + /// } + /// } /// ``` #[clippy::version = "1.74.0"] pub DANGER_NOT_ACCEPTED, nursery, - "default lint description" + "checks for use of functions marked as dangerous" } #[derive(Default)] @@ -40,7 +87,7 @@ impl LateLintPass<'_> for DangerNotAccepted { fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) { // If we're calling a method... if let ExprKind::MethodCall(_path, _, _self_arg, ..) = &expr.kind - && let Some(fn_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) + && let Some(fn_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) // And that function is dangerous to us... && let Some(dangers) = self.get_unaccepted_dangers(cx, fn_id) { @@ -67,7 +114,7 @@ impl LateLintPass<'_> for DangerNotAccepted { for attr_name in ["accept_danger", "dangerous"] { for attr in get_attr(cx.sess(), attrs, attr_name) { let dangers = parse_dangers_attr(cx, attr); - for danger in dangers { + for (_span, danger) in dangers { *self.accepted_dangers.entry(danger).or_default() += 1; } } @@ -79,7 +126,7 @@ impl LateLintPass<'_> for DangerNotAccepted { for attr_name in ["accept_danger", "dangerous"] { for attr in get_attr(cx.sess(), attrs, attr_name) { let dangers = parse_dangers_attr(cx, attr); - for danger in dangers { + for (_span, danger) in dangers { match self.accepted_dangers.entry(danger) { StdEntry::Occupied(mut entry) => { *entry.get_mut() -= 1; @@ -96,7 +143,7 @@ impl LateLintPass<'_> for DangerNotAccepted { } impl DangerNotAccepted { - fn get_unaccepted_dangers(&self, cx: &LateContext<'_>, item_id: def_id::DefId) -> Option> { + fn get_unaccepted_dangers(&self, cx: &LateContext<'_>, item_id: def_id::DefId) -> Option> { let mut unaccepted_dangers = Vec::new(); let mut item_iter = Some(item_id); @@ -111,7 +158,7 @@ impl DangerNotAccepted { for attr in get_attr(cx.sess(), cx.tcx.get_attrs_unchecked(item_id), "dangerous") { for danger in parse_dangers_attr(cx, attr) { - if self.accepted_dangers.contains_key(&danger) { + if self.accepted_dangers.contains_key(&danger.1) { continue; } @@ -124,18 +171,7 @@ impl DangerNotAccepted { } } -fn emit_dangerous_call_lint(cx: &LateContext<'_>, expr: &'_ Expr<'_>, dangers: &[Symbol]) { - span_lint_and_help( - cx, - DANGER_NOT_ACCEPTED, - expr.span, - "Called a method marked with `#[clippy::dangerous]` without blessing the calling module with `#![clippy::accept_danger]`.", - None, - format!("This method poses the following unaccepted dangers: {dangers:?}").as_str(), - ); -} - -fn parse_dangers_attr(cx: &LateContext<'_>, attr: &ast::Attribute) -> Vec { +fn parse_dangers_attr(cx: &LateContext<'_>, attr: &ast::Attribute) -> Vec<(Span, Symbol)> { const EXPECTATION: &str = "Expected a delimited attribute with a list of danger identifiers."; let span = attr.span; @@ -173,7 +209,7 @@ fn parse_dangers_attr(cx: &LateContext<'_>, attr: &ast::Attribute) -> Vec { @@ -191,3 +227,54 @@ fn parse_dangers_attr(cx: &LateContext<'_>, attr: &ast::Attribute) -> Vec, expr: &'_ Expr<'_>, unaccepted_dangers: &[(Span, Symbol)]) { + // Define formatting helpers + struct FmtInline(F); + + impl fmt::Display for FmtInline + where + F: Fn(&mut fmt::Formatter<'_>) -> fmt::Result, + { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.0(f) + } + } + + fn fmt_inline(f: F) -> FmtInline + where + F: Fn(&mut fmt::Formatter<'_>) -> fmt::Result, + { + FmtInline(f) + } + + // Collect all unique dangers + let unique_dangers = unaccepted_dangers.iter().map(|(_, sym)| sym).collect::>(); + + // Create a lint + span_lint_and_then( + cx, + DANGER_NOT_ACCEPTED, + expr.span, + &format!( + "Called a method marked with `#[clippy::dangerous(...)]` without blessing the calling \ + module with `#![clippy::accept_danger({})]`.", + fmt_inline(|f| { + let mut is_subsequent = false; + for danger in &unique_dangers { + if is_subsequent { + f.write_str(", ")?; + } + is_subsequent = true; + f.write_str(danger.as_str())?; + } + Ok(()) + }), + ), + |diag| { + for (danger_span, danger_name) in unaccepted_dangers { + diag.span_note(*danger_span, format!("Danger `{danger_name}` declared here.")); + } + }, + ); +} diff --git a/tests/ui/danger_not_accepted.rs b/tests/ui/danger_not_accepted.rs index add0f09a8464..ee3a6551c9d4 100644 --- a/tests/ui/danger_not_accepted.rs +++ b/tests/ui/danger_not_accepted.rs @@ -22,6 +22,11 @@ fn main() { #[clippy::accept_danger(may_deadlock, may_delete_system)] Maz.faz2(); + + waz::woo2(); + + #[clippy::accept_danger(may_deadlock)] + waz::woo2(); } fn wee() {} @@ -32,6 +37,9 @@ struct Maz; mod waz { pub fn woo() {} + #[clippy::dangerous(may_deadlock)] + pub fn woo2() {} + impl super::Maz { #[clippy::dangerous(may_delete_system)] pub fn faz(&self) {} diff --git a/tests/ui/danger_not_accepted.stderr b/tests/ui/danger_not_accepted.stderr index 7ab7460ec20b..ce9586295d7a 100644 --- a/tests/ui/danger_not_accepted.stderr +++ b/tests/ui/danger_not_accepted.stderr @@ -1,44 +1,86 @@ -error: Called a method marked with `#[clippy::dangerous]` without blessing the calling module with `#![clippy::accept_danger]`. +error: Called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_deadlock)]`. --> $DIR/danger_not_accepted.rs:5:5 | LL | waz::woo(); | ^^^^^^^^ | - = help: This method poses the following unaccepted dangers: ["may_deadlock"] +note: Danger `may_deadlock` declared here. + --> $DIR/danger_not_accepted.rs:36:21 + | +LL | #[clippy::dangerous(may_deadlock)] + | ^^^^^^^^^^^^ = note: `-D clippy::danger-not-accepted` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::danger_not_accepted)]` -error: Called a method marked with `#[clippy::dangerous]` without blessing the calling module with `#![clippy::accept_danger]`. +error: Called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system, may_deadlock)]`. --> $DIR/danger_not_accepted.rs:10:5 | LL | Maz.faz(); | ^^^^^^^^^ | - = help: This method poses the following unaccepted dangers: ["may_delete_system", "may_deadlock"] +note: Danger `may_delete_system` declared here. + --> $DIR/danger_not_accepted.rs:44:29 + | +LL | #[clippy::dangerous(may_delete_system)] + | ^^^^^^^^^^^^^^^^^ +note: Danger `may_deadlock` declared here. + --> $DIR/danger_not_accepted.rs:36:21 + | +LL | #[clippy::dangerous(may_deadlock)] + | ^^^^^^^^^^^^ -error: Called a method marked with `#[clippy::dangerous]` without blessing the calling module with `#![clippy::accept_danger]`. +error: Called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]`. --> $DIR/danger_not_accepted.rs:13:5 | LL | Maz.faz(); | ^^^^^^^^^ | - = help: This method poses the following unaccepted dangers: ["may_delete_system"] +note: Danger `may_delete_system` declared here. + --> $DIR/danger_not_accepted.rs:44:29 + | +LL | #[clippy::dangerous(may_delete_system)] + | ^^^^^^^^^^^^^^^^^ -error: Called a method marked with `#[clippy::dangerous]` without blessing the calling module with `#![clippy::accept_danger]`. +error: Called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]`. --> $DIR/danger_not_accepted.rs:18:5 | LL | Maz.faz2(); | ^^^^^^^^^^ | - = help: This method poses the following unaccepted dangers: ["may_delete_system"] +note: Danger `may_delete_system` declared here. + --> $DIR/danger_not_accepted.rs:54:25 + | +LL | #[clippy::dangerous(may_delete_system)] + | ^^^^^^^^^^^^^^^^^ -error: Called a method marked with `#[clippy::dangerous]` without blessing the calling module with `#![clippy::accept_danger]`. +error: Called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]`. --> $DIR/danger_not_accepted.rs:21:5 | LL | Maz.faz2(); | ^^^^^^^^^^ | - = help: This method poses the following unaccepted dangers: ["may_delete_system"] +note: Danger `may_delete_system` declared here. + --> $DIR/danger_not_accepted.rs:54:25 + | +LL | #[clippy::dangerous(may_delete_system)] + | ^^^^^^^^^^^^^^^^^ + +error: Called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_deadlock)]`. + --> $DIR/danger_not_accepted.rs:26:5 + | +LL | waz::woo2(); + | ^^^^^^^^^ + | +note: Danger `may_deadlock` declared here. + --> $DIR/danger_not_accepted.rs:40:25 + | +LL | #[clippy::dangerous(may_deadlock)] + | ^^^^^^^^^^^^ +note: Danger `may_deadlock` declared here. + --> $DIR/danger_not_accepted.rs:36:21 + | +LL | #[clippy::dangerous(may_deadlock)] + | ^^^^^^^^^^^^ -error: aborting due to 5 previous errors +error: aborting due to 6 previous errors From 7a8b3559c3453a858ae266dc5d672d51365930b5 Mon Sep 17 00:00:00 2001 From: Radbuglet <10578060+Radbuglet@users.noreply.github.com> Date: Mon, 2 Oct 2023 13:57:08 -0600 Subject: [PATCH 06/16] Clean up lint implementation --- clippy_lints/src/danger_not_accepted.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/danger_not_accepted.rs b/clippy_lints/src/danger_not_accepted.rs index 730623b8dfed..44c488f54777 100644 --- a/clippy_lints/src/danger_not_accepted.rs +++ b/clippy_lints/src/danger_not_accepted.rs @@ -4,13 +4,20 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::get_attr; use rustc_ast::{ast, token, tokenstream}; use rustc_data_structures::fx::{FxHashMap, FxHashSet, StdEntry}; -use rustc_hir::*; +use rustc_hir::{def, def_id, Expr, ExprKind, QPath}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::{Span, Symbol}; -// TODO: Safety override -// TODO: Config +// Future improvements: +// +// - Allow users to override modules as *not* having a specific danger. +// - Allow users to specify additional dangerous items in the clippy config. +// - Devise a scheme (maybe path compression?) to reduce the amount of ancestry tracing we have to +// do to determine the dangers posed by a method? +// - Implement a way to forbid `accept_danger` in a given module. +// - Allow `accept_danger` and `dangerous` as internal attributes on stable Rust? +// declare_clippy_lint! { /// ### What it does @@ -84,6 +91,7 @@ pub struct DangerNotAccepted { impl_lint_pass!(DangerNotAccepted => [DANGER_NOT_ACCEPTED]); impl LateLintPass<'_> for DangerNotAccepted { + #[allow(clippy::needless_return, reason = "unified syntax improves readability")] fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) { // If we're calling a method... if let ExprKind::MethodCall(_path, _, _self_arg, ..) = &expr.kind From 95eabae9ad0f5a2821703c70289f486dae192772 Mon Sep 17 00:00:00 2001 From: Radbuglet <10578060+Radbuglet@users.noreply.github.com> Date: Mon, 2 Oct 2023 14:17:30 -0600 Subject: [PATCH 07/16] Fix lint message capitalization --- clippy_lints/src/danger_not_accepted.rs | 6 +++--- tests/ui/danger_not_accepted.stderr | 28 ++++++++++++------------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/clippy_lints/src/danger_not_accepted.rs b/clippy_lints/src/danger_not_accepted.rs index 44c488f54777..a4fe615e9026 100644 --- a/clippy_lints/src/danger_not_accepted.rs +++ b/clippy_lints/src/danger_not_accepted.rs @@ -265,8 +265,8 @@ fn emit_dangerous_call_lint(cx: &LateContext<'_>, expr: &'_ Expr<'_>, unaccepted DANGER_NOT_ACCEPTED, expr.span, &format!( - "Called a method marked with `#[clippy::dangerous(...)]` without blessing the calling \ - module with `#![clippy::accept_danger({})]`.", + "called a method marked with `#[clippy::dangerous(...)]` without blessing the calling \ + module with `#![clippy::accept_danger({})]`", fmt_inline(|f| { let mut is_subsequent = false; for danger in &unique_dangers { @@ -281,7 +281,7 @@ fn emit_dangerous_call_lint(cx: &LateContext<'_>, expr: &'_ Expr<'_>, unaccepted ), |diag| { for (danger_span, danger_name) in unaccepted_dangers { - diag.span_note(*danger_span, format!("Danger `{danger_name}` declared here.")); + diag.span_note(*danger_span, format!("danger `{danger_name}` declared here")); } }, ); diff --git a/tests/ui/danger_not_accepted.stderr b/tests/ui/danger_not_accepted.stderr index ce9586295d7a..f592772d1745 100644 --- a/tests/ui/danger_not_accepted.stderr +++ b/tests/ui/danger_not_accepted.stderr @@ -1,10 +1,10 @@ -error: Called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_deadlock)]`. +error: called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_deadlock)]` --> $DIR/danger_not_accepted.rs:5:5 | LL | waz::woo(); | ^^^^^^^^ | -note: Danger `may_deadlock` declared here. +note: danger `may_deadlock` declared here --> $DIR/danger_not_accepted.rs:36:21 | LL | #[clippy::dangerous(may_deadlock)] @@ -12,71 +12,71 @@ LL | #[clippy::dangerous(may_deadlock)] = note: `-D clippy::danger-not-accepted` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::danger_not_accepted)]` -error: Called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system, may_deadlock)]`. +error: called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system, may_deadlock)]` --> $DIR/danger_not_accepted.rs:10:5 | LL | Maz.faz(); | ^^^^^^^^^ | -note: Danger `may_delete_system` declared here. +note: danger `may_delete_system` declared here --> $DIR/danger_not_accepted.rs:44:29 | LL | #[clippy::dangerous(may_delete_system)] | ^^^^^^^^^^^^^^^^^ -note: Danger `may_deadlock` declared here. +note: danger `may_deadlock` declared here --> $DIR/danger_not_accepted.rs:36:21 | LL | #[clippy::dangerous(may_deadlock)] | ^^^^^^^^^^^^ -error: Called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]`. +error: called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]` --> $DIR/danger_not_accepted.rs:13:5 | LL | Maz.faz(); | ^^^^^^^^^ | -note: Danger `may_delete_system` declared here. +note: danger `may_delete_system` declared here --> $DIR/danger_not_accepted.rs:44:29 | LL | #[clippy::dangerous(may_delete_system)] | ^^^^^^^^^^^^^^^^^ -error: Called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]`. +error: called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]` --> $DIR/danger_not_accepted.rs:18:5 | LL | Maz.faz2(); | ^^^^^^^^^^ | -note: Danger `may_delete_system` declared here. +note: danger `may_delete_system` declared here --> $DIR/danger_not_accepted.rs:54:25 | LL | #[clippy::dangerous(may_delete_system)] | ^^^^^^^^^^^^^^^^^ -error: Called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]`. +error: called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]` --> $DIR/danger_not_accepted.rs:21:5 | LL | Maz.faz2(); | ^^^^^^^^^^ | -note: Danger `may_delete_system` declared here. +note: danger `may_delete_system` declared here --> $DIR/danger_not_accepted.rs:54:25 | LL | #[clippy::dangerous(may_delete_system)] | ^^^^^^^^^^^^^^^^^ -error: Called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_deadlock)]`. +error: called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_deadlock)]` --> $DIR/danger_not_accepted.rs:26:5 | LL | waz::woo2(); | ^^^^^^^^^ | -note: Danger `may_deadlock` declared here. +note: danger `may_deadlock` declared here --> $DIR/danger_not_accepted.rs:40:25 | LL | #[clippy::dangerous(may_deadlock)] | ^^^^^^^^^^^^ -note: Danger `may_deadlock` declared here. +note: danger `may_deadlock` declared here --> $DIR/danger_not_accepted.rs:36:21 | LL | #[clippy::dangerous(may_deadlock)] From 5acd889d524d002216f8a1acc49282e5551c17e4 Mon Sep 17 00:00:00 2001 From: Radbuglet <10578060+Radbuglet@users.noreply.github.com> Date: Mon, 2 Oct 2023 14:26:19 -0600 Subject: [PATCH 08/16] Clean up documentation wording --- clippy_lints/src/danger_not_accepted.rs | 9 +++++---- tests/ui/danger_not_accepted.stderr | 12 ++++++------ 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/danger_not_accepted.rs b/clippy_lints/src/danger_not_accepted.rs index a4fe615e9026..0902cbe9f911 100644 --- a/clippy_lints/src/danger_not_accepted.rs +++ b/clippy_lints/src/danger_not_accepted.rs @@ -23,11 +23,12 @@ declare_clippy_lint! { /// ### What it does /// /// Checks for uses of functions, inherent methods, and trait methods which have been marked as - /// dangerous with the `#[clippy::dangerous(...)]` attribute. + /// dangerous with the `#[clippy::dangerous(...)]` attribute and whose dangers have not been + /// explicitly accepted. /// /// Each `#[clippy::dangerous(reason_1, reason_2, ...)]` attribute specifies a list of dangers /// that the user must accept using the `#[clippy::accept_danger(reason_1, reason_2, ...)]` - /// attribute before using the dangerous item. + /// attribute before using the dangerous item to avoid triggering this lint. /// /// ### Why is this bad? /// @@ -51,7 +52,7 @@ declare_clippy_lint! { /// /// use unsuspecting_module { /// fn do_something() { - /// // This method call causes clippy to issue a warning + /// // This function call causes clippy to issue a warning /// dangerous_module::do_something_innocuous_looking(); /// } /// } @@ -265,7 +266,7 @@ fn emit_dangerous_call_lint(cx: &LateContext<'_>, expr: &'_ Expr<'_>, unaccepted DANGER_NOT_ACCEPTED, expr.span, &format!( - "called a method marked with `#[clippy::dangerous(...)]` without blessing the calling \ + "called a function marked with `#[clippy::dangerous(...)]` without blessing the calling \ module with `#![clippy::accept_danger({})]`", fmt_inline(|f| { let mut is_subsequent = false; diff --git a/tests/ui/danger_not_accepted.stderr b/tests/ui/danger_not_accepted.stderr index f592772d1745..45287fbd7876 100644 --- a/tests/ui/danger_not_accepted.stderr +++ b/tests/ui/danger_not_accepted.stderr @@ -1,4 +1,4 @@ -error: called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_deadlock)]` +error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_deadlock)]` --> $DIR/danger_not_accepted.rs:5:5 | LL | waz::woo(); @@ -12,7 +12,7 @@ LL | #[clippy::dangerous(may_deadlock)] = note: `-D clippy::danger-not-accepted` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::danger_not_accepted)]` -error: called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system, may_deadlock)]` +error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system, may_deadlock)]` --> $DIR/danger_not_accepted.rs:10:5 | LL | Maz.faz(); @@ -29,7 +29,7 @@ note: danger `may_deadlock` declared here LL | #[clippy::dangerous(may_deadlock)] | ^^^^^^^^^^^^ -error: called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]` +error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]` --> $DIR/danger_not_accepted.rs:13:5 | LL | Maz.faz(); @@ -41,7 +41,7 @@ note: danger `may_delete_system` declared here LL | #[clippy::dangerous(may_delete_system)] | ^^^^^^^^^^^^^^^^^ -error: called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]` +error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]` --> $DIR/danger_not_accepted.rs:18:5 | LL | Maz.faz2(); @@ -53,7 +53,7 @@ note: danger `may_delete_system` declared here LL | #[clippy::dangerous(may_delete_system)] | ^^^^^^^^^^^^^^^^^ -error: called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]` +error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]` --> $DIR/danger_not_accepted.rs:21:5 | LL | Maz.faz2(); @@ -65,7 +65,7 @@ note: danger `may_delete_system` declared here LL | #[clippy::dangerous(may_delete_system)] | ^^^^^^^^^^^^^^^^^ -error: called a method marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_deadlock)]` +error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_deadlock)]` --> $DIR/danger_not_accepted.rs:26:5 | LL | waz::woo2(); From 2fa90b9f7816c534fef2f36ef4b2d919aaa345ea Mon Sep 17 00:00:00 2001 From: Radbuglet <10578060+Radbuglet@users.noreply.github.com> Date: Mon, 2 Oct 2023 14:27:24 -0600 Subject: [PATCH 09/16] Clarify wording in future improvements section --- clippy_lints/src/danger_not_accepted.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/danger_not_accepted.rs b/clippy_lints/src/danger_not_accepted.rs index 0902cbe9f911..db921db11460 100644 --- a/clippy_lints/src/danger_not_accepted.rs +++ b/clippy_lints/src/danger_not_accepted.rs @@ -14,9 +14,9 @@ use rustc_span::{Span, Symbol}; // - Allow users to override modules as *not* having a specific danger. // - Allow users to specify additional dangerous items in the clippy config. // - Devise a scheme (maybe path compression?) to reduce the amount of ancestry tracing we have to -// do to determine the dangers posed by a method? +// do to determine the dangers posed by a method. // - Implement a way to forbid `accept_danger` in a given module. -// - Allow `accept_danger` and `dangerous` as internal attributes on stable Rust? +// - Allow `accept_danger` and `dangerous` to be used as inner attributes on stable Rust. // declare_clippy_lint! { From e39dd242ada64904fd082140a811f6147a37df78 Mon Sep 17 00:00:00 2001 From: Radbuglet <10578060+Radbuglet@users.noreply.github.com> Date: Mon, 2 Oct 2023 16:14:18 -0600 Subject: [PATCH 10/16] Fix doc-tests --- clippy_lints/src/danger_not_accepted.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/danger_not_accepted.rs b/clippy_lints/src/danger_not_accepted.rs index db921db11460..744675e8709d 100644 --- a/clippy_lints/src/danger_not_accepted.rs +++ b/clippy_lints/src/danger_not_accepted.rs @@ -41,8 +41,9 @@ declare_clippy_lint! { /// /// ### Example /// ```rust + /// # fn main() {} // Ensures that doc-test doesn't automatically wrap us in a main function. /// #[clippy::dangerous(use_of_lib_1_dangerous_module)] - /// mod dangerous_module { + /// pub mod dangerous_module { /// # fn break_the_program() {} /// #[clippy::dangerous(may_break_program)] /// pub fn do_something_innocuous_looking() { @@ -50,17 +51,18 @@ declare_clippy_lint! { /// } /// } /// - /// use unsuspecting_module { + /// pub mod unsuspecting_module { /// fn do_something() { /// // This function call causes clippy to issue a warning - /// dangerous_module::do_something_innocuous_looking(); + /// crate::dangerous_module::do_something_innocuous_looking(); /// } /// } /// ``` /// Use instead: /// ```rust + /// # fn main() {} // Ensures that doc-test doesn't automatically wrap us in a main function. /// #[clippy::dangerous(use_of_lib_1_dangerous_module)] - /// mod dangerous_module { + /// pub mod dangerous_module { /// # fn break_the_program() {} /// #[clippy::dangerous(may_break_program)] /// pub fn do_something_innocuous_looking() { @@ -70,11 +72,11 @@ declare_clippy_lint! { /// /// // This entire module can use functions with the danger `use_of_lib_1_dangerous_module`. /// #[clippy::accept_danger(use_of_lib_1_dangerous_module)] - /// use unsuspecting_module { + /// pub mod unsuspecting_module { /// fn do_something() { /// // Only this statement can call functions with the danger `may_break_program`. /// #[clippy::accept_danger(may_break_program)] - /// dangerous_module::do_something_innocuous_looking(); + /// crate::dangerous_module::do_something_innocuous_looking(); /// } /// } /// ``` From a01a03a46fdb2ae50933cd3412580f251b67353b Mon Sep 17 00:00:00 2001 From: Radbuglet <10578060+Radbuglet@users.noreply.github.com> Date: Wed, 11 Oct 2023 08:37:57 -0700 Subject: [PATCH 11/16] allow `#[dangerous]` to specify a `reason` --- clippy_lints/src/danger_not_accepted.rs | 190 +++++++++++++++++++----- tests/ui/danger_not_accepted.rs | 26 +++- tests/ui/danger_not_accepted.stderr | 86 +++++++++-- 3 files changed, 248 insertions(+), 54 deletions(-) diff --git a/clippy_lints/src/danger_not_accepted.rs b/clippy_lints/src/danger_not_accepted.rs index 744675e8709d..30dde165034a 100644 --- a/clippy_lints/src/danger_not_accepted.rs +++ b/clippy_lints/src/danger_not_accepted.rs @@ -11,12 +11,14 @@ use rustc_span::{Span, Symbol}; // Future improvements: // -// - Allow users to override modules as *not* having a specific danger. +// - Allow users to override modules as *not* posing a specific danger. // - Allow users to specify additional dangerous items in the clippy config. // - Devise a scheme (maybe path compression?) to reduce the amount of ancestry tracing we have to // do to determine the dangers posed by a method. -// - Implement a way to forbid `accept_danger` in a given module. -// - Allow `accept_danger` and `dangerous` to be used as inner attributes on stable Rust. +// - Implement a way to forbid additional `accept_danger` calls in a given module. +// - Allow `accept_danger` and `dangerous` to be used as inner attributes on stable Rust. As +// discussed, this would likely involve granting the clippy attribute namespace special status, +// similar to `#[allow(...)]`. // declare_clippy_lint! { @@ -124,7 +126,10 @@ impl LateLintPass<'_> for DangerNotAccepted { // Both `accept_danger` and `dangerous` contribute to the accepted danger map. for attr_name in ["accept_danger", "dangerous"] { for attr in get_attr(cx.sess(), attrs, attr_name) { - let dangers = parse_dangers_attr(cx, attr); + // We don't really care about why a user might have accepted a danger or marked a + // section as dangerous. + let (dangers, _ignored_reason) = parse_dangers_attr(cx, attr); + for (_span, danger) in dangers { *self.accepted_dangers.entry(danger).or_default() += 1; } @@ -136,7 +141,7 @@ impl LateLintPass<'_> for DangerNotAccepted { // Both `accept_danger` and `dangerous` contribute to the accepted danger map. for attr_name in ["accept_danger", "dangerous"] { for attr in get_attr(cx.sess(), attrs, attr_name) { - let dangers = parse_dangers_attr(cx, attr); + let (dangers, _ignored_reason) = parse_dangers_attr(cx, attr); for (_span, danger) in dangers { match self.accepted_dangers.entry(danger) { StdEntry::Occupied(mut entry) => { @@ -153,27 +158,38 @@ impl LateLintPass<'_> for DangerNotAccepted { } } +struct UnacceptedDanger { + span: Span, + id: Symbol, + reason: Option, +} + impl DangerNotAccepted { - fn get_unaccepted_dangers(&self, cx: &LateContext<'_>, item_id: def_id::DefId) -> Option> { + fn get_unaccepted_dangers(&self, cx: &LateContext<'_>, item_id: def_id::DefId) -> Option> { let mut unaccepted_dangers = Vec::new(); let mut item_iter = Some(item_id); while let Some(item_id) = item_iter { item_iter = cx.tcx.opt_parent(item_id); - // HACK: Ensure that this is not an intrinsic because calling `get_attrs_unchecked` on - // a foreign module breaks everything. + // HACK: Ensure that this is not a foreign module because calling `get_attrs_unchecked` on + // an intrinsic foreign module breaks everything. if cx.tcx.def_kind(item_id) == def::DefKind::ForeignMod { continue; } for attr in get_attr(cx.sess(), cx.tcx.get_attrs_unchecked(item_id), "dangerous") { - for danger in parse_dangers_attr(cx, attr) { - if self.accepted_dangers.contains_key(&danger.1) { + let (dangers, danger_reason) = parse_dangers_attr(cx, attr); + for (danger_span, danger_id) in dangers { + if self.accepted_dangers.contains_key(&danger_id) { continue; } - unaccepted_dangers.push(danger); + unaccepted_dangers.push(UnacceptedDanger { + span: danger_span, + id: danger_id, + reason: danger_reason, + }); } } } @@ -182,64 +198,146 @@ impl DangerNotAccepted { } } -fn parse_dangers_attr(cx: &LateContext<'_>, attr: &ast::Attribute) -> Vec<(Span, Symbol)> { - const EXPECTATION: &str = "Expected a delimited attribute with a list of danger identifiers."; +fn parse_dangers_attr(cx: &LateContext<'_>, attr: &ast::Attribute) -> (Vec<(Span, Symbol)>, Option) { + const EXPECTATION: &str = "expected a delimited attribute with a list of danger identifiers"; + const NOTHING_AFTER_ERR: &str = "nothing should come after the reason attribute besides an optional comma"; let span = attr.span; + let reason_sym = sym!(reason); + // Expect a normal non doc-comment attribute. let rustc_ast::AttrKind::Normal(attr) = &attr.kind else { cx.sess().span_err(span, EXPECTATION); - return Vec::new(); + return (Vec::new(), None); }; + // Expect it to be a delimited attribute of the form #[attr(...)] and not #[attr {...}] let ast::AttrArgs::Delimited(attr) = &attr.item.args else { cx.sess().span_err(span, EXPECTATION); - return Vec::new(); + return (Vec::new(), None); }; if attr.delim != token::Delimiter::Parenthesis { cx.sess().span_err(span, EXPECTATION); - return Vec::new(); + return (Vec::new(), None); } + // Parse the attribute arguments let mut stream = attr.tokens.trees(); let mut dangers = Vec::new(); + let mut specified_reason = None; loop { + // Expect an identifier let sym = match stream.next() { - Some(tokenstream::TokenTree::Token(sym, _)) => sym, + Some(tokenstream::TokenTree::Token(sym, _)) if let Some((sym, _)) = sym.ident() => sym, + // An EOS is also valid here. None => break, + + // Otherwise, raise an error. + Some(tokenstream::TokenTree::Token(sym, _)) => { + cx.sess().span_err(sym.span, format!("{EXPECTATION}; this was not an identifier")); + + return (Vec::new(), None); + }, _ => { cx.sess().span_err(span, EXPECTATION); - return Vec::new(); + return (Vec::new(), None); }, }; - let Some((sym, _)) = sym.ident() else { - cx.sess().span_err(span, EXPECTATION); - return Vec::new(); - }; + // If the identifier is not "reason", add it as a danger + #[allow(clippy::if_not_else, reason = "it is clearer to put the common path first")] + if sym.name != reason_sym { + // Push it to the danger list + dangers.push((sym.span, sym.name)); - dangers.push((sym.span, sym.name)); + // If we find a comma, continue. If we find an EOS for the inner stream, break. + match stream.next() { + // If we find an equality sign, continue. + Some(tokenstream::TokenTree::Token(sym, _)) if sym.kind == token::TokenKind::Comma => { + continue; + }, + None => break, + // Otherwise, raise an error. + Some(tokenstream::TokenTree::Token(sym, _)) => { + cx.sess() + .span_err(sym.span, format!("{EXPECTATION}; this was not a comma delimiter")); + }, + _ => { + cx.sess().span_err(span, EXPECTATION); + return (Vec::new(), None); + }, + } + } else { + // If the identifier was "reason", expect an equality sign. + let eq_tok = match stream.next() { + // If we find a comma, continue. + Some(tokenstream::TokenTree::Token(tok, _)) if tok.kind == token::TokenKind::Eq => tok, + // Otherwise, raise an error. + Some(tokenstream::TokenTree::Token(tok, _)) => { + cx.sess().span_err(tok.span, "expected = after a reason attribute"); + return (Vec::new(), None); + }, + _ => { + cx.sess().span_err(sym.span, "expected = after a reason attribute"); + return (Vec::new(), None); + }, + }; - match stream.next() { - Some(tokenstream::TokenTree::Token(sym, _)) if sym.kind == token::TokenKind::Comma => { - continue; - }, - None => { - break; - }, - _ => { - cx.sess().span_err(span, EXPECTATION); - return Vec::new(); - }, + // ...then, expect a string literal. + let (reason_tok, reason_lit) = match stream.next() { + // If we find a string literal, continue. + Some(tokenstream::TokenTree::Token(tok, _)) + if + let token::TokenKind::Literal(lit) = tok.kind && + // We don't admit byte string literals because they wouldn't be `&str`s in regular + // scenarios so why treat them as strings here? + let token::LitKind::Str | token::LitKind::StrRaw(_) | + token::LitKind::CStr | token::LitKind::CStrRaw(_) = lit.kind + => (tok, lit), + // Otherwise, raise an error. + Some(tokenstream::TokenTree::Token(tok, _)) => { + cx.sess().span_err(tok.span, "expected a string literal after a reason attribute"); + return (Vec::new(), None); + }, + _ => { + cx.sess().span_err(eq_tok.span, "expected a string literal after a reason attribute"); + return (Vec::new(), None); + }, + }; + + specified_reason = Some(reason_lit); + + // Finally, because the reason must be specified as the last attribute, expect either an + // optional comma or an EOS and break. + match stream.next() { + Some(tokenstream::TokenTree::Token(tok, _)) if tok.kind == token::TokenKind::Comma => { + // The token after the comma must be an EOS + if stream.next().is_some() { + cx.sess().span_err(tok.span, NOTHING_AFTER_ERR); + } + }, + None => {}, + // Otherwise, raise an error. + Some(tokenstream::TokenTree::Token(tok, _)) => { + cx.sess().span_err(tok.span, NOTHING_AFTER_ERR); + return (Vec::new(), None); + }, + _ => { + cx.sess().span_err(reason_tok.span, NOTHING_AFTER_ERR); + return (Vec::new(), None); + }, + } + + break; } } - dangers + (dangers, specified_reason) } -fn emit_dangerous_call_lint(cx: &LateContext<'_>, expr: &'_ Expr<'_>, unaccepted_dangers: &[(Span, Symbol)]) { +fn emit_dangerous_call_lint(cx: &LateContext<'_>, expr: &'_ Expr<'_>, unaccepted_dangers: &[UnacceptedDanger]) { // Define formatting helpers struct FmtInline(F); @@ -260,7 +358,10 @@ fn emit_dangerous_call_lint(cx: &LateContext<'_>, expr: &'_ Expr<'_>, unaccepted } // Collect all unique dangers - let unique_dangers = unaccepted_dangers.iter().map(|(_, sym)| sym).collect::>(); + let unique_dangers = unaccepted_dangers + .iter() + .map(|danger| danger.id) + .collect::>(); // Create a lint span_lint_and_then( @@ -283,8 +384,19 @@ fn emit_dangerous_call_lint(cx: &LateContext<'_>, expr: &'_ Expr<'_>, unaccepted }), ), |diag| { - for (danger_span, danger_name) in unaccepted_dangers { - diag.span_note(*danger_span, format!("danger `{danger_name}` declared here")); + for danger in unaccepted_dangers { + if let Some(reason) = danger.reason { + diag.span_note( + danger.span, + format!( + "danger `{}` declared here with the justification `{}`", + danger.id, + reason.symbol.as_str(), + ), + ); + } else { + diag.span_note(danger.span, format!("danger `{}` declared here", danger.id)); + } } }, ); diff --git a/tests/ui/danger_not_accepted.rs b/tests/ui/danger_not_accepted.rs index ee3a6551c9d4..be3de842294c 100644 --- a/tests/ui/danger_not_accepted.rs +++ b/tests/ui/danger_not_accepted.rs @@ -37,11 +37,14 @@ struct Maz; mod waz { pub fn woo() {} - #[clippy::dangerous(may_deadlock)] + #[clippy::dangerous(may_deadlock, reason = "your program may deadlock in calling this function")] pub fn woo2() {} impl super::Maz { - #[clippy::dangerous(may_delete_system)] + #[clippy::dangerous( + may_delete_system, + reason = "calling this has a very strong chance of just deleting your computer" + )] pub fn faz(&self) {} } @@ -54,3 +57,22 @@ trait FazTrait { #[clippy::dangerous(may_delete_system)] fn faz2(&self); } + +// Edge case attr tests +#[rustfmt::skip] +#[clippy::dangerous(whee, woo,)] +#[clippy::dangerous(whee, woo, reason = "sdfhsdf",)] +fn dummy_1() {} + +// Invalid attr tests +#[clippy::dangerous{}] +#[clippy::dangerous[]] +#[clippy::dangerous(,)] +#[clippy::dangerous(whee, reason)] +#[clippy::dangerous(whee, reason, abc)] +#[clippy::dangerous(whee, reason =)] +#[clippy::dangerous(whee, reason =, weh)] +#[clippy::dangerous(whee, reason = "", weh)] +#[clippy::dangerous(whee, reason = "" weh)] +#[clippy::dangerous(whee, reason = 4)] +fn dummy_2() {} diff --git a/tests/ui/danger_not_accepted.stderr b/tests/ui/danger_not_accepted.stderr index 45287fbd7876..cd146b867854 100644 --- a/tests/ui/danger_not_accepted.stderr +++ b/tests/ui/danger_not_accepted.stderr @@ -18,11 +18,11 @@ error: called a function marked with `#[clippy::dangerous(...)]` without blessin LL | Maz.faz(); | ^^^^^^^^^ | -note: danger `may_delete_system` declared here - --> $DIR/danger_not_accepted.rs:44:29 +note: danger `may_delete_system` declared here with the justification `calling this has a very strong chance of just deleting your computer` + --> $DIR/danger_not_accepted.rs:45:13 | -LL | #[clippy::dangerous(may_delete_system)] - | ^^^^^^^^^^^^^^^^^ +LL | may_delete_system, + | ^^^^^^^^^^^^^^^^^ note: danger `may_deadlock` declared here --> $DIR/danger_not_accepted.rs:36:21 | @@ -35,11 +35,11 @@ error: called a function marked with `#[clippy::dangerous(...)]` without blessin LL | Maz.faz(); | ^^^^^^^^^ | -note: danger `may_delete_system` declared here - --> $DIR/danger_not_accepted.rs:44:29 +note: danger `may_delete_system` declared here with the justification `calling this has a very strong chance of just deleting your computer` + --> $DIR/danger_not_accepted.rs:45:13 | -LL | #[clippy::dangerous(may_delete_system)] - | ^^^^^^^^^^^^^^^^^ +LL | may_delete_system, + | ^^^^^^^^^^^^^^^^^ error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]` --> $DIR/danger_not_accepted.rs:18:5 @@ -48,7 +48,7 @@ LL | Maz.faz2(); | ^^^^^^^^^^ | note: danger `may_delete_system` declared here - --> $DIR/danger_not_accepted.rs:54:25 + --> $DIR/danger_not_accepted.rs:57:25 | LL | #[clippy::dangerous(may_delete_system)] | ^^^^^^^^^^^^^^^^^ @@ -60,7 +60,7 @@ LL | Maz.faz2(); | ^^^^^^^^^^ | note: danger `may_delete_system` declared here - --> $DIR/danger_not_accepted.rs:54:25 + --> $DIR/danger_not_accepted.rs:57:25 | LL | #[clippy::dangerous(may_delete_system)] | ^^^^^^^^^^^^^^^^^ @@ -71,10 +71,10 @@ error: called a function marked with `#[clippy::dangerous(...)]` without blessin LL | waz::woo2(); | ^^^^^^^^^ | -note: danger `may_deadlock` declared here +note: danger `may_deadlock` declared here with the justification `your program may deadlock in calling this function` --> $DIR/danger_not_accepted.rs:40:25 | -LL | #[clippy::dangerous(may_deadlock)] +LL | #[clippy::dangerous(may_deadlock, reason = "your program may deadlock in calling this function")] | ^^^^^^^^^^^^ note: danger `may_deadlock` declared here --> $DIR/danger_not_accepted.rs:36:21 @@ -82,5 +82,65 @@ note: danger `may_deadlock` declared here LL | #[clippy::dangerous(may_deadlock)] | ^^^^^^^^^^^^ -error: aborting due to 6 previous errors +error: expected a delimited attribute with a list of danger identifiers + --> $DIR/danger_not_accepted.rs:68:1 + | +LL | #[clippy::dangerous{}] + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: expected a delimited attribute with a list of danger identifiers + --> $DIR/danger_not_accepted.rs:69:1 + | +LL | #[clippy::dangerous[]] + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: expected a delimited attribute with a list of danger identifiers; this was not an identifier + --> $DIR/danger_not_accepted.rs:70:21 + | +LL | #[clippy::dangerous(,)] + | ^ + +error: expected = after a reason attribute + --> $DIR/danger_not_accepted.rs:71:27 + | +LL | #[clippy::dangerous(whee, reason)] + | ^^^^^^ + +error: expected = after a reason attribute + --> $DIR/danger_not_accepted.rs:72:33 + | +LL | #[clippy::dangerous(whee, reason, abc)] + | ^ + +error: expected a string literal after a reason attribute + --> $DIR/danger_not_accepted.rs:73:34 + | +LL | #[clippy::dangerous(whee, reason =)] + | ^ + +error: expected a string literal after a reason attribute + --> $DIR/danger_not_accepted.rs:74:35 + | +LL | #[clippy::dangerous(whee, reason =, weh)] + | ^ + +error: nothing should come after the reason attribute besides an optional comma + --> $DIR/danger_not_accepted.rs:75:38 + | +LL | #[clippy::dangerous(whee, reason = "", weh)] + | ^ + +error: nothing should come after the reason attribute besides an optional comma + --> $DIR/danger_not_accepted.rs:76:39 + | +LL | #[clippy::dangerous(whee, reason = "" weh)] + | ^^^ + +error: expected a string literal after a reason attribute + --> $DIR/danger_not_accepted.rs:77:36 + | +LL | #[clippy::dangerous(whee, reason = 4)] + | ^ + +error: aborting due to 16 previous errors From 2c6005134af07274ad78c02ca8858e2573de1ae6 Mon Sep 17 00:00:00 2001 From: Radbuglet <10578060+Radbuglet@users.noreply.github.com> Date: Wed, 11 Oct 2023 08:54:20 -0700 Subject: [PATCH 12/16] Address internal dog-food lints --- clippy_lints/src/danger_not_accepted.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/danger_not_accepted.rs b/clippy_lints/src/danger_not_accepted.rs index 30dde165034a..fdc17a3775c7 100644 --- a/clippy_lints/src/danger_not_accepted.rs +++ b/clippy_lints/src/danger_not_accepted.rs @@ -7,7 +7,7 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet, StdEntry}; use rustc_hir::{def, def_id, Expr, ExprKind, QPath}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::{Span, Symbol}; +use rustc_span::{sym, Span, Symbol}; // Future improvements: // @@ -203,7 +203,6 @@ fn parse_dangers_attr(cx: &LateContext<'_>, attr: &ast::Attribute) -> (Vec<(Span const NOTHING_AFTER_ERR: &str = "nothing should come after the reason attribute besides an optional comma"; let span = attr.span; - let reason_sym = sym!(reason); // Expect a normal non doc-comment attribute. let rustc_ast::AttrKind::Normal(attr) = &attr.kind else { @@ -248,7 +247,7 @@ fn parse_dangers_attr(cx: &LateContext<'_>, attr: &ast::Attribute) -> (Vec<(Span // If the identifier is not "reason", add it as a danger #[allow(clippy::if_not_else, reason = "it is clearer to put the common path first")] - if sym.name != reason_sym { + if sym.name != sym::reason { // Push it to the danger list dangers.push((sym.span, sym.name)); From 77f708024c80494b740deff0bf42bdfb6fe8130e Mon Sep 17 00:00:00 2001 From: Radbuglet <10578060+Radbuglet@users.noreply.github.com> Date: Wed, 1 Nov 2023 11:20:30 -0600 Subject: [PATCH 13/16] Simplify diagnostic formatting logic --- clippy_lints/src/danger_not_accepted.rs | 34 ++----------------------- 1 file changed, 2 insertions(+), 32 deletions(-) diff --git a/clippy_lints/src/danger_not_accepted.rs b/clippy_lints/src/danger_not_accepted.rs index fdc17a3775c7..255a961a08f3 100644 --- a/clippy_lints/src/danger_not_accepted.rs +++ b/clippy_lints/src/danger_not_accepted.rs @@ -1,5 +1,3 @@ -use std::fmt; - use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::get_attr; use rustc_ast::{ast, token, tokenstream}; @@ -337,25 +335,6 @@ fn parse_dangers_attr(cx: &LateContext<'_>, attr: &ast::Attribute) -> (Vec<(Span } fn emit_dangerous_call_lint(cx: &LateContext<'_>, expr: &'_ Expr<'_>, unaccepted_dangers: &[UnacceptedDanger]) { - // Define formatting helpers - struct FmtInline(F); - - impl fmt::Display for FmtInline - where - F: Fn(&mut fmt::Formatter<'_>) -> fmt::Result, - { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.0(f) - } - } - - fn fmt_inline(f: F) -> FmtInline - where - F: Fn(&mut fmt::Formatter<'_>) -> fmt::Result, - { - FmtInline(f) - } - // Collect all unique dangers let unique_dangers = unaccepted_dangers .iter() @@ -370,17 +349,8 @@ fn emit_dangerous_call_lint(cx: &LateContext<'_>, expr: &'_ Expr<'_>, unaccepted &format!( "called a function marked with `#[clippy::dangerous(...)]` without blessing the calling \ module with `#![clippy::accept_danger({})]`", - fmt_inline(|f| { - let mut is_subsequent = false; - for danger in &unique_dangers { - if is_subsequent { - f.write_str(", ")?; - } - is_subsequent = true; - f.write_str(danger.as_str())?; - } - Ok(()) - }), + // This redundant allocation is okay because it's on the cold path. + unique_dangers.iter().map(Symbol::as_str).collect::>().join(", "), ), |diag| { for danger in unaccepted_dangers { From e8929c6f76197fe0dbb578bb762e97edbcdff1ee Mon Sep 17 00:00:00 2001 From: Radbuglet <10578060+Radbuglet@users.noreply.github.com> Date: Wed, 1 Nov 2023 11:35:21 -0600 Subject: [PATCH 14/16] Make order of dangers in diagnostics consistent --- clippy_lints/src/danger_not_accepted.rs | 7 +++++-- tests/ui/danger_not_accepted.stderr | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/danger_not_accepted.rs b/clippy_lints/src/danger_not_accepted.rs index 255a961a08f3..de61a98040d4 100644 --- a/clippy_lints/src/danger_not_accepted.rs +++ b/clippy_lints/src/danger_not_accepted.rs @@ -349,8 +349,11 @@ fn emit_dangerous_call_lint(cx: &LateContext<'_>, expr: &'_ Expr<'_>, unaccepted &format!( "called a function marked with `#[clippy::dangerous(...)]` without blessing the calling \ module with `#![clippy::accept_danger({})]`", - // This redundant allocation is okay because it's on the cold path. - unique_dangers.iter().map(Symbol::as_str).collect::>().join(", "), + { + let mut danger_list = unique_dangers.iter().map(Symbol::as_str).collect::>(); + danger_list.sort_unstable(); + danger_list.join(", ") + }, ), |diag| { for danger in unaccepted_dangers { diff --git a/tests/ui/danger_not_accepted.stderr b/tests/ui/danger_not_accepted.stderr index cd146b867854..6a3065b3e470 100644 --- a/tests/ui/danger_not_accepted.stderr +++ b/tests/ui/danger_not_accepted.stderr @@ -12,7 +12,7 @@ LL | #[clippy::dangerous(may_deadlock)] = note: `-D clippy::danger-not-accepted` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::danger_not_accepted)]` -error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system, may_deadlock)]` +error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_deadlock, may_delete_system)]` --> $DIR/danger_not_accepted.rs:10:5 | LL | Maz.faz(); From 9d732adbe0b2c54250310401a61557d3f2467a68 Mon Sep 17 00:00:00 2001 From: Radbuglet <10578060+Radbuglet@users.noreply.github.com> Date: Fri, 1 Dec 2023 21:58:33 -0700 Subject: [PATCH 15/16] Update `#[dangerous]` attribute reason syntax --- clippy_lints/src/danger_not_accepted.rs | 757 ++++++++++++++++++------ tests/ui/danger_not_accepted.rs | 22 +- tests/ui/danger_not_accepted.stderr | 300 ++++++++-- 3 files changed, 848 insertions(+), 231 deletions(-) diff --git a/clippy_lints/src/danger_not_accepted.rs b/clippy_lints/src/danger_not_accepted.rs index de61a98040d4..d8480c14efb4 100644 --- a/clippy_lints/src/danger_not_accepted.rs +++ b/clippy_lints/src/danger_not_accepted.rs @@ -1,11 +1,10 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::get_attr; -use rustc_ast::{ast, token, tokenstream}; use rustc_data_structures::fx::{FxHashMap, FxHashSet, StdEntry}; use rustc_hir::{def, def_id, Expr, ExprKind, QPath}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::{sym, Span, Symbol}; +use rustc_span::{Span, Symbol}; // Future improvements: // @@ -122,35 +121,42 @@ impl LateLintPass<'_> for DangerNotAccepted { fn enter_lint_attrs(&mut self, cx: &LateContext<'_>, attrs: &'_ [rustc_ast::Attribute]) { // Both `accept_danger` and `dangerous` contribute to the accepted danger map. - for attr_name in ["accept_danger", "dangerous"] { - for attr in get_attr(cx.sess(), attrs, attr_name) { - // We don't really care about why a user might have accepted a danger or marked a - // section as dangerous. - let (dangers, _ignored_reason) = parse_dangers_attr(cx, attr); - - for (_span, danger) in dangers { - *self.accepted_dangers.entry(danger).or_default() += 1; - } + let mut inc = |id| *self.accepted_dangers.entry(id).or_default() += 1; + + for attr in get_attr(cx.sess(), attrs, "accept_danger") { + for (_span, danger) in parsing::parse_single_reason_danger_list_attr(cx, attr).0 { + inc(danger); + } + } + + for attr in get_attr(cx.sess(), attrs, "dangerous") { + for (_span, danger, _ignored_reason) in parsing::parse_individually_reasoned_danger_list_attr(cx, attr) { + inc(danger); } } } fn exit_lint_attrs(&mut self, cx: &LateContext<'_>, attrs: &'_ [rustc_ast::Attribute]) { // Both `accept_danger` and `dangerous` contribute to the accepted danger map. - for attr_name in ["accept_danger", "dangerous"] { - for attr in get_attr(cx.sess(), attrs, attr_name) { - let (dangers, _ignored_reason) = parse_dangers_attr(cx, attr); - for (_span, danger) in dangers { - match self.accepted_dangers.entry(danger) { - StdEntry::Occupied(mut entry) => { - *entry.get_mut() -= 1; - if *entry.get() == 0 { - entry.remove(); - } - }, - StdEntry::Vacant(_) => unreachable!(), - } + let mut dec = |id| match self.accepted_dangers.entry(id) { + StdEntry::Occupied(mut entry) => { + *entry.get_mut() -= 1; + if *entry.get() == 0 { + entry.remove(); } + }, + StdEntry::Vacant(_) => unreachable!(), + }; + + for attr in get_attr(cx.sess(), attrs, "accept_danger") { + for (_span, danger) in parsing::parse_single_reason_danger_list_attr(cx, attr).0 { + dec(danger); + } + } + + for attr in get_attr(cx.sess(), attrs, "dangerous") { + for (_span, danger, _ignored_reason) in parsing::parse_individually_reasoned_danger_list_attr(cx, attr) { + dec(danger); } } } @@ -159,7 +165,7 @@ impl LateLintPass<'_> for DangerNotAccepted { struct UnacceptedDanger { span: Span, id: Symbol, - reason: Option, + reason: Symbol, } impl DangerNotAccepted { @@ -177,8 +183,9 @@ impl DangerNotAccepted { } for attr in get_attr(cx.sess(), cx.tcx.get_attrs_unchecked(item_id), "dangerous") { - let (dangers, danger_reason) = parse_dangers_attr(cx, attr); - for (danger_span, danger_id) in dangers { + for (danger_span, danger_id, danger_reason) in + parsing::parse_individually_reasoned_danger_list_attr(cx, attr) + { if self.accepted_dangers.contains_key(&danger_id) { continue; } @@ -196,180 +203,586 @@ impl DangerNotAccepted { } } -fn parse_dangers_attr(cx: &LateContext<'_>, attr: &ast::Attribute) -> (Vec<(Span, Symbol)>, Option) { - const EXPECTATION: &str = "expected a delimited attribute with a list of danger identifiers"; - const NOTHING_AFTER_ERR: &str = "nothing should come after the reason attribute besides an optional comma"; +fn emit_dangerous_call_lint(cx: &LateContext<'_>, expr: &'_ Expr<'_>, unaccepted_dangers: &[UnacceptedDanger]) { + // Collect all unique dangers + let unique_dangers = unaccepted_dangers + .iter() + .map(|danger| danger.id) + .collect::>(); - let span = attr.span; + // Create a lint + span_lint_and_then( + cx, + DANGER_NOT_ACCEPTED, + expr.span, + &format!( + "called a function marked with `#[clippy::dangerous(...)]` without blessing the calling \ + module with `#![clippy::accept_danger({})]`", + { + let mut danger_list = unique_dangers.iter().map(Symbol::as_str).collect::>(); + danger_list.sort_unstable(); + danger_list.join(", ") + }, + ), + |diag| { + for danger in unaccepted_dangers { + diag.span_note( + danger.span, + format!( + "danger `{}` declared here with the justification `{}`", + danger.id, + danger.reason.as_str(), + ), + ); + } + }, + ); +} - // Expect a normal non doc-comment attribute. - let rustc_ast::AttrKind::Normal(attr) = &attr.kind else { - cx.sess().span_err(span, EXPECTATION); - return (Vec::new(), None); - }; +// === Parsing === // + +// I had a feeling this is going to change a lot so I built some actual parser infrastructure... +mod parsing { + use rustc_ast::ast::Attribute; + use rustc_ast::token::{Delimiter, Lit, LitKind, Token, TokenKind}; + use rustc_ast::tokenstream::{DelimSpan, RefTokenTreeCursor, TokenStream, TokenTree}; + use rustc_ast::{AttrArgs, AttrKind}; + use rustc_data_structures::fx::FxHashSet; + use rustc_errors::DiagnosticMessage; + use rustc_lint::{LateContext, LintContext}; + use rustc_session::Session; + use rustc_span::{sym, Span, Symbol}; + use std::cell::{Cell, RefCell}; + + const RESERVED_PREFIXES: [&str; 9] = [ + "rust", "rustc", "clippy", "core", "std", "common", "mem", "race", "sync", + ]; + + const RESERVED_DANGERS: [&str; 9] = [ + "reason", + "justification", + "cfg", + "edition", + "version", + "since", + "author", + "history", + "panics", + ]; + + // === Core === // + + // LookaheadResult + trait LookaheadResult { + fn is_truthy(&self) -> bool; + } - // Expect it to be a delimited attribute of the form #[attr(...)] and not #[attr {...}] - let ast::AttrArgs::Delimited(attr) = &attr.item.args else { - cx.sess().span_err(span, EXPECTATION); - return (Vec::new(), None); - }; + impl LookaheadResult for bool { + fn is_truthy(&self) -> bool { + *self + } + } - if attr.delim != token::Delimiter::Parenthesis { - cx.sess().span_err(span, EXPECTATION); - return (Vec::new(), None); + impl LookaheadResult for Option { + fn is_truthy(&self) -> bool { + self.is_some() + } } - // Parse the attribute arguments - let mut stream = attr.tokens.trees(); - let mut dangers = Vec::new(); - let mut specified_reason = None; + impl LookaheadResult for Result { + fn is_truthy(&self) -> bool { + self.is_ok() + } + } - loop { - // Expect an identifier - let sym = match stream.next() { - Some(tokenstream::TokenTree::Token(sym, _)) if let Some((sym, _)) = sym.ident() => sym, - // An EOS is also valid here. - None => break, + // ParseContext + struct ParseContext<'s> { + rustc_session: &'s Session, + while_parsing: RefCell>, + got_stuck: Cell, + } - // Otherwise, raise an error. - Some(tokenstream::TokenTree::Token(sym, _)) => { - cx.sess().span_err(sym.span, format!("{EXPECTATION}; this was not an identifier")); + #[must_use] + struct WhileParsingGuard<'c> { + cx: &'c ParseContext<'c>, + top: Symbol, + } - return (Vec::new(), None); - }, - _ => { - cx.sess().span_err(span, EXPECTATION); - return (Vec::new(), None); - }, - }; + impl Drop for WhileParsingGuard<'_> { + fn drop(&mut self) { + let popped = self.cx.while_parsing.borrow_mut().pop(); + debug_assert_eq!(popped, Some(self.top)); + } + } - // If the identifier is not "reason", add it as a danger - #[allow(clippy::if_not_else, reason = "it is clearer to put the common path first")] - if sym.name != sym::reason { - // Push it to the danger list - dangers.push((sym.span, sym.name)); + impl<'s> ParseContext<'s> { + fn new(rustc_session: &'s Session) -> Self { + Self { + rustc_session, + while_parsing: RefCell::new(Vec::new()), + got_stuck: Cell::new(false), + } + } - // If we find a comma, continue. If we find an EOS for the inner stream, break. - match stream.next() { - // If we find an equality sign, continue. - Some(tokenstream::TokenTree::Token(sym, _)) if sym.kind == token::TokenKind::Comma => { - continue; - }, - None => break, - // Otherwise, raise an error. - Some(tokenstream::TokenTree::Token(sym, _)) => { - cx.sess() - .span_err(sym.span, format!("{EXPECTATION}; this was not a comma delimiter")); - }, - _ => { - cx.sess().span_err(span, EXPECTATION); - return (Vec::new(), None); + fn enter<'c, 't>(&'c self, span: DelimSpan, stream: &'t TokenStream) -> ParseSequence<'c, 't> { + ParseSequence { + context: self, + cursor: ParseCursor { + raw: stream.trees(), + span, }, + expectations: Vec::new(), } - } else { - // If the identifier was "reason", expect an equality sign. - let eq_tok = match stream.next() { - // If we find a comma, continue. - Some(tokenstream::TokenTree::Token(tok, _)) if tok.kind == token::TokenKind::Eq => tok, - // Otherwise, raise an error. - Some(tokenstream::TokenTree::Token(tok, _)) => { - cx.sess().span_err(tok.span, "expected = after a reason attribute"); - return (Vec::new(), None); - }, - _ => { - cx.sess().span_err(sym.span, "expected = after a reason attribute"); - return (Vec::new(), None); - }, + } + + fn while_parsing(&self, what: Symbol) -> WhileParsingGuard<'_> { + self.while_parsing.borrow_mut().push(what); + + WhileParsingGuard { cx: self, top: what } + } + + fn got_stuck(&self) -> bool { + self.got_stuck.get() + } + } + + // ParseSequence + struct ParseSequence<'c, 't> { + context: &'c ParseContext<'c>, + cursor: ParseCursor<'t>, + expectations: Vec, + } + + impl<'c, 't> ParseSequence<'c, 't> { + // fn enter<'t2>(&self, span: DelimSpan, stream: &'t2 TokenStream) -> ParseSequence<'c, 't2> { + // self.context.enter(span, stream) + // } + + fn while_parsing(&self, what: Symbol) -> WhileParsingGuard<'c> { + self.context.while_parsing(what) + } + + fn expect(&mut self, expectation: Symbol, f: impl FnOnce(&mut ParseCursor<'t>) -> R) -> R { + let res = self.cursor.lookahead(|c| f(c)); + if res.is_truthy() { + self.expectations.clear(); + } else { + self.expectations.push(expectation); + } + res + } + + fn stuck(&mut self, recover: impl FnOnce(&mut ParseCursor<'t>)) { + // Mark that we got stuck + self.context.got_stuck.set(true); + + // Emit the error message + let span = self.cursor.next_span(); + + let expectations = self.expectations.iter().copied().collect::>(); + let mut expectations = expectations.iter().map(Symbol::as_str).collect::>(); + expectations.sort_unstable(); + + let expectations = expectations.join(", "); + + let while_parsing = { + let stack = self.context.while_parsing.borrow(); + if stack.is_empty() { + String::new() + } else { + format!( + " while parsing {}", + stack.iter().rev().map(Symbol::as_str).collect::>().join(" in ") + ) + } }; - // ...then, expect a string literal. - let (reason_tok, reason_lit) = match stream.next() { - // If we find a string literal, continue. - Some(tokenstream::TokenTree::Token(tok, _)) - if - let token::TokenKind::Literal(lit) = tok.kind && - // We don't admit byte string literals because they wouldn't be `&str`s in regular - // scenarios so why treat them as strings here? - let token::LitKind::Str | token::LitKind::StrRaw(_) | - token::LitKind::CStr | token::LitKind::CStrRaw(_) = lit.kind - => (tok, lit), - // Otherwise, raise an error. - Some(tokenstream::TokenTree::Token(tok, _)) => { - cx.sess().span_err(tok.span, "expected a string literal after a reason attribute"); - return (Vec::new(), None); - }, - _ => { - cx.sess().span_err(eq_tok.span, "expected a string literal after a reason attribute"); - return (Vec::new(), None); + self.rustc_session() + .span_err(span, format!("expected {expectations}{while_parsing}")); + + // Attempt to get unstuck + recover(&mut self.cursor); + } + + fn error(&mut self, sp: Span, msg: impl Into, recover: impl FnOnce(&mut ParseCursor<'t>)) { + self.context.got_stuck.set(true); + self.rustc_session().span_err(sp, msg); + recover(&mut self.cursor); + } + + fn next_span(&self) -> Span { + self.cursor.next_span() + } + + fn rustc_session(&self) -> &'c Session { + self.context.rustc_session + } + } + + // ParseCursor + #[derive(Clone)] + struct ParseCursor<'t> { + raw: RefTokenTreeCursor<'t>, + span: DelimSpan, + } + + impl<'t> ParseCursor<'t> { + fn lookahead(&mut self, f: impl FnOnce(&mut Self) -> R) -> R { + let mut fork = self.clone(); + let res = f(&mut fork); + if res.is_truthy() { + *self = fork; + } + res + } + + fn consume(&mut self) -> Option<&'t TokenTree> { + self.raw.next() + } + + fn peek(&self) -> Option<&'t TokenTree> { + self.raw.clone().next() + } + + fn next_span(&self) -> Span { + self.peek().map_or(self.span.close, TokenTree::span) + } + } + + // === Helpers === // + + fn parse_eos(c: &mut ParseCursor<'_>) -> bool { + c.lookahead(|c| c.consume().is_none()) + } + + fn parse_turbo(c: &mut ParseCursor<'_>) -> Option { + c.lookahead(|c| { + if let Some(TokenTree::Token( + Token { + kind: TokenKind::ModSep, + span, }, - }; + _, + )) = c.consume() + { + Some(*span) + } else { + None + } + }) + } - specified_reason = Some(reason_lit); + fn parse_ident(c: &mut ParseCursor<'_>) -> Option<(Symbol, bool)> { + c.lookahead(|c| { + if let Some(TokenTree::Token( + Token { + kind: TokenKind::Ident(sym, raw), + .. + }, + _, + )) = c.consume() + { + Some((*sym, *raw)) + } else { + None + } + }) + } - // Finally, because the reason must be specified as the last attribute, expect either an - // optional comma or an EOS and break. - match stream.next() { - Some(tokenstream::TokenTree::Token(tok, _)) if tok.kind == token::TokenKind::Comma => { - // The token after the comma must be an EOS - if stream.next().is_some() { - cx.sess().span_err(tok.span, NOTHING_AFTER_ERR); - } + fn parse_comma(c: &mut ParseCursor<'_>) -> Option { + c.lookahead(|c| { + if let Some(TokenTree::Token( + Token { + kind: TokenKind::Comma, + span, }, - None => {}, - // Otherwise, raise an error. - Some(tokenstream::TokenTree::Token(tok, _)) => { - cx.sess().span_err(tok.span, NOTHING_AFTER_ERR); - return (Vec::new(), None); + _, + )) = c.consume() + { + Some(*span) + } else { + None + } + }) + } + + fn parse_equals(c: &mut ParseCursor<'_>) -> Option { + c.lookahead(|c| { + if let Some(TokenTree::Token( + Token { + kind: TokenKind::Eq, + span, }, - _ => { - cx.sess().span_err(reason_tok.span, NOTHING_AFTER_ERR); - return (Vec::new(), None); + _, + )) = c.consume() + { + Some(*span) + } else { + None + } + }) + } + + fn parse_str_lit(c: &mut ParseCursor<'_>) -> Option { + c.lookahead(|c| { + if let Some(TokenTree::Token( + Token { + kind: + TokenKind::Literal(Lit { + symbol, + kind: LitKind::Str | LitKind::StrRaw(_), + .. + }), + .. }, + _, + )) = c.consume() + { + Some(*symbol) + } else { + None } + }) + } - break; + fn skip_until_before_next_comma_or_eos(c: &mut ParseCursor<'_>) { + while !parse_eos(c) && parse_comma(&mut c.clone()).is_none() { + c.consume(); } } - (dangers, specified_reason) -} + // === Grammar === // + + fn parse_path(s: &mut ParseSequence<'_, '_>) -> Option { + let _guard = s.while_parsing(Symbol::intern("a path")); + let mut is_subsequent = false; + let mut builder = String::new(); + let start = s.next_span(); + + loop { + // Parse turbo delimiter + if is_subsequent { + if s.expect(Symbol::intern("`::`"), parse_turbo).is_none() { + // If we don't have one, assume that the path is done. + + // ...but first, we need to validate the identifier. + if let Some(reserved) = RESERVED_DANGERS.iter().find(|v| **v == builder) { + s.error( + start.until(s.next_span()), + format!("`{reserved}` cannot be the name of a danger"), + skip_until_before_next_comma_or_eos, + ); + return None; + } -fn emit_dangerous_call_lint(cx: &LateContext<'_>, expr: &'_ Expr<'_>, unaccepted_dangers: &[UnacceptedDanger]) { - // Collect all unique dangers - let unique_dangers = unaccepted_dangers - .iter() - .map(|danger| danger.id) - .collect::>(); + // N.B. the fact that this can only happen if we attempt to parse a subsequent + // identifier ensures that we can't just build a path out of nothing. + return Some(Symbol::intern(&builder)); + } - // Create a lint - span_lint_and_then( - cx, - DANGER_NOT_ACCEPTED, - expr.span, - &format!( - "called a function marked with `#[clippy::dangerous(...)]` without blessing the calling \ - module with `#![clippy::accept_danger({})]`", - { - let mut danger_list = unique_dangers.iter().map(Symbol::as_str).collect::>(); - danger_list.sort_unstable(); - danger_list.join(", ") - }, - ), - |diag| { - for danger in unaccepted_dangers { - if let Some(reason) = danger.reason { - diag.span_note( - danger.span, - format!( - "danger `{}` declared here with the justification `{}`", - danger.id, - reason.symbol.as_str(), - ), + builder.push_str("::"); + } + + // Parse identifier + let sess = s.rustc_session(); + let Some((ident, _)) = s.expect(Symbol::intern(""), |c| { + parse_ident(c).filter(|(ident, _)| !ident.is_reserved(|| sess.edition())) + }) else { + // Whoops! This is malformed. + s.stuck(skip_until_before_next_comma_or_eos); + return None; + }; + + // Ensure that this isn't a reserved prefix + if !is_subsequent { + if let Some(reserved) = RESERVED_PREFIXES.iter().find(|v| &***v == ident.as_str()) { + s.error( + start.until(s.next_span()), + format!("`{reserved}` is a reserved danger prefix"), + skip_until_before_next_comma_or_eos, ); - } else { - diag.span_note(danger.span, format!("danger `{}` declared here", danger.id)); } } - }, - ); + + builder.push_str(ident.as_str()); + + is_subsequent = true; + } + } + + fn parse_individually_reasoned_danger_list(s: &mut ParseSequence<'_, '_>) -> Vec<(Span, Symbol, Symbol)> { + let _guard = s.while_parsing(Symbol::intern("the dangers list")); + + let mut dangers = Vec::new(); + let mut is_subsequent = false; + + loop { + // Handle EOS + if s.expect(Symbol::intern("`)`"), parse_eos) { + break; + } + + // Handle comma if necessary + if is_subsequent && s.expect(Symbol::intern("`,`"), parse_comma).is_none() { + s.stuck(skip_until_before_next_comma_or_eos); + continue; + } + + // Handle another EOS because we don't want to get stuck in `parse_path`, which treats + // empty paths as errors. + if s.expect(Symbol::intern("`)`"), parse_eos) { + break; + } + + let danger_start = s.next_span(); + + // Handle a non-empty path. + let Some(danger) = parse_path(s) else { + // Our recovery routine has already put us into the position of parsing the next + // danger. + is_subsequent = true; + continue; + }; + + // Handle the reason. + let reason = { + let _guard = s.while_parsing(Symbol::intern("the danger's reason string")); + + if s.expect(Symbol::intern("`=`"), parse_equals).is_none() { + s.stuck(skip_until_before_next_comma_or_eos); + continue; + } + + let Some(reason) = s.expect(Symbol::intern("a reason string"), parse_str_lit) else { + s.stuck(skip_until_before_next_comma_or_eos); + continue; + }; + reason + }; + + dangers.push((danger_start.until(s.next_span()), danger, reason)); + is_subsequent = true; + } + + dangers + } + + fn parse_single_reason_danger_list(s: &mut ParseSequence<'_, '_>) -> (Vec<(Span, Symbol)>, Option) { + let _guard = s.while_parsing(Symbol::intern("the dangers list")); + + let mut dangers = Vec::new(); + let mut is_subsequent = false; + + loop { + // Handle EOS + if s.expect(Symbol::intern("`)`"), parse_eos) { + return (dangers, None); + } + + // Handle comma if necessary + if is_subsequent && s.expect(Symbol::intern("`,`"), parse_comma).is_none() { + s.stuck(skip_until_before_next_comma_or_eos); + continue; + } + + // Handle another EOS because we don't want to get stuck in `parse_path`, which treats + // empty paths as errors. + if s.expect(Symbol::intern("`)`"), parse_eos) { + return (dangers, None); + } + + // Handle `reason = "text"` syntax + if s.expect(Symbol::intern("`reason`"), |c| { + parse_ident(c).filter(|(s, _)| *s == sym::reason) + }) + .is_some() + { + let _guard = s.while_parsing(Symbol::intern("the reason attribute")); + + // Expect `=` + if s.expect(Symbol::intern("`=`"), parse_equals).is_none() { + s.stuck(skip_until_before_next_comma_or_eos); + continue; + } + + // Expect a reason literal + let Some(reason) = s.expect(Symbol::intern(""), parse_str_lit) else { + s.stuck(skip_until_before_next_comma_or_eos); + continue; + }; + + // Allow an optional `,` + let _guard = s.expect(Symbol::intern("`,`"), parse_comma); + + // Expect an EOS + if !s.expect(Symbol::intern("`)`"), parse_eos) { + s.stuck(skip_until_before_next_comma_or_eos); + continue; + } + + return (dangers, Some(reason)); + }; + + let danger_start = s.next_span(); + + // Handle a non-empty path. + let Some(danger) = parse_path(s) else { + // Our recovery routine has already put us into the position of parsing the next + // danger. + is_subsequent = true; + continue; + }; + + dangers.push((danger_start.until(s.next_span()), danger)); + is_subsequent = true; + } + } + + // === Drivers === // + + fn parse_paren_attr( + cx: &LateContext<'_>, + attr: &Attribute, + f: impl FnOnce(&mut ParseSequence<'_, '_>) -> R, + ) -> R { + const EXPECTATION: &str = "expected a delimited attribute with a list of danger identifiers"; + + let span = attr.span; + + // Expect a normal non doc-comment attribute. + let AttrKind::Normal(attr) = &attr.kind else { + cx.sess().span_err(span, EXPECTATION); + return R::default(); + }; + + // Expect it to be a delimited attribute of the form #[attr(...)] and not #[attr {...}] + let AttrArgs::Delimited(attr) = &attr.item.args else { + cx.sess().span_err(span, EXPECTATION); + return R::default(); + }; + + if attr.delim != Delimiter::Parenthesis { + cx.sess().span_err(span, EXPECTATION); + return R::default(); + } + + // Parse the attribute arguments + let cx = ParseContext::new(cx.sess()); + let res = f(&mut cx.enter(attr.dspan, &attr.tokens)); + if cx.got_stuck() { R::default() } else { res } + } + + pub fn parse_individually_reasoned_danger_list_attr( + cx: &LateContext<'_>, + attr: &Attribute, + ) -> Vec<(Span, Symbol, Symbol)> { + parse_paren_attr(cx, attr, parse_individually_reasoned_danger_list) + } + + pub fn parse_single_reason_danger_list_attr( + cx: &LateContext<'_>, + attr: &Attribute, + ) -> (Vec<(Span, Symbol)>, Option) { + parse_paren_attr(cx, attr, parse_single_reason_danger_list) + } } diff --git a/tests/ui/danger_not_accepted.rs b/tests/ui/danger_not_accepted.rs index be3de842294c..132e5394a2ee 100644 --- a/tests/ui/danger_not_accepted.rs +++ b/tests/ui/danger_not_accepted.rs @@ -9,10 +9,10 @@ fn main() { Maz.faz(); - #[clippy::accept_danger(may_deadlock)] + #[clippy::accept_danger(may_deadlock, reason = "this is fine :)")] Maz.faz(); - #[clippy::accept_danger(may_deadlock, may_delete_system)] + #[clippy::accept_danger(may_deadlock, not_a_virus::may_delete_system)] Maz.faz(); Maz.faz2(); @@ -20,7 +20,7 @@ fn main() { #[clippy::accept_danger(may_deadlock)] Maz.faz2(); - #[clippy::accept_danger(may_deadlock, may_delete_system)] + #[clippy::accept_danger(may_deadlock, not_a_virus::may_delete_system)] Maz.faz2(); waz::woo2(); @@ -33,17 +33,16 @@ fn wee() {} struct Maz; -#[clippy::dangerous(may_deadlock)] +#[clippy::dangerous(may_deadlock = "this entire module is just really messed up")] mod waz { pub fn woo() {} - #[clippy::dangerous(may_deadlock, reason = "your program may deadlock in calling this function")] + #[clippy::dangerous(may_deadlock = "your program may deadlock in calling this function")] pub fn woo2() {} impl super::Maz { #[clippy::dangerous( - may_delete_system, - reason = "calling this has a very strong chance of just deleting your computer" + not_a_virus::may_delete_system = "calling this has a very strong chance of just deleting your computer" )] pub fn faz(&self) {} } @@ -54,14 +53,15 @@ mod waz { } trait FazTrait { - #[clippy::dangerous(may_delete_system)] + #[clippy::dangerous(not_a_virus::may_delete_system = "this is a justification")] fn faz2(&self); } // Edge case attr tests #[rustfmt::skip] #[clippy::dangerous(whee, woo,)] -#[clippy::dangerous(whee, woo, reason = "sdfhsdf",)] +#[clippy::dangerous(whee, sdjfkl::woo, reason = "sdfhsdf",)] +#[clippy::accept_danger(hehe::haha = "sjdfkljf",)] fn dummy_1() {} // Invalid attr tests @@ -75,4 +75,8 @@ fn dummy_1() {} #[clippy::dangerous(whee, reason = "", weh)] #[clippy::dangerous(whee, reason = "" weh)] #[clippy::dangerous(whee, reason = 4)] +#[clippy::dangerous(unsafe::bar,, ehe = "dhf", bar::unsafe == "hehe")] +#[clippy::dangerous(crate::bar, reason)] +#[clippy::accept_danger(clippy:: = "" ::, ::)] +#[clippy::accept_danger(clippy::boo = "")] fn dummy_2() {} diff --git a/tests/ui/danger_not_accepted.stderr b/tests/ui/danger_not_accepted.stderr index 6a3065b3e470..8f0be92ac3de 100644 --- a/tests/ui/danger_not_accepted.stderr +++ b/tests/ui/danger_not_accepted.stderr @@ -4,66 +4,68 @@ error: called a function marked with `#[clippy::dangerous(...)]` without blessin LL | waz::woo(); | ^^^^^^^^ | -note: danger `may_deadlock` declared here +note: danger `may_deadlock` declared here with the justification `this entire module is just really messed up` --> $DIR/danger_not_accepted.rs:36:21 | -LL | #[clippy::dangerous(may_deadlock)] - | ^^^^^^^^^^^^ +LL | #[clippy::dangerous(may_deadlock = "this entire module is just really messed up")] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ = note: `-D clippy::danger-not-accepted` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::danger_not_accepted)]` -error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_deadlock, may_delete_system)]` +error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_deadlock, not_a_virus::may_delete_system)]` --> $DIR/danger_not_accepted.rs:10:5 | LL | Maz.faz(); | ^^^^^^^^^ | -note: danger `may_delete_system` declared here with the justification `calling this has a very strong chance of just deleting your computer` +note: danger `not_a_virus::may_delete_system` declared here with the justification `calling this has a very strong chance of just deleting your computer` --> $DIR/danger_not_accepted.rs:45:13 | -LL | may_delete_system, - | ^^^^^^^^^^^^^^^^^ -note: danger `may_deadlock` declared here +LL | / not_a_virus::may_delete_system = "calling this has a very strong chance of just deleting your computer" +LL | | )] + | |________^ +note: danger `may_deadlock` declared here with the justification `this entire module is just really messed up` --> $DIR/danger_not_accepted.rs:36:21 | -LL | #[clippy::dangerous(may_deadlock)] - | ^^^^^^^^^^^^ +LL | #[clippy::dangerous(may_deadlock = "this entire module is just really messed up")] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]` +error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(not_a_virus::may_delete_system)]` --> $DIR/danger_not_accepted.rs:13:5 | LL | Maz.faz(); | ^^^^^^^^^ | -note: danger `may_delete_system` declared here with the justification `calling this has a very strong chance of just deleting your computer` +note: danger `not_a_virus::may_delete_system` declared here with the justification `calling this has a very strong chance of just deleting your computer` --> $DIR/danger_not_accepted.rs:45:13 | -LL | may_delete_system, - | ^^^^^^^^^^^^^^^^^ +LL | / not_a_virus::may_delete_system = "calling this has a very strong chance of just deleting your computer" +LL | | )] + | |________^ -error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]` +error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(not_a_virus::may_delete_system)]` --> $DIR/danger_not_accepted.rs:18:5 | LL | Maz.faz2(); | ^^^^^^^^^^ | -note: danger `may_delete_system` declared here - --> $DIR/danger_not_accepted.rs:57:25 +note: danger `not_a_virus::may_delete_system` declared here with the justification `this is a justification` + --> $DIR/danger_not_accepted.rs:56:25 | -LL | #[clippy::dangerous(may_delete_system)] - | ^^^^^^^^^^^^^^^^^ +LL | #[clippy::dangerous(not_a_virus::may_delete_system = "this is a justification")] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_delete_system)]` +error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(not_a_virus::may_delete_system)]` --> $DIR/danger_not_accepted.rs:21:5 | LL | Maz.faz2(); | ^^^^^^^^^^ | -note: danger `may_delete_system` declared here - --> $DIR/danger_not_accepted.rs:57:25 +note: danger `not_a_virus::may_delete_system` declared here with the justification `this is a justification` + --> $DIR/danger_not_accepted.rs:56:25 | -LL | #[clippy::dangerous(may_delete_system)] - | ^^^^^^^^^^^^^^^^^ +LL | #[clippy::dangerous(not_a_virus::may_delete_system = "this is a justification")] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: called a function marked with `#[clippy::dangerous(...)]` without blessing the calling module with `#![clippy::accept_danger(may_deadlock)]` --> $DIR/danger_not_accepted.rs:26:5 @@ -74,13 +76,79 @@ LL | waz::woo2(); note: danger `may_deadlock` declared here with the justification `your program may deadlock in calling this function` --> $DIR/danger_not_accepted.rs:40:25 | -LL | #[clippy::dangerous(may_deadlock, reason = "your program may deadlock in calling this function")] - | ^^^^^^^^^^^^ -note: danger `may_deadlock` declared here +LL | #[clippy::dangerous(may_deadlock = "your program may deadlock in calling this function")] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +note: danger `may_deadlock` declared here with the justification `this entire module is just really messed up` --> $DIR/danger_not_accepted.rs:36:21 | -LL | #[clippy::dangerous(may_deadlock)] - | ^^^^^^^^^^^^ +LL | #[clippy::dangerous(may_deadlock = "this entire module is just really messed up")] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: expected `)`, `,`, `::` while parsing the dangers list + --> $DIR/danger_not_accepted.rs:64:36 + | +LL | #[clippy::accept_danger(hehe::haha = "sjdfkljf",)] + | ^ + +error: expected `::`, `=` while parsing the danger's reason string in the dangers list + --> $DIR/danger_not_accepted.rs:62:25 + | +LL | #[clippy::dangerous(whee, woo,)] + | ^ + +error: expected , `)`, `::`, `=` while parsing a path in the dangers list + --> $DIR/danger_not_accepted.rs:62:25 + | +LL | #[clippy::dangerous(whee, woo,)] + | ^ + +error: expected `::`, `=` while parsing the danger's reason string in the dangers list + --> $DIR/danger_not_accepted.rs:62:30 + | +LL | #[clippy::dangerous(whee, woo,)] + | ^ + +error: expected `::`, `=` while parsing the danger's reason string in the dangers list + --> $DIR/danger_not_accepted.rs:63:25 + | +LL | #[clippy::dangerous(whee, sdjfkl::woo, reason = "sdfhsdf",)] + | ^ + +error: expected , `)`, `::`, `=` while parsing a path in the dangers list + --> $DIR/danger_not_accepted.rs:63:25 + | +LL | #[clippy::dangerous(whee, sdjfkl::woo, reason = "sdfhsdf",)] + | ^ + +error: expected `::`, `=` while parsing the danger's reason string in the dangers list + --> $DIR/danger_not_accepted.rs:63:38 + | +LL | #[clippy::dangerous(whee, sdjfkl::woo, reason = "sdfhsdf",)] + | ^ + +error: `reason` cannot be the name of a danger + --> $DIR/danger_not_accepted.rs:63:40 + | +LL | #[clippy::dangerous(whee, sdjfkl::woo, reason = "sdfhsdf",)] + | ^^^^^^^ + +error: `clippy` is a reserved danger prefix + --> $DIR/danger_not_accepted.rs:80:25 + | +LL | #[clippy::accept_danger(clippy:: = "" ::, ::)] + | ^^^^^^ + +error: expected , `)`, `reason` while parsing a path in the dangers list + --> $DIR/danger_not_accepted.rs:80:43 + | +LL | #[clippy::accept_danger(clippy:: = "" ::, ::)] + | ^^ + +error: `clippy` is a reserved danger prefix + --> $DIR/danger_not_accepted.rs:81:25 + | +LL | #[clippy::accept_danger(clippy::boo = "")] + | ^^^^^^ error: expected a delimited attribute with a list of danger identifiers --> $DIR/danger_not_accepted.rs:68:1 @@ -94,53 +162,185 @@ error: expected a delimited attribute with a list of danger identifiers LL | #[clippy::dangerous[]] | ^^^^^^^^^^^^^^^^^^^^^^ -error: expected a delimited attribute with a list of danger identifiers; this was not an identifier +error: expected , `)` while parsing a path in the dangers list --> $DIR/danger_not_accepted.rs:70:21 | LL | #[clippy::dangerous(,)] | ^ -error: expected = after a reason attribute +error: expected `::`, `=` while parsing the danger's reason string in the dangers list + --> $DIR/danger_not_accepted.rs:71:25 + | +LL | #[clippy::dangerous(whee, reason)] + | ^ + +error: expected , `)`, `::`, `=` while parsing a path in the dangers list + --> $DIR/danger_not_accepted.rs:71:25 + | +LL | #[clippy::dangerous(whee, reason)] + | ^ + +error: `reason` cannot be the name of a danger --> $DIR/danger_not_accepted.rs:71:27 | LL | #[clippy::dangerous(whee, reason)] | ^^^^^^ -error: expected = after a reason attribute - --> $DIR/danger_not_accepted.rs:72:33 +error: expected `::`, `=` while parsing the danger's reason string in the dangers list + --> $DIR/danger_not_accepted.rs:72:25 | LL | #[clippy::dangerous(whee, reason, abc)] - | ^ + | ^ + +error: expected , `)`, `::`, `=` while parsing a path in the dangers list + --> $DIR/danger_not_accepted.rs:72:25 + | +LL | #[clippy::dangerous(whee, reason, abc)] + | ^ -error: expected a string literal after a reason attribute - --> $DIR/danger_not_accepted.rs:73:34 +error: `reason` cannot be the name of a danger + --> $DIR/danger_not_accepted.rs:72:27 + | +LL | #[clippy::dangerous(whee, reason, abc)] + | ^^^^^^ + +error: expected `::`, `=` while parsing the danger's reason string in the dangers list + --> $DIR/danger_not_accepted.rs:72:38 + | +LL | #[clippy::dangerous(whee, reason, abc)] + | ^ + +error: expected `::`, `=` while parsing the danger's reason string in the dangers list + --> $DIR/danger_not_accepted.rs:73:25 | LL | #[clippy::dangerous(whee, reason =)] - | ^ + | ^ -error: expected a string literal after a reason attribute - --> $DIR/danger_not_accepted.rs:74:35 +error: expected , `)`, `::`, `=` while parsing a path in the dangers list + --> $DIR/danger_not_accepted.rs:73:25 + | +LL | #[clippy::dangerous(whee, reason =)] + | ^ + +error: `reason` cannot be the name of a danger + --> $DIR/danger_not_accepted.rs:73:27 + | +LL | #[clippy::dangerous(whee, reason =)] + | ^^^^^^^ + +error: expected `::`, `=` while parsing the danger's reason string in the dangers list + --> $DIR/danger_not_accepted.rs:74:25 + | +LL | #[clippy::dangerous(whee, reason =, weh)] + | ^ + +error: expected , `)`, `::`, `=` while parsing a path in the dangers list + --> $DIR/danger_not_accepted.rs:74:25 + | +LL | #[clippy::dangerous(whee, reason =, weh)] + | ^ + +error: `reason` cannot be the name of a danger + --> $DIR/danger_not_accepted.rs:74:27 + | +LL | #[clippy::dangerous(whee, reason =, weh)] + | ^^^^^^^ + +error: expected `::`, `=` while parsing the danger's reason string in the dangers list + --> $DIR/danger_not_accepted.rs:74:40 | LL | #[clippy::dangerous(whee, reason =, weh)] - | ^ + | ^ -error: nothing should come after the reason attribute besides an optional comma - --> $DIR/danger_not_accepted.rs:75:38 +error: expected `::`, `=` while parsing the danger's reason string in the dangers list + --> $DIR/danger_not_accepted.rs:75:25 | LL | #[clippy::dangerous(whee, reason = "", weh)] - | ^ + | ^ -error: nothing should come after the reason attribute besides an optional comma - --> $DIR/danger_not_accepted.rs:76:39 +error: expected , `)`, `::`, `=` while parsing a path in the dangers list + --> $DIR/danger_not_accepted.rs:75:25 + | +LL | #[clippy::dangerous(whee, reason = "", weh)] + | ^ + +error: `reason` cannot be the name of a danger + --> $DIR/danger_not_accepted.rs:75:27 + | +LL | #[clippy::dangerous(whee, reason = "", weh)] + | ^^^^^^^ + +error: expected `::`, `=` while parsing the danger's reason string in the dangers list + --> $DIR/danger_not_accepted.rs:75:43 + | +LL | #[clippy::dangerous(whee, reason = "", weh)] + | ^ + +error: expected `::`, `=` while parsing the danger's reason string in the dangers list + --> $DIR/danger_not_accepted.rs:76:25 | LL | #[clippy::dangerous(whee, reason = "" weh)] - | ^^^ + | ^ -error: expected a string literal after a reason attribute - --> $DIR/danger_not_accepted.rs:77:36 +error: expected , `)`, `::`, `=` while parsing a path in the dangers list + --> $DIR/danger_not_accepted.rs:76:25 + | +LL | #[clippy::dangerous(whee, reason = "" weh)] + | ^ + +error: `reason` cannot be the name of a danger + --> $DIR/danger_not_accepted.rs:76:27 + | +LL | #[clippy::dangerous(whee, reason = "" weh)] + | ^^^^^^^ + +error: expected `::`, `=` while parsing the danger's reason string in the dangers list + --> $DIR/danger_not_accepted.rs:77:25 | LL | #[clippy::dangerous(whee, reason = 4)] - | ^ + | ^ + +error: expected , `)`, `::`, `=` while parsing a path in the dangers list + --> $DIR/danger_not_accepted.rs:77:25 + | +LL | #[clippy::dangerous(whee, reason = 4)] + | ^ + +error: `reason` cannot be the name of a danger + --> $DIR/danger_not_accepted.rs:77:27 + | +LL | #[clippy::dangerous(whee, reason = 4)] + | ^^^^^^^ + +error: expected , `)` while parsing a path in the dangers list + --> $DIR/danger_not_accepted.rs:78:21 + | +LL | #[clippy::dangerous(unsafe::bar,, ehe = "dhf", bar::unsafe == "hehe")] + | ^^^^^^ + +error: expected , `)` while parsing a path in the dangers list + --> $DIR/danger_not_accepted.rs:78:33 + | +LL | #[clippy::dangerous(unsafe::bar,, ehe = "dhf", bar::unsafe == "hehe")] + | ^ + +error: expected while parsing a path in the dangers list + --> $DIR/danger_not_accepted.rs:78:53 + | +LL | #[clippy::dangerous(unsafe::bar,, ehe = "dhf", bar::unsafe == "hehe")] + | ^^^^^^ + +error: expected , `)` while parsing a path in the dangers list + --> $DIR/danger_not_accepted.rs:79:21 + | +LL | #[clippy::dangerous(crate::bar, reason)] + | ^^^^^ + +error: `reason` cannot be the name of a danger + --> $DIR/danger_not_accepted.rs:79:33 + | +LL | #[clippy::dangerous(crate::bar, reason)] + | ^^^^^^ -error: aborting due to 16 previous errors +error: aborting due to 49 previous errors From a1497a461f41a0a3b55d64ffe7c9d48491d50417 Mon Sep 17 00:00:00 2001 From: Radbuglet <10578060+Radbuglet@users.noreply.github.com> Date: Fri, 1 Dec 2023 22:21:25 -0700 Subject: [PATCH 16/16] Fix bad merge --- clippy_lints/src/danger_not_accepted.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/danger_not_accepted.rs b/clippy_lints/src/danger_not_accepted.rs index d8480c14efb4..4d04bdc446c3 100644 --- a/clippy_lints/src/danger_not_accepted.rs +++ b/clippy_lints/src/danger_not_accepted.rs @@ -3,7 +3,7 @@ use clippy_utils::get_attr; use rustc_data_structures::fx::{FxHashMap, FxHashSet, StdEntry}; use rustc_hir::{def, def_id, Expr, ExprKind, QPath}; use rustc_lint::{LateContext, LateLintPass, LintContext}; -use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_session::impl_lint_pass; use rustc_span::{Span, Symbol}; // Future improvements: