Repository: carbon-language/carbon-lang
File: toolchain/check/handle_impl.cpp (C++)

Code before:
    auto class_scope =
        TryAsClassScope(context, context.decl_name_stack().PeekParentScopeId());
    if (class_scope) {
      // 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);
  context.node_stack().Push(node_id, self_type.inst_id);
  return true;
}

auto HandleParseNode(Context& context, Parse::ImplDefaultSelfAsId node_id)
    -> bool {
  auto self_inst_id = SemIR::TypeInstId::None;

  auto class_scope =
      TryAsClassScope(context, context.decl_name_stack().PeekParentScopeId());
  if (class_scope) {
    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
    // context is a class and found its `Self`, so additionally performing an
    // unqualified name lookup would be redundant work, but would avoid

Review comment (suggestion):
I think we should also call `name_scope->set_has_error()` if there's an error in the self type itself. So maybe:

```suggestion
      auto diag =
          self_type.type_id == SemIR::ErrorInst::TypeId
              ? context.emitter().BuildSuppressed()
              : 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);
      } else {
        // Otherwise, the self-type is an error.
        class_scope->name_scope->set_has_error();
        self_type.inst_id = SemIR::ErrorInst::TypeInstId;
      }
    }
    diag.Emit();
```

Code after:
    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);
  context.node_stack().Push(node_id, self_type.inst_id);
  return true;
}

auto HandleParseNode(Context& context, Parse::ImplDefaultSelfAsId node_id)
    -> bool {
  auto self_inst_id = SemIR::TypeInstId::None;

  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
    // context is a class and found its `Self`, so additionally performing an
    // unqualified name lookup would be redundant work, but would avoid
    // duplicating the handling of the `Self` expression.
    self_inst_id = AddTypeInst(