Switch [extern_without_abi] to LateLintPass

CBSpeir · CBSpeir · commit faebef5e496d · 2024-09-18T16:03:13.000-05:00
Also:
- Add test if inside external macro and/or proc macro
- Update lint documentation
- Emit `MachineApplicable` suggestion with lint
diff --git a/clippy_lints/src/extern_without_abi.rs b/clippy_lints/src/extern_without_abi.rs
@@ -1,19 +1,35 @@
-use clippy_utils::diagnostics::span_lint;
-use rustc_ast::visit::FnKind;
-use rustc_ast::{Extern, FnHeader, FnSig, ForeignMod, Item, ItemKind, NodeId};
-use rustc_lint::{EarlyContext, EarlyLintPass};
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::is_from_proc_macro;
+use clippy_utils::source::snippet;
+use rustc_errors::Applicability;
+use rustc_hir::def_id::LocalDefId;
+use rustc_hir::intravisit::FnKind;
+use rustc_hir::{Body, FnDecl, FnHeader, Item, ItemKind};
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_middle::lint::in_external_macro;
 use rustc_session::declare_lint_pass;
 use rustc_span::Span;
+use rustc_target::spec::abi::Abi;
 
 const LINT_MSG: &str = "`extern` missing explicit ABI";
+const LINT_HELP_MSG: &str = "consider using";
+
+const EXTERN: &str = "extern";
+const FN: &str = "fn";
+const OPEN_BRACE: &str = "{";
+const ABI: &str = "\"C\"";
 
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for usage of `extern` without an explicit ABI.
     ///
     /// ### Why is this bad?
-    /// Explicitly declaring the ABI improves readability.
-    //
+    /// Explicitly declaring the ABI is the recommended convention. See:
+    /// [Rust Style Guide - `extern` items](https://doc.rust-lang.org/nightly/style-guide/items.html#extern-items)
+    ///
+    /// It's also enforced by `rustfmt` when the `force_explicit_abi` option is enabled. See:
+    /// [Configuring Rustfmt](https://rust-lang.github.io/rustfmt/?version=master&search=#force_explicit_abi)
+    ///
     /// ### Example
     /// ```no_run
     /// extern fn foo() {}
@@ -38,24 +54,60 @@ declare_clippy_lint! {
 
 declare_lint_pass!(ExternWithoutAbi => [EXTERN_WITHOUT_ABI]);
 
-impl EarlyLintPass for ExternWithoutAbi {
-    fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) {
-        if let ItemKind::ForeignMod(ref foreign_mod) = item.kind
-            && let ForeignMod { abi: None, .. } = foreign_mod
+impl<'tcx> LateLintPass<'tcx> for ExternWithoutAbi {
+    fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
+        if let ItemKind::ForeignMod { abi: Abi::C { .. }, .. } = item.kind
+            && !in_external_macro(cx.sess(), item.span)
+            && !is_from_proc_macro(cx, item)
+            && let snippet = snippet(cx.sess(), item.span, "").as_ref()
+            && is_extern_followed_by(OPEN_BRACE, snippet)
         {
-            span_lint(cx, EXTERN_WITHOUT_ABI, item.span, LINT_MSG);
+            emit_lint(cx, item.span, snippet);
         }
     }
 
-    fn check_fn(&mut self, cx: &EarlyContext<'_>, kind: FnKind<'_>, _: Span, _: NodeId) {
-        if let FnKind::Fn(_, _, sig, ..) = kind
-            && let FnSig { header, .. } = sig
-            && let FnHeader {
-                ext: Extern::Implicit(span),
-                ..
-            } = header
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        kind: FnKind<'tcx>,
+        _: &'tcx FnDecl<'tcx>,
+        body: &'tcx Body<'tcx>,
+        span: Span,
+        _: LocalDefId,
+    ) {
+        if let FnKind::ItemFn(_, _, header) = kind
+            && let FnHeader { abi: Abi::C { .. }, .. } = header
+            && !in_external_macro(cx.sess(), span)
+            && let hir_id = cx.tcx.hir().body_owner(body.id())
+            && !is_from_proc_macro(cx, &(&kind, body, hir_id, span))
+            && let snippet = snippet(cx.sess(), span, "").as_ref()
+            && is_extern_followed_by(FN, snippet)
         {
-            span_lint(cx, EXTERN_WITHOUT_ABI, *span, LINT_MSG);
+            emit_lint(cx, span, snippet);
         }
     }
 }
+
+fn is_extern_followed_by(item: &str, snippet: &str) -> bool {
+    let mut tokens = snippet.split_whitespace();
+
+    if let (_, Some(i)) = (tokens.next(), tokens.next())
+        && i.starts_with(item)
+    {
+        return true;
+    }
+    false
+}
+
+fn emit_lint(cx: &LateContext<'_>, span: Span, snippet: &str) {
+    let sugg = snippet.replacen(EXTERN, format!("{EXTERN} {ABI}").as_str(), 1);
+    span_lint_and_sugg(
+        cx,
+        EXTERN_WITHOUT_ABI,
+        span,
+        LINT_MSG,
+        LINT_HELP_MSG,
+        sugg,
+        Applicability::MachineApplicable,
+    );
+}
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -943,6 +943,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(move |_| Box::new(manual_div_ceil::ManualDivCeil::new(conf)));
     store.register_late_pass(|_| Box::new(manual_is_power_of_two::ManualIsPowerOfTwo));
     store.register_late_pass(|_| Box::new(non_zero_suggestions::NonZeroSuggestions));
-    store.register_early_pass(|| Box::new(extern_without_abi::ExternWithoutAbi));
+    store.register_late_pass(|_| Box::new(extern_without_abi::ExternWithoutAbi));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
diff --git a/tests/ui/extern_without_abi.fixed b/tests/ui/extern_without_abi.fixed
@@ -0,0 +1,60 @@
+//@aux-build:proc_macros.rs
+
+#![warn(clippy::extern_without_abi)]
+
+extern crate proc_macros;
+use proc_macros::{external, with_span};
+
+#[rustfmt::skip]
+extern "C" fn foo() {}
+//~^ ERROR: `extern` missing explicit ABI
+
+#[rustfmt::skip]
+extern "C" 	
+fn foo_two() {}
+//~^^ ERROR: `extern` missing explicit ABI
+
+extern "C" fn bar() {}
+
+#[rustfmt::skip]
+extern 	
+"C" 
+fn bar_two() {}
+
+extern "system" fn baz() {}
+
+#[rustfmt::skip]
+extern "C" {
+//~^ ERROR: `extern` missing explicit ABI
+    fn qux();
+}
+
+#[rustfmt::skip]
+extern "C" 	
+{
+//~^^ ERROR: `extern` missing explicit ABI
+    fn qux_two();
+}
+
+#[rustfmt::skip]
+extern "C" {fn qux_three();}
+//~^ ERROR: `extern` missing explicit ABI
+
+extern "C" {
+    fn grault();
+}
+
+extern "system" {
+    fn grault_two();
+}
+
+external! {
+    extern fn waldo() {}
+}
+
+with_span! {
+    span
+    extern fn waldo_two() {}
+}
+
+fn main() {}
diff --git a/tests/ui/extern_without_abi.rs b/tests/ui/extern_without_abi.rs
@@ -1,31 +1,60 @@
+//@aux-build:proc_macros.rs
+
 #![warn(clippy::extern_without_abi)]
 
-fn main() {}
+extern crate proc_macros;
+use proc_macros::{external, with_span};
 
 #[rustfmt::skip]
 extern fn foo() {}
 //~^ ERROR: `extern` missing explicit ABI
 
+#[rustfmt::skip]
+extern 	
+fn foo_two() {}
+//~^^ ERROR: `extern` missing explicit ABI
+
 extern "C" fn bar() {}
 
+#[rustfmt::skip]
+extern 	
+"C" 
+fn bar_two() {}
+
 extern "system" fn baz() {}
 
 #[rustfmt::skip]
 extern {
 //~^ ERROR: `extern` missing explicit ABI
-    static QUX: std::ffi::c_int;
+    fn qux();
+}
 
-    fn quux();
+#[rustfmt::skip]
+extern 	
+{
+//~^^ ERROR: `extern` missing explicit ABI
+    fn qux_two();
 }
 
-extern "C" {
-    static CORGE: std::ffi::c_int;
+#[rustfmt::skip]
+extern {fn qux_three();}
+//~^ ERROR: `extern` missing explicit ABI
 
+extern "C" {
     fn grault();
 }
 
 extern "system" {
-    static GARPLY: std::ffi::c_int;
+    fn grault_two();
+}
 
-    fn waldo();
+external! {
+    extern fn waldo() {}
 }
+
+with_span! {
+    span
+    extern fn waldo_two() {}
+}
+
+fn main() {}
diff --git a/tests/ui/extern_without_abi.stderr b/tests/ui/extern_without_abi.stderr
@@ -1,21 +1,66 @@
 error: `extern` missing explicit ABI
-  --> tests/ui/extern_without_abi.rs:6:1
+  --> tests/ui/extern_without_abi.rs:9:1
    |
 LL | extern fn foo() {}
-   | ^^^^^^
+   | ^^^^^^^^^^^^^^^^^^ help: consider using: `extern "C" fn foo() {}`
    |
    = note: `-D clippy::extern-without-abi` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::extern_without_abi)]`
 
 error: `extern` missing explicit ABI
-  --> tests/ui/extern_without_abi.rs:14:1
+  --> tests/ui/extern_without_abi.rs:13:1
+   |
+LL | / extern     
+LL | | fn foo_two() {}
+   | |_______________^
+   |
+help: consider using
+   |
+LL ~ extern "C"     
+LL + fn foo_two() {}
+   |
+
+error: `extern` missing explicit ABI
+  --> tests/ui/extern_without_abi.rs:27:1
    |
 LL | / extern {
 LL | |
-LL | |     static QUX: std::ffi::c_int;
-...  |
+LL | |     fn qux();
 LL | | }
    | |_^
+   |
+help: consider using
+   |
+LL + extern "C" {
+LL +
+LL +     fn qux();
+LL + }
+   |
+
+error: `extern` missing explicit ABI
+  --> tests/ui/extern_without_abi.rs:33:1
+   |
+LL | / extern     
+LL | | {
+LL | |
+LL | |     fn qux_two();
+LL | | }
+   | |_^
+   |
+help: consider using
+   |
+LL ~ extern "C"     
+LL + {
+LL +
+LL +     fn qux_two();
+LL + }
+   |
+
+error: `extern` missing explicit ABI
+  --> tests/ui/extern_without_abi.rs:40:1
+   |
+LL | extern {fn qux_three();}
+   | ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `extern "C" {fn qux_three();}`
 
-error: aborting due to 2 previous errors
+error: aborting due to 5 previous errors
 

Original file line number	Diff line number	Diff line change
`@@ -943,6 +943,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {`
`943`	`943`	`store.register_late_pass(move \|_\| Box::new(manual_div_ceil::ManualDivCeil::new(conf)));`
`944`	`944`	`store.register_late_pass(\|_\| Box::new(manual_is_power_of_two::ManualIsPowerOfTwo));`
`945`	`945`	`store.register_late_pass(\|_\| Box::new(non_zero_suggestions::NonZeroSuggestions));`
`946`		`- store.register_early_pass(\|\| Box::new(extern_without_abi::ExternWithoutAbi));`
	`946`	`+ store.register_late_pass(\|_\| Box::new(extern_without_abi::ExternWithoutAbi));`
`947`	`947`	// add lints here, do not remove this comment, it's used in `new_lint`
`948`	`948`	`}`