Add diagnostics for `extend impl` misuse. (#3721)

To make the implementation simpler, make `PopWithParseNodeIf` return
`pair<NodeId, optional<value>>` rather than `optional<pair<NodeId,
value>>`. While wrapping the whole result in `optional` seems more
principled, it's significantly harder to work with.

toolchain/check/handle_function.cpp

@@ -70,26 +70,25 @@ static auto BuildFunctionDecl(Context& context,
 
   auto return_type_id = SemIR::TypeId::Invalid;
   auto return_slot_id = SemIR::InstId::Invalid;
-  if (auto return_node_and_id =
+  if (auto [return_node, return_storage_id] =
           context.node_stack()
-              .PopWithParseNodeIf<Parse::NodeKind::ReturnType>()) {
-    auto return_storage_id = return_node_and_id->second;
-    return_type_id = context.insts().Get(return_storage_id).type_id();
+              .PopWithParseNodeIf<Parse::NodeKind::ReturnType>();
+      return_storage_id) {
+    return_type_id = context.insts().Get(*return_storage_id).type_id();
 
     return_type_id = context.AsCompleteType(return_type_id, [&] {
       CARBON_DIAGNOSTIC(IncompleteTypeInFunctionReturnType, Error,
                         "Function returns incomplete type `{0}`.",
                         SemIR::TypeId);
-      return context.emitter().Build(return_node_and_id->first,
-                                     IncompleteTypeInFunctionReturnType,
-                                     return_type_id);
+      return context.emitter().Build(
+          return_node, IncompleteTypeInFunctionReturnType, return_type_id);
     });
 
     if (!SemIR::GetInitRepr(context.sem_ir(), return_type_id)
              .has_return_slot()) {
       // The function only has a return slot if it uses in-place initialization.
     } else {
-      return_slot_id = return_storage_id;
+      return_slot_id = *return_storage_id;
     }
   }
 

toolchain/check/handle_impl.cpp

@@ -6,6 +6,7 @@
 #include "toolchain/check/convert.h"
 #include "toolchain/check/decl_name_stack.h"
 #include "toolchain/check/modifiers.h"
+#include "toolchain/parse/typed_nodes.h"
 #include "toolchain/sem_ir/ids.h"
 #include "toolchain/sem_ir/typed_insts.h"
 
@@ -63,8 +64,7 @@ static auto TryAsClassScope(Context& context, SemIR::NameScopeId scope_id)
   return context.insts().TryGetAs<SemIR::ClassDecl>(scope.inst_id);
 }
 
-static auto GetDefaultSelfType(Context& context, Parse::NodeId parse_node)
-    -> SemIR::TypeId {
+static auto GetDefaultSelfType(Context& context) -> SemIR::TypeId {
   auto enclosing_scope_id = context.decl_name_stack().PeekTargetScope();
 
   if (auto class_decl = TryAsClassScope(context, enclosing_scope_id)) {
@@ -73,24 +73,32 @@ static auto GetDefaultSelfType(Context& context, Parse::NodeId parse_node)
 
   // TODO: This is also valid in a mixin.
 
-  CARBON_DIAGNOSTIC(ImplAsOutsideClass, Error,
-                    "`impl as` can only be used in a class.");
-  context.emitter().Emit(parse_node, ImplAsOutsideClass);
-  return SemIR::TypeId::Error;
+  return SemIR::TypeId::Invalid;
 }
 
 auto HandleDefaultSelfImplAs(Context& context,
                              Parse::DefaultSelfImplAsId parse_node) -> bool {
-  auto self_type_id = GetDefaultSelfType(context, parse_node);
+  auto self_type_id = GetDefaultSelfType(context);
+  if (!self_type_id.is_valid()) {
+    CARBON_DIAGNOSTIC(ImplAsOutsideClass, Error,
+                      "`impl as` can only be used in a class.");
+    context.emitter().Emit(parse_node, ImplAsOutsideClass);
+    self_type_id = SemIR::TypeId::Error;
+  }
+
   context.node_stack().Push(parse_node, self_type_id);
   return true;
 }
 
 // Process an `extend impl` declaration by extending the impl scope with the
 // `impl`'s scope.
-static auto ExtendImpl(Context& context, Parse::AnyImplDeclId parse_node,
-                       SemIR::TypeId constraint_id) -> void {
+static auto ExtendImpl(Context& context, Parse::NodeId extend_node,
+                       Parse::AnyImplDeclId parse_node,
+                       Parse::NodeId self_type_node, SemIR::TypeId self_type_id,
+                       Parse::NodeId params_node, SemIR::TypeId constraint_id)
+    -> void {
   auto enclosing_scope_id = context.decl_name_stack().PeekTargetScope();
+  auto& enclosing_scope = context.name_scopes().Get(enclosing_scope_id);
 
   // TODO: This is also valid in a mixin.
   if (!TryAsClassScope(context, enclosing_scope_id)) {
@@ -99,7 +107,38 @@ static auto ExtendImpl(Context& context, Parse::AnyImplDeclId parse_node,
     context.emitter().Emit(parse_node, ExtendImplOutsideClass);
     return;
   }
-  auto& enclosing_scope = context.name_scopes().Get(enclosing_scope_id);
+
+  if (params_node.is_valid()) {
+    CARBON_DIAGNOSTIC(ExtendImplForall, Error,
+                      "Cannot `extend` a parameterized `impl`.");
+    context.emitter().Emit(extend_node, ExtendImplForall);
+    enclosing_scope.has_error = true;
+    return;
+  }
+
+  if (context.parse_tree().node_kind(self_type_node) ==
+      Parse::NodeKind::TypeImplAs) {
+    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 != GetDefaultSelfType(context)) {
+      diag.Emit();
+      enclosing_scope.has_error = true;
+      return;
+    }
+
+    // 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().ExtractAs<Parse::TypeImplAs>(self_type_node)) {
+      CARBON_DIAGNOSTIC(ExtendImplSelfAsDefault, Note,
+                        "Remove the explicit `Self` type here.");
+      diag.Note(self_as->type_expr, ExtendImplSelfAsDefault);
+    }
+    diag.Emit();
+  }
 
   auto interface_type =
       context.types().TryGetAs<SemIR::InterfaceType>(constraint_id);
@@ -132,9 +171,10 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId parse_node)
     -> std::pair<SemIR::ImplId, SemIR::InstId> {
   auto [constraint_node, constraint_id] =
       context.node_stack().PopExprWithParseNode();
-  auto self_type_id = context.node_stack().Pop<Parse::NodeCategory::ImplAs>();
-
-  auto params_id = context.node_stack().PopIf<Parse::NodeKind::ImplForall>();
+  auto [self_type_node, self_type_id] =
+      context.node_stack().PopWithParseNode<Parse::NodeCategory::ImplAs>();
+  auto [params_node, params_id] =
+      context.node_stack().PopWithParseNodeIf<Parse::NodeKind::ImplForall>();
   auto decl_block_id = context.inst_block_stack().Pop();
   context.node_stack().PopForSoloParseNode<Parse::NodeKind::ImplIntroducer>();
 
@@ -168,9 +208,9 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId parse_node)
   // For an `extend impl` declaration, mark the impl as extending this `impl`.
   if (!!(context.decl_state_stack().innermost().modifier_set &
          KeywordModifierSet::Extend)) {
-    // TODO: Diagnose combining `extend` with `forall`.
-    // TODO: Diagnose combining `extend` with an explicit self type.
-    ExtendImpl(context, parse_node, constraint_type_id);
+    auto extend_node = context.decl_state_stack().innermost().saw_decl_modifier;
+    ExtendImpl(context, extend_node, parse_node, self_type_node, self_type_id,
+               params_node, constraint_type_id);
   }
 
   context.decl_state_stack().Pop(DeclState::Impl);

toolchain/check/node_stack.h

@@ -9,6 +9,7 @@
 
 #include "common/vlog.h"
 #include "llvm/ADT/SmallVector.h"
+#include "toolchain/parse/node_ids.h"
 #include "toolchain/parse/node_kind.h"
 #include "toolchain/parse/tree.h"
 #include "toolchain/parse/typed_nodes.h"
@@ -247,28 +248,6 @@ class NodeStack {
     return std::make_pair(parse_node, id);
   }
 
-  // Pops the top of the stack and returns the parse_node and the ID if it is
-  // of the specified kind.
-  template <const Parse::NodeKind& RequiredParseKind>
-  auto PopWithParseNodeIf()
-      -> std::optional<decltype(PopWithParseNode<RequiredParseKind>())> {
-    if (!PeekIs<RequiredParseKind>()) {
-      return std::nullopt;
-    }
-    return PopWithParseNode<RequiredParseKind>();
-  }
-
-  // Pops the top of the stack and returns the parse_node and the ID if it is
-  // of the specified category
-  template <Parse::NodeCategory RequiredParseCategory>
-  auto PopWithParseNodeIf()
-      -> std::optional<decltype(PopWithParseNode<RequiredParseCategory>())> {
-    if (!PeekIs<RequiredParseCategory>()) {
-      return std::nullopt;
-    }
-    return PopWithParseNode<RequiredParseCategory>();
-  }
-
   // Pops an expression from the top of the stack and returns the ID.
   // Expressions always map Parse::NodeCategory::Expr nodes to SemIR::InstId.
   auto PopExpr() -> SemIR::InstId { return PopExprWithParseNode().second; }
@@ -320,6 +299,30 @@ class NodeStack {
     return std::nullopt;
   }
 
+  // Pops the top of the stack and returns the parse_node and the ID if it is
+  // of the specified kind.
+  template <const Parse::NodeKind& RequiredParseKind>
+  auto PopWithParseNodeIf()
+      -> std::pair<Parse::NodeIdForKind<RequiredParseKind>,
+                   decltype(PopIf<RequiredParseKind>())> {
+    if (!PeekIs<RequiredParseKind>()) {
+      return {Parse::NodeId::Invalid, std::nullopt};
+    }
+    return PopWithParseNode<RequiredParseKind>();
+  }
+
+  // Pops the top of the stack and returns the parse_node and the ID if it is
+  // of the specified category.
+  template <Parse::NodeCategory RequiredParseCategory>
+  auto PopWithParseNodeIf()
+      -> std::pair<Parse::NodeIdInCategory<RequiredParseCategory>,
+                   decltype(PopIf<RequiredParseCategory>())> {
+    if (!PeekIs<RequiredParseCategory>()) {
+      return {Parse::NodeId::Invalid, std::nullopt};
+    }
+    return PopWithParseNode<RequiredParseCategory>();
+  }
+
   // Peeks at the parse node of the top of the node stack.
   auto PeekParseNode() const -> Parse::NodeId {
     return stack_.back().parse_node;

toolchain/check/testdata/impl/fail_extend_impl_forall.carbon

@@ -12,10 +12,9 @@ interface GenericInterface(T:! type) {
 }
 
 class C {
-  // TODO: We should reject this for combining `extend` with `forall`.
-  // CHECK:STDERR: fail_extend_impl_forall.carbon:[[@LINE+6]]:3: ERROR: Semantics TODO: `extending non-interface constraint`.
+  // CHECK:STDERR: fail_extend_impl_forall.carbon:[[@LINE+6]]:3: ERROR: Cannot `extend` a parameterized `impl`.
   // CHECK:STDERR:   extend impl forall [T:! type] as GenericInterface(T) {
-  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+  // CHECK:STDERR:   ^~~~~~
   // CHECK:STDERR: fail_extend_impl_forall.carbon:[[@LINE+3]]:36: ERROR: Value of type `type` is not callable.
   // CHECK:STDERR:   extend impl forall [T:! type] as GenericInterface(T) {
   // CHECK:STDERR:                                    ^~~~~~~~~~~~~~~~~

toolchain/check/testdata/impl/fail_extend_impl_type_as.carbon

@@ -0,0 +1,91 @@
+// 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
+//
+// AUTOUPDATE
+
+interface I {}
+
+class C {
+  // CHECK:STDERR: fail_extend_impl_type_as.carbon:[[@LINE+3]]:3: ERROR: Cannot `extend` an `impl` with an explicit self type.
+  // CHECK:STDERR:   extend impl i32 as I {}
+  // CHECK:STDERR:   ^~~~~~
+  extend impl i32 as I {}
+}
+
+class D {
+  // CHECK:STDERR: fail_extend_impl_type_as.carbon:[[@LINE+6]]:3: ERROR: Cannot `extend` an `impl` with an explicit self type.
+  // CHECK:STDERR:   extend impl D as I;
+  // CHECK:STDERR:   ^~~~~~
+  // CHECK:STDERR: fail_extend_impl_type_as.carbon:[[@LINE+3]]:15: Remove the explicit `Self` type here.
+  // CHECK:STDERR:   extend impl D as I;
+  // CHECK:STDERR:               ^
+  extend impl D as I;
+}
+
+class E {
+  // CHECK:STDERR: fail_extend_impl_type_as.carbon:[[@LINE+6]]:3: ERROR: Cannot `extend` an `impl` with an explicit self type.
+  // CHECK:STDERR:   extend impl Self as I {}
+  // CHECK:STDERR:   ^~~~~~
+  // CHECK:STDERR: fail_extend_impl_type_as.carbon:[[@LINE+3]]:15: Remove the explicit `Self` type here.
+  // CHECK:STDERR:   extend impl Self as I {}
+  // CHECK:STDERR:               ^~~~
+  extend impl Self as I {}
+}
+
+// CHECK:STDOUT: --- fail_extend_impl_type_as.carbon
+// CHECK:STDOUT:
+// CHECK:STDOUT: constants {
+// CHECK:STDOUT:   %C: type = class_type @C [template]
+// CHECK:STDOUT:   %.1: type = interface_type @I [template]
+// CHECK:STDOUT:   %.2: type = struct_type {} [template]
+// CHECK:STDOUT:   %D: type = class_type @D [template]
+// CHECK:STDOUT:   %E: type = class_type @E [template]
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: file {
+// CHECK:STDOUT:   package: <namespace> = namespace {.I = %I.decl, .C = %C.decl, .D = %D.decl, .E = %E.decl} [template]
+// CHECK:STDOUT:   %I.decl = interface_decl @I, ()
+// CHECK:STDOUT:   %C.decl = class_decl @C, ()
+// CHECK:STDOUT:   %D.decl = class_decl @D, ()
+// CHECK:STDOUT:   %E.decl = class_decl @E, ()
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: interface @I {
+// CHECK:STDOUT:
+// CHECK:STDOUT: !members:
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: impl @impl.1: i32 as I {
+// CHECK:STDOUT:
+// CHECK:STDOUT: !members:
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: impl @impl.2: D as I;
+// CHECK:STDOUT:
+// CHECK:STDOUT: impl @impl.3: E as I {
+// CHECK:STDOUT:
+// CHECK:STDOUT: !members:
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: class @C {
+// CHECK:STDOUT:   impl_decl @impl.1, (<unexpected instref inst+5>)
+// CHECK:STDOUT:
+// CHECK:STDOUT: !members:
+// CHECK:STDOUT:   has_error
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: class @D {
+// CHECK:STDOUT:   impl_decl @impl.2, (<unexpected instref inst+10>, <unexpected instref inst+11>)
+// CHECK:STDOUT:
+// CHECK:STDOUT: !members:
+// CHECK:STDOUT:   extend name_scope1
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: class @E {
+// CHECK:STDOUT:   impl_decl @impl.3, (<unexpected instref inst+15>, <unexpected instref inst+16>)
+// CHECK:STDOUT:
+// CHECK:STDOUT: !members:
+// CHECK:STDOUT:   extend name_scope1
+// CHECK:STDOUT: }
+// CHECK:STDOUT:

toolchain/check/testdata/impl/todo_extend_impl_type_as.carbon

@@ -1,44 +0,0 @@
-// 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
-//
-// AUTOUPDATE
-
-interface I {}
-
-class C {
-  // TODO: We should reject this for combining `extend` with a specific self type.
-  extend impl i32 as I {}
-}
-
-// CHECK:STDOUT: --- todo_extend_impl_type_as.carbon
-// CHECK:STDOUT:
-// CHECK:STDOUT: constants {
-// CHECK:STDOUT:   %C: type = class_type @C [template]
-// CHECK:STDOUT:   %.1: type = interface_type @I [template]
-// CHECK:STDOUT:   %.2: type = struct_type {} [template]
-// CHECK:STDOUT: }
-// CHECK:STDOUT:
-// CHECK:STDOUT: file {
-// CHECK:STDOUT:   package: <namespace> = namespace {.I = %I.decl, .C = %C.decl} [template]
-// CHECK:STDOUT:   %I.decl = interface_decl @I, ()
-// CHECK:STDOUT:   %C.decl = class_decl @C, ()
-// CHECK:STDOUT: }
-// CHECK:STDOUT:
-// CHECK:STDOUT: interface @I {
-// CHECK:STDOUT:
-// CHECK:STDOUT: !members:
-// CHECK:STDOUT: }
-// CHECK:STDOUT:
-// CHECK:STDOUT: impl @impl: i32 as I {
-// CHECK:STDOUT:
-// CHECK:STDOUT: !members:
-// CHECK:STDOUT: }
-// CHECK:STDOUT:
-// CHECK:STDOUT: class @C {
-// CHECK:STDOUT:   impl_decl @impl, (<unexpected instref inst+5>)
-// CHECK:STDOUT:
-// CHECK:STDOUT: !members:
-// CHECK:STDOUT:   extend name_scope1
-// CHECK:STDOUT: }
-// CHECK:STDOUT:

toolchain/diagnostics/diagnostic_kind.def

@@ -175,7 +175,10 @@ CARBON_DIAGNOSTIC_KIND(InterfaceForwardDeclaredHere)
 CARBON_DIAGNOSTIC_KIND(InterfaceUndefinedWithinDefinition)
 
 // Impl checking.
+CARBON_DIAGNOSTIC_KIND(ExtendImplForall)
 CARBON_DIAGNOSTIC_KIND(ExtendImplOutsideClass)
+CARBON_DIAGNOSTIC_KIND(ExtendImplSelfAs)
+CARBON_DIAGNOSTIC_KIND(ExtendImplSelfAsDefault)
 CARBON_DIAGNOSTIC_KIND(ExtendUndefinedInterface)
 CARBON_DIAGNOSTIC_KIND(ImplAsOutsideClass)
 CARBON_DIAGNOSTIC_KIND(ImplPreviousDefinition)

toolchain/parse/typed_nodes.h

@@ -956,7 +956,7 @@ struct TypeImplAs {
   static constexpr auto Kind =
       NodeKind::TypeImplAs.Define(NodeCategory::ImplAs);
 
-  std::optional<AnyExprId> type_expr;
+  AnyExprId type_expr;
 };
 
 // `impl T as I`