Skip to content

Commit 78f03c4

Browse files
danakjdanakj
committed
less-completing-impl-as-chain-1
1 parent 78025fe commit 78f03c4

File tree

10 files changed

+160
-61
lines changed

10 files changed

+160
-61
lines changed

‎toolchain/check/BUILD‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ cc_library(
5757
"name_component.cpp",
5858
"name_lookup.cpp",
5959
"name_ref.cpp",
60+
"name_scope.cpp",
6061
"operator.cpp",
6162
"pattern.cpp",
6263
"pattern_match.cpp",
@@ -113,6 +114,7 @@ cc_library(
113114
"name_component.h",
114115
"name_lookup.h",
115116
"name_ref.h",
117+
"name_scope.h",
116118
"operator.h",
117119
"param_and_arg_refs_stack.h",
118120
"pattern.h",

‎toolchain/check/handle_impl.cpp‎

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#include "toolchain/check/inst.h"
1515
#include "toolchain/check/modifiers.h"
1616
#include "toolchain/check/name_lookup.h"
17+
#include "toolchain/check/name_scope.h"
1718
#include "toolchain/check/pattern_match.h"
1819
#include "toolchain/check/type.h"
1920
#include "toolchain/check/type_completion.h"
@@ -24,6 +25,16 @@
2425

2526
namespace Carbon::Check {
2627

28+
// Returns the implicit `Self` type for an `impl` when it's in a `class`
29+
// declaration.
30+
//
31+
// TODO: Mixin scopes also have a default `Self` type.
32+
static auto GetImplDefaultSelfType(Context& context,
33+
const ClassScope& class_scope)
34+
-> SemIR::TypeId {
35+
return context.classes().Get(class_scope.class_decl.class_id).self_type_id;
36+
}
37+
2738
auto HandleParseNode(Context& context, Parse::ImplIntroducerId node_id)
2839
-> bool {
2940
// This might be a generic impl.
@@ -56,23 +67,57 @@ auto HandleParseNode(Context& context, Parse::ForallId /*node_id*/) -> bool {
5667

5768
auto HandleParseNode(Context& context, Parse::ImplTypeAsId node_id) -> bool {
5869
auto [self_node, self_id] = context.node_stack().PopExprWithNodeId();
59-
auto self_type_inst_id = ExprAsType(context, self_node, self_id).inst_id;
60-
context.node_stack().Push(node_id, self_type_inst_id);
70+
auto self_type = ExprAsType(context, self_node, self_id);
71+
72+
const auto& introducer = context.decl_introducer_state_stack().innermost();
73+
if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extend)) {
74+
// TODO: Also handle the parent scope being a mixin.
75+
auto class_scope =
76+
TryAsClassScope(context, context.decl_name_stack().PeekParentScopeId());
77+
if (class_scope) {
78+
// If we're not inside a class at all, that will be diagnosed against the
79+
// `extend` elsewhere.
80+
auto extend_node = introducer.modifier_node_id(ModifierOrder::Extend);
81+
CARBON_DIAGNOSTIC(ExtendImplSelfAs, Error,
82+
"cannot `extend` an `impl` with an explicit self type");
83+
auto diag = context.emitter().Build(extend_node, ExtendImplSelfAs);
84+
85+
if (self_type.type_id == GetImplDefaultSelfType(context, *class_scope)) {
86+
// If the explicit self type is the default, suggest removing it with a
87+
// diagnostic, but continue as if no error occurred since the self-type
88+
// is semantically valid.
89+
CARBON_DIAGNOSTIC(ExtendImplSelfAsDefault, Note,
90+
"remove the explicit `Self` type here");
91+
diag.Note(self_node, ExtendImplSelfAsDefault);
92+
if (self_type.type_id != SemIR::ErrorInst::TypeId) {
93+
diag.Emit();
94+
}
95+
} else if (self_type.type_id != SemIR::ErrorInst::TypeId) {
96+
// Otherwise, the self-type is an error.
97+
diag.Emit();
98+
class_scope->name_scope->set_has_error();
99+
self_type.inst_id = SemIR::ErrorInst::TypeInstId;
100+
}
101+
}
102+
}
61103

62104
// Introduce `Self`. Note that we add this name lexically rather than adding
63105
// to the `NameScopeId` of the `impl`, because this happens before we enter
64106
// the `impl` scope or even identify which `impl` we're declaring.
65107
// TODO: Revisit this once #3714 is resolved.
66-
AddNameToLookup(context, SemIR::NameId::SelfType, self_type_inst_id);
108+
AddNameToLookup(context, SemIR::NameId::SelfType, self_type.inst_id);
109+
context.node_stack().Push(node_id, self_type.inst_id);
67110
return true;
68111
}
69112

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

74-
if (auto self_type_id = GetImplDefaultSelfType(context);
75-
self_type_id.has_value()) {
117+
auto class_scope =
118+
TryAsClassScope(context, context.decl_name_stack().PeekParentScopeId());
119+
if (class_scope) {
120+
auto self_type_id = GetImplDefaultSelfType(context, *class_scope);
76121
// Build the implicit access to the enclosing `Self`.
77122
// TODO: Consider calling `HandleNameAsExpr` to build this implicit `Self`
78123
// expression. We've already done the work to check that the enclosing

‎toolchain/check/handle_require.cpp‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ auto HandleParseNode(Context& context, Parse::RequireDefaultSelfImplsId node_id)
7575
}
7676

7777
CARBON_CHECK(context.types().Is<SemIR::FacetType>(self_type_id));
78+
// TODO: We could simplify with a call to ExprAsType, like below?
7879
auto self_facet_as_type = AddTypeInst<SemIR::FacetAccessType>(
7980
context, node_id,
8081
{.type_id = SemIR::TypeType::TypeId,
@@ -86,16 +87,21 @@ auto HandleParseNode(Context& context, Parse::RequireDefaultSelfImplsId node_id)
8687
auto HandleParseNode(Context& context, Parse::RequireTypeImplsId node_id)
8788
-> bool {
8889
auto [self_node_id, self_inst_id] = context.node_stack().PopExprWithNodeId();
90+
auto self_type = ExprAsType(context, self_node_id, self_inst_id);
8991

90-
auto introducer = context.decl_introducer_state_stack().innermost();
92+
const auto& introducer = context.decl_introducer_state_stack().innermost();
9193
if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extend)) {
92-
CARBON_DIAGNOSTIC(RequireImplsExtendWithExplicitSelf, Error,
93-
"`extend require impls` with explicit type");
94-
context.emitter().Emit(self_node_id, RequireImplsExtendWithExplicitSelf);
95-
self_inst_id = SemIR::ErrorInst::InstId;
94+
if (self_type.type_id != SemIR::ErrorInst::TypeId) {
95+
CARBON_DIAGNOSTIC(RequireImplsExtendWithExplicitSelf, Error,
96+
"`extend require impls` with explicit type");
97+
// TODO: If the explicit self-type matches a lookup of NameId::SelfType,
98+
// add a note to the diagnostic: "remove the explicit `Self` type here",
99+
// and continue without an ErrorInst. See ExtendImplSelfAsDefault.
100+
context.emitter().Emit(self_node_id, RequireImplsExtendWithExplicitSelf);
101+
}
102+
self_type.inst_id = SemIR::ErrorInst::TypeInstId;
96103
}
97104

98-
auto self_type = ExprAsType(context, self_node_id, self_inst_id);
99105
context.node_stack().Push(node_id, self_type.inst_id);
100106
return true;
101107
}

‎toolchain/check/impl.cpp‎

Lines changed: 1 addition & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -399,24 +399,10 @@ static auto TryAsClassScope(Context& context, SemIR::NameScopeId scope_id)
399399
return context.insts().TryGetAs<SemIR::ClassDecl>(scope.inst_id());
400400
}
401401

402-
auto GetImplDefaultSelfType(Context& context) -> SemIR::TypeId {
403-
auto parent_scope_id = context.decl_name_stack().PeekParentScopeId();
404-
405-
if (auto class_decl = TryAsClassScope(context, parent_scope_id)) {
406-
return context.classes().Get(class_decl->class_id).self_type_id;
407-
}
408-
409-
// TODO: This is also valid in a mixin.
410-
411-
return SemIR::TypeId::None;
412-
}
413-
414402
// Process an `extend impl` declaration by extending the impl scope with the
415403
// `impl`'s scope.
416404
static auto ExtendImpl(Context& context, Parse::NodeId extend_node,
417405
SemIR::LocId loc_id, SemIR::ImplId impl_id,
418-
Parse::NodeId self_type_node_id,
419-
SemIR::TypeId self_type_id,
420406
SemIR::LocId implicit_params_loc_id,
421407
SemIR::TypeInstId constraint_type_inst_id,
422408
SemIR::TypeId constraint_type_id) -> bool {
@@ -443,31 +429,6 @@ static auto ExtendImpl(Context& context, Parse::NodeId extend_node,
443429

444430
const auto& impl = context.impls().Get(impl_id);
445431

446-
if (context.parse_tree().node_kind(self_type_node_id) ==
447-
Parse::NodeKind::ImplTypeAs) {
448-
CARBON_DIAGNOSTIC(ExtendImplSelfAs, Error,
449-
"cannot `extend` an `impl` with an explicit self type");
450-
auto diag = context.emitter().Build(extend_node, ExtendImplSelfAs);
451-
452-
// If the explicit self type is not the default, just bail out.
453-
if (self_type_id != GetImplDefaultSelfType(context)) {
454-
diag.Emit();
455-
parent_scope.set_has_error();
456-
return false;
457-
}
458-
459-
// The explicit self type is the same as the default self type, so suggest
460-
// removing it and recover as if it were not present.
461-
if (auto self_as =
462-
context.parse_tree_and_subtrees().ExtractAs<Parse::ImplTypeAs>(
463-
self_type_node_id)) {
464-
CARBON_DIAGNOSTIC(ExtendImplSelfAsDefault, Note,
465-
"remove the explicit `Self` type here");
466-
diag.Note(self_as->type_expr, ExtendImplSelfAsDefault);
467-
}
468-
diag.Emit();
469-
}
470-
471432
if (impl.witness_id == SemIR::ErrorInst::InstId) {
472433
parent_scope.set_has_error();
473434
} else {
@@ -618,8 +579,7 @@ auto StartImplDecl(Context& context, SemIR::LocId loc_id,
618579
stored_impl_info.generic_id)});
619580
}
620581
if (!ExtendImpl(context, extend_impl->extend_node_id, loc_id,
621-
impl_decl.impl_id, extend_impl->self_type_node_id,
622-
self_type_id, implicit_params_loc_id, constraint_id,
582+
impl_decl.impl_id, implicit_params_loc_id, constraint_id,
623583
extend_impl->constraint_type_id)) {
624584
// Don't allow the invalid impl to be used.
625585
FillImplWitnessWithErrors(context, stored_impl_info);

‎toolchain/check/impl.h‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,6 @@ auto CheckConstraintIsInterface(Context& context, SemIR::InstId impl_decl_id,
5252
SemIR::TypeInstId constraint_id)
5353
-> SemIR::SpecificInterface;
5454

55-
// Returns the implicit `Self` type for an `impl` when it's in a `class`
56-
// declaration.
57-
auto GetImplDefaultSelfType(Context& context) -> SemIR::TypeId;
58-
5955
// For `StartImplDecl`, additional details for an `extend impl` declaration.
6056
struct ExtendImplDecl {
6157
Parse::NodeId self_type_node_id;

‎toolchain/check/name_scope.cpp‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
2+
// Exceptions. See /LICENSE for license information.
3+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
4+
5+
#include "toolchain/check/name_scope.h"
6+
7+
namespace Carbon::Check {
8+
9+
auto TryAsClassScope(Context& context, SemIR::NameScopeId scope_id)
10+
-> std::optional<ClassScope> {
11+
if (!scope_id.has_value()) {
12+
return std::nullopt;
13+
}
14+
auto& scope = context.name_scopes().Get(scope_id);
15+
if (!scope.inst_id().has_value()) {
16+
return std::nullopt;
17+
}
18+
auto class_decl = context.insts().TryGetAs<SemIR::ClassDecl>(scope.inst_id());
19+
if (!class_decl) {
20+
return std::nullopt;
21+
}
22+
return {{.class_decl = *class_decl, .name_scope = &scope}};
23+
}
24+
25+
} // namespace Carbon::Check

‎toolchain/check/name_scope.h‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
2+
// Exceptions. See /LICENSE for license information.
3+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
4+
5+
#ifndef CARBON_TOOLCHAIN_CHECK_NAME_SCOPE_H_
6+
#define CARBON_TOOLCHAIN_CHECK_NAME_SCOPE_H_
7+
8+
#include <optional>
9+
10+
#include "toolchain/check/context.h"
11+
#include "toolchain/sem_ir/ids.h"
12+
#include "toolchain/sem_ir/typed_insts.h"
13+
14+
namespace Carbon::Check {
15+
16+
struct ClassScope {
17+
SemIR::ClassDecl class_decl;
18+
SemIR::NameScope* name_scope;
19+
};
20+
21+
// If the specified name scope corresponds to a class, returns the corresponding
22+
// class declaration.
23+
auto TryAsClassScope(Context& context, SemIR::NameScopeId scope_id)
24+
-> std::optional<ClassScope>;
25+
26+
} // namespace Carbon::Check
27+
28+
#endif // CARBON_TOOLCHAIN_CHECK_NAME_SCOPE_H_

‎toolchain/check/testdata/facet/require_invalid.carbon‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,3 +123,29 @@ interface Z {
123123
// CHECK:STDERR:
124124
extend require () impls Y;
125125
}
126+
127+
// --- fail_extend_require_with_nonexistant_type.carbon
128+
library "[[@TEST_NAME]]";
129+
130+
interface Y {}
131+
132+
interface Z {
133+
// CHECK:STDERR: fail_extend_require_with_nonexistant_type.carbon:[[@LINE+4]]:18: error: name `nonexistant` not found [NameNotFound]
134+
// CHECK:STDERR: extend require nonexistant impls Y;
135+
// CHECK:STDERR: ^~~~~~~~~~~
136+
// CHECK:STDERR:
137+
extend require nonexistant impls Y;
138+
}
139+
140+
// --- fail_extend_require_with_nonexistant_type_pointer.carbon
141+
library "[[@TEST_NAME]]";
142+
143+
interface Y {}
144+
145+
interface Z {
146+
// CHECK:STDERR: fail_extend_require_with_nonexistant_type_pointer.carbon:[[@LINE+4]]:18: error: name `nonexistant` not found [NameNotFound]
147+
// CHECK:STDERR: extend require nonexistant* impls Y;
148+
// CHECK:STDERR: ^~~~~~~~~~~
149+
// CHECK:STDERR:
150+
extend require nonexistant* impls Y;
151+
}

‎toolchain/check/testdata/impl/extend_impl.carbon‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,20 @@ interface I {}
3939

4040
class C {
4141
// CHECK:STDERR: fail_extend_impl_nonexistent.carbon:[[@LINE+4]]:15: error: name `nonexistent` not found [NameNotFound]
42+
// CHECK:STDERR: extend impl nonexistent as I {}
43+
// CHECK:STDERR: ^~~~~~~~~~~
44+
// CHECK:STDERR:
45+
extend impl nonexistent as I {}
46+
}
47+
48+
// --- fail_extend_impl_nonexistent_pointer.carbon
49+
50+
library "[[@TEST_NAME]]";
51+
52+
interface I {}
53+
54+
class C {
55+
// CHECK:STDERR: fail_extend_impl_nonexistent_pointer.carbon:[[@LINE+4]]:15: error: name `nonexistent` not found [NameNotFound]
4256
// CHECK:STDERR: extend impl nonexistent* as I {}
4357
// CHECK:STDERR: ^~~~~~~~~~~
4458
// CHECK:STDERR:

‎toolchain/check/testdata/impl/fail_extend_impl_type_as.carbon‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,6 @@ class E {
5858
// CHECK:STDOUT: %Int.type: type = generic_class_type @Int [concrete]
5959
// CHECK:STDOUT: %Int.generic: %Int.type = struct_value () [concrete]
6060
// CHECK:STDOUT: %i32: type = class_type @Int, @Int(%int_32) [concrete]
61-
// CHECK:STDOUT: %I.impl_witness.943: <witness> = impl_witness @C.%I.impl_witness_table [concrete]
6261
// CHECK:STDOUT: %empty_struct_type: type = struct_type {} [concrete]
6362
// CHECK:STDOUT: %complete_type.357: <witness> = complete_type_witness %empty_struct_type [concrete]
6463
// CHECK:STDOUT: %D: type = class_type @D [concrete]
@@ -101,7 +100,7 @@ class E {
101100
// CHECK:STDOUT: !requires:
102101
// CHECK:STDOUT: }
103102
// CHECK:STDOUT:
104-
// CHECK:STDOUT: impl @i32.as.I.impl: %i32 as %I.ref {
103+
// CHECK:STDOUT: impl @<error>.as.I.impl: <error> as %I.ref {
105104
// CHECK:STDOUT: !members:
106105
// CHECK:STDOUT: witness = <error>
107106
// CHECK:STDOUT: }
@@ -114,13 +113,11 @@ class E {
114113
// CHECK:STDOUT: }
115114
// CHECK:STDOUT:
116115
// CHECK:STDOUT: class @C {
117-
// CHECK:STDOUT: impl_decl @i32.as.I.impl [concrete] {} {
116+
// CHECK:STDOUT: impl_decl @<error>.as.I.impl [concrete] {} {
118117
// CHECK:STDOUT: %int_32: Core.IntLiteral = int_value 32 [concrete = constants.%int_32]
119118
// CHECK:STDOUT: %i32: type = class_type @Int, @Int(constants.%int_32) [concrete = constants.%i32]
120119
// CHECK:STDOUT: %I.ref: type = name_ref I, file.%I.decl [concrete = constants.%I.type]
121120
// CHECK:STDOUT: }
122-
// CHECK:STDOUT: %I.impl_witness_table = impl_witness_table (), @i32.as.I.impl [concrete]
123-
// CHECK:STDOUT: %I.impl_witness: <witness> = impl_witness %I.impl_witness_table [concrete = constants.%I.impl_witness.943]
124121
// CHECK:STDOUT: %complete_type: <witness> = complete_type_witness constants.%empty_struct_type [concrete = constants.%complete_type.357]
125122
// CHECK:STDOUT: complete_type_witness = %complete_type
126123
// CHECK:STDOUT:

0 commit comments

Comments
 (0)