Rollup merge of #109923 - ElectrifyPro:visibility, r=wesleywiser

Update `error [E0449]: unnecessary visibility qualifier` to be more clear

This updates the error message `error[E0449]: unnecessary visibility qualifier` by clearly indicating that visibility qualifiers already inherit their visibility from a parent item. The error message previously implied that the qualifiers were permitted, which is not the case anymore.

Resolves #109822.

Author: JohnTitor

compiler/rustc_ast_passes/messages.ftl

@@ -17,9 +17,10 @@ ast_passes_keyword_lifetime =
 ast_passes_invalid_label =
     invalid label name `{$name}`
 
-ast_passes_invalid_visibility =
-    unnecessary visibility qualifier
-    .implied = `pub` not permitted here because it's implied
+ast_passes_visibility_not_permitted =
+    visibility qualifiers are not permitted here
+    .enum_variant = enum variants and their fields always share the visibility of the enum they are in
+    .trait_impl = trait items always share the visibility of their trait
     .individual_impl_items = place qualifiers on individual impl items instead
     .individual_foreign_items = place qualifiers on individual foreign items instead
 

compiler/rustc_ast_passes/src/ast_validation.rs

@@ -240,16 +240,12 @@ impl<'a> AstValidator<'a> {
         }
     }
 
-    fn invalid_visibility(&self, vis: &Visibility, note: Option<errors::InvalidVisibilityNote>) {
+    fn visibility_not_permitted(&self, vis: &Visibility, note: errors::VisibilityNotPermittedNote) {
         if let VisibilityKind::Inherited = vis.kind {
             return;
         }
 
-        self.session.emit_err(errors::InvalidVisibility {
-            span: vis.span,
-            implied: vis.kind.is_pub().then_some(vis.span),
-            note,
-        });
+        self.session.emit_err(errors::VisibilityNotPermitted { span: vis.span, note });
     }
 
     fn check_decl_no_pat(decl: &FnDecl, mut report_err: impl FnMut(Span, Option<Ident>, bool)) {
@@ -819,7 +815,10 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
                 items,
             }) => {
                 self.with_in_trait_impl(true, Some(*constness), |this| {
-                    this.invalid_visibility(&item.vis, None);
+                    this.visibility_not_permitted(
+                        &item.vis,
+                        errors::VisibilityNotPermittedNote::TraitImpl,
+                    );
                     if let TyKind::Err = self_ty.kind {
                         this.err_handler().emit_err(errors::ObsoleteAuto { span: item.span });
                     }
@@ -866,9 +865,9 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
                         only_trait: only_trait.then_some(()),
                     };
 
-                self.invalid_visibility(
+                self.visibility_not_permitted(
                     &item.vis,
-                    Some(errors::InvalidVisibilityNote::IndividualImplItems),
+                    errors::VisibilityNotPermittedNote::IndividualImplItems,
                 );
                 if let &Unsafe::Yes(span) = unsafety {
                     self.err_handler().emit_err(errors::InherentImplCannotUnsafe {
@@ -924,9 +923,9 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
             }
             ItemKind::ForeignMod(ForeignMod { abi, unsafety, .. }) => {
                 let old_item = mem::replace(&mut self.extern_mod, Some(item));
-                self.invalid_visibility(
+                self.visibility_not_permitted(
                     &item.vis,
-                    Some(errors::InvalidVisibilityNote::IndividualForeignItems),
+                    errors::VisibilityNotPermittedNote::IndividualForeignItems,
                 );
                 if let &Unsafe::Yes(span) = unsafety {
                     self.err_handler().emit_err(errors::UnsafeItem { span, kind: "extern block" });
@@ -940,9 +939,15 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
             }
             ItemKind::Enum(def, _) => {
                 for variant in &def.variants {
-                    self.invalid_visibility(&variant.vis, None);
+                    self.visibility_not_permitted(
+                        &variant.vis,
+                        errors::VisibilityNotPermittedNote::EnumVariant,
+                    );
                     for field in variant.data.fields() {
-                        self.invalid_visibility(&field.vis, None);
+                        self.visibility_not_permitted(
+                            &field.vis,
+                            errors::VisibilityNotPermittedNote::EnumVariant,
+                        );
                     }
                 }
             }
@@ -1301,7 +1306,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
         }
 
         if ctxt == AssocCtxt::Trait || self.in_trait_impl {
-            self.invalid_visibility(&item.vis, None);
+            self.visibility_not_permitted(&item.vis, errors::VisibilityNotPermittedNote::TraitImpl);
             if let AssocItemKind::Fn(box Fn { sig, .. }) = &item.kind {
                 self.check_trait_fn_not_const(sig.header.constness);
             }

compiler/rustc_ast_passes/src/errors.rs

@@ -42,18 +42,20 @@ pub struct InvalidLabel {
 }
 
 #[derive(Diagnostic)]
-#[diag(ast_passes_invalid_visibility, code = "E0449")]
-pub struct InvalidVisibility {
+#[diag(ast_passes_visibility_not_permitted, code = "E0449")]
+pub struct VisibilityNotPermitted {
     #[primary_span]
     pub span: Span,
-    #[label(ast_passes_implied)]
-    pub implied: Option<Span>,
     #[subdiagnostic]
-    pub note: Option<InvalidVisibilityNote>,
+    pub note: VisibilityNotPermittedNote,
 }
 
 #[derive(Subdiagnostic)]
-pub enum InvalidVisibilityNote {
+pub enum VisibilityNotPermittedNote {
+    #[note(ast_passes_enum_variant)]
+    EnumVariant,
+    #[note(ast_passes_trait_impl)]
+    TraitImpl,
     #[note(ast_passes_individual_impl_items)]
     IndividualImplItems,
     #[note(ast_passes_individual_foreign_items)]

compiler/rustc_error_codes/src/error_codes/E0449.md

@@ -1,4 +1,6 @@
-A visibility qualifier was used when it was unnecessary.
+A visibility qualifier was used where one is not permitted. Visibility
+qualifiers are not permitted on enum variants, trait items, impl blocks, and
+extern blocks, as they already share the visibility of the parent item.
 
 Erroneous code examples:
 
@@ -9,15 +11,18 @@ trait Foo {
     fn foo();
 }
 
-pub impl Bar {} // error: unnecessary visibility qualifier
+enum Baz {
+    pub Qux, // error: visibility qualifiers are not permitted here
+}
+
+pub impl Bar {} // error: visibility qualifiers are not permitted here
 
-pub impl Foo for Bar { // error: unnecessary visibility qualifier
-    pub fn foo() {} // error: unnecessary visibility qualifier
+pub impl Foo for Bar { // error: visibility qualifiers are not permitted here
+    pub fn foo() {} // error: visibility qualifiers are not permitted here
 }
 ```
 
-To fix this error, please remove the visibility qualifier when it is not
-required. Example:
+To fix this error, simply remove the visibility qualifier. Example:
 
 ```
 struct Bar;
@@ -26,12 +31,18 @@ trait Foo {
     fn foo();
 }
 
+enum Baz {
+    // Enum variants share the visibility of the enum they are in, so
+    // `pub` is not allowed here
+    Qux,
+}
+
 // Directly implemented methods share the visibility of the type itself,
-// so `pub` is unnecessary here
+// so `pub` is not allowed here
 impl Bar {}
 
-// Trait methods share the visibility of the trait, so `pub` is
-// unnecessary in either case
+// Trait methods share the visibility of the trait, so `pub` is not
+// allowed in either case
 impl Foo for Bar {
     fn foo() {}
 }

tests/ui/error-codes/E0449.stderr

@@ -1,22 +1,26 @@
-error[E0449]: unnecessary visibility qualifier
+error[E0449]: visibility qualifiers are not permitted here
   --> $DIR/E0449.rs:7:1
    |
 LL | pub impl Bar {}
-   | ^^^ `pub` not permitted here because it's implied
+   | ^^^
    |
    = note: place qualifiers on individual impl items instead
 
-error[E0449]: unnecessary visibility qualifier
+error[E0449]: visibility qualifiers are not permitted here
   --> $DIR/E0449.rs:9:1
    |
 LL | pub impl Foo for Bar {
-   | ^^^ `pub` not permitted here because it's implied
+   | ^^^
+   |
+   = note: trait items always share the visibility of their trait
 
-error[E0449]: unnecessary visibility qualifier
+error[E0449]: visibility qualifiers are not permitted here
   --> $DIR/E0449.rs:10:5
    |
 LL |     pub fn foo() {}
-   |     ^^^ `pub` not permitted here because it's implied
+   |     ^^^
+   |
+   = note: trait items always share the visibility of their trait
 
 error: aborting due to 3 previous errors
 

tests/ui/issues/issue-28433.rs

@@ -1,9 +1,9 @@
 enum Bird {
     pub Duck,
-    //~^ ERROR unnecessary visibility qualifier
+    //~^ ERROR visibility qualifiers are not permitted here
     Goose,
     pub(crate) Dove
-    //~^ ERROR unnecessary visibility qualifier
+    //~^ ERROR visibility qualifiers are not permitted here
 }
 
 

tests/ui/issues/issue-28433.stderr

@@ -1,14 +1,18 @@
-error[E0449]: unnecessary visibility qualifier
+error[E0449]: visibility qualifiers are not permitted here
   --> $DIR/issue-28433.rs:2:5
    |
 LL |     pub Duck,
-   |     ^^^ `pub` not permitted here because it's implied
+   |     ^^^
+   |
+   = note: enum variants and their fields always share the visibility of the enum they are in
 
-error[E0449]: unnecessary visibility qualifier
+error[E0449]: visibility qualifiers are not permitted here
   --> $DIR/issue-28433.rs:5:5
    |
 LL |     pub(crate) Dove
    |     ^^^^^^^^^^
+   |
+   = note: enum variants and their fields always share the visibility of the enum they are in
 
 error: aborting due to 2 previous errors
 

tests/ui/parser/assoc-static-semantic-fail.rs

@@ -31,7 +31,7 @@ trait T {
     //~| ERROR a static item cannot be `default`
     pub(crate) default static TD: u8;
     //~^ ERROR associated `static` items are not allowed
-    //~| ERROR unnecessary visibility qualifier
+    //~| ERROR visibility qualifiers are not permitted here
     //~| ERROR a static item cannot be `default`
 }
 
@@ -47,6 +47,6 @@ impl T for S {
     pub default static TD: u8;
     //~^ ERROR associated `static` items are not allowed
     //~| ERROR associated constant in `impl` without body
-    //~| ERROR unnecessary visibility qualifier
+    //~| ERROR visibility qualifiers are not permitted here
     //~| ERROR a static item cannot be `default`
 }

tests/ui/parser/assoc-static-semantic-fail.stderr

@@ -134,11 +134,13 @@ LL |     pub(crate) default static ID: u8;
    |                                     |
    |                                     help: provide a definition for the constant: `= <expr>;`
 
-error[E0449]: unnecessary visibility qualifier
+error[E0449]: visibility qualifiers are not permitted here
   --> $DIR/assoc-static-semantic-fail.rs:32:5
    |
 LL |     pub(crate) default static TD: u8;
    |     ^^^^^^^^^^
+   |
+   = note: trait items always share the visibility of their trait
 
 error: associated constant in `impl` without body
   --> $DIR/assoc-static-semantic-fail.rs:41:5
@@ -156,11 +158,13 @@ LL |     pub default static TD: u8;
    |                              |
    |                              help: provide a definition for the constant: `= <expr>;`
 
-error[E0449]: unnecessary visibility qualifier
+error[E0449]: visibility qualifiers are not permitted here
   --> $DIR/assoc-static-semantic-fail.rs:47:5
    |
 LL |     pub default static TD: u8;
-   |     ^^^ `pub` not permitted here because it's implied
+   |     ^^^
+   |
+   = note: trait items always share the visibility of their trait
 
 warning: the feature `specialization` is incomplete and may not be safe to use and/or cause compiler crashes
   --> $DIR/assoc-static-semantic-fail.rs:3:12

tests/ui/parser/default.rs

@@ -14,7 +14,7 @@ impl Foo for u8 {
 }
 
 impl Foo for u16 {
-    pub default fn foo<T: Default>() -> T { //~ ERROR unnecessary visibility qualifier
+    pub default fn foo<T: Default>() -> T { //~ ERROR visibility qualifiers are not permitted here
         T::default()
     }
 }