From fa4e3aac198d14dbc5c66877af1127501fd87616 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sat, 9 Mar 2024 00:50:46 +0100 Subject: [PATCH 1/3] add documentation to the `span_lint_hir` functions --- clippy.toml | 4 ++-- clippy_utils/src/diagnostics.rs | 38 +++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/clippy.toml b/clippy.toml index 8c405ac6a4e..3a68fa1670d 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1,7 +1,7 @@ avoid-breaking-exported-api = false -# use the various `span_lint_*` methods instead, which also add a link to the docs +# use the various `clippy_utils::diagnostics::span_lint_*` functions instead, which also add a link to the docs disallowed-methods = [ "rustc_lint::context::LintContext::span_lint", - "rustc_middle::ty::context::TyCtxt::node_span_lint" + "rustc_middle::ty::context::TyCtxt::node_span_lint", ] diff --git a/clippy_utils/src/diagnostics.rs b/clippy_utils/src/diagnostics.rs index 6ed46e5dde0..303b23ba3a0 100644 --- a/clippy_utils/src/diagnostics.rs +++ b/clippy_utils/src/diagnostics.rs @@ -152,6 +152,25 @@ pub fn span_lint_and_then(cx: &C, lint: &'static Lint, sp: S, msg: &str }); } +/// Like [`span_lint`], but allows providing the `HirId` of the node that caused us to emit this +/// lint. +/// +/// The `HirId` is used for checking lint level attributes. +/// +/// For example: +/// ```ignore +/// fn f() { /* '1 */ +/// +/// #[allow(clippy::some_lint)] +/// let _x = /* '2 */; +/// } +/// ``` +/// If `some_lint` does its analysis in `LintPass::check_fn` (at `'1`) and emits a lint at `'2` +/// using [`span_lint`], then allowing the lint at `'2` as attempted in the snippet will not work! +/// Even though that is where the warning points at, which would be confusing to users. +/// +/// Instead, use this function and also pass the `HirId` of the node at `'2`, which will let the +/// compiler check lint level attributes at `'2` as one would expect, and the `#[allow]` will work. pub fn span_lint_hir(cx: &LateContext<'_>, lint: &'static Lint, hir_id: HirId, sp: Span, msg: &str) { #[expect(clippy::disallowed_methods)] cx.tcx.node_span_lint(lint, hir_id, sp, msg.to_string(), |diag| { @@ -159,6 +178,25 @@ pub fn span_lint_hir(cx: &LateContext<'_>, lint: &'static Lint, hir_id: HirId, s }); } +/// Like [`span_lint_and_then`], but allows providing the `HirId` of the node that caused us to emit +/// this lint. +/// +/// The `HirId` is used for checking lint level attributes. +/// +/// For example: +/// ```ignore +/// fn f() { /* '1 */ +/// +/// #[allow(clippy::some_lint)] +/// let _x = /* '2 */; +/// } +/// ``` +/// If `some_lint` does its analysis in `LintPass::check_fn` (at `'1`) and emits a lint at `'2` +/// using [`span_lint_and_then`], then allowing the lint at `'2` as attempted in the snippet will +/// not work! Even though that is where the warning points at, which would be confusing to users. +/// +/// Instead, use this function and also pass the `HirId` of the node at `'2`, which will let the +/// compiler check lint level attributes at `'2` as one would expect, and the `#[allow]` will work. pub fn span_lint_hir_and_then( cx: &LateContext<'_>, lint: &'static Lint, From eb5ce85932e4b9fb7f7a1765f3760c3363a282d9 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sat, 9 Mar 2024 19:40:39 +0100 Subject: [PATCH 2/3] mention `span_lint_hir` in `span_lint` and add a reason to disallowed_methods --- clippy.toml | 13 ++-- clippy_utils/src/diagnostics.rs | 81 +++++++++++++++++---- tests/ui-internal/disallow_span_lint.stderr | 3 + 3 files changed, 77 insertions(+), 20 deletions(-) diff --git a/clippy.toml b/clippy.toml index 3a68fa1670d..62ed55beb1f 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1,7 +1,10 @@ avoid-breaking-exported-api = false -# use the various `clippy_utils::diagnostics::span_lint_*` functions instead, which also add a link to the docs -disallowed-methods = [ - "rustc_lint::context::LintContext::span_lint", - "rustc_middle::ty::context::TyCtxt::node_span_lint", -] +[[disallowed-methods]] +path = "rustc_lint::context::LintContext::span_lint" +reason = "this function does not add a link to our documentation, please use the `clippy_utils::diagnostics::span_lint*` functions instead" + + +[[disallowed-methods]] +path = "rustc_middle::ty::context::TyCtxt::node_span_lint" +reason = "this function does not add a link to our documentation, please use the `clippy_utils::diagnostics::span_lint_hir*` functions instead" diff --git a/clippy_utils/src/diagnostics.rs b/clippy_utils/src/diagnostics.rs index 303b23ba3a0..df551495993 100644 --- a/clippy_utils/src/diagnostics.rs +++ b/clippy_utils/src/diagnostics.rs @@ -36,6 +36,15 @@ fn docs_link(diag: &mut Diag<'_, ()>, lint: &'static Lint) { /// Usually it's nicer to provide more context for lint messages. /// Be sure the output is understandable when you use this method. /// +/// NOTE: only lint-level attributes at the `LintPass::check_*` node from which you are calling this +/// will be considered. +/// This can be confusing if the given span is at a different place, because users won't know where +/// `#[allow]` or `#[expect]` attributes need to be placed. +/// +/// This can happen if, for example, you are in `LintPass::check_block` and you are emitting a lint +/// for a particular expression within that block. +/// In those cases, consider using [`span_lint_hir`], and pass the `HirId` of that expression. +/// /// # Example /// /// ```ignore @@ -61,6 +70,15 @@ pub fn span_lint(cx: &T, lint: &'static Lint, sp: impl Into( /// /// If you change the signature, remember to update the internal lint `CollapsibleCalls` /// +/// NOTE: only lint-level attributes at the `LintPass::check_*` node from which you are calling this +/// will be considered. +/// This can be confusing if the given span is at a different place, because users won't know where +/// `#[allow]` or `#[expect]` attributes need to be placed. +/// +/// This can happen if, for example, you are in `LintPass::check_block` and you are emitting a lint +/// for a particular expression within that block. +/// In those cases, consider using [`span_lint_hir`], and pass the `HirId` of that expression. +/// /// # Example /// /// ```text @@ -139,6 +166,15 @@ pub fn span_lint_and_note( /// /// If you need to customize your lint output a lot, use this function. /// If you change the signature, remember to update the internal lint `CollapsibleCalls` +/// +/// NOTE: only lint-level attributes at the `LintPass::check_*` node from which you are calling this +/// will be considered. +/// This can be confusing if the given span is at a different place, because users won't know where +/// `#[allow]` or `#[expect]` attributes need to be placed. +/// +/// This can happen if, for example, you are in `LintPass::check_block` and you are emitting a lint +/// for a particular expression within that block. +/// In those cases, consider using [`span_lint_hir`], and pass the `HirId` of that expression. pub fn span_lint_and_then(cx: &C, lint: &'static Lint, sp: S, msg: &str, f: F) where C: LintContext, @@ -155,22 +191,25 @@ pub fn span_lint_and_then(cx: &C, lint: &'static Lint, sp: S, msg: &str /// Like [`span_lint`], but allows providing the `HirId` of the node that caused us to emit this /// lint. /// -/// The `HirId` is used for checking lint level attributes. +/// The `HirId` is used for checking lint level attributes and to fulfill lint expectations defined +/// via the `#[expect]` attribute. /// /// For example: /// ```ignore -/// fn f() { /* '1 */ +/// fn f() { /* */ /// /// #[allow(clippy::some_lint)] -/// let _x = /* '2 */; +/// let _x = /* */; /// } /// ``` -/// If `some_lint` does its analysis in `LintPass::check_fn` (at `'1`) and emits a lint at `'2` -/// using [`span_lint`], then allowing the lint at `'2` as attempted in the snippet will not work! +/// If `some_lint` does its analysis in `LintPass::check_fn` (at ``) and emits a lint at +/// `` using [`span_lint`], then allowing the lint at `` as attempted in the snippet +/// will not work! /// Even though that is where the warning points at, which would be confusing to users. /// -/// Instead, use this function and also pass the `HirId` of the node at `'2`, which will let the -/// compiler check lint level attributes at `'2` as one would expect, and the `#[allow]` will work. +/// Instead, use this function and also pass the `HirId` of ``, which will let +/// the compiler check lint level attributes at the place of the expression and +/// the `#[allow]` will work. pub fn span_lint_hir(cx: &LateContext<'_>, lint: &'static Lint, hir_id: HirId, sp: Span, msg: &str) { #[expect(clippy::disallowed_methods)] cx.tcx.node_span_lint(lint, hir_id, sp, msg.to_string(), |diag| { @@ -181,22 +220,25 @@ pub fn span_lint_hir(cx: &LateContext<'_>, lint: &'static Lint, hir_id: HirId, s /// Like [`span_lint_and_then`], but allows providing the `HirId` of the node that caused us to emit /// this lint. /// -/// The `HirId` is used for checking lint level attributes. +/// The `HirId` is used for checking lint level attributes and to fulfill lint expectations defined +/// via the `#[expect]` attribute. /// /// For example: /// ```ignore -/// fn f() { /* '1 */ +/// fn f() { /* */ /// /// #[allow(clippy::some_lint)] -/// let _x = /* '2 */; +/// let _x = /* */; /// } /// ``` -/// If `some_lint` does its analysis in `LintPass::check_fn` (at `'1`) and emits a lint at `'2` -/// using [`span_lint_and_then`], then allowing the lint at `'2` as attempted in the snippet will -/// not work! Even though that is where the warning points at, which would be confusing to users. +/// If `some_lint` does its analysis in `LintPass::check_fn` (at ``) and emits a lint at +/// `` using [`span_lint`], then allowing the lint at `` as attempted in the snippet +/// will not work! +/// Even though that is where the warning points at, which would be confusing to users. /// -/// Instead, use this function and also pass the `HirId` of the node at `'2`, which will let the -/// compiler check lint level attributes at `'2` as one would expect, and the `#[allow]` will work. +/// Instead, use this function and also pass the `HirId` of ``, which will let +/// the compiler check lint level attributes at the place of the expression and +/// the `#[allow]` will work. pub fn span_lint_hir_and_then( cx: &LateContext<'_>, lint: &'static Lint, @@ -220,6 +262,15 @@ pub fn span_lint_hir_and_then( /// /// If you change the signature, remember to update the internal lint `CollapsibleCalls` /// +/// NOTE: only lint-level attributes at the `LintPass::check_*` node from which you are calling this +/// will be considered. +/// This can be confusing if the given span is at a different place, because users won't know where +/// `#[allow]` or `#[expect]` attributes need to be placed. +/// +/// This can happen if, for example, you are in `LintPass::check_block` and you are emitting a lint +/// for a particular expression within that block. +/// In those cases, consider using [`span_lint_hir`], and pass the `HirId` of that expression. +/// /// # Example /// /// ```text diff --git a/tests/ui-internal/disallow_span_lint.stderr b/tests/ui-internal/disallow_span_lint.stderr index ae5d6843406..cfc590bed36 100644 --- a/tests/ui-internal/disallow_span_lint.stderr +++ b/tests/ui-internal/disallow_span_lint.stderr @@ -4,6 +4,7 @@ error: use of a disallowed method `rustc_lint::context::LintContext::span_lint` LL | cx.span_lint(lint, span, msg, |_| {}); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | + = note: this function does not add a link to our documentation, please use the `clippy_utils::diagnostics::span_lint*` functions instead (from clippy.toml) = note: `-D clippy::disallowed-methods` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::disallowed_methods)]` @@ -12,6 +13,8 @@ error: use of a disallowed method `rustc_middle::ty::context::TyCtxt::node_span_ | LL | tcx.node_span_lint(lint, hir_id, span, msg, |_| {}); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: this function does not add a link to our documentation, please use the `clippy_utils::diagnostics::span_lint_hir*` functions instead (from clippy.toml) error: aborting due to 2 previous errors From 5b1f95cbbbb0db8a0a6b046432851c8870e320d0 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sat, 9 Mar 2024 23:20:02 +0100 Subject: [PATCH 3/3] apply review suggestions --- clippy_utils/src/diagnostics.rs | 107 ++++++++++++++++++++------------ 1 file changed, 68 insertions(+), 39 deletions(-) diff --git a/clippy_utils/src/diagnostics.rs b/clippy_utils/src/diagnostics.rs index df551495993..0352696f93e 100644 --- a/clippy_utils/src/diagnostics.rs +++ b/clippy_utils/src/diagnostics.rs @@ -36,14 +36,19 @@ fn docs_link(diag: &mut Diag<'_, ()>, lint: &'static Lint) { /// Usually it's nicer to provide more context for lint messages. /// Be sure the output is understandable when you use this method. /// -/// NOTE: only lint-level attributes at the `LintPass::check_*` node from which you are calling this -/// will be considered. -/// This can be confusing if the given span is at a different place, because users won't know where -/// `#[allow]` or `#[expect]` attributes need to be placed. +/// NOTE: Lint emissions are always bound to a node in the HIR, which is used to determine +/// the lint level. +/// For the `span_lint` function, the node that was passed into the `LintPass::check_*` function is +/// used. /// -/// This can happen if, for example, you are in `LintPass::check_block` and you are emitting a lint -/// for a particular expression within that block. -/// In those cases, consider using [`span_lint_hir`], and pass the `HirId` of that expression. +/// If you're emitting the lint at the span of a different node than the one provided by the +/// `LintPass::check_*` function, consider using [`span_lint_hir`] instead. +/// This is needed for `#[allow]` and `#[expect]` attributes to work on the node +/// highlighted in the displayed warning. +/// +/// If you're unsure which function you should use, you can test if the `#[allow]` attribute works +/// where you would expect it to. +/// If it doesn't, you likely need to use [`span_lint_hir`] instead. /// /// # Example /// @@ -70,14 +75,19 @@ pub fn span_lint(cx: &T, lint: &'static Lint, sp: impl Into( /// /// If you change the signature, remember to update the internal lint `CollapsibleCalls` /// -/// NOTE: only lint-level attributes at the `LintPass::check_*` node from which you are calling this -/// will be considered. -/// This can be confusing if the given span is at a different place, because users won't know where -/// `#[allow]` or `#[expect]` attributes need to be placed. +/// NOTE: Lint emissions are always bound to a node in the HIR, which is used to determine +/// the lint level. +/// For the `span_lint_and_note` function, the node that was passed into the `LintPass::check_*` +/// function is used. /// -/// This can happen if, for example, you are in `LintPass::check_block` and you are emitting a lint -/// for a particular expression within that block. -/// In those cases, consider using [`span_lint_hir`], and pass the `HirId` of that expression. +/// If you're emitting the lint at the span of a different node than the one provided by the +/// `LintPass::check_*` function, consider using [`span_lint_hir_and_then`] instead. +/// This is needed for `#[allow]` and `#[expect]` attributes to work on the node +/// highlighted in the displayed warning. +/// +/// If you're unsure which function you should use, you can test if the `#[allow]` attribute works +/// where you would expect it to. +/// If it doesn't, you likely need to use [`span_lint_hir_and_then`] instead. /// /// # Example /// @@ -167,14 +182,19 @@ pub fn span_lint_and_note( /// If you need to customize your lint output a lot, use this function. /// If you change the signature, remember to update the internal lint `CollapsibleCalls` /// -/// NOTE: only lint-level attributes at the `LintPass::check_*` node from which you are calling this -/// will be considered. -/// This can be confusing if the given span is at a different place, because users won't know where -/// `#[allow]` or `#[expect]` attributes need to be placed. +/// NOTE: Lint emissions are always bound to a node in the HIR, which is used to determine +/// the lint level. +/// For the `span_lint_and_then` function, the node that was passed into the `LintPass::check_*` +/// function is used. /// -/// This can happen if, for example, you are in `LintPass::check_block` and you are emitting a lint -/// for a particular expression within that block. -/// In those cases, consider using [`span_lint_hir`], and pass the `HirId` of that expression. +/// If you're emitting the lint at the span of a different node than the one provided by the +/// `LintPass::check_*` function, consider using [`span_lint_hir_and_then`] instead. +/// This is needed for `#[allow]` and `#[expect]` attributes to work on the node +/// highlighted in the displayed warning. +/// +/// If you're unsure which function you should use, you can test if the `#[allow]` attribute works +/// where you would expect it to. +/// If it doesn't, you likely need to use [`span_lint_hir_and_then`] instead. pub fn span_lint_and_then(cx: &C, lint: &'static Lint, sp: S, msg: &str, f: F) where C: LintContext, @@ -188,8 +208,10 @@ pub fn span_lint_and_then(cx: &C, lint: &'static Lint, sp: S, msg: &str }); } -/// Like [`span_lint`], but allows providing the `HirId` of the node that caused us to emit this -/// lint. +/// Like [`span_lint`], but emits the lint at the node identified by the given `HirId`. +/// +/// This is in contrast to [`span_lint`], which always emits the lint at the node that was last +/// passed to the `LintPass::check_*` function. /// /// The `HirId` is used for checking lint level attributes and to fulfill lint expectations defined /// via the `#[expect]` attribute. @@ -217,8 +239,10 @@ pub fn span_lint_hir(cx: &LateContext<'_>, lint: &'static Lint, hir_id: HirId, s }); } -/// Like [`span_lint_and_then`], but allows providing the `HirId` of the node that caused us to emit -/// this lint. +/// Like [`span_lint_and_then`], but emits the lint at the node identified by the given `HirId`. +/// +/// This is in contrast to [`span_lint_and_then`], which always emits the lint at the node that was +/// last passed to the `LintPass::check_*` function. /// /// The `HirId` is used for checking lint level attributes and to fulfill lint expectations defined /// via the `#[expect]` attribute. @@ -262,14 +286,19 @@ pub fn span_lint_hir_and_then( /// /// If you change the signature, remember to update the internal lint `CollapsibleCalls` /// -/// NOTE: only lint-level attributes at the `LintPass::check_*` node from which you are calling this -/// will be considered. -/// This can be confusing if the given span is at a different place, because users won't know where -/// `#[allow]` or `#[expect]` attributes need to be placed. +/// NOTE: Lint emissions are always bound to a node in the HIR, which is used to determine +/// the lint level. +/// For the `span_lint_and_sugg` function, the node that was passed into the `LintPass::check_*` +/// function is used. /// -/// This can happen if, for example, you are in `LintPass::check_block` and you are emitting a lint -/// for a particular expression within that block. -/// In those cases, consider using [`span_lint_hir`], and pass the `HirId` of that expression. +/// If you're emitting the lint at the span of a different node than the one provided by the +/// `LintPass::check_*` function, consider using [`span_lint_hir_and_then`] instead. +/// This is needed for `#[allow]` and `#[expect]` attributes to work on the node +/// highlighted in the displayed warning. +/// +/// If you're unsure which function you should use, you can test if the `#[allow]` attribute works +/// where you would expect it to. +/// If it doesn't, you likely need to use [`span_lint_hir_and_then`] instead. /// /// # Example ///