Diagnose explicit `Self` in extend in the parse node handler (Refactor Impl construction 1/7) (#6465)

We encode the state of looking that the parent scope is a Class into a
type so that we can avoid extra lookups. Then use that in refactoring
where diagnostics are generated for an explicit `Self` in an `extend
impl` declaration.

We add tests that we don't double-diagnose the Self type when it's
already an error, and make the behaviour of `extend require` match that
of `extend impl as`.

This avoids some fragile/complex parse-node lookups (such as
`context.parse_tree_and_subtrees().ExtractAs<Parse::ImplTypeAs>`) by
using the parse node at the point where we are handling it instead of
much later.

This is part of #6420 which is being split up into a chain of smaller
PRs.

toolchain/check/BUILD

@@ -57,6 +57,7 @@ cc_library(
         "name_component.cpp",
         "name_lookup.cpp",
         "name_ref.cpp",
+        "name_scope.cpp",
         "operator.cpp",
         "pattern.cpp",
         "pattern_match.cpp",
@@ -113,6 +114,7 @@ cc_library(
         "name_component.h",
         "name_lookup.h",
         "name_ref.h",
+        "name_scope.h",
         "operator.h",
         "param_and_arg_refs_stack.h",
         "pattern.h",

toolchain/check/handle_impl.cpp

@@ -14,6 +14,7 @@
 #include "toolchain/check/inst.h"
 #include "toolchain/check/modifiers.h"
 #include "toolchain/check/name_lookup.h"
+#include "toolchain/check/name_scope.h"
 #include "toolchain/check/pattern_match.h"
 #include "toolchain/check/type.h"
 #include "toolchain/check/type_completion.h"
@@ -24,6 +25,16 @@
 
 namespace Carbon::Check {
 
+// Returns the implicit `Self` type for an `impl` when it's in a `class`
+// declaration.
+//
+// TODO: Mixin scopes also have a default `Self` type.
+static auto GetImplDefaultSelfType(Context& context,
+                                   const ClassScope& class_scope)
+    -> SemIR::TypeId {
+  return context.classes().Get(class_scope.class_decl.class_id).self_type_id;
+}
+
 auto HandleParseNode(Context& context, Parse::ImplIntroducerId node_id)
     -> bool {
   // This might be a generic impl.
@@ -56,14 +67,45 @@ auto HandleParseNode(Context& context, Parse::ForallId /*node_id*/) -> bool {
 
 auto HandleParseNode(Context& context, Parse::ImplTypeAsId node_id) -> bool {
   auto [self_node, self_id] = context.node_stack().PopExprWithNodeId();
-  auto self_type_inst_id = ExprAsType(context, self_node, self_id).inst_id;
-  context.node_stack().Push(node_id, self_type_inst_id);
+  auto self_type = ExprAsType(context, self_node, self_id);
+
+  const auto& introducer = context.decl_introducer_state_stack().innermost();
+  if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extend)) {
+    // TODO: Also handle the parent scope being a mixin.
+    if (auto class_scope = TryAsClassScope(
+            context, context.decl_name_stack().PeekParentScopeId())) {
+      // If we're not inside a class at all, that will be diagnosed against the
+      // `extend` elsewhere.
+      auto extend_node = introducer.modifier_node_id(ModifierOrder::Extend);
+      CARBON_DIAGNOSTIC(ExtendImplSelfAs, Error,
+                        "cannot `extend` an `impl` with an explicit self type");
+      auto diag = context.emitter().Build(extend_node, ExtendImplSelfAs);
+
+      if (self_type.type_id == GetImplDefaultSelfType(context, *class_scope)) {
+        // If the explicit self type is the default, suggest removing it with a
+        // diagnostic, but continue as if no error occurred since the self-type
+        // is semantically valid.
+        CARBON_DIAGNOSTIC(ExtendImplSelfAsDefault, Note,
+                          "remove the explicit `Self` type here");
+        diag.Note(self_node, ExtendImplSelfAsDefault);
+        if (self_type.type_id != SemIR::ErrorInst::TypeId) {
+          diag.Emit();
+        }
+      } else if (self_type.type_id != SemIR::ErrorInst::TypeId) {
+        // Otherwise, the self-type is an error.
+        diag.Emit();
+        class_scope->name_scope->set_has_error();
+        self_type.inst_id = SemIR::ErrorInst::TypeInstId;
+      }
+    }
+  }
 
   // Introduce `Self`. Note that we add this name lexically rather than adding
   // to the `NameScopeId` of the `impl`, because this happens before we enter
   // the `impl` scope or even identify which `impl` we're declaring.
   // TODO: Revisit this once #3714 is resolved.
-  AddNameToLookup(context, SemIR::NameId::SelfType, self_type_inst_id);
+  AddNameToLookup(context, SemIR::NameId::SelfType, self_type.inst_id);
+  context.node_stack().Push(node_id, self_type.inst_id);
   return true;
 }
 
@@ -71,8 +113,9 @@ auto HandleParseNode(Context& context, Parse::ImplDefaultSelfAsId node_id)
     -> bool {
   auto self_inst_id = SemIR::TypeInstId::None;
 
-  if (auto self_type_id = GetImplDefaultSelfType(context);
-      self_type_id.has_value()) {
+  if (auto class_scope = TryAsClassScope(
+          context, context.decl_name_stack().PeekParentScopeId())) {
+    auto self_type_id = GetImplDefaultSelfType(context, *class_scope);
     // Build the implicit access to the enclosing `Self`.
     // TODO: Consider calling `HandleNameAsExpr` to build this implicit `Self`
     // expression. We've already done the work to check that the enclosing

toolchain/check/handle_require.cpp

@@ -75,6 +75,7 @@ auto HandleParseNode(Context& context, Parse::RequireDefaultSelfImplsId node_id)
   }
 
   CARBON_CHECK(context.types().Is<SemIR::FacetType>(self_type_id));
+  // TODO: We could simplify with a call to ExprAsType, like below?
   auto self_facet_as_type = AddTypeInst<SemIR::FacetAccessType>(
       context, node_id,
       {.type_id = SemIR::TypeType::TypeId,
@@ -86,16 +87,21 @@ auto HandleParseNode(Context& context, Parse::RequireDefaultSelfImplsId node_id)
 auto HandleParseNode(Context& context, Parse::RequireTypeImplsId node_id)
     -> bool {
   auto [self_node_id, self_inst_id] = context.node_stack().PopExprWithNodeId();
+  auto self_type = ExprAsType(context, self_node_id, self_inst_id);
 
-  auto introducer = context.decl_introducer_state_stack().innermost();
+  const auto& introducer = context.decl_introducer_state_stack().innermost();
   if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extend)) {
-    CARBON_DIAGNOSTIC(RequireImplsExtendWithExplicitSelf, Error,
-                      "`extend require impls` with explicit type");
-    context.emitter().Emit(self_node_id, RequireImplsExtendWithExplicitSelf);
-    self_inst_id = SemIR::ErrorInst::InstId;
+    if (self_type.type_id != SemIR::ErrorInst::TypeId) {
+      CARBON_DIAGNOSTIC(RequireImplsExtendWithExplicitSelf, Error,
+                        "`extend require impls` with explicit type");
+      // TODO: If the explicit self-type matches a lookup of NameId::SelfType,
+      // add a note to the diagnostic: "remove the explicit `Self` type here",
+      // and continue without an ErrorInst. See ExtendImplSelfAsDefault.
+      context.emitter().Emit(self_node_id, RequireImplsExtendWithExplicitSelf);
+    }
+    self_type.inst_id = SemIR::ErrorInst::TypeInstId;
   }
 
-  auto self_type = ExprAsType(context, self_node_id, self_inst_id);
   context.node_stack().Push(node_id, self_type.inst_id);
   return true;
 }
@@ -258,6 +264,8 @@ auto HandleParseNode(Context& context, Parse::RequireDeclId node_id) -> bool {
     return true;
   }
 
+  // TODO: When extend is true, any errors should propagate up to the parent
+  // scope.
   bool extend = introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extend);
 
   auto require_impls_decl =

toolchain/check/impl.cpp

@@ -399,24 +399,10 @@ static auto TryAsClassScope(Context& context, SemIR::NameScopeId scope_id)
   return context.insts().TryGetAs<SemIR::ClassDecl>(scope.inst_id());
 }
 
-auto GetImplDefaultSelfType(Context& context) -> SemIR::TypeId {
-  auto parent_scope_id = context.decl_name_stack().PeekParentScopeId();
-
-  if (auto class_decl = TryAsClassScope(context, parent_scope_id)) {
-    return context.classes().Get(class_decl->class_id).self_type_id;
-  }
-
-  // TODO: This is also valid in a mixin.
-
-  return SemIR::TypeId::None;
-}
-
 // Process an `extend impl` declaration by extending the impl scope with the
 // `impl`'s scope.
 static auto ExtendImpl(Context& context, Parse::NodeId extend_node,
                        SemIR::LocId loc_id, SemIR::ImplId impl_id,
-                       Parse::NodeId self_type_node_id,
-                       SemIR::TypeId self_type_id,
                        SemIR::LocId implicit_params_loc_id,
                        SemIR::TypeInstId constraint_type_inst_id,
                        SemIR::TypeId constraint_type_id) -> bool {
@@ -443,31 +429,6 @@ static auto ExtendImpl(Context& context, Parse::NodeId extend_node,
 
   const auto& impl = context.impls().Get(impl_id);
 
-  if (context.parse_tree().node_kind(self_type_node_id) ==
-      Parse::NodeKind::ImplTypeAs) {
-    CARBON_DIAGNOSTIC(ExtendImplSelfAs, Error,
-                      "cannot `extend` an `impl` with an explicit self type");
-    auto diag = context.emitter().Build(extend_node, ExtendImplSelfAs);
-
-    // If the explicit self type is not the default, just bail out.
-    if (self_type_id != GetImplDefaultSelfType(context)) {
-      diag.Emit();
-      parent_scope.set_has_error();
-      return false;
-    }
-
-    // The explicit self type is the same as the default self type, so suggest
-    // removing it and recover as if it were not present.
-    if (auto self_as =
-            context.parse_tree_and_subtrees().ExtractAs<Parse::ImplTypeAs>(
-                self_type_node_id)) {
-      CARBON_DIAGNOSTIC(ExtendImplSelfAsDefault, Note,
-                        "remove the explicit `Self` type here");
-      diag.Note(self_as->type_expr, ExtendImplSelfAsDefault);
-    }
-    diag.Emit();
-  }
-
   if (impl.witness_id == SemIR::ErrorInst::InstId) {
     parent_scope.set_has_error();
   } else {
@@ -618,8 +579,7 @@ auto StartImplDecl(Context& context, SemIR::LocId loc_id,
                  stored_impl_info.generic_id)});
       }
       if (!ExtendImpl(context, extend_impl->extend_node_id, loc_id,
-                      impl_decl.impl_id, extend_impl->self_type_node_id,
-                      self_type_id, implicit_params_loc_id, constraint_id,
+                      impl_decl.impl_id, implicit_params_loc_id, constraint_id,
                       extend_impl->constraint_type_id)) {
         // Don't allow the invalid impl to be used.
         FillImplWitnessWithErrors(context, stored_impl_info);

toolchain/check/impl.h

@@ -52,10 +52,6 @@ auto CheckConstraintIsInterface(Context& context, SemIR::InstId impl_decl_id,
                                 SemIR::TypeInstId constraint_id)
     -> SemIR::SpecificInterface;
 
-// Returns the implicit `Self` type for an `impl` when it's in a `class`
-// declaration.
-auto GetImplDefaultSelfType(Context& context) -> SemIR::TypeId;
-
 // For `StartImplDecl`, additional details for an `extend impl` declaration.
 struct ExtendImplDecl {
   Parse::NodeId self_type_node_id;

toolchain/check/name_scope.cpp

@@ -0,0 +1,25 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include "toolchain/check/name_scope.h"
+
+namespace Carbon::Check {
+
+auto TryAsClassScope(Context& context, SemIR::NameScopeId scope_id)
+    -> std::optional<ClassScope> {
+  if (!scope_id.has_value()) {
+    return std::nullopt;
+  }
+  auto& scope = context.name_scopes().Get(scope_id);
+  if (!scope.inst_id().has_value()) {
+    return std::nullopt;
+  }
+  auto class_decl = context.insts().TryGetAs<SemIR::ClassDecl>(scope.inst_id());
+  if (!class_decl) {
+    return std::nullopt;
+  }
+  return {{.class_decl = *class_decl, .name_scope = &scope}};
+}
+
+}  // namespace Carbon::Check

toolchain/check/name_scope.h

@@ -0,0 +1,28 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#ifndef CARBON_TOOLCHAIN_CHECK_NAME_SCOPE_H_
+#define CARBON_TOOLCHAIN_CHECK_NAME_SCOPE_H_
+
+#include <optional>
+
+#include "toolchain/check/context.h"
+#include "toolchain/sem_ir/ids.h"
+#include "toolchain/sem_ir/typed_insts.h"
+
+namespace Carbon::Check {
+
+struct ClassScope {
+  SemIR::ClassDecl class_decl;
+  SemIR::NameScope* name_scope;
+};
+
+// If the specified name scope corresponds to a class, returns the corresponding
+// class declaration.
+auto TryAsClassScope(Context& context, SemIR::NameScopeId scope_id)
+    -> std::optional<ClassScope>;
+
+}  // namespace Carbon::Check
+
+#endif  // CARBON_TOOLCHAIN_CHECK_NAME_SCOPE_H_

toolchain/check/testdata/facet/require_invalid.carbon

@@ -123,3 +123,29 @@ interface Z {
   // CHECK:STDERR:
   extend require () impls Y;
 }
+
+// --- fail_extend_require_with_nonexistent_type.carbon
+library "[[@TEST_NAME]]";
+
+interface Y {}
+
+interface Z {
+  // CHECK:STDERR: fail_extend_require_with_nonexistent_type.carbon:[[@LINE+4]]:18: error: name `nonexistent` not found [NameNotFound]
+  // CHECK:STDERR:   extend require nonexistent impls Y;
+  // CHECK:STDERR:                  ^~~~~~~~~~~
+  // CHECK:STDERR:
+  extend require nonexistent impls Y;
+}
+
+// --- fail_extend_require_with_nonexistent_type_pointer.carbon
+library "[[@TEST_NAME]]";
+
+interface Y {}
+
+interface Z {
+  // CHECK:STDERR: fail_extend_require_with_nonexistent_type_pointer.carbon:[[@LINE+4]]:18: error: name `nonexistent` not found [NameNotFound]
+  // CHECK:STDERR:   extend require nonexistent* impls Y;
+  // CHECK:STDERR:                  ^~~~~~~~~~~
+  // CHECK:STDERR:
+  extend require nonexistent* impls Y;
+}

toolchain/check/testdata/impl/extend_impl.carbon

@@ -39,6 +39,20 @@ interface I {}
 
 class C {
   // CHECK:STDERR: fail_extend_impl_nonexistent.carbon:[[@LINE+4]]:15: error: name `nonexistent` not found [NameNotFound]
+  // CHECK:STDERR:   extend impl nonexistent as I {}
+  // CHECK:STDERR:               ^~~~~~~~~~~
+  // CHECK:STDERR:
+  extend impl nonexistent as I {}
+}
+
+// --- fail_extend_impl_nonexistent_pointer.carbon
+
+library "[[@TEST_NAME]]";
+
+interface I {}
+
+class C {
+  // CHECK:STDERR: fail_extend_impl_nonexistent_pointer.carbon:[[@LINE+4]]:15: error: name `nonexistent` not found [NameNotFound]
   // CHECK:STDERR:   extend impl nonexistent* as I {}
   // CHECK:STDERR:               ^~~~~~~~~~~
   // CHECK:STDERR:

toolchain/check/testdata/impl/fail_extend_impl_type_as.carbon

@@ -58,7 +58,6 @@ class E {
 // CHECK:STDOUT:   %Int.type: type = generic_class_type @Int [concrete]
 // CHECK:STDOUT:   %Int.generic: %Int.type = struct_value () [concrete]
 // CHECK:STDOUT:   %i32: type = class_type @Int, @Int(%int_32) [concrete]
-// CHECK:STDOUT:   %I.impl_witness.943: <witness> = impl_witness @C.%I.impl_witness_table [concrete]
 // CHECK:STDOUT:   %empty_struct_type: type = struct_type {} [concrete]
 // CHECK:STDOUT:   %complete_type.357: <witness> = complete_type_witness %empty_struct_type [concrete]
 // CHECK:STDOUT:   %D: type = class_type @D [concrete]
@@ -101,7 +100,7 @@ class E {
 // CHECK:STDOUT: !requires:
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: impl @i32.as.I.impl: %i32 as %I.ref {
+// CHECK:STDOUT: impl @<error>.as.I.impl: <error> as %I.ref {
 // CHECK:STDOUT: !members:
 // CHECK:STDOUT:   witness = <error>
 // CHECK:STDOUT: }
@@ -114,13 +113,11 @@ class E {
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: class @C {
-// CHECK:STDOUT:   impl_decl @i32.as.I.impl [concrete] {} {
+// CHECK:STDOUT:   impl_decl @<error>.as.I.impl [concrete] {} {
 // CHECK:STDOUT:     %int_32: Core.IntLiteral = int_value 32 [concrete = constants.%int_32]
 // CHECK:STDOUT:     %i32: type = class_type @Int, @Int(constants.%int_32) [concrete = constants.%i32]
 // CHECK:STDOUT:     %I.ref: type = name_ref I, file.%I.decl [concrete = constants.%I.type]
 // CHECK:STDOUT:   }
-// CHECK:STDOUT:   %I.impl_witness_table = impl_witness_table (), @i32.as.I.impl [concrete]
-// CHECK:STDOUT:   %I.impl_witness: <witness> = impl_witness %I.impl_witness_table [concrete = constants.%I.impl_witness.943]
 // CHECK:STDOUT:   %complete_type: <witness> = complete_type_witness constants.%empty_struct_type [concrete = constants.%complete_type.357]
 // CHECK:STDOUT:   complete_type_witness = %complete_type
 // CHECK:STDOUT: