From a5c8cbdbf8553fc7eb04e642f634e326f873d0d6 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Wed, 28 Jun 2023 14:56:27 -0700 Subject: [PATCH] Require a complete type when enumerating the fields of a class. (#2956) Fixes a crash on invalid found by fuzzing. --- explorer/interpreter/impl_scope.cpp | 6 +- explorer/interpreter/type_checker.cpp | 87 +++++++++++-------- explorer/interpreter/type_checker.h | 24 +++-- .../fail_incomplete_in_conversion.carbon | 15 ++++ 4 files changed, 80 insertions(+), 52 deletions(-) create mode 100644 explorer/testdata/class/fail_incomplete_in_conversion.carbon diff --git a/explorer/interpreter/impl_scope.cpp b/explorer/interpreter/impl_scope.cpp index 6654a001858f..d6e29f71adab 100644 --- a/explorer/interpreter/impl_scope.cpp +++ b/explorer/interpreter/impl_scope.cpp @@ -192,9 +192,9 @@ auto ImplScope::TryResolve(Nonnull constraint_type, type_checker.Substitute(local_bindings, argument)); converted.arguments.push_back(subst_arg); } - CARBON_ASSIGN_OR_RETURN( - bool intrinsic_satisfied, - type_checker.IsIntrinsicConstraintSatisfied(converted, *this)); + CARBON_ASSIGN_OR_RETURN(bool intrinsic_satisfied, + type_checker.IsIntrinsicConstraintSatisfied( + source_loc, converted, *this)); if (!intrinsic_satisfied) { if (!diagnose_missing_impl) { return {std::nullopt}; diff --git a/explorer/interpreter/type_checker.cpp b/explorer/interpreter/type_checker.cpp index ea2e95832927..7d3ee113fb0c 100644 --- a/explorer/interpreter/type_checker.cpp +++ b/explorer/interpreter/type_checker.cpp @@ -507,8 +507,12 @@ static auto FindField(llvm::ArrayRef fields, return *it; } -auto TypeChecker::FieldTypes(const NominalClassType& class_type) const +auto TypeChecker::FieldTypes(SourceLocation source_loc, + std::string_view context, + const NominalClassType& class_type) const -> ErrorOr> { + CARBON_RETURN_IF_ERROR(ExpectCompleteType(source_loc, context, &class_type)); + std::vector field_types; for (Nonnull m : class_type.declaration().members()) { switch (m->kind()) { @@ -527,12 +531,16 @@ auto TypeChecker::FieldTypes(const NominalClassType& class_type) const return field_types; } -auto TypeChecker::FieldTypesWithBase(const NominalClassType& class_type) const +auto TypeChecker::FieldTypesWithBase(SourceLocation source_loc, + std::string_view context, + const NominalClassType& class_type) const -> ErrorOr> { - CARBON_ASSIGN_OR_RETURN(auto fields, FieldTypes(class_type)); + CARBON_ASSIGN_OR_RETURN(auto fields, + FieldTypes(source_loc, context, class_type)); if (const auto base_type = class_type.base()) { - CARBON_ASSIGN_OR_RETURN(std::vector base_fields, - FieldTypesWithBase(*base_type.value())); + CARBON_ASSIGN_OR_RETURN( + std::vector base_fields, + FieldTypesWithBase(source_loc, context, *base_type.value())); fields.emplace_back(NamedValue{std::string(NominalClassValue::BaseField), base_type.value()}); } @@ -540,9 +548,9 @@ auto TypeChecker::FieldTypesWithBase(const NominalClassType& class_type) const } auto TypeChecker::IsImplicitlyConvertible( - Nonnull source, Nonnull destination, - const ImplScope& impl_scope, bool allow_user_defined_conversions) const - -> ErrorOr { + SourceLocation source_loc, Nonnull source, + Nonnull destination, const ImplScope& impl_scope, + bool allow_user_defined_conversions) const -> ErrorOr { // Check for an exact match to avoid impl lookup in this common case. CARBON_CHECK(IsNonDeduceableType(source)); CARBON_CHECK(IsNonDeduceableType(destination)); @@ -560,8 +568,8 @@ auto TypeChecker::IsImplicitlyConvertible( // to the actual destination type. We'll catch any problems when we actually // come to perform the conversion. if (isa(source) && IsTypeOfType(destination)) { - return IsBuiltinConversion(source, arena_->New(), impl_scope, - allow_user_defined_conversions); + return IsBuiltinConversion(source_loc, source, arena_->New(), + impl_scope, allow_user_defined_conversions); } if (IsTypeOfType(source) && IsTypeOfType(destination)) { return true; @@ -570,13 +578,12 @@ auto TypeChecker::IsImplicitlyConvertible( // If we're not supposed to look for a user-defined conversion, check for // builtin conversions, which are normally found by impl lookup. if (!allow_user_defined_conversions) { - return IsBuiltinConversion(source, destination, impl_scope, + return IsBuiltinConversion(source_loc, source, destination, impl_scope, allow_user_defined_conversions); } // We didn't find a builtin implicit conversion. Check if a user-defined one // exists. - SourceLocation source_loc = SourceLocation::DiagnosticsIgnored(); CARBON_ASSIGN_OR_RETURN( Nonnull iface_type, GetBuiltinInterfaceType( @@ -588,7 +595,8 @@ auto TypeChecker::IsImplicitlyConvertible( return conversion_witness.has_value(); } -auto TypeChecker::IsBuiltinConversion(Nonnull source, +auto TypeChecker::IsBuiltinConversion(SourceLocation source_loc, + Nonnull source, Nonnull destination, const ImplScope& impl_scope, bool allow_user_defined_conversions) const @@ -616,7 +624,7 @@ auto TypeChecker::IsBuiltinConversion(Nonnull source, } CARBON_ASSIGN_OR_RETURN( bool convertible, - IsImplicitlyConvertible(source_field->value, + IsImplicitlyConvertible(source_loc, source_field->value, destination_field.value, impl_scope, allow_user_defined_conversions)); if (!convertible) { @@ -628,12 +636,13 @@ auto TypeChecker::IsBuiltinConversion(Nonnull source, case Value::Kind::NominalClassType: { CARBON_ASSIGN_OR_RETURN( std::vector field_types, - FieldTypesWithBase(cast(*destination))); + FieldTypesWithBase(source_loc, "implicit conversion", + cast(*destination))); CARBON_ASSIGN_OR_RETURN( bool convertible, IsImplicitlyConvertible( - source, arena_->New(field_types), impl_scope, - allow_user_defined_conversions)); + source_loc, source, arena_->New(field_types), + impl_scope, allow_user_defined_conversions)); if (convertible) { return true; } @@ -665,9 +674,10 @@ auto TypeChecker::IsBuiltinConversion(Nonnull source, for (size_t i = 0; i < source_tuple.elements().size(); ++i) { CARBON_ASSIGN_OR_RETURN( bool convertible, - IsImplicitlyConvertible( - source_tuple.elements()[i], destination_tuple.elements()[i], - impl_scope, allow_user_defined_conversions)); + IsImplicitlyConvertible(source_loc, source_tuple.elements()[i], + destination_tuple.elements()[i], + impl_scope, + allow_user_defined_conversions)); if (!convertible) { all_ok = false; break; @@ -687,9 +697,10 @@ auto TypeChecker::IsBuiltinConversion(Nonnull source, for (Nonnull source_element : source_tuple.elements()) { CARBON_ASSIGN_OR_RETURN( bool convertible, - IsImplicitlyConvertible( - source_element, &destination_array.element_type(), - impl_scope, allow_user_defined_conversions)); + IsImplicitlyConvertible(source_loc, source_element, + &destination_array.element_type(), + impl_scope, + allow_user_defined_conversions)); if (!convertible) { all_ok = false; break; @@ -706,7 +717,8 @@ auto TypeChecker::IsBuiltinConversion(Nonnull source, for (Nonnull source_element : source_tuple.elements()) { CARBON_ASSIGN_OR_RETURN( bool convertible, - IsImplicitlyConvertible(source_element, destination, impl_scope, + IsImplicitlyConvertible(source_loc, source_element, destination, + impl_scope, allow_user_defined_conversions)); if (!convertible) { all_types = false; @@ -841,7 +853,8 @@ auto TypeChecker::BuildBuiltinConversion(Nonnull source, case Value::Kind::NominalClassType: { CARBON_ASSIGN_OR_RETURN( std::vector field_types, - FieldTypesWithBase(cast(*destination))); + FieldTypesWithBase(source->source_loc(), "implicit conversion", + cast(*destination))); CARBON_ASSIGN_OR_RETURN( Nonnull result, ImplicitlyConvert("implicit conversion", impl_scope, source, @@ -972,7 +985,8 @@ auto TypeChecker::ImplicitlyConvert(std::string_view context, auto* type_type = arena_->New(); CARBON_ASSIGN_OR_RETURN( bool convertible, - IsBuiltinConversion(source_type, type_type, impl_scope, + IsBuiltinConversion(source->source_loc(), source_type, type_type, + impl_scope, /*allow_user_defined_conversions=*/true)); if (convertible) { CARBON_ASSIGN_OR_RETURN( @@ -1072,8 +1086,8 @@ auto TypeChecker::ImplicitlyConvert(std::string_view context, } auto TypeChecker::IsIntrinsicConstraintSatisfied( - const IntrinsicConstraint& constraint, const ImplScope& impl_scope) const - -> ErrorOr { + SourceLocation source_loc, const IntrinsicConstraint& constraint, + const ImplScope& impl_scope) const -> ErrorOr { // TODO: Check to see if this constraint is known in the current impl scope. switch (constraint.kind) { case IntrinsicConstraint::ImplicitAs: @@ -1081,8 +1095,8 @@ auto TypeChecker::IsIntrinsicConstraintSatisfied( << "wrong number of arguments for `__intrinsic_implicit_as`"; CARBON_ASSIGN_OR_RETURN( bool convertible, - IsBuiltinConversion(constraint.type, constraint.arguments[0], - impl_scope, + IsBuiltinConversion(source_loc, constraint.type, + constraint.arguments[0], impl_scope, /*allow_user_defined_conversions=*/true)); if (trace_stream_->is_enabled()) { *trace_stream_ << constraint << " evaluated to " << convertible << "\n"; @@ -1637,9 +1651,10 @@ auto TypeChecker::ArgumentDeduction::Finish( bool type = IsType(subst_param) && IsType(mismatch.arg); if (type && mismatch.allow_implicit_conversion) { - CARBON_ASSIGN_OR_RETURN(bool convertible, - type_checker.IsImplicitlyConvertible( - mismatch.arg, subst_param, impl_scope, true)); + CARBON_ASSIGN_OR_RETURN( + bool convertible, + type_checker.IsImplicitlyConvertible(source_loc_, mismatch.arg, + subst_param, impl_scope, true)); if (!convertible) { if (!diagnose_deduction_failure) { return {std::nullopt}; @@ -4419,11 +4434,13 @@ auto TypeChecker::TypeCheckWhereClause(Nonnull clause, Nonnull rhs_type = &equals_clause.rhs().static_type(); CARBON_ASSIGN_OR_RETURN( bool lhs_converts_to_rhs, - IsImplicitlyConvertible(lhs_type, rhs_type, impl_scope, + IsImplicitlyConvertible(equals_clause.lhs().source_loc(), lhs_type, + rhs_type, impl_scope, /*allow_user_defined_conversions=*/false)); CARBON_ASSIGN_OR_RETURN( bool rhs_converts_to_lhs, - IsImplicitlyConvertible(rhs_type, lhs_type, impl_scope, + IsImplicitlyConvertible(equals_clause.rhs().source_loc(), rhs_type, + lhs_type, impl_scope, /*allow_user_defined_conversions=*/false)); if (!lhs_converts_to_rhs && !rhs_converts_to_lhs) { return ProgramError(clause->source_loc()) diff --git a/explorer/interpreter/type_checker.h b/explorer/interpreter/type_checker.h index a6fb222c65a4..6945440fb686 100644 --- a/explorer/interpreter/type_checker.h +++ b/explorer/interpreter/type_checker.h @@ -135,7 +135,8 @@ class TypeChecker { // Determine whether the given intrinsic constraint is known to be satisfied // in the given scope. - auto IsIntrinsicConstraintSatisfied(const IntrinsicConstraint& constraint, + auto IsIntrinsicConstraintSatisfied(SourceLocation source_loc, + const IntrinsicConstraint& constraint, const ImplScope& impl_scope) const -> ErrorOr; @@ -397,29 +398,23 @@ class TypeChecker { SourceLocation source_loc) -> ErrorOr; // Returns the field names of the class together with their types. - auto FieldTypes(const NominalClassType& class_type) const + auto FieldTypes(SourceLocation source_loc, std::string_view context, + const NominalClassType& class_type) const -> ErrorOr>; // Returns the field names and types of the class and its parents. - auto FieldTypesWithBase(const NominalClassType& class_type) const + auto FieldTypesWithBase(SourceLocation source_loc, std::string_view context, + const NominalClassType& class_type) const -> ErrorOr>; - // Returns true if source_fields and destination_fields contain the same set - // of names, and each value in source_fields is implicitly convertible to - // the corresponding value in destination_fields. All values in both arguments - // must be types. - auto FieldTypesImplicitlyConvertible( - llvm::ArrayRef source_fields, - llvm::ArrayRef destination_fields, - const ImplScope& impl_scope) const -> ErrorOr; - // Returns true if *source is implicitly convertible to *destination. *source // and *destination must be concrete types. // // If allow_user_defined_conversions, conversions requiring a user-defined // `ImplicitAs` implementation are not considered, and only builtin // conversions will be allowed. - auto IsImplicitlyConvertible(Nonnull source, + auto IsImplicitlyConvertible(SourceLocation source_loc, + Nonnull source, Nonnull destination, const ImplScope& impl_scope, bool allow_user_defined_conversions) const @@ -427,7 +422,8 @@ class TypeChecker { // Returns true if the conversion from `source` to `destination` is a builtin // conversion that `BuildBuiltinConversion` can perform. - auto IsBuiltinConversion(Nonnull source, + auto IsBuiltinConversion(SourceLocation source_loc, + Nonnull source, Nonnull destination, const ImplScope& impl_scope, bool allow_user_defined_conversions) const diff --git a/explorer/testdata/class/fail_incomplete_in_conversion.carbon b/explorer/testdata/class/fail_incomplete_in_conversion.carbon new file mode 100644 index 000000000000..39fbd6ebbe31 --- /dev/null +++ b/explorer/testdata/class/fail_incomplete_in_conversion.carbon @@ -0,0 +1,15 @@ +// 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 +// +// AUTOUPDATE + +package ExplorerTest api; + +class C { + // CHECK:STDERR: COMPILATION ERROR: fail_incomplete_in_conversion.carbon:[[@LINE+2]]: type error in `as`: `{}` is not explicitly convertible to `class C`: + // CHECK:STDERR: incomplete type `class C` used in implicit conversion + var x: ({} as C); +} + +fn Main() -> i32 { return 0; }