Skip to content

Commit

Permalink
Syntactic impl declaration matching updates (#4762)
Browse files Browse the repository at this point in the history
* Implement ignoring the difference between `Self as` and `as`, as well
as `where` clauses at the end of an `impl` declaration when checking
whether `impl` declarations match, from #3763.
* Allow impl declarations with different constraint ids to match, as
long as the facet type of the constraint has the same interface_id and
specific_id.
* Add some TODOs reflecting future facet type resolution.

---------

Co-authored-by: Josh L <[email protected]>
  • Loading branch information
josh11b and josh11b authored Jan 7, 2025
1 parent 8cac0c3 commit 1d379ff
Show file tree
Hide file tree
Showing 6 changed files with 321 additions and 22 deletions.
43 changes: 36 additions & 7 deletions toolchain/check/handle_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -212,21 +212,45 @@ static auto PopImplIntroducerAndParamsAsNameComponent(

Parse::NodeId first_param_node_id =
context.node_stack().PopForSoloNodeId<Parse::NodeKind::ImplIntroducer>();

// Subtracting 1 since we don't want to include the final `{` or `;` of the
// declaration when performing syntactic match.
// TODO: Following proposal #3763, we should exclude any `where` clause, and
// add `Self` before `as` if needed, see:
auto end_node_kind = context.parse_tree().node_kind(end_of_decl_node_id);
CARBON_CHECK(end_node_kind == Parse::NodeKind::ImplDefinitionStart ||
end_node_kind == Parse::NodeKind::ImplDecl);
Parse::Tree::PostorderIterator last_param_iter(end_of_decl_node_id);
--last_param_iter;

// Following proposal #3763, exclude a final `where` clause, if present. See:
// https://github.com/carbon-language/carbon-lang/blob/trunk/proposals/p3763.md#redeclarations
auto node_kind = context.parse_tree().node_kind(end_of_decl_node_id);
CARBON_CHECK(node_kind == Parse::NodeKind::ImplDefinitionStart ||
node_kind == Parse::NodeKind::ImplDecl);
Parse::NodeId last_param_node_id(end_of_decl_node_id.index - 1);

// Caches the NodeKind for the current value of *last_param_iter so
if (context.parse_tree().node_kind(*last_param_iter) ==
Parse::NodeKind::WhereExpr) {
int where_operands_to_skip = 1;
--last_param_iter;
CARBON_CHECK(Parse::Tree::PostorderIterator(first_param_node_id) <
last_param_iter);
do {
auto node_kind = context.parse_tree().node_kind(*last_param_iter);
if (node_kind == Parse::NodeKind::WhereExpr) {
// If we have a nested `where`, we need to see another `WhereOperand`
// before we find the one that matches our original `WhereExpr` node.
++where_operands_to_skip;
} else if (node_kind == Parse::NodeKind::WhereOperand) {
--where_operands_to_skip;
}
--last_param_iter;
CARBON_CHECK(Parse::Tree::PostorderIterator(first_param_node_id) <
last_param_iter);
} while (where_operands_to_skip > 0);
}

return {
.name_loc_id = Parse::NodeId::Invalid,
.name_id = SemIR::NameId::Invalid,
.first_param_node_id = first_param_node_id,
.last_param_node_id = last_param_node_id,
.last_param_node_id = *last_param_iter,
.implicit_params_loc_id = implicit_params_loc_id,
.implicit_param_patterns_id =
implicit_param_patterns_id.value_or(SemIR::InstBlockId::Invalid),
Expand Down Expand Up @@ -297,6 +321,11 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id,
// TODO: Check that its constant value is a constraint.
auto [constraint_inst_id, constraint_type_id] =
ExprAsType(context, constraint_node, constraint_id);
// TODO: Do facet type resolution here.
// TODO: Determine `interface_id` and `specific_id` once and save it in the
// resolved facet type, instead of in multiple functions called below.
// TODO: Skip work below if facet type resolution fails, so we don't have a
// valid/non-error `interface_id` at all.

// Process modifiers.
// TODO: Should we somehow permit access specifiers on `impl`s?
Expand Down
53 changes: 42 additions & 11 deletions toolchain/check/merge.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -394,18 +394,38 @@ static auto CheckRedeclParamSyntax(Context& context,
CARBON_CHECK(prev_last_param_node_id.is_valid(),
"prev_last_param_node_id.is_valid should match "
"prev_first_param_node_id.is_valid");

auto new_range = Parse::Tree::PostorderIterator::MakeRange(
new_first_param_node_id, new_last_param_node_id);
auto prev_range = Parse::Tree::PostorderIterator::MakeRange(
prev_first_param_node_id, prev_last_param_node_id);

// zip is using the shortest range. If they differ in length, there should be
// some difference inside the range because the range includes parameter
// brackets. As a consequence, we don't explicitly handle different range
// sizes here.
for (auto [new_node_id, prev_node_id] : llvm::zip(new_range, prev_range)) {
Parse::Tree::PostorderIterator new_iter(new_first_param_node_id);
Parse::Tree::PostorderIterator new_end(new_last_param_node_id);
Parse::Tree::PostorderIterator prev_iter(prev_first_param_node_id);
Parse::Tree::PostorderIterator prev_end(prev_last_param_node_id);
// Done when one past the last node to check.
++new_end;
++prev_end;

// Compare up to the shortest length.
for (; new_iter != new_end && prev_iter != prev_end;
++new_iter, ++prev_iter) {
auto new_node_id = *new_iter;
auto prev_node_id = *prev_iter;
if (!IsNodeSyntaxEqual(context, new_node_id, prev_node_id)) {
// Skip difference if it is `Self as` vs. `as` in an `impl` declaration.
// https://github.com/carbon-language/carbon-lang/blob/trunk/proposals/p3763.md#redeclarations
auto new_node_kind = context.parse_tree().node_kind(new_node_id);
auto prev_node_kind = context.parse_tree().node_kind(prev_node_id);
if (new_node_kind == Parse::NodeKind::DefaultSelfImplAs &&
prev_node_kind == Parse::NodeKind::SelfTypeNameExpr &&
context.parse_tree().node_kind(prev_iter[1]) ==
Parse::NodeKind::TypeImplAs) {
++prev_iter;
continue;
}
if (prev_node_kind == Parse::NodeKind::DefaultSelfImplAs &&
new_node_kind == Parse::NodeKind::SelfTypeNameExpr &&
context.parse_tree().node_kind(new_iter[1]) ==
Parse::NodeKind::TypeImplAs) {
++new_iter;
continue;
}
if (!diagnose) {
return false;
}
Expand All @@ -420,6 +440,17 @@ static auto CheckRedeclParamSyntax(Context& context,
return false;
}
}
// The prefixes are the same, but the lengths may still be different. This is
// only relevant for `impl` declarations where the final bracketing node is
// not included in the range of nodes being compared, and in those cases
// `diagnose` is false.
if (new_iter != new_end) {
CARBON_CHECK(!diagnose);
return false;
} else if (prev_iter != prev_end) {
CARBON_CHECK(!diagnose);
return false;
}

return true;
}
Expand Down
180 changes: 180 additions & 0 deletions toolchain/check/testdata/impl/no_prelude/impl_self_as.carbon
Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@
// 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
//
// EXTRA-ARGS: --no-dump-sem-ir
//
// AUTOUPDATE
// TIP: To test this file alone, run:
// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/impl/no_prelude/impl_self_as.carbon
// TIP: To dump output, run:
// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/no_prelude/impl_self_as.carbon

// --- match.carbon
library "[[@TEST_NAME]]";

interface I1 {}
interface I2 {}
interface J1(T1:! type) {}
interface J2(T2:! type) {}

// `impl Self as` should match `impl as`, so these should not trigger impl
// declaration without definition diagnostics.

class C1 {
impl Self as I1;
impl as I1 {}

impl as I2;
impl Self as I2 {}

impl forall [U:! type] Self as J1(U);
impl forall [U:! type] as J1(U) {}

impl forall [V:! type] as J2(V);
impl forall [V:! type] Self as J2(V) {}
}

class C2(W:! type) {
impl Self as I1;
impl as I1 {}

impl as I2;
impl Self as I2 {}

impl forall [X:! type] Self as J1(X);
impl forall [X:! type] as J1(X) {}

impl forall [Y:! type] as J2(Y);
impl forall [Y:! type] Self as J2(Y) {}
}


// --- fail_no_match.carbon
library "[[@TEST_NAME]]";

interface I3 {}
interface I4 {}
interface I5 {}
interface I6 {}
interface J3(T3:! type) {}
interface J4(T4:! type) {}
interface J5(T5:! type) {}
interface J6(T6:! type) {}

// `impl C as` should not match `impl Self as` or `impl as`.

class C3 {
// CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
// CHECK:STDERR: impl C3 as I3;
// CHECK:STDERR: ^~~~~~~~~~~~~~
// CHECK:STDERR:
impl C3 as I3;
impl as I3 {}

// CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
// CHECK:STDERR: impl C3 as I4;
// CHECK:STDERR: ^~~~~~~~~~~~~~
// CHECK:STDERR:
impl C3 as I4;
impl Self as I4 {}

// CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
// CHECK:STDERR: impl as I5;
// CHECK:STDERR: ^~~~~~~~~~~
// CHECK:STDERR:
impl as I5;
impl C3 as I5 {}

// CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
// CHECK:STDERR: impl Self as I6;
// CHECK:STDERR: ^~~~~~~~~~~~~~~~
// CHECK:STDERR:
impl Self as I6;
impl C3 as I6 {}

// CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
// CHECK:STDERR: impl forall [Z3:! type] C3 as J3(Z3);
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// CHECK:STDERR:
impl forall [Z3:! type] C3 as J3(Z3);
impl forall [Z3:! type] as J3(Z3) {}

// CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
// CHECK:STDERR: impl forall [Z4:! type] C3 as J4(Z4);
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// CHECK:STDERR:
impl forall [Z4:! type] C3 as J4(Z4);
impl forall [Z4:! type] Self as J4(Z4) {}

// CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
// CHECK:STDERR: impl forall [Z5:! type] as J5(Z5);
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// CHECK:STDERR:
impl forall [Z5:! type] as J5(Z5);
impl forall [Z5:! type] C3 as J5(Z5) {}

// CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
// CHECK:STDERR: impl forall [Z6:! type] Self as J6(Z6);
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// CHECK:STDERR:
impl forall [Z6:! type] Self as J6(Z6);
impl forall [Z6:! type] C3 as J6(Z6) {}
}

class C4(A:! type) {
// CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
// CHECK:STDERR: impl C4(A) as I3;
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~
// CHECK:STDERR:
impl C4(A) as I3;
impl as I3 {}

// CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
// CHECK:STDERR: impl C4(A) as I4;
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~
// CHECK:STDERR:
impl C4(A) as I4;
impl Self as I4 {}

// CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
// CHECK:STDERR: impl as I5;
// CHECK:STDERR: ^~~~~~~~~~~
// CHECK:STDERR:
impl as I5;
impl C4(A) as I5 {}

// CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
// CHECK:STDERR: impl Self as I6;
// CHECK:STDERR: ^~~~~~~~~~~~~~~~
// CHECK:STDERR:
impl Self as I6;
impl C4(A) as I6 {}

// CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
// CHECK:STDERR: impl forall [B3:! type] C4(A) as J3(B3);
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// CHECK:STDERR:
impl forall [B3:! type] C4(A) as J3(B3);
impl forall [B3:! type] as J3(B3) {}

// CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
// CHECK:STDERR: impl forall [B4:! type] C4(A) as J4(B4);
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// CHECK:STDERR:
impl forall [B4:! type] C4(A) as J4(B4);
impl forall [B4:! type] Self as J4(B4) {}

// CHECK:STDERR: fail_no_match.carbon:[[@LINE+4]]:3: error: impl declared but not defined [MissingImplDefinition]
// CHECK:STDERR: impl forall [B5:! type] as J5(B5);
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// CHECK:STDERR:
impl forall [B5:! type] as J5(B5);
impl forall [B5:! type] C4(A) as J5(B5) {}

// CHECK:STDERR: fail_no_match.carbon:[[@LINE+3]]:3: error: impl declared but not defined [MissingImplDefinition]
// CHECK:STDERR: impl forall [B6:! type] Self as J6(B6);
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
impl forall [B6:! type] Self as J6(B6);
impl forall [B6:! type] C4(A) as J6(B6) {}
}
47 changes: 47 additions & 0 deletions toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// 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
//
// EXTRA-ARGS: --no-dump-sem-ir
//
// AUTOUPDATE
// TIP: To test this file alone, run:
// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon
// TIP: To dump output, run:
// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon

// --- match.carbon
library "[[@TEST_NAME]]";

interface J {}

// `impl` matching ignores the `where` clause. It should not get confused by
// nested `where`, so the following should not trigger impl declaration without
// definition diagnostics.

impl () as J;
impl () as J where .Self impls type and .Self impls (type where .Self impls type) {}

impl {} as J where .Self impls type and .Self impls (type where .Self impls type);
impl {} as J {}

// --- parens_other_nesting.carbon
library "[[@TEST_NAME]]";

interface K {}

// `impl` matching only ignores a root-level `where` clause.

impl {} as (K where .Self impls type) where .Self impls type;
impl {} as (K where .Self impls type) {}

// --- fail_other_nesting.carbon
library "[[@TEST_NAME]]";

interface L {}

// CHECK:STDERR: fail_other_nesting.carbon:[[@LINE+3]]:1: error: impl declared but not defined [MissingImplDefinition]
// CHECK:STDERR: impl () as (L where .Self impls type) where .Self impls type;
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
impl () as (L where .Self impls type) where .Self impls type;
impl () as L {}
17 changes: 14 additions & 3 deletions toolchain/sem_ir/impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,22 @@ namespace Carbon::SemIR {

auto ImplStore::GetOrAddLookupBucket(const Impl& impl) -> LookupBucketRef {
auto self_id = sem_ir_.constant_values().GetConstantInstId(impl.self_id);
auto constraint_id =
sem_ir_.constant_values().GetConstantInstId(impl.constraint_id);
InterfaceId interface_id = InterfaceId::Invalid;
SpecificId specific_id = SpecificId::Invalid;
auto facet_type_id = TypeId::ForTypeConstant(
sem_ir_.constant_values().Get(impl.constraint_id));
if (auto facet_type =
sem_ir_.types().TryGetAs<SemIR::FacetType>(facet_type_id)) {
const SemIR::FacetTypeInfo& facet_type_info =
sem_ir_.facet_types().Get(facet_type->facet_type_id);
if (auto interface_type = facet_type_info.TryAsSingleInterface()) {
interface_id = interface_type->interface_id;
specific_id = interface_type->specific_id;
}
}
return LookupBucketRef(
*this, lookup_
.Insert(std::pair{self_id, constraint_id},
.Insert(std::tuple{self_id, interface_id, specific_id},
[] { return ImplOrLookupBucketId::Invalid; })
.value());
}
Expand Down
3 changes: 2 additions & 1 deletion toolchain/sem_ir/impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,8 @@ class ImplStore {
private:
File& sem_ir_;
ValueStore<ImplId> values_;
Map<std::pair<InstId, InstId>, ImplOrLookupBucketId> lookup_;
Map<std::tuple<InstId, InterfaceId, SpecificId>, ImplOrLookupBucketId>
lookup_;
// Buckets with at least 2 entries, which will be rare; see LookupBucketRef.
llvm::SmallVector<llvm::SmallVector<ImplId, 2>> lookup_buckets_;
};
Expand Down

0 comments on commit 1d379ff

Please sign in to comment.