diff --git a/toolchain/check/handle_impl.cpp b/toolchain/check/handle_impl.cpp index 808aa89d44d4..30367ceaa7e0 100644 --- a/toolchain/check/handle_impl.cpp +++ b/toolchain/check/handle_impl.cpp @@ -212,21 +212,45 @@ static auto PopImplIntroducerAndParamsAsNameComponent( Parse::NodeId first_param_node_id = context.node_stack().PopForSoloNodeId(); + // 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), @@ -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? diff --git a/toolchain/check/merge.cpp b/toolchain/check/merge.cpp index b19eb0cc87dd..20340a93cb07 100644 --- a/toolchain/check/merge.cpp +++ b/toolchain/check/merge.cpp @@ -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"); + 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; - 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)) { + // 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; } @@ -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; } diff --git a/toolchain/check/testdata/impl/no_prelude/impl_self_as.carbon b/toolchain/check/testdata/impl/no_prelude/impl_self_as.carbon new file mode 100644 index 000000000000..cdb16c10026b --- /dev/null +++ b/toolchain/check/testdata/impl/no_prelude/impl_self_as.carbon @@ -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) {} +} diff --git a/toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon b/toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon new file mode 100644 index 000000000000..c59c393f6471 --- /dev/null +++ b/toolchain/check/testdata/impl/no_prelude/impl_where_redecl.carbon @@ -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 {} diff --git a/toolchain/sem_ir/impl.cpp b/toolchain/sem_ir/impl.cpp index 9bb5cb0b4763..89d369cef7b8 100644 --- a/toolchain/sem_ir/impl.cpp +++ b/toolchain/sem_ir/impl.cpp @@ -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(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()); } diff --git a/toolchain/sem_ir/impl.h b/toolchain/sem_ir/impl.h index 3dbd2973d5dd..a7769477e7e9 100644 --- a/toolchain/sem_ir/impl.h +++ b/toolchain/sem_ir/impl.h @@ -189,7 +189,8 @@ class ImplStore { private: File& sem_ir_; ValueStore values_; - Map, ImplOrLookupBucketId> lookup_; + Map, ImplOrLookupBucketId> + lookup_; // Buckets with at least 2 entries, which will be rare; see LookupBucketRef. llvm::SmallVector> lookup_buckets_; };