Auto merge of #70946 - jumbatm:clashing-extern-decl, r=nagisa

Add a lint to catch clashing `extern` fn declarations.

Closes #69390.

Adds lint `clashing_extern_decl` to detect when, within a single crate, an extern function of the same name is declared with different types. Because two symbols of the same name cannot be resolved to two different functions at link time, and one function cannot possibly have two types, a clashing extern declaration is almost certainly a mistake.

This lint does not run between crates because a project may have dependencies which both rely on the same extern function, but declare it in a different (but valid) way. For example, they may both declare an opaque type for one or more of the arguments (which would end up distinct types), or use types that are valid conversions in the language the extern fn is defined in. In these cases, we can't say that the clashing declaration is incorrect.

r? @eddyb

Author: bors

src/libcore/lib.rs

@@ -277,6 +277,9 @@ pub mod primitive;
 // crate uses the this crate as its libcore.
 #[path = "../stdarch/crates/core_arch/src/mod.rs"]
 #[allow(missing_docs, missing_debug_implementations, dead_code, unused_imports)]
+// FIXME: This annotation should be moved into rust-lang/stdarch after clashing_extern_decl is
+// merged. It currently cannot because bootstrap fails as the lint hasn't been defined yet.
+#[cfg_attr(not(bootstrap), allow(clashing_extern_decl))]
 #[unstable(feature = "stdsimd", issue = "48556")]
 mod core_arch;
 

src/librustc_lint/builtin.rs

@@ -26,15 +26,15 @@ use rustc_ast::attr::{self, HasAttrs};
 use rustc_ast::tokenstream::{TokenStream, TokenTree};
 use rustc_ast::visit::{FnCtxt, FnKind};
 use rustc_ast_pretty::pprust::{self, expr_to_string};
-use rustc_data_structures::fx::FxHashSet;
-use rustc_errors::{Applicability, DiagnosticBuilder};
+use rustc_data_structures::fx::{FxHashMap, FxHashSet};
+use rustc_errors::{Applicability, DiagnosticBuilder, DiagnosticStyledString};
 use rustc_feature::{deprecated_attributes, AttributeGate, AttributeTemplate, AttributeType};
 use rustc_feature::{GateIssue, Stability};
 use rustc_hir as hir;
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::DefId;
-use rustc_hir::{GenericParamKind, PatKind};
-use rustc_hir::{HirIdSet, Node};
+use rustc_hir::{ForeignItemKind, GenericParamKind, PatKind};
+use rustc_hir::{HirId, HirIdSet, Node};
 use rustc_middle::lint::LintDiagnosticBuilder;
 use rustc_middle::ty::subst::GenericArgKind;
 use rustc_middle::ty::{self, Ty, TyCtxt};
@@ -48,7 +48,7 @@ use rustc_trait_selection::traits::misc::can_type_implement_copy;
 
 use crate::nonstandard_style::{method_context, MethodLateContext};
 
-use log::debug;
+use log::{debug, trace};
 use std::fmt::Write;
 
 // hardwired lints from librustc_middle
@@ -2053,3 +2053,224 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for InvalidValue {
         }
     }
 }
+
+declare_lint! {
+    pub CLASHING_EXTERN_DECL,
+    Warn,
+    "detects when an extern fn has been declared with the same name but different types"
+}
+
+pub struct ClashingExternDecl {
+    seen_decls: FxHashMap<Symbol, HirId>,
+}
+
+/// Differentiate between whether the name for an extern decl came from the link_name attribute or
+/// just from declaration itself. This is important because we don't want to report clashes on
+/// symbol name if they don't actually clash because one or the other links against a symbol with a
+/// different name.
+enum SymbolName {
+    /// The name of the symbol + the span of the annotation which introduced the link name.
+    Link(Symbol, Span),
+    /// No link name, so just the name of the symbol.
+    Normal(Symbol),
+}
+
+impl SymbolName {
+    fn get_name(&self) -> Symbol {
+        match self {
+            SymbolName::Link(s, _) | SymbolName::Normal(s) => *s,
+        }
+    }
+}
+
+impl ClashingExternDecl {
+    crate fn new() -> Self {
+        ClashingExternDecl { seen_decls: FxHashMap::default() }
+    }
+    /// Insert a new foreign item into the seen set. If a symbol with the same name already exists
+    /// for the item, return its HirId without updating the set.
+    fn insert(&mut self, tcx: TyCtxt<'_>, fi: &hir::ForeignItem<'_>) -> Option<HirId> {
+        let hid = fi.hir_id;
+
+        let name =
+            &tcx.codegen_fn_attrs(tcx.hir().local_def_id(hid)).link_name.unwrap_or(fi.ident.name);
+
+        if self.seen_decls.contains_key(name) {
+            // Avoid updating the map with the new entry when we do find a collision. We want to
+            // make sure we're always pointing to the first definition as the previous declaration.
+            // This lets us avoid emitting "knock-on" diagnostics.
+            Some(*self.seen_decls.get(name).unwrap())
+        } else {
+            self.seen_decls.insert(*name, hid)
+        }
+    }
+
+    /// Get the name of the symbol that's linked against for a given extern declaration. That is,
+    /// the name specified in a #[link_name = ...] attribute if one was specified, else, just the
+    /// symbol's name.
+    fn name_of_extern_decl(tcx: TyCtxt<'_>, fi: &hir::ForeignItem<'_>) -> SymbolName {
+        let did = tcx.hir().local_def_id(fi.hir_id);
+        if let Some((overridden_link_name, overridden_link_name_span)) =
+            tcx.codegen_fn_attrs(did).link_name.map(|overridden_link_name| {
+                // FIXME: Instead of searching through the attributes again to get span
+                // information, we could have codegen_fn_attrs also give span information back for
+                // where the attribute was defined. However, until this is found to be a
+                // bottleneck, this does just fine.
+                (
+                    overridden_link_name,
+                    tcx.get_attrs(did.to_def_id())
+                        .iter()
+                        .find(|at| at.check_name(sym::link_name))
+                        .unwrap()
+                        .span,
+                )
+            })
+        {
+            SymbolName::Link(overridden_link_name, overridden_link_name_span)
+        } else {
+            SymbolName::Normal(fi.ident.name)
+        }
+    }
+
+    /// Checks whether two types are structurally the same enough that the declarations shouldn't
+    /// clash. We need this so we don't emit a lint when two modules both declare an extern struct,
+    /// with the same members (as the declarations shouldn't clash).
+    fn structurally_same_type<'a, 'tcx>(
+        cx: &LateContext<'a, 'tcx>,
+        a: Ty<'tcx>,
+        b: Ty<'tcx>,
+    ) -> bool {
+        let tcx = cx.tcx;
+        if a == b || rustc_middle::ty::TyS::same_type(a, b) {
+            // All nominally-same types are structurally same, too.
+            true
+        } else {
+            // Do a full, depth-first comparison between the two.
+            use rustc_middle::ty::TyKind::*;
+            let a_kind = &a.kind;
+            let b_kind = &b.kind;
+
+            match (a_kind, b_kind) {
+                (Adt(..), Adt(..)) => {
+                    // Adts are pretty straightforward: just compare the layouts.
+                    use rustc_target::abi::LayoutOf;
+                    let a_layout = cx.layout_of(a).unwrap().layout;
+                    let b_layout = cx.layout_of(b).unwrap().layout;
+                    a_layout == b_layout
+                }
+                (Array(a_ty, a_const), Array(b_ty, b_const)) => {
+                    // For arrays, we also check the constness of the type.
+                    a_const.val == b_const.val
+                        && Self::structurally_same_type(cx, a_const.ty, b_const.ty)
+                        && Self::structurally_same_type(cx, a_ty, b_ty)
+                }
+                (Slice(a_ty), Slice(b_ty)) => Self::structurally_same_type(cx, a_ty, b_ty),
+                (RawPtr(a_tymut), RawPtr(b_tymut)) => {
+                    a_tymut.mutbl == a_tymut.mutbl
+                        && Self::structurally_same_type(cx, &a_tymut.ty, &b_tymut.ty)
+                }
+                (Ref(_a_region, a_ty, a_mut), Ref(_b_region, b_ty, b_mut)) => {
+                    // For structural sameness, we don't need the region to be same.
+                    a_mut == b_mut && Self::structurally_same_type(cx, a_ty, b_ty)
+                }
+                (FnDef(..), FnDef(..)) => {
+                    // As we don't compare regions, skip_binder is fine.
+                    let a_poly_sig = a.fn_sig(tcx);
+                    let b_poly_sig = b.fn_sig(tcx);
+
+                    let a_sig = a_poly_sig.skip_binder();
+                    let b_sig = b_poly_sig.skip_binder();
+
+                    (a_sig.abi, a_sig.unsafety, a_sig.c_variadic)
+                        == (b_sig.abi, b_sig.unsafety, b_sig.c_variadic)
+                        && a_sig.inputs().iter().eq_by(b_sig.inputs().iter(), |a, b| {
+                            Self::structurally_same_type(cx, a, b)
+                        })
+                        && Self::structurally_same_type(cx, a_sig.output(), b_sig.output())
+                }
+                (Tuple(a_substs), Tuple(b_substs)) => {
+                    a_substs.types().eq_by(b_substs.types(), |a_ty, b_ty| {
+                        Self::structurally_same_type(cx, a_ty, b_ty)
+                    })
+                }
+                // For these, it's not quite as easy to define structural-sameness quite so easily.
+                // For the purposes of this lint, take the conservative approach and mark them as
+                // not structurally same.
+                (Dynamic(..), Dynamic(..))
+                | (Error(..), Error(..))
+                | (Closure(..), Closure(..))
+                | (Generator(..), Generator(..))
+                | (GeneratorWitness(..), GeneratorWitness(..))
+                | (Projection(..), Projection(..))
+                | (Opaque(..), Opaque(..)) => false,
+                // These definitely should have been caught above.
+                (Bool, Bool) | (Char, Char) | (Never, Never) | (Str, Str) => unreachable!(),
+                _ => false,
+            }
+        }
+    }
+}
+
+impl_lint_pass!(ClashingExternDecl => [CLASHING_EXTERN_DECL]);
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ClashingExternDecl {
+    fn check_foreign_item(&mut self, cx: &LateContext<'a, 'tcx>, this_fi: &hir::ForeignItem<'_>) {
+        trace!("ClashingExternDecl: check_foreign_item: {:?}", this_fi);
+        if let ForeignItemKind::Fn(..) = this_fi.kind {
+            let tcx = *&cx.tcx;
+            if let Some(existing_hid) = self.insert(tcx, this_fi) {
+                let existing_decl_ty = tcx.type_of(tcx.hir().local_def_id(existing_hid));
+                let this_decl_ty = tcx.type_of(tcx.hir().local_def_id(this_fi.hir_id));
+                debug!(
+                    "ClashingExternDecl: Comparing existing {:?}: {:?} to this {:?}: {:?}",
+                    existing_hid, existing_decl_ty, this_fi.hir_id, this_decl_ty
+                );
+                // Check that the declarations match.
+                if !Self::structurally_same_type(cx, existing_decl_ty, this_decl_ty) {
+                    let orig_fi = tcx.hir().expect_foreign_item(existing_hid);
+                    let orig = Self::name_of_extern_decl(tcx, orig_fi);
+
+                    // We want to ensure that we use spans for both decls that include where the
+                    // name was defined, whether that was from the link_name attribute or not.
+                    let get_relevant_span =
+                        |fi: &hir::ForeignItem<'_>| match Self::name_of_extern_decl(tcx, fi) {
+                            SymbolName::Normal(_) => fi.span,
+                            SymbolName::Link(_, annot_span) => fi.span.to(annot_span),
+                        };
+                    // Finally, emit the diagnostic.
+                    tcx.struct_span_lint_hir(
+                        CLASHING_EXTERN_DECL,
+                        this_fi.hir_id,
+                        get_relevant_span(this_fi),
+                        |lint| {
+                            let mut expected_str = DiagnosticStyledString::new();
+                            expected_str.push(existing_decl_ty.fn_sig(tcx).to_string(), false);
+                            let mut found_str = DiagnosticStyledString::new();
+                            found_str.push(this_decl_ty.fn_sig(tcx).to_string(), true);
+
+                            lint.build(&format!(
+                                "`{}` redeclare{} with a different signature",
+                                this_fi.ident.name,
+                                if orig.get_name() == this_fi.ident.name {
+                                    "d".to_string()
+                                } else {
+                                    format!("s `{}`", orig.get_name())
+                                }
+                            ))
+                            .span_label(
+                                get_relevant_span(orig_fi),
+                                &format!("`{}` previously declared here", orig.get_name()),
+                            )
+                            .span_label(
+                                get_relevant_span(this_fi),
+                                "this signature doesn't match the previous declaration",
+                            )
+                            .note_expected_found(&"", expected_str, &"", found_str)
+                            .emit()
+                        },
+                    );
+                }
+            }
+        }
+    }
+}

src/librustc_lint/lib.rs

@@ -30,6 +30,7 @@
 #![feature(bool_to_option)]
 #![feature(box_syntax)]
 #![feature(crate_visibility_modifier)]
+#![feature(iter_order_by)]
 #![feature(never_type)]
 #![feature(nll)]
 #![feature(or_patterns)]
@@ -154,6 +155,7 @@ macro_rules! late_lint_passes {
                 // and change this to a module lint pass
                 MissingDebugImplementations: MissingDebugImplementations::default(),
                 ArrayIntoIter: ArrayIntoIter,
+                ClashingExternDecl: ClashingExternDecl::new(),
             ]
         );
     };

src/libstd/sys/unix/args.rs

@@ -205,13 +205,15 @@ mod imp {
         #[cfg(target_arch = "aarch64")]
         extern "C" {
             fn objc_msgSend(obj: NsId, sel: Sel) -> NsId;
+            #[cfg_attr(not(bootstrap), allow(clashing_extern_decl))]
             #[link_name = "objc_msgSend"]
             fn objc_msgSend_ul(obj: NsId, sel: Sel, i: libc::c_ulong) -> NsId;
         }
 
         #[cfg(not(target_arch = "aarch64"))]
         extern "C" {
             fn objc_msgSend(obj: NsId, sel: Sel, ...) -> NsId;
+            #[cfg_attr(not(bootstrap), allow(clashing_extern_decl))]
             #[link_name = "objc_msgSend"]
             fn objc_msgSend_ul(obj: NsId, sel: Sel, ...) -> NsId;
         }

src/test/ui/issues/issue-1866.rs

@@ -1,6 +1,7 @@
 // build-pass
 #![allow(dead_code)]
 #![allow(non_camel_case_types)]
+#![warn(clashing_extern_decl)]
 
 // pretty-expanded FIXME #23616
 
@@ -20,6 +21,7 @@ mod b {
         use super::rust_task;
         extern {
             pub fn rust_task_is_unwinding(rt: *const rust_task) -> bool;
+            //~^ WARN `rust_task_is_unwinding` redeclared with a different signature
         }
     }
 }

src/test/ui/issues/issue-1866.stderr

@@ -0,0 +1,19 @@
+warning: `rust_task_is_unwinding` redeclared with a different signature
+  --> $DIR/issue-1866.rs:23:13
+   |
+LL |             pub fn rust_task_is_unwinding(rt: *const rust_task) -> bool;
+   |             ------------------------------------------------------------ `rust_task_is_unwinding` previously declared here
+...
+LL |             pub fn rust_task_is_unwinding(rt: *const rust_task) -> bool;
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
+   |
+note: the lint level is defined here
+  --> $DIR/issue-1866.rs:4:9
+   |
+LL | #![warn(clashing_extern_decl)]
+   |         ^^^^^^^^^^^^^^^^^^^^
+   = note: expected `unsafe extern "C" fn(*const usize) -> bool`
+              found `unsafe extern "C" fn(*const bool) -> bool`
+
+warning: 1 warning emitted
+

src/test/ui/issues/issue-5791.rs

@@ -1,11 +1,13 @@
 // run-pass
 #![allow(dead_code)]
+#![warn(clashing_extern_decl)]
 // pretty-expanded FIXME #23616
 
 extern {
     #[link_name = "malloc"]
     fn malloc1(len: i32) -> *const u8;
     #[link_name = "malloc"]
+    //~^ WARN `malloc2` redeclares `malloc` with a different signature
     fn malloc2(len: i32, foo: i32) -> *const u8;
 }
 

src/test/ui/issues/issue-5791.stderr

@@ -0,0 +1,21 @@
+warning: `malloc2` redeclares `malloc` with a different signature
+  --> $DIR/issue-5791.rs:9:5
+   |
+LL | /     #[link_name = "malloc"]
+LL | |     fn malloc1(len: i32) -> *const u8;
+   | |______________________________________- `malloc` previously declared here
+LL | /     #[link_name = "malloc"]
+LL | |
+LL | |     fn malloc2(len: i32, foo: i32) -> *const u8;
+   | |________________________________________________^ this signature doesn't match the previous declaration
+   |
+note: the lint level is defined here
+  --> $DIR/issue-5791.rs:3:9
+   |
+LL | #![warn(clashing_extern_decl)]
+   |         ^^^^^^^^^^^^^^^^^^^^
+   = note: expected `unsafe extern "C" fn(i32) -> *const u8`
+              found `unsafe extern "C" fn(i32, i32) -> *const u8`
+
+warning: 1 warning emitted
+

src/test/ui/lint/auxiliary/external_extern_fn.rs

@@ -0,0 +1,3 @@
+extern {
+    pub fn extern_fn(x: u8);
+}