Make "unneccesary visibility qualifier" error much more clear

Author: ElectrifyPro

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,
+                        );
                     }
                 }
             }
@@ -1303,7 +1308,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() {}
 }