better errors when resolving bad Self in impl block

compiler/rustc_hir/src/def.rs

@@ -317,7 +317,7 @@ pub enum Res<Id = hir::HirId> {
         /// Optionally, the trait associated with this `Self` type.
         Option<DefId>,
         /// Optionally, the impl associated with this `Self` type.
-        Option<(DefId, bool)>,
+        Option<ResImpl>,
     ),
     /// A tool attribute module; e.g., the `rustfmt` in `#[rustfmt::skip]`.
     ///
@@ -353,6 +353,14 @@ pub enum Res<Id = hir::HirId> {
     Err,
 }
 
+#[derive(Clone, Copy, PartialEq, Eq, Encodable, Decodable, Hash, Debug)]
+#[derive(HashStable_Generic)]
+pub struct ResImpl {
+    pub def_id: DefId,
+    pub generics_allowed: bool,
+    pub in_trait_ref: bool,
+}
+
 /// The result of resolving a path before lowering to HIR,
 /// with "module" segments resolved and associated item
 /// segments deferred to type checking.

compiler/rustc_lint/src/internal.rs

@@ -202,8 +202,8 @@ fn is_ty_or_ty_ctxt(cx: &LateContext<'_>, ty: &Ty<'_>) -> Option<String> {
                 }
             }
             // Only lint on `&Ty` and `&TyCtxt` if it is used outside of a trait.
-            Res::SelfTy(None, Some((did, _))) => {
-                if let ty::Adt(adt, substs) = cx.tcx.type_of(did).kind() {
+            Res::SelfTy(None, Some(res_impl)) => {
+                if let ty::Adt(adt, substs) = cx.tcx.type_of(res_impl.def_id).kind() {
                     if let Some(name @ (sym::Ty | sym::TyCtxt)) =
                         cx.tcx.get_diagnostic_name(adt.did)
                     {

compiler/rustc_lint/src/pass_by_value.rs

@@ -54,8 +54,8 @@ fn path_for_pass_by_value(cx: &LateContext<'_>, ty: &hir::Ty<'_>) -> Option<Stri
                 let path_segment = path.segments.last().unwrap();
                 return Some(format!("{}{}", name, gen_args(cx, path_segment)));
             }
-            Res::SelfTy(None, Some((did, _))) => {
-                if let ty::Adt(adt, substs) = cx.tcx.type_of(did).kind() {
+            Res::SelfTy(None, Some(res_impl)) => {
+                if let ty::Adt(adt, substs) = cx.tcx.type_of(res_impl.def_id).kind() {
                     if cx.tcx.has_attr(adt.did, sym::rustc_pass_by_value) {
                         return Some(cx.tcx.def_path_str_with_substs(adt.did, substs));
                     }

compiler/rustc_passes/src/dead.rs

@@ -108,8 +108,8 @@ impl<'tcx> MarkSymbolVisitor<'tcx> {
                 if let Some(t) = t {
                     self.check_def_id(t);
                 }
-                if let Some((i, _)) = i {
-                    self.check_def_id(i);
+                if let Some(res_impl) = i {
+                    self.check_def_id(res_impl.def_id);
                 }
             }
             Res::ToolMod | Res::NonMacroAttr(..) | Res::Err => {}

compiler/rustc_resolve/src/diagnostics.rs

@@ -123,16 +123,16 @@ impl<'a> Resolver<'a> {
 
                 let sm = self.session.source_map();
                 match outer_res {
-                    Res::SelfTy(maybe_trait_defid, maybe_impl_defid) => {
+                    Res::SelfTy(maybe_trait_defid, maybe_res_impl) => {
                         if let Some(impl_span) =
-                            maybe_impl_defid.and_then(|(def_id, _)| self.opt_span(def_id))
+                            maybe_res_impl.and_then(|res_impl| self.opt_span(res_impl.def_id))
                         {
                             err.span_label(
                                 reduce_impl_span_to_impl_keyword(sm, impl_span),
                                 "`Self` type implicitly declared here, by this `impl`",
                             );
                         }
-                        match (maybe_trait_defid, maybe_impl_defid) {
+                        match (maybe_trait_defid, maybe_res_impl) {
                             (Some(_), None) => {
                                 err.span_label(span, "can't use `Self` here");
                             }

compiler/rustc_resolve/src/late.rs

@@ -18,7 +18,7 @@ use rustc_ast_lowering::ResolverAstLowering;
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_errors::DiagnosticId;
 use rustc_hir::def::Namespace::{self, *};
-use rustc_hir::def::{self, CtorKind, DefKind, PartialRes, PerNS};
+use rustc_hir::def::{self, CtorKind, DefKind, PartialRes, PerNS, ResImpl};
 use rustc_hir::def_id::{DefId, CRATE_DEF_INDEX};
 use rustc_hir::{PrimTy, TraitCandidate};
 use rustc_middle::{bug, span_bug};
@@ -911,9 +911,19 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
         self.with_current_self_item(item, |this| {
             this.with_generic_param_rib(generics, ItemRibKind(HasGenericParams::Yes), |this| {
                 let item_def_id = this.r.local_def_id(item.id).to_def_id();
-                this.with_self_rib(Res::SelfTy(None, Some((item_def_id, false))), |this| {
-                    visit::walk_item(this, item);
-                });
+                this.with_self_rib(
+                    Res::SelfTy(
+                        None,
+                        Some(ResImpl {
+                            def_id: item_def_id,
+                            generics_allowed: true,
+                            in_trait_ref: false,
+                        }),
+                    ),
+                    |this| {
+                        visit::walk_item(this, item);
+                    },
+                );
             });
         });
     }
@@ -1211,6 +1221,14 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
         result
     }
 
+    fn with_no_self_type<T>(&mut self, f: impl FnOnce(&mut Self) -> T) -> T {
+        // Handle nested impls (inside fn bodies)
+        let previous_value = replace(&mut self.diagnostic_metadata.current_self_type, None);
+        let result = f(self);
+        self.diagnostic_metadata.current_self_type = previous_value;
+        result
+    }
+
     fn with_current_self_item<T>(&mut self, self_item: &Item, f: impl FnOnce(&mut Self) -> T) -> T {
         let previous_value =
             replace(&mut self.diagnostic_metadata.current_self_item, Some(self_item.id));
@@ -1295,30 +1313,52 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
         debug!("resolve_implementation");
         // If applicable, create a rib for the type parameters.
         self.with_generic_param_rib(generics, ItemRibKind(HasGenericParams::Yes), |this| {
-            // Dummy self type for better errors if `Self` is used in the trait path.
             this.with_self_rib(Res::SelfTy(None, None), |this| {
+                // FIXME(compiler-errors): `Self` may still exist in the value namespace,
+                // so we might be able to resolve an outer `Self` in, e.g., a const generic default.
+                this.with_no_self_type(|this| {
+                    // Resolve the self type.
+                    this.visit_ty(self_type);
+                });
+                let item_def_id = this.r.local_def_id(item_id);
                 // Resolve the trait reference, if necessary.
                 this.with_optional_trait_ref(opt_trait_reference.as_ref(), |this, trait_id| {
-                    let item_def_id = this.r.local_def_id(item_id);
-
                     // Register the trait definitions from here.
                     if let Some(trait_id) = trait_id {
                         this.r.trait_impls.entry(trait_id).or_default().push(item_def_id);
                     }
-
                     let item_def_id = item_def_id.to_def_id();
-                    this.with_self_rib(Res::SelfTy(trait_id, Some((item_def_id, false))), |this| {
-                        if let Some(trait_ref) = opt_trait_reference.as_ref() {
-                            // Resolve type arguments in the trait path.
-                            visit::walk_trait_ref(this, trait_ref);
-                        }
-                        // Resolve the self type.
-                        this.visit_ty(self_type);
-                        // Resolve the generic parameters.
-                        this.visit_generics(generics);
-                        // Resolve the items within the impl.
-                        this.with_current_self_type(self_type, |this| {
-                            this.with_self_rib_ns(ValueNS, Res::SelfCtor(item_def_id), |this| {
+                    if let Some(trait_ref) = opt_trait_reference.as_ref() {
+                        this.with_self_rib(
+                            Res::SelfTy(
+                                trait_id,
+                                Some(ResImpl {
+                                    def_id: item_def_id,
+                                    generics_allowed: true,
+                                    in_trait_ref: true,
+                                }),
+                            ),
+                            |this| {
+                                // Resolve type arguments in the trait path.
+                                visit::walk_trait_ref(this, trait_ref);
+                            },
+                        );
+                    }
+                    this.with_self_rib(
+                        Res::SelfTy(
+                            trait_id,
+                            Some(ResImpl {
+                                def_id: item_def_id,
+                                generics_allowed: true,
+                                in_trait_ref: false,
+                            }),
+                        ),
+                        |this| {
+                            // Resolve the generic parameters.
+                            this.visit_generics(generics);
+                            // Resolve the items within the impl.
+                            this.with_current_self_type(self_type, |this| {
+                                this.with_self_rib_ns(ValueNS, Res::SelfCtor(item_def_id), |this| {
                                 debug!("resolve_implementation with_self_rib_ns(ValueNS, ...)");
                                 for item in impl_items {
                                     use crate::ResolutionError::*;
@@ -1414,8 +1454,9 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
                                     }
                                 }
                             });
-                        });
-                    });
+                            });
+                        },
+                    );
                 });
             });
         });

compiler/rustc_resolve/src/lib.rs

@@ -42,8 +42,8 @@ use rustc_data_structures::ptr_key::PtrKey;
 use rustc_data_structures::sync::Lrc;
 use rustc_errors::{struct_span_err, Applicability, DiagnosticBuilder};
 use rustc_expand::base::{DeriveResolutions, SyntaxExtension, SyntaxExtensionKind};
-use rustc_hir::def::Namespace::*;
 use rustc_hir::def::{self, CtorOf, DefKind, NonMacroAttrKind, PartialRes};
+use rustc_hir::def::{Namespace::*, ResImpl};
 use rustc_hir::def_id::{CrateNum, DefId, DefIdMap, DefPathHash, LocalDefId};
 use rustc_hir::def_id::{CRATE_DEF_ID, CRATE_DEF_INDEX, LOCAL_CRATE};
 use rustc_hir::definitions::{DefKey, DefPathData, Definitions};
@@ -2804,8 +2804,11 @@ impl<'a> Resolver<'a> {
                                 // HACK(min_const_generics): If we encounter `Self` in an anonymous constant
                                 // we can't easily tell if it's generic at this stage, so we instead remember
                                 // this and then enforce the self type to be concrete later on.
-                                if let Res::SelfTy(trait_def, Some((impl_def, _))) = res {
-                                    res = Res::SelfTy(trait_def, Some((impl_def, true)));
+                                if let Res::SelfTy(trait_def, Some(res_impl)) = res {
+                                    res = Res::SelfTy(
+                                        trait_def,
+                                        Some(ResImpl { generics_allowed: false, ..res_impl }),
+                                    );
                                 } else {
                                     if record_used {
                                         self.report_error(

compiler/rustc_typeck/src/astconv/mod.rs

@@ -13,6 +13,7 @@ use crate::errors::{
 };
 use crate::middle::resolve_lifetime as rl;
 use crate::require_c_abi_if_c_variadic;
+use hir::def::ResImpl;
 use rustc_ast::TraitObjectSyntax;
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_errors::{struct_span_err, Applicability, ErrorReported, FatalError};
@@ -1805,7 +1806,13 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
         // Find the type of the associated item, and the trait where the associated
         // item is declared.
         let bound = match (&qself_ty.kind(), qself_res) {
-            (_, Res::SelfTy(Some(_), Some((impl_def_id, _)))) => {
+            (
+                _,
+                Res::SelfTy(
+                    Some(_),
+                    Some(ResImpl { def_id: impl_def_id, in_trait_ref: false, .. }),
+                ),
+            ) => {
                 // `Self` in an impl of a trait -- we have a concrete self type and a
                 // trait reference.
                 let trait_ref = match tcx.impl_trait_ref(impl_def_id) {
@@ -2276,7 +2283,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
                 self.prohibit_generics(path.segments);
                 tcx.types.self_param
             }
-            Res::SelfTy(_, Some((def_id, forbid_generic))) => {
+            Res::SelfTy(_, Some(ResImpl { def_id, generics_allowed, .. })) => {
                 // `Self` in impl (we know the concrete type).
                 assert_eq!(opt_self_ty, None);
                 self.prohibit_generics(path.segments);
@@ -2301,7 +2308,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
                 // the anon const, which is empty. This is why the
                 // `AlwaysApplicable` impl needs a `T: ?Sized` bound for
                 // this to compile if we were to normalize here.
-                if forbid_generic && ty.needs_subst() {
+                if !generics_allowed && ty.needs_subst() {
                     let mut err = tcx.sess.struct_span_err(
                         path.span,
                         "generic `Self` types are currently not permitted in anonymous constants",
@@ -2319,6 +2326,15 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
                     self.normalize_ty(span, ty)
                 }
             }
+            Res::SelfTy(None, None) => {
+                let mut err = tcx.sess.struct_span_err(
+                    path.span,
+                    "`Self` is not allowed in the self type of an `impl` block",
+                );
+                err.note("referencing `Self` in this position would produce an infinitely recursive type");
+                err.emit();
+                self.tcx().ty_error()
+            }
             Res::Def(DefKind::AssocTy, def_id) => {
                 debug_assert!(path.segments.len() >= 2);
                 self.prohibit_generics(&path.segments[..path.segments.len() - 2]);

compiler/rustc_typeck/src/check/check.rs

@@ -524,7 +524,7 @@ pub(super) fn check_opaque_for_inheriting_lifetimes<'tcx>(
                 hir::TyKind::Path(hir::QPath::Resolved(None, path)) => match &path.segments {
                     [PathSegment { res: Some(Res::SelfTy(_, impl_ref)), .. }] => {
                         let impl_ty_name =
-                            impl_ref.map(|(def_id, _)| self.tcx.def_path_str(def_id));
+                            impl_ref.map(|res_impl| self.tcx.def_path_str(res_impl.def_id));
                         self.selftys.push((path.span, impl_ty_name));
                     }
                     _ => {}

src/librustdoc/clean/utils.rs

@@ -9,6 +9,7 @@ use crate::core::DocContext;
 use crate::formats::item_type::ItemType;
 use crate::visit_lib::LibEmbargoVisitor;
 
+use hir::def::ResImpl;
 use rustc_ast as ast;
 use rustc_ast::tokenstream::TokenTree;
 use rustc_data_structures::thin_vec::ThinVec;
@@ -400,8 +401,8 @@ crate fn register_res(cx: &mut DocContext<'_>, res: Res) -> DefId {
         // This is part of a trait definition; document the trait.
         Res::SelfTy(Some(trait_def_id), _) => (trait_def_id, ItemType::Trait),
         // This is an inherent impl; it doesn't have its own page.
-        Res::SelfTy(None, Some((impl_def_id, _))) => return impl_def_id,
-        Res::SelfTy(None, None)
+        Res::SelfTy(None, Some(ResImpl { def_id: impl_def_id, .. })) => return impl_def_id,
+        Res::SelfTy(None, _)
         | Res::PrimTy(_)
         | Res::ToolMod
         | Res::SelfCtor(_)

src/test/ui/resolve/issue-23305.rs

@@ -3,6 +3,6 @@ pub trait ToNbt<T> {
 }
 
 impl dyn ToNbt<Self> {}
-//~^ ERROR cycle detected
+//~^ ERROR `Self` is not allowed in the self type of an `impl` block
 
 fn main() {}

src/test/ui/resolve/issue-23305.stderr

@@ -1,16 +1,10 @@
-error[E0391]: cycle detected when computing type of `<impl at $DIR/issue-23305.rs:5:1: 5:24>`
+error: `Self` is not allowed in the self type of an `impl` block
   --> $DIR/issue-23305.rs:5:16
    |
 LL | impl dyn ToNbt<Self> {}
    |                ^^^^
    |
-   = note: ...which immediately requires computing type of `<impl at $DIR/issue-23305.rs:5:1: 5:24>` again
-note: cycle used when collecting item types in top-level module
-  --> $DIR/issue-23305.rs:1:1
-   |
-LL | pub trait ToNbt<T> {
-   | ^^^^^^^^^^^^^^^^^^
+   = note: referencing `Self` in this position would produce an infinitely recursive type
 
 error: aborting due to previous error
 
-For more information about this error, try `rustc --explain E0391`.

src/test/ui/resolve/resolve-self-in-impl.rs

@@ -11,10 +11,15 @@ impl Tr for S where Self: Copy {} // OK
 impl Tr for S where S<Self>: Copy {} // OK
 impl Tr for S where Self::A: Copy {} // OK
 
-impl Tr for Self {} //~ ERROR cycle detected
-impl Tr for S<Self> {} //~ ERROR cycle detected
-impl Self {} //~ ERROR cycle detected
-impl S<Self> {} //~ ERROR cycle detected
-impl Tr<Self::A> for S {} //~ ERROR cycle detected
+impl Tr for Self {}
+//~^ ERROR `Self` is not allowed in the self type of an `impl` block
+impl Tr for S<Self> {}
+//~^ ERROR `Self` is not allowed in the self type of an `impl` block
+impl Self {}
+//~^ ERROR `Self` is not allowed in the self type of an `impl` block
+impl S<Self> {}
+//~^ ERROR `Self` is not allowed in the self type of an `impl` block
+impl Tr<Self::A> for S {}
+//~^ ERROR ambiguous associated type
 
 fn main() {}