From 2d38978756559246f5d28705b2e4ba2d1159467f Mon Sep 17 00:00:00 2001 From: Dana Jansens Date: Tue, 9 Dec 2025 15:38:50 -0500 Subject: [PATCH] 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`) 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 | 2 + toolchain/check/handle_impl.cpp | 53 +++++++++++++++++-- toolchain/check/handle_require.cpp | 20 ++++--- toolchain/check/impl.cpp | 42 +-------------- toolchain/check/impl.h | 4 -- toolchain/check/name_scope.cpp | 25 +++++++++ toolchain/check/name_scope.h | 28 ++++++++++ .../testdata/facet/require_invalid.carbon | 26 +++++++++ .../check/testdata/impl/extend_impl.carbon | 14 +++++ .../impl/fail_extend_impl_type_as.carbon | 7 +-- 10 files changed, 160 insertions(+), 61 deletions(-) create mode 100644 toolchain/check/name_scope.cpp create mode 100644 toolchain/check/name_scope.h diff --git a/toolchain/check/BUILD b/toolchain/check/BUILD index 902d4f26c44f..8c96157a17c2 100644 --- a/toolchain/check/BUILD +++ b/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", diff --git a/toolchain/check/handle_impl.cpp b/toolchain/check/handle_impl.cpp index badc5891cbc2..99b6c76dd951 100644 --- a/toolchain/check/handle_impl.cpp +++ b/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 diff --git a/toolchain/check/handle_require.cpp b/toolchain/check/handle_require.cpp index 91526d4bcfc5..af8e819f65d8 100644 --- a/toolchain/check/handle_require.cpp +++ b/toolchain/check/handle_require.cpp @@ -75,6 +75,7 @@ auto HandleParseNode(Context& context, Parse::RequireDefaultSelfImplsId node_id) } CARBON_CHECK(context.types().Is(self_type_id)); + // TODO: We could simplify with a call to ExprAsType, like below? auto self_facet_as_type = AddTypeInst( 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 = diff --git a/toolchain/check/impl.cpp b/toolchain/check/impl.cpp index 78f17c64021a..4ca196d4f1d7 100644 --- a/toolchain/check/impl.cpp +++ b/toolchain/check/impl.cpp @@ -399,24 +399,10 @@ static auto TryAsClassScope(Context& context, SemIR::NameScopeId scope_id) return context.insts().TryGetAs(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( - 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); diff --git a/toolchain/check/impl.h b/toolchain/check/impl.h index efcc3b6a1853..e3e7a9f06c33 100644 --- a/toolchain/check/impl.h +++ b/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; diff --git a/toolchain/check/name_scope.cpp b/toolchain/check/name_scope.cpp new file mode 100644 index 000000000000..51ff1b9b20b0 --- /dev/null +++ b/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 { + 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(scope.inst_id()); + if (!class_decl) { + return std::nullopt; + } + return {{.class_decl = *class_decl, .name_scope = &scope}}; +} + +} // namespace Carbon::Check diff --git a/toolchain/check/name_scope.h b/toolchain/check/name_scope.h new file mode 100644 index 000000000000..04867d0ce678 --- /dev/null +++ b/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 + +#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; + +} // namespace Carbon::Check + +#endif // CARBON_TOOLCHAIN_CHECK_NAME_SCOPE_H_ diff --git a/toolchain/check/testdata/facet/require_invalid.carbon b/toolchain/check/testdata/facet/require_invalid.carbon index a17d687e44c6..8efa127a2614 100644 --- a/toolchain/check/testdata/facet/require_invalid.carbon +++ b/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; +} diff --git a/toolchain/check/testdata/impl/extend_impl.carbon b/toolchain/check/testdata/impl/extend_impl.carbon index 021185d79b09..c28bc3553b8a 100644 --- a/toolchain/check/testdata/impl/extend_impl.carbon +++ b/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: diff --git a/toolchain/check/testdata/impl/fail_extend_impl_type_as.carbon b/toolchain/check/testdata/impl/fail_extend_impl_type_as.carbon index 4528c12185de..0516cba56a34 100644 --- a/toolchain/check/testdata/impl/fail_extend_impl_type_as.carbon +++ b/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: = impl_witness @C.%I.impl_witness_table [concrete] // CHECK:STDOUT: %empty_struct_type: type = struct_type {} [concrete] // CHECK:STDOUT: %complete_type.357: = 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 @.as.I.impl: as %I.ref { // CHECK:STDOUT: !members: // CHECK:STDOUT: witness = // 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 @.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: = impl_witness %I.impl_witness_table [concrete = constants.%I.impl_witness.943] // CHECK:STDOUT: %complete_type: = complete_type_witness constants.%empty_struct_type [concrete = constants.%complete_type.357] // CHECK:STDOUT: complete_type_witness = %complete_type // CHECK:STDOUT: