From 0e41c569b11cb91b33d68105ec5e363af4b5a03c Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Tue, 14 Feb 2023 17:44:16 -0800 Subject: [PATCH] Implement the termination algorithm for impl selection described in #2458 (#2602) Detect when evaluating an impl recursively tries to evaluate the same impl for the same or a more complex set of parameters. In order to perform the check after we have tested that the type structure matches and before we check that constraints are recursively satisfied, argument deduction is extended to check structural matching properties earlier. This requires us to separate match failures into two kinds: hard failures that produce errors that should never be swallowed, and soft failures such as a missing impl that lead us to merely discard an impl as a candidate. A flag has been added to `ImplScope` and `ArgumentDeduction` to specify whether soft failures should produce an error message or not. --- explorer/interpreter/BUILD | 2 + explorer/interpreter/impl_scope.cpp | 131 +++++++----- explorer/interpreter/impl_scope.h | 43 ++-- explorer/interpreter/matching_impl_set.cpp | 168 +++++++++++++++ explorer/interpreter/matching_impl_set.h | 129 ++++++++++++ explorer/interpreter/type_checker.cpp | 196 +++++++++++------- explorer/interpreter/type_checker.h | 9 +- .../fail_match_in_deduction.carbon | 2 +- .../fail_value_param_mismatch.carbon | 2 +- .../testdata/impl_match/build_tree.carbon | 35 ++++ .../impl_match/fail_self_recurse.carbon | 23 ++ .../fail_self_recurse_with_match.carbon | 24 +++ .../fail_self_recurse_with_match_first.carbon | 27 +++ .../fail_simple_self_recurse.carbon | 21 ++ .../impl_match/fail_three_step_recurse.carbon | 27 +++ .../mixin/fail_self_substitution.carbon | 2 +- 16 files changed, 698 insertions(+), 143 deletions(-) create mode 100644 explorer/interpreter/matching_impl_set.cpp create mode 100644 explorer/interpreter/matching_impl_set.h create mode 100644 explorer/testdata/impl_match/build_tree.carbon create mode 100644 explorer/testdata/impl_match/fail_self_recurse.carbon create mode 100644 explorer/testdata/impl_match/fail_self_recurse_with_match.carbon create mode 100644 explorer/testdata/impl_match/fail_self_recurse_with_match_first.carbon create mode 100644 explorer/testdata/impl_match/fail_simple_self_recurse.carbon create mode 100644 explorer/testdata/impl_match/fail_three_step_recurse.carbon diff --git a/explorer/interpreter/BUILD b/explorer/interpreter/BUILD index 41897e7b11f2..acdfa10046ec 100644 --- a/explorer/interpreter/BUILD +++ b/explorer/interpreter/BUILD @@ -208,11 +208,13 @@ cc_library( srcs = [ "builtins.cpp", "impl_scope.cpp", + "matching_impl_set.cpp", "type_checker.cpp", ], hdrs = [ "builtins.h", "impl_scope.h", + "matching_impl_set.h", "type_checker.h", ], deps = [ diff --git a/explorer/interpreter/impl_scope.cpp b/explorer/interpreter/impl_scope.cpp index c3f8c6b114a4..6a215d239ab2 100644 --- a/explorer/interpreter/impl_scope.cpp +++ b/explorer/interpreter/impl_scope.cpp @@ -65,18 +65,17 @@ void ImplScope::AddParent(Nonnull parent) { parent_scopes_.push_back(parent); } -// Checks that `a_evaluated == b_evaluated` for the purpose of an equality -// constraint. Produces an error if not. -static auto CheckEqualOrDiagnose(SourceLocation source_loc, - Nonnull a_written, - Nonnull a_evaluated, - Nonnull b_written, - Nonnull b_evaluated, - Nonnull equality_ctx) - -> ErrorOr { - if (ValueEqual(a_evaluated, b_evaluated, equality_ctx)) { - return Success(); - } +// Diagnose that `a_evaluated != b_evaluated` for the purpose of an equality +// constraint. +static auto DiagnoseUnequalValues(SourceLocation source_loc, + Nonnull a_written, + Nonnull a_evaluated, + Nonnull b_written, + Nonnull b_evaluated, + Nonnull equality_ctx) + -> Error { + CARBON_CHECK(!ValueEqual(a_evaluated, b_evaluated, equality_ctx)) + << "expected unequal values"; auto error = ProgramError(source_loc); error << "constraint requires that " << *a_written; if (!ValueEqual(a_written, a_evaluated, std::nullopt)) { @@ -96,10 +95,26 @@ auto ImplScope::Resolve(Nonnull constraint_type, const TypeChecker& type_checker, const Bindings& bindings) const -> ErrorOr> { + CARBON_ASSIGN_OR_RETURN( + std::optional> witness, + TryResolve(constraint_type, impl_type, source_loc, type_checker, bindings, + /*diagnose_missing_impl=*/true)); + CARBON_CHECK(witness) << "should have diagnosed missing impl"; + return *witness; +} + +auto ImplScope::TryResolve(Nonnull constraint_type, + Nonnull impl_type, + SourceLocation source_loc, + const TypeChecker& type_checker, + const Bindings& bindings, + bool diagnose_missing_impl) const + -> ErrorOr>> { if (const auto* iface_type = dyn_cast(constraint_type)) { iface_type = cast(type_checker.Substitute(bindings, iface_type)); - return ResolveInterface(iface_type, impl_type, source_loc, type_checker); + return TryResolveInterface(iface_type, impl_type, source_loc, type_checker, + diagnose_missing_impl); } if (const auto* constraint = dyn_cast(constraint_type)) { std::vector> witnesses; @@ -120,12 +135,16 @@ auto ImplScope::Resolve(Nonnull constraint_type, Bindings local_bindings = bindings; local_bindings.Add(constraint->self_binding(), impl_type, witness); CARBON_ASSIGN_OR_RETURN( - Nonnull result, - ResolveInterface(cast(type_checker.Substitute( - local_bindings, impl.interface)), - type_checker.Substitute(local_bindings, impl.type), - source_loc, type_checker)); - witnesses.push_back(result); + std::optional> result, + TryResolveInterface( + cast( + type_checker.Substitute(local_bindings, impl.interface)), + type_checker.Substitute(local_bindings, impl.type), source_loc, + type_checker, diagnose_missing_impl)); + if (!result) { + return {std::nullopt}; + } + witnesses.push_back(*result); } // Check that all intrinsic, equality, and rewrite constraints @@ -155,6 +174,9 @@ auto ImplScope::Resolve(Nonnull constraint_type, type_checker.Substitute(local_bindings, argument)); } if (!type_checker.IsIntrinsicConstraintSatisfied(converted, *this)) { + if (!diagnose_missing_impl) { + return {std::nullopt}; + } return ProgramError(source_loc) << "constraint requires that " << converted; } @@ -166,9 +188,13 @@ auto ImplScope::Resolve(Nonnull constraint_type, for (; it != equal.values.end(); ++it) { Nonnull current = type_checker.Substitute(local_bindings, *it); - CARBON_RETURN_IF_ERROR( - CheckEqualOrDiagnose(source_loc, equal.values.front(), first, *it, - current, &equality_ctx)); + if (!ValueEqual(first, current, &equality_ctx)) { + if (!diagnose_missing_impl) { + return {std::nullopt}; + } + return DiagnoseUnequalValues(source_loc, equal.values.front(), + first, *it, current, &equality_ctx); + } } } for (const auto& rewrite : rewrites) { @@ -176,12 +202,17 @@ auto ImplScope::Resolve(Nonnull constraint_type, type_checker.Substitute(local_bindings, rewrite.constant); Nonnull value = type_checker.Substitute( local_bindings, rewrite.converted_replacement); - CARBON_RETURN_IF_ERROR(CheckEqualOrDiagnose( - source_loc, rewrite.constant, constant, - rewrite.converted_replacement, value, &equality_ctx)); + if (!ValueEqual(constant, value, &equality_ctx)) { + if (!diagnose_missing_impl) { + return {std::nullopt}; + } + return DiagnoseUnequalValues(source_loc, rewrite.constant, constant, + rewrite.converted_replacement, value, + &equality_ctx); + } } } - return type_checker.MakeConstraintWitness(std::move(witnesses)); + return {type_checker.MakeConstraintWitness(std::move(witnesses))}; } CARBON_FATAL() << "expected a constraint, not " << *constraint_type; } @@ -202,19 +233,21 @@ auto ImplScope::VisitEqualValues( return true; } -auto ImplScope::ResolveInterface(Nonnull iface_type, - Nonnull type, - SourceLocation source_loc, - const TypeChecker& type_checker) const - -> ErrorOr> { +auto ImplScope::TryResolveInterface(Nonnull iface_type, + Nonnull type, + SourceLocation source_loc, + const TypeChecker& type_checker, + bool diagnose_missing_impl) const + -> ErrorOr>> { CARBON_ASSIGN_OR_RETURN( std::optional> result, - TryResolve(iface_type, type, source_loc, *this, type_checker)); - if (!result.has_value()) { + TryResolveInterfaceRecursively(iface_type, type, source_loc, *this, + type_checker)); + if (!result.has_value() && diagnose_missing_impl) { return ProgramError(source_loc) << "could not find implementation of " << *iface_type << " for " << *type; } - return *result; + return result; } // Combines the results of two impl lookups. In the event of a tie, arbitrarily @@ -273,36 +306,36 @@ static auto CombineResults(Nonnull iface_type, << "ambiguous implementations of " << *iface_type << " for " << *type; } -auto ImplScope::TryResolve(Nonnull iface_type, - Nonnull type, - SourceLocation source_loc, - const ImplScope& original_scope, - const TypeChecker& type_checker) const +auto ImplScope::TryResolveInterfaceRecursively( + Nonnull iface_type, Nonnull type, + SourceLocation source_loc, const ImplScope& original_scope, + const TypeChecker& type_checker) const -> ErrorOr>> { CARBON_ASSIGN_OR_RETURN( std::optional> result, - ResolveHere(iface_type, type, source_loc, original_scope, type_checker)); + TryResolveInterfaceHere(iface_type, type, source_loc, original_scope, + type_checker)); for (Nonnull parent : parent_scopes_) { CARBON_ASSIGN_OR_RETURN( std::optional> parent_result, - parent->TryResolve(iface_type, type, source_loc, original_scope, - type_checker)); + parent->TryResolveInterfaceRecursively(iface_type, type, source_loc, + original_scope, type_checker)); CARBON_ASSIGN_OR_RETURN(result, CombineResults(iface_type, type, source_loc, result, parent_result)); } return result; } -auto ImplScope::ResolveHere(Nonnull iface_type, - Nonnull impl_type, - SourceLocation source_loc, - const ImplScope& original_scope, - const TypeChecker& type_checker) const +auto ImplScope::TryResolveInterfaceHere( + Nonnull iface_type, Nonnull impl_type, + SourceLocation source_loc, const ImplScope& original_scope, + const TypeChecker& type_checker) const -> ErrorOr>> { std::optional> result = std::nullopt; for (const Impl& impl : impls_) { - std::optional> m = type_checker.MatchImpl( - *iface_type, impl_type, impl, original_scope, source_loc); + CARBON_ASSIGN_OR_RETURN(std::optional> m, + type_checker.MatchImpl(*iface_type, impl_type, impl, + original_scope, source_loc)); CARBON_ASSIGN_OR_RETURN( result, CombineResults(iface_type, impl_type, source_loc, result, m)); } diff --git a/explorer/interpreter/impl_scope.h b/explorer/interpreter/impl_scope.h index 5e1a425df490..f615e53a5c80 100644 --- a/explorer/interpreter/impl_scope.h +++ b/explorer/interpreter/impl_scope.h @@ -87,6 +87,16 @@ class ImplScope { const Bindings& bindings = {}) const -> ErrorOr>; + // Same as Resolve, except that failure due to a missing implementation of a + // constraint produces `nullopt` instead of an error if + // `diagnose_missing_impl` is `false`. This is intended for cases where we're + // selecting between options based on whether constraints are satisfied, such + // as during `impl` selection. + auto TryResolve(Nonnull constraint, Nonnull type, + SourceLocation source_loc, const TypeChecker& type_checker, + const Bindings& bindings, bool diagnose_missing_impl) const + -> ErrorOr>>; + // Visits the values that are a single step away from `value` according to an // equality constraint that is in scope. That is, the values `v` such that we // have a `value == v` equality constraint in scope. @@ -117,12 +127,15 @@ class ImplScope { private: // Returns the associated impl for the given `iface` and `type` in - // the ancestor graph of this scope, or reports a compilation error - // at `source_loc` there isn't exactly one matching impl. - auto ResolveInterface(Nonnull iface, - Nonnull type, SourceLocation source_loc, - const TypeChecker& type_checker) const - -> ErrorOr>; + // the ancestor graph of this scope. Reports a compilation error + // at `source_loc` if there's an ambiguity, or if `diagnose_missing_impl` is + // set and there's no matching impl. + auto TryResolveInterface(Nonnull iface, + Nonnull type, + SourceLocation source_loc, + const TypeChecker& type_checker, + bool diagnose_missing_impl) const + -> ErrorOr>>; // Returns the associated impl for the given `iface` and `type` in // the ancestor graph of this scope, returns std::nullopt if there @@ -130,10 +143,11 @@ class ImplScope { // specific impl for the given `iface` and `type`. // Use `original_scope` to satisfy requirements of any generic impl // that matches `iface` and `type`. - auto TryResolve(Nonnull iface_type, - Nonnull type, SourceLocation source_loc, - const ImplScope& original_scope, - const TypeChecker& type_checker) const + auto TryResolveInterfaceRecursively(Nonnull iface_type, + Nonnull type, + SourceLocation source_loc, + const ImplScope& original_scope, + const TypeChecker& type_checker) const -> ErrorOr>>; // Returns the associated impl for the given `iface` and `type` in @@ -142,10 +156,11 @@ class ImplScope { // given `iface` and `type`. // Use `original_scope` to satisfy requirements of any generic impl // that matches `iface` and `type`. - auto ResolveHere(Nonnull iface_type, - Nonnull impl_type, SourceLocation source_loc, - const ImplScope& original_scope, - const TypeChecker& type_checker) const + auto TryResolveInterfaceHere(Nonnull iface_type, + Nonnull impl_type, + SourceLocation source_loc, + const ImplScope& original_scope, + const TypeChecker& type_checker) const -> ErrorOr>>; std::vector impls_; diff --git a/explorer/interpreter/matching_impl_set.cpp b/explorer/interpreter/matching_impl_set.cpp new file mode 100644 index 000000000000..8a92106de356 --- /dev/null +++ b/explorer/interpreter/matching_impl_set.cpp @@ -0,0 +1,168 @@ +// 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 "common/check.h" +#include "common/error.h" +#include "explorer/ast/declaration.h" +#include "explorer/common/error_builders.h" +#include "explorer/common/nonnull.h" +#include "explorer/common/source_location.h" +#include "explorer/interpreter/type_checker.h" +#include "explorer/interpreter/value.h" + +namespace Carbon { + +// A visitor for type values that collects leaf 'labels', such as class names, +// and adds them to the signature of a `Match` object. +class MatchingImplSet::LeafCollector { + public: + LeafCollector(Match* match) : match_(match) {} + + void Collect(const Value* value) { + value->Visit( + [&](const auto* derived_value) { VisitValue(derived_value); }); + } + + void Collect(Label label) { ++match_->signature_[label]; } + + private: + // Most kinds of value don't contribute to the signature. + void VisitValue(const Value*) {} + + void VisitValue(const TupleType* tuple) { + for (auto* elem : tuple->elements()) { + Collect(elem); + } + } + + void VisitValue(const TypeType*) { Collect(Label::TypeType); } + + void VisitValue(const BoolType*) { Collect(Label::BoolType); } + + void VisitValue(const IntType*) { Collect(Label::IntType); } + + void VisitValue(const StringType*) { Collect(Label::StringType); } + + void VisitValue(const StaticArrayType* array) { + Collect(Label::ArrayType); + Collect(&array->element_type()); + } + + void VisitValue(const PointerType* pointer) { + Collect(Label::PointerType); + Collect(&pointer->pointee_type()); + } + + void VisitValue(const StructType* struct_type) { + for (auto [name, type] : struct_type->fields()) { + Collect(type); + } + } + + void VisitValue(const NominalClassType* class_type) { + VisitDeclarationAndArgs(class_type->declaration(), class_type->bindings()); + } + + void VisitValue(const MixinPseudoType* mixin_type) { + VisitDeclarationAndArgs(mixin_type->declaration(), mixin_type->bindings()); + } + + void VisitValue(const InterfaceType* iface_type) { + VisitDeclarationAndArgs(iface_type->declaration(), iface_type->bindings()); + } + + void VisitValue(const NamedConstraintType* constraint_type) { + VisitDeclarationAndArgs(constraint_type->declaration(), + constraint_type->bindings()); + } + + void VisitValue(const ChoiceType* choice_type) { + VisitDeclarationAndArgs(choice_type->declaration(), + choice_type->bindings()); + } + + void VisitDeclarationAndArgs(const Declaration& declaration, + const Bindings& bindings) { + Collect(match_->parent_->GetLabelForDeclaration(declaration)); + for (auto [key, value] : bindings.args()) { + Collect(value); + } + } + + private: + Match* match_; +}; + +auto MatchingImplSet::GetLabelForDeclaration(const Declaration& declaration) + -> Label { + auto [it, added] = declaration_labels_.insert( + {&declaration, + Label(int(Label::FirstDeclarationLabel) + declaration_labels_.size())}); + return it->second; +} + +MatchingImplSet::Match::Match(Nonnull parent, + Nonnull impl, + Nonnull type, + Nonnull interface) + : parent_(parent), impl_(impl), type_(type), interface_(interface) { + // Build our signature. + LeafCollector collector(this); + collector.Collect(type); + collector.Collect(interface); + + parent_->matches_.push_back(this); +} + +MatchingImplSet::Match::~Match() { + CARBON_CHECK(parent_->matches_.back() == this) << "match stack broken"; + parent_->matches_.pop_back(); +} + +auto MatchingImplSet::Match::DiagnosePotentialCycle(SourceLocation source_loc) + -> ErrorOr { + for (auto* match : parent_->matches_) { + if (match != this && match->impl_ == impl_) { + // Whether all labels appear a greater or equal number of times in this + // match than in `match`. + bool all_greater_or_equal = true; + // Whether any label appears strictly more times in this match than in + // `match`. + bool any_greater = false; + + for (auto [key, value] : signature_) { + int other_value = match->signature_.lookup(key); + if (value < other_value) { + all_greater_or_equal = false; + break; + } + if (value > other_value) { + any_greater = true; + } + } + + if (all_greater_or_equal) { + if (any_greater) { + return ProgramError(source_loc) + << "impl matching recursively performed a more complex match " + "using the same impl\n" + << " outer match: " << *match->type_ << " as " + << *match->interface_ << "\n" + << " inner match: " << *type_ << " as " << *interface_; + } + if (ValueEqual(match->type_, type_, std::nullopt) && + ValueEqual(match->interface_, interface_, std::nullopt)) { + return ProgramError(source_loc) + << "impl matching for " << *type_ << " as " << *interface_ + << " recursively performed a match for the same type and " + "interface"; + } + } + } + } + + return Success(); +} + +} // namespace Carbon diff --git a/explorer/interpreter/matching_impl_set.h b/explorer/interpreter/matching_impl_set.h new file mode 100644 index 000000000000..b79d2268f059 --- /dev/null +++ b/explorer/interpreter/matching_impl_set.h @@ -0,0 +1,129 @@ +// 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_EXPLORER_INTERPRETER_MATCHING_IMPL_SET_H_ +#define CARBON_EXPLORER_INTERPRETER_MATCHING_IMPL_SET_H_ + +#include + +#include "common/ostream.h" +#include "explorer/ast/declaration.h" +#include "explorer/common/nonnull.h" +#include "explorer/interpreter/impl_scope.h" +#include "explorer/interpreter/value.h" +#include "llvm/ADT/DenseMap.h" + +namespace Carbon { + +// A set of impl matches that we're currently performing. Each `Match` +// represents an attempt to match a `type as interface` query against an `impl` +// declaration. This is used to detect and reject non-termination when impl +// matching recursively triggers further impl matching. +// +// The language rule we use to detect potential non-termination is to count the +// number of times each "label" appears within the type and interface, where a +// label is the name of a declared entity such as a class or interface, or a +// primitive like `type` or `bool`. For example, `Optional(i32*)` contains the +// labels for `Optional`, `i32`, and `*` (built-in pointer type) once each. If +// we ever try matching the same `impl` twice, where the inner match contains +// at least as many appearances of each label as the outer match, we reject the +// program as invalid. We also reject if a query results in the exact same +// query being performed again. +// +// This class is an implementation detail of `TypeChecker::MatchImpl`. +class MatchingImplSet { + private: + class LeafCollector; + enum class Label : int; + + public: + // An RAII type that tracks an impl match that we're currently performing. + // One instance of this class will exist for each in-progress call to + // `MatchImpl`. + class Match { + public: + explicit Match(Nonnull parent, + Nonnull impl, + Nonnull type, Nonnull interface); + ~Match(); + + Match(const Match&) = delete; + Match& operator=(const Match&) = delete; + + // Check to see if this match duplicates any prior one within the same set, + // or if there's a simpler form of this match in the set. If so, returns a + // suitable error. This should be delayed until we know that the impl + // structurally matches the type and interface. + auto DiagnosePotentialCycle(SourceLocation source_loc) -> ErrorOr; + + private: + friend class LeafCollector; + + // The set that this match is part of. + Nonnull parent_; + // The `impl` that is being matched against. + Nonnull impl_; + // The type that is being matched against the impl. + Nonnull type_; + // The interface that is being matched against the impl. + Nonnull interface_; + // The number of times each label appears in the type or interface. + llvm::DenseMap signature_; + }; + + private: + friend class llvm::DenseMapInfo