Tweak output for invalid negative impl errors

rust-lang · Mar 23, 2020 · b8f9866 · b8f9866
1 parent 5ae85f4
commit b8f9866
Show file tree

Hide file tree

Showing 14 changed files with 95 additions and 50 deletions.
diff --git a/src/librustc_ast_lowering/item.rs b/src/librustc_ast_lowering/item.rs
@@ -403,10 +403,15 @@ impl<'hir> LoweringContext<'_, 'hir> {
                         )
                     });
 
+                // `defaultness.has_value()` is never called for an `impl`, always `true` in order
+                // to not cause an assertion failure inside the `lower_defaultness` function.
+                let has_val = true;
+                let (defaultness, defaultness_span) = self.lower_defaultness(defaultness, has_val);
                 hir::ItemKind::Impl {
                     unsafety: self.lower_unsafety(unsafety),
                     polarity,
-                    defaultness: self.lower_defaultness(defaultness, true /* [1] */),
+                    defaultness,
+                    defaultness_span,
                     constness: self.lower_constness(constness),
                     generics,
                     of_trait: trait_ref,
@@ -435,9 +440,6 @@ impl<'hir> LoweringContext<'_, 'hir> {
                 bug!("`TyMac` should have been expanded by now")
             }
         }
-
-        // [1] `defaultness.has_value()` is never called for an `impl`, always `true` in order to
-        //     not cause an assertion failure inside the `lower_defaultness` function.
     }
 
     fn lower_const_item(
@@ -868,27 +870,31 @@ impl<'hir> LoweringContext<'_, 'hir> {
             AssocItemKind::MacCall(..) => bug!("`TyMac` should have been expanded by now"),
         };
 
+        // Since `default impl` is not yet implemented, this is always true in impls.
+        let has_value = true;
+        let (defaultness, _) = self.lower_defaultness(i.kind.defaultness(), has_value);
         hir::ImplItem {
             hir_id: self.lower_node_id(i.id),
             ident: i.ident,
             attrs: self.lower_attrs(&i.attrs),
             generics,
             vis: self.lower_visibility(&i.vis, None),
-            defaultness: self.lower_defaultness(i.kind.defaultness(), true /* [1] */),
+            defaultness,
             kind,
             span: i.span,
         }
-
-        // [1] since `default impl` is not yet implemented, this is always true in impls
     }
 
     fn lower_impl_item_ref(&mut self, i: &AssocItem) -> hir::ImplItemRef<'hir> {
+        // Since `default impl` is not yet implemented, this is always true in impls.
+        let has_value = true;
+        let (defaultness, _) = self.lower_defaultness(i.kind.defaultness(), has_value);
         hir::ImplItemRef {
             id: hir::ImplItemId { hir_id: self.lower_node_id(i.id) },
             ident: i.ident,
             span: i.span,
             vis: self.lower_visibility(&i.vis, Some(i.id)),
-            defaultness: self.lower_defaultness(i.kind.defaultness(), true /* [1] */),
+            defaultness,
             kind: match &i.kind {
                 AssocItemKind::Const(..) => hir::AssocItemKind::Const,
                 AssocItemKind::TyAlias(.., ty) => {
@@ -903,8 +909,6 @@ impl<'hir> LoweringContext<'_, 'hir> {
                 AssocItemKind::MacCall(..) => unimplemented!(),
             },
         }
-
-        // [1] since `default impl` is not yet implemented, this is always true in impls
     }
 
     /// If an `explicit_owner` is given, this method allocates the `HirId` in
@@ -939,12 +943,16 @@ impl<'hir> LoweringContext<'_, 'hir> {
         respan(v.span, node)
     }
 
-    fn lower_defaultness(&self, d: Defaultness, has_value: bool) -> hir::Defaultness {
+    fn lower_defaultness(
+        &self,
+        d: Defaultness,
+        has_value: bool,
+    ) -> (hir::Defaultness, Option<Span>) {
         match d {
-            Defaultness::Default(_) => hir::Defaultness::Default { has_value },
+            Defaultness::Default(sp) => (hir::Defaultness::Default { has_value }, Some(sp)),
             Defaultness::Final => {
                 assert!(has_value);
-                hir::Defaultness::Final
+                (hir::Defaultness::Final, None)
             }
         }
     }

diff --git a/src/librustc_hir/hir.rs b/src/librustc_hir/hir.rs
@@ -2153,7 +2153,7 @@ pub enum Defaultness {
 impl Defaultness {
     pub fn has_value(&self) -> bool {
         match *self {
-            Defaultness::Default { has_value, .. } => has_value,
+            Defaultness::Default { has_value } => has_value,
             Defaultness::Final => true,
         }
     }
@@ -2502,6 +2502,9 @@ pub enum ItemKind<'hir> {
         unsafety: Unsafety,
         polarity: ImplPolarity,
         defaultness: Defaultness,
+        // We do not put a `Span` in `Defaultness` because it breaks foreign crate metadata
+        // decoding as `Span`s cannot be decoded when a `Session` is not available.
+        defaultness_span: Option<Span>,
         constness: Constness,
         generics: Generics<'hir>,
 

diff --git a/src/librustc_hir/intravisit.rs b/src/librustc_hir/intravisit.rs
@@ -590,6 +590,7 @@ pub fn walk_item<'v, V: Visitor<'v>>(visitor: &mut V, item: &'v Item<'v>) {
             defaultness: _,
             polarity: _,
             constness: _,
+            defaultness_span: _,
             ref generics,
             ref of_trait,
             ref self_ty,

diff --git a/src/librustc_hir/print.rs b/src/librustc_hir/print.rs
@@ -632,6 +632,7 @@ impl<'a> State<'a> {
                 polarity,
                 defaultness,
                 constness,
+                defaultness_span: _,
                 ref generics,
                 ref of_trait,
                 ref self_ty,

diff --git a/src/librustc_typeck/check/wfcheck.rs b/src/librustc_typeck/check/wfcheck.rs
@@ -98,34 +98,50 @@ pub fn check_item_well_formed(tcx: TyCtxt<'_>, def_id: DefId) {
         //
         // won't be allowed unless there's an *explicit* implementation of `Send`
         // for `T`
-        hir::ItemKind::Impl { defaultness, ref of_trait, ref self_ty, .. } => {
+        hir::ItemKind::Impl {
+            defaultness,
+            defaultness_span,
+            polarity,
+            ref of_trait,
+            ref self_ty,
+            ..
+        } => {
             let is_auto = tcx
                 .impl_trait_ref(tcx.hir().local_def_id(item.hir_id))
                 .map_or(false, |trait_ref| tcx.trait_is_auto(trait_ref.def_id));
-            let polarity = tcx.impl_polarity(def_id);
             if let (hir::Defaultness::Default { .. }, true) = (defaultness, is_auto) {
-                tcx.sess.span_err(item.span, "impls of auto traits cannot be default");
+                let sp = of_trait.as_ref().map(|t| t.path.span).unwrap_or(item.span);
+                let mut err =
+                    tcx.sess.struct_span_err(sp, "impls of auto traits cannot be default");
+                err.span_labels(defaultness_span, "default because of this");
+                err.span_label(sp, "auto trait");
+                err.emit();
             }
-            match polarity {
-                ty::ImplPolarity::Positive => {
+            // We match on both `ty::ImplPolarity` and `ast::ImplPolarity` just to get the `!` span.
+            match (tcx.impl_polarity(def_id), polarity) {
+                (ty::ImplPolarity::Positive, _) => {
                     check_impl(tcx, item, self_ty, of_trait);
                 }
-                ty::ImplPolarity::Negative => {
+                (ty::ImplPolarity::Negative, ast::ImplPolarity::Negative(span)) => {
                     // FIXME(#27579): what amount of WF checking do we need for neg impls?
-                    if of_trait.is_some() && !is_auto {
+                    if let (Some(of_trait), false) = (of_trait, is_auto) {
                         struct_span_err!(
                             tcx.sess,
-                            item.span,
+                            span.to(of_trait.path.span),
                             E0192,
-                            "negative impls are only allowed for \
-                                   auto traits (e.g., `Send` and `Sync`)"
+                            "invalid negative impl"
+                        )
+                        .note(
+                            "negative impls are only allowed for auto traits, like `Send` and \
+                             `Sync`",
                         )
                         .emit()
                     }
                 }
-                ty::ImplPolarity::Reservation => {
+                (ty::ImplPolarity::Reservation, _) => {
                     // FIXME: what amount of WF checking do we need for reservation impls?
                 }
+                _ => unreachable!(),
             }
         }
         hir::ItemKind::Fn(..) => {

diff --git a/src/librustc_typeck/collect.rs b/src/librustc_typeck/collect.rs
@@ -1580,9 +1580,10 @@ fn impl_polarity(tcx: TyCtxt<'_>, def_id: DefId) -> ty::ImplPolarity {
     let is_rustc_reservation = tcx.has_attr(def_id, sym::rustc_reservation_impl);
     let item = tcx.hir().expect_item(hir_id);
     match &item.kind {
-        hir::ItemKind::Impl { polarity: hir::ImplPolarity::Negative(_), .. } => {
+        hir::ItemKind::Impl { polarity: hir::ImplPolarity::Negative(span), of_trait, .. } => {
             if is_rustc_reservation {
-                tcx.sess.span_err(item.span, "reservation impls can't be negative");
+                let span = span.to(of_trait.as_ref().map(|t| t.path.span).unwrap_or(*span));
+                tcx.sess.span_err(span, "reservation impls can't be negative");
             }
             ty::ImplPolarity::Negative
         }

diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs
@@ -563,6 +563,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
                 polarity,
                 defaultness,
                 constness,
+                defaultness_span: _,
                 ref generics,
                 ref of_trait,
                 self_ty,

diff --git a/src/test/ui/error-codes/E0192.stderr b/src/test/ui/error-codes/E0192.stderr
@@ -1,8 +1,10 @@
-error[E0192]: negative impls are only allowed for auto traits (e.g., `Send` and `Sync`)
-  --> $DIR/E0192.rs:9:1
+error[E0192]: invalid negative impl
+  --> $DIR/E0192.rs:9:6
    |
 LL | impl !Trait for Foo { }
-   | ^^^^^^^^^^^^^^^^^^^^^^^
+   |      ^^^^^^
+   |
+   = note: negative impls are only allowed for auto traits, like `Send` and `Sync`
 
 error: aborting due to previous error
 

diff --git a/src/test/ui/specialization/defaultimpl/validation.rs b/src/test/ui/specialization/defaultimpl/validation.rs
@@ -10,6 +10,6 @@ default unsafe impl Send for S {} //~ ERROR impls of auto traits cannot be defau
 default impl !Send for Z {} //~ ERROR impls of auto traits cannot be default
 
 trait Tr {}
-default impl !Tr for S {} //~ ERROR negative impls are only allowed for auto traits
+default impl !Tr for S {} //~ ERROR invalid negative impl
 
 fn main() {}
diff --git a/src/test/ui/specialization/defaultimpl/validation.stderr b/src/test/ui/specialization/defaultimpl/validation.stderr
@@ -9,22 +9,28 @@ LL | default impl S {}
    = note: only trait implementations may be annotated with `default`
 
 error: impls of auto traits cannot be default
-  --> $DIR/validation.rs:9:1
+  --> $DIR/validation.rs:9:21
    |
 LL | default unsafe impl Send for S {}
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   | -------             ^^^^ auto trait
+   | |
+   | default because of this
 
 error: impls of auto traits cannot be default
-  --> $DIR/validation.rs:10:1
+  --> $DIR/validation.rs:10:15
    |
 LL | default impl !Send for Z {}
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   | -------       ^^^^ auto trait
+   | |
+   | default because of this
 
-error[E0192]: negative impls are only allowed for auto traits (e.g., `Send` and `Sync`)
-  --> $DIR/validation.rs:13:1
+error[E0192]: invalid negative impl
+  --> $DIR/validation.rs:13:14
    |
 LL | default impl !Tr for S {}
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^
+   |              ^^^
+   |
+   = note: negative impls are only allowed for auto traits, like `Send` and `Sync`
 
 error: aborting due to 4 previous errors
 

diff --git a/src/test/ui/syntax-trait-polarity.rs b/src/test/ui/syntax-trait-polarity.rs
@@ -12,7 +12,7 @@ trait TestTrait {}
 unsafe impl !Send for TestType {}
 //~^ ERROR negative impls cannot be unsafe
 impl !TestTrait for TestType {}
-//~^ ERROR negative impls are only allowed for auto traits
+//~^ ERROR invalid negative impl
 
 struct TestType2<T>(T);
 
@@ -22,6 +22,6 @@ impl<T> !TestType2<T> {}
 unsafe impl<T> !Send for TestType2<T> {}
 //~^ ERROR negative impls cannot be unsafe
 impl<T> !TestTrait for TestType2<T> {}
-//~^ ERROR negative impls are only allowed for auto traits
+//~^ ERROR invalid negative impl
 
 fn main() {}
diff --git a/src/test/ui/syntax-trait-polarity.stderr b/src/test/ui/syntax-trait-polarity.stderr
@@ -32,17 +32,21 @@ LL | unsafe impl<T> !Send for TestType2<T> {}
    | |              negative because of this
    | unsafe because of this
 
-error[E0192]: negative impls are only allowed for auto traits (e.g., `Send` and `Sync`)
-  --> $DIR/syntax-trait-polarity.rs:14:1
+error[E0192]: invalid negative impl
+  --> $DIR/syntax-trait-polarity.rs:14:6
    |
 LL | impl !TestTrait for TestType {}
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |      ^^^^^^^^^^
+   |
+   = note: negative impls are only allowed for auto traits, like `Send` and `Sync`
 
-error[E0192]: negative impls are only allowed for auto traits (e.g., `Send` and `Sync`)
-  --> $DIR/syntax-trait-polarity.rs:24:1
+error[E0192]: invalid negative impl
+  --> $DIR/syntax-trait-polarity.rs:24:9
    |
 LL | impl<T> !TestTrait for TestType2<T> {}
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |         ^^^^^^^^^^
+   |
+   = note: negative impls are only allowed for auto traits, like `Send` and `Sync`
 
 error: aborting due to 6 previous errors
 

diff --git a/src/test/ui/typeck/typeck-negative-impls-builtin.rs b/src/test/ui/typeck/typeck-negative-impls-builtin.rs
@@ -7,6 +7,6 @@ trait TestTrait {
 }
 
 impl !TestTrait for TestType {}
-//~^ ERROR negative impls are only allowed for auto traits (e.g., `Send` and `Sync`)
+//~^ ERROR invalid negative impl
 
 fn main() {}
diff --git a/src/test/ui/typeck/typeck-negative-impls-builtin.stderr b/src/test/ui/typeck/typeck-negative-impls-builtin.stderr
@@ -1,8 +1,10 @@
-error[E0192]: negative impls are only allowed for auto traits (e.g., `Send` and `Sync`)
-  --> $DIR/typeck-negative-impls-builtin.rs:9:1
+error[E0192]: invalid negative impl
+  --> $DIR/typeck-negative-impls-builtin.rs:9:6
    |
 LL | impl !TestTrait for TestType {}
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |      ^^^^^^^^^^
+   |
+   = note: negative impls are only allowed for auto traits, like `Send` and `Sync`
 
 error: aborting due to previous error