Auto merge of rust-lang#86492 - hyd-dev:no-mangle-method, r=petrochenkov

Associated functions that contain extern indicator or have `#[rustc_std_internal_symbol]` are reachable

Previously these fails to link with ``undefined reference to `foo'``:

<details>
<summary>Example 1</summary>

```rs
struct AssocFn;

impl AssocFn {
    #[no_mangle]
    fn foo() {}
}

fn main() {
    extern "Rust" {
        fn foo();
    }
    unsafe { foo() }
}
```
([Playground](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=f1244afcdd26e2a28445f6e82ca46b50))
</details>

<details>
<summary>Example 2</summary>

```rs
#![crate_name = "lib"]
#![crate_type = "lib"]

struct AssocFn;

impl AssocFn {
    #[no_mangle]
    fn foo() {}
}
```
```rs
extern crate lib;

fn main() {
    extern "Rust" {
        fn foo();
    }
    unsafe { foo() }
}
```
</details>

But I believe they should link successfully, because this works:
<details>

```rs
#[no_mangle]
fn foo() {}

fn main() {
    extern "Rust" {
        fn foo();
    }
    unsafe { foo() }
}
```
([Playground](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=789b3f283ee6126f53939429103ed98d))
</details>

This PR fixes the problem, by adding associated functions that have "custom linkage" to `reachable_set`, just like normal functions.

I haven't tested whether rust-lang#76211 and [Miri](rust-lang/miri#1837) are fixed by this PR yet, but I'm submitting this anyway since this fixes the examples above.

I added a `run-pass` test that combines my two examples above, but I'm not sure if that's the right way to test this. Maybe I should add / modify an existing codegen test (`src/test/codegen/export-no-mangle.rs`?) instead?

Author: bors

compiler/rustc_ast_passes/src/ast_validation.rs

@@ -1499,6 +1499,10 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
     }
 
     fn visit_assoc_item(&mut self, item: &'a AssocItem, ctxt: AssocCtxt) {
+        if self.session.contains_name(&item.attrs, sym::no_mangle) {
+            self.check_nomangle_item_asciionly(item.ident, item.span);
+        }
+
         if ctxt == AssocCtxt::Trait || !self.in_trait_impl {
             self.check_defaultness(item.span, item.kind.defaultness());
         }

compiler/rustc_lint/src/builtin.rs

@@ -417,6 +417,25 @@ impl EarlyLintPass for UnsafeCode {
         }
     }
 
+    fn check_impl_item(&mut self, cx: &EarlyContext<'_>, it: &ast::AssocItem) {
+        if let ast::AssocItemKind::Fn(..) = it.kind {
+            if let Some(attr) = cx.sess().find_by_name(&it.attrs, sym::no_mangle) {
+                self.report_overriden_symbol_name(
+                    cx,
+                    attr.span,
+                    "declaration of a `no_mangle` method",
+                );
+            }
+            if let Some(attr) = cx.sess().find_by_name(&it.attrs, sym::export_name) {
+                self.report_overriden_symbol_name(
+                    cx,
+                    attr.span,
+                    "declaration of a method with `export_name`",
+                );
+            }
+        }
+    }
+
     fn check_fn(&mut self, cx: &EarlyContext<'_>, fk: FnKind<'_>, span: Span, _: ast::NodeId) {
         if let FnKind::Fn(
             ctxt,
@@ -1115,31 +1134,37 @@ declare_lint_pass!(InvalidNoMangleItems => [NO_MANGLE_CONST_ITEMS, NO_MANGLE_GEN
 impl<'tcx> LateLintPass<'tcx> for InvalidNoMangleItems {
     fn check_item(&mut self, cx: &LateContext<'_>, it: &hir::Item<'_>) {
         let attrs = cx.tcx.hir().attrs(it.hir_id());
+        let check_no_mangle_on_generic_fn = |no_mangle_attr: &ast::Attribute,
+                                             impl_generics: Option<&hir::Generics<'_>>,
+                                             generics: &hir::Generics<'_>,
+                                             span| {
+            for param in
+                generics.params.iter().chain(impl_generics.map(|g| g.params).into_iter().flatten())
+            {
+                match param.kind {
+                    GenericParamKind::Lifetime { .. } => {}
+                    GenericParamKind::Type { .. } | GenericParamKind::Const { .. } => {
+                        cx.struct_span_lint(NO_MANGLE_GENERIC_ITEMS, span, |lint| {
+                            lint.build("functions generic over types or consts must be mangled")
+                                .span_suggestion_short(
+                                    no_mangle_attr.span,
+                                    "remove this attribute",
+                                    String::new(),
+                                    // Use of `#[no_mangle]` suggests FFI intent; correct
+                                    // fix may be to monomorphize source by hand
+                                    Applicability::MaybeIncorrect,
+                                )
+                                .emit();
+                        });
+                        break;
+                    }
+                }
+            }
+        };
         match it.kind {
             hir::ItemKind::Fn(.., ref generics, _) => {
                 if let Some(no_mangle_attr) = cx.sess().find_by_name(attrs, sym::no_mangle) {
-                    for param in generics.params {
-                        match param.kind {
-                            GenericParamKind::Lifetime { .. } => {}
-                            GenericParamKind::Type { .. } | GenericParamKind::Const { .. } => {
-                                cx.struct_span_lint(NO_MANGLE_GENERIC_ITEMS, it.span, |lint| {
-                                    lint.build(
-                                        "functions generic over types or consts must be mangled",
-                                    )
-                                    .span_suggestion_short(
-                                        no_mangle_attr.span,
-                                        "remove this attribute",
-                                        String::new(),
-                                        // Use of `#[no_mangle]` suggests FFI intent; correct
-                                        // fix may be to monomorphize source by hand
-                                        Applicability::MaybeIncorrect,
-                                    )
-                                    .emit();
-                                });
-                                break;
-                            }
-                        }
-                    }
+                    check_no_mangle_on_generic_fn(no_mangle_attr, None, generics, it.span);
                 }
             }
             hir::ItemKind::Const(..) => {
@@ -1170,6 +1195,23 @@ impl<'tcx> LateLintPass<'tcx> for InvalidNoMangleItems {
                     });
                 }
             }
+            hir::ItemKind::Impl(hir::Impl { ref generics, items, .. }) => {
+                for it in items {
+                    if let hir::AssocItemKind::Fn { .. } = it.kind {
+                        if let Some(no_mangle_attr) = cx
+                            .sess()
+                            .find_by_name(cx.tcx.hir().attrs(it.id.hir_id()), sym::no_mangle)
+                        {
+                            check_no_mangle_on_generic_fn(
+                                no_mangle_attr,
+                                Some(generics),
+                                cx.tcx.hir().get_generics(it.id.def_id.to_def_id()).unwrap(),
+                                it.span,
+                            );
+                        }
+                    }
+                }
+            }
             _ => {}
         }
     }

compiler/rustc_lint/src/nonstandard_style.rs

@@ -391,9 +391,14 @@ impl<'tcx> LateLintPass<'tcx> for NonSnakeCase {
         _: Span,
         id: hir::HirId,
     ) {
+        let attrs = cx.tcx.hir().attrs(id);
         match &fk {
-            FnKind::Method(ident, ..) => match method_context(cx, id) {
+            FnKind::Method(ident, sig, ..) => match method_context(cx, id) {
                 MethodLateContext::PlainImpl => {
+                    if sig.header.abi != Abi::Rust && cx.sess().contains_name(attrs, sym::no_mangle)
+                    {
+                        return;
+                    }
                     self.check_snake_case(cx, "method", ident);
                 }
                 MethodLateContext::TraitAutoImpl => {
@@ -402,7 +407,6 @@ impl<'tcx> LateLintPass<'tcx> for NonSnakeCase {
                 _ => (),
             },
             FnKind::ItemFn(ident, _, header, _) => {
-                let attrs = cx.tcx.hir().attrs(id);
                 // Skip foreign-ABI #[no_mangle] functions (Issue #31924)
                 if header.abi != Abi::Rust && cx.sess().contains_name(attrs, sym::no_mangle) {
                     return;

compiler/rustc_passes/src/check_attr.rs

@@ -962,6 +962,10 @@ impl CheckAttrVisitor<'tcx> {
         }
     }
 
+    fn is_impl_item(&self, hir_id: HirId) -> bool {
+        matches!(self.tcx.hir().get(hir_id), hir::Node::ImplItem(..))
+    }
+
     /// Checks if `#[export_name]` is applied to a function or static. Returns `true` if valid.
     fn check_export_name(
         &self,
@@ -971,7 +975,8 @@ impl CheckAttrVisitor<'tcx> {
         target: Target,
     ) -> bool {
         match target {
-            Target::Static | Target::Fn | Target::Method(..) => true,
+            Target::Static | Target::Fn => true,
+            Target::Method(..) if self.is_impl_item(hir_id) => true,
             // FIXME(#80564): We permit struct fields, match arms and macro defs to have an
             // `#[export_name]` attribute with just a lint, because we previously
             // erroneously allowed it and some crates used it accidentally, to to be compatible
@@ -985,9 +990,9 @@ impl CheckAttrVisitor<'tcx> {
                     .sess
                     .struct_span_err(
                         attr.span,
-                        "attribute should be applied to a function or static",
+                        "attribute should be applied to a free function, impl method or static",
                     )
-                    .span_label(*span, "not a function or static")
+                    .span_label(*span, "not a free function, impl method or static")
                     .emit();
                 false
             }
@@ -1169,7 +1174,8 @@ impl CheckAttrVisitor<'tcx> {
     /// Checks if `#[no_mangle]` is applied to a function or static.
     fn check_no_mangle(&self, hir_id: HirId, attr: &Attribute, span: &Span, target: Target) {
         match target {
-            Target::Static | Target::Fn | Target::Method(..) => {}
+            Target::Static | Target::Fn => {}
+            Target::Method(..) if self.is_impl_item(hir_id) => {}
             // FIXME(#80564): We permit struct fields, match arms and macro defs to have an
             // `#[no_mangle]` attribute with just a lint, because we previously
             // erroneously allowed it and some crates used it accidentally, to to be compatible
@@ -1181,14 +1187,16 @@ impl CheckAttrVisitor<'tcx> {
                 // FIXME: #[no_mangle] was previously allowed on non-functions/statics and some
                 // crates used this, so only emit a warning.
                 self.tcx.struct_span_lint_hir(UNUSED_ATTRIBUTES, hir_id, attr.span, |lint| {
-                    lint.build("attribute should be applied to a function or static")
-                        .warn(
-                            "this was previously accepted by the compiler but is \
-                             being phased out; it will become a hard error in \
-                             a future release!",
-                        )
-                        .span_label(*span, "not a function or static")
-                        .emit();
+                    lint.build(
+                        "attribute should be applied to a free function, impl method or static",
+                    )
+                    .warn(
+                        "this was previously accepted by the compiler but is \
+                         being phased out; it will become a hard error in \
+                         a future release!",
+                    )
+                    .span_label(*span, "not a free function, impl method or static")
+                    .emit();
                 });
             }
         }

compiler/rustc_passes/src/reachable.rs

@@ -211,13 +211,15 @@ impl<'tcx> ReachableContext<'tcx> {
         if !self.any_library {
             // If we are building an executable, only explicitly extern
             // types need to be exported.
-            if let Node::Item(item) = *node {
-                let reachable = if let hir::ItemKind::Fn(ref sig, ..) = item.kind {
-                    sig.header.abi != Abi::Rust
-                } else {
-                    false
-                };
-                let codegen_attrs = self.tcx.codegen_fn_attrs(item.def_id);
+            if let Node::Item(hir::Item { kind: hir::ItemKind::Fn(sig, ..), def_id, .. })
+            | Node::ImplItem(hir::ImplItem {
+                kind: hir::ImplItemKind::Fn(sig, ..),
+                def_id,
+                ..
+            }) = *node
+            {
+                let reachable = sig.header.abi != Abi::Rust;
+                let codegen_attrs = self.tcx.codegen_fn_attrs(*def_id);
                 let is_extern = codegen_attrs.contains_extern_indicator();
                 let std_internal =
                     codegen_attrs.flags.contains(CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL);
@@ -335,17 +337,23 @@ struct CollectPrivateImplItemsVisitor<'a, 'tcx> {
     worklist: &'a mut Vec<LocalDefId>,
 }
 
-impl<'a, 'tcx> ItemLikeVisitor<'tcx> for CollectPrivateImplItemsVisitor<'a, 'tcx> {
-    fn visit_item(&mut self, item: &hir::Item<'_>) {
+impl CollectPrivateImplItemsVisitor<'_, '_> {
+    fn push_to_worklist_if_has_custom_linkage(&mut self, def_id: LocalDefId) {
         // Anything which has custom linkage gets thrown on the worklist no
         // matter where it is in the crate, along with "special std symbols"
         // which are currently akin to allocator symbols.
-        let codegen_attrs = self.tcx.codegen_fn_attrs(item.def_id);
+        let codegen_attrs = self.tcx.codegen_fn_attrs(def_id);
         if codegen_attrs.contains_extern_indicator()
             || codegen_attrs.flags.contains(CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL)
         {
-            self.worklist.push(item.def_id);
+            self.worklist.push(def_id);
         }
+    }
+}
+
+impl<'a, 'tcx> ItemLikeVisitor<'tcx> for CollectPrivateImplItemsVisitor<'a, 'tcx> {
+    fn visit_item(&mut self, item: &hir::Item<'_>) {
+        self.push_to_worklist_if_has_custom_linkage(item.def_id);
 
         // We need only trait impls here, not inherent impls, and only non-exported ones
         if let hir::ItemKind::Impl(hir::Impl { of_trait: Some(ref trait_ref), ref items, .. }) =
@@ -375,8 +383,8 @@ impl<'a, 'tcx> ItemLikeVisitor<'tcx> for CollectPrivateImplItemsVisitor<'a, 'tcx
 
     fn visit_trait_item(&mut self, _trait_item: &hir::TraitItem<'_>) {}
 
-    fn visit_impl_item(&mut self, _impl_item: &hir::ImplItem<'_>) {
-        // processed in visit_item above
+    fn visit_impl_item(&mut self, impl_item: &hir::ImplItem<'_>) {
+        self.push_to_worklist_if_has_custom_linkage(impl_item.def_id);
     }
 
     fn visit_foreign_item(&mut self, _foreign_item: &hir::ForeignItem<'_>) {

src/test/ui/auxiliary/no-mangle-associated-fn.rs

@@ -0,0 +1,21 @@
+#![crate_type = "lib"]
+
+struct Bar;
+
+impl Bar {
+    #[no_mangle]
+    fn bar() -> u8 {
+        2
+    }
+}
+
+trait Foo {
+    fn baz() -> u8;
+}
+
+impl Foo for Bar {
+    #[no_mangle]
+    fn baz() -> u8 {
+        3
+    }
+}

src/test/ui/feature-gates/issue-43106-gating-of-builtin-attrs-error.rs

@@ -1,5 +1,5 @@
 //~ NOTE: not an `extern crate` item
-//~^ NOTE: not a function or static
+//~^ NOTE: not a free function, impl method or static
 //~^^ NOTE: not a function or closure
 // This is testing whether various builtin attributes signals an
 // error or warning when put in "weird" places.
@@ -25,7 +25,7 @@
 #![no_link]
 //~^ ERROR: attribute should be applied to an `extern crate` item
 #![export_name = "2200"]
-//~^ ERROR: attribute should be applied to a function or static
+//~^ ERROR: attribute should be applied to a free function, impl method or static
 #![inline]
 //~^ ERROR: attribute should be applied to function or closure
 #[inline]
@@ -83,27 +83,37 @@ mod no_link {
 }
 
 #[export_name = "2200"]
-//~^ ERROR attribute should be applied to a function or static
+//~^ ERROR attribute should be applied to a free function, impl method or static
 mod export_name {
-    //~^ NOTE not a function or static
+    //~^ NOTE not a free function, impl method or static
 
     mod inner { #![export_name="2200"] }
-    //~^ ERROR attribute should be applied to a function or static
-    //~| NOTE not a function or static
+    //~^ ERROR attribute should be applied to a free function, impl method or static
+    //~| NOTE not a free function, impl method or static
 
     #[export_name = "2200"] fn f() { }
 
     #[export_name = "2200"] struct S;
-    //~^ ERROR attribute should be applied to a function or static
-    //~| NOTE not a function or static
+    //~^ ERROR attribute should be applied to a free function, impl method or static
+    //~| NOTE not a free function, impl method or static
 
     #[export_name = "2200"] type T = S;
-    //~^ ERROR attribute should be applied to a function or static
-    //~| NOTE not a function or static
+    //~^ ERROR attribute should be applied to a free function, impl method or static
+    //~| NOTE not a free function, impl method or static
 
     #[export_name = "2200"] impl S { }
-    //~^ ERROR attribute should be applied to a function or static
-    //~| NOTE not a function or static
+    //~^ ERROR attribute should be applied to a free function, impl method or static
+    //~| NOTE not a free function, impl method or static
+
+    trait Tr {
+        #[export_name = "2200"] fn foo();
+        //~^ ERROR attribute should be applied to a free function, impl method or static
+        //~| NOTE not a free function, impl method or static
+
+        #[export_name = "2200"] fn bar() {}
+        //~^ ERROR attribute should be applied to a free function, impl method or static
+        //~| NOTE not a free function, impl method or static
+    }
 }
 
 #[start]