From 7c7e169994914fcd0119b49b2a77f60666820525 Mon Sep 17 00:00:00 2001 From: Dana Jansens Date: Wed, 19 Feb 2025 16:46:46 -0500 Subject: [PATCH] Avoid diagnosing conversion errors inside deduction of impl arguments (#4976) When conversion fails, the impl should simply not match, no error should be generated. --- toolchain/check/convert.cpp | 182 +++++++++++------- toolchain/check/convert.h | 19 +- toolchain/check/deduce.cpp | 32 +-- .../impl/no_prelude/impl_cycle.carbon | 19 +- 4 files changed, 147 insertions(+), 105 deletions(-) diff --git a/toolchain/check/convert.cpp b/toolchain/check/convert.cpp index 3d6a3098d482b..8016a5f630b4f 100644 --- a/toolchain/check/convert.cpp +++ b/toolchain/check/convert.cpp @@ -256,26 +256,31 @@ static auto ConvertTupleToArray(Context& context, SemIR::TupleType tuple_type, sem_ir.GetArrayBoundValue(array_type.bound_id); if (!array_bound) { // TODO: Should this fall back to using `ImplicitAs`? - CARBON_DIAGNOSTIC(ArrayInitDependentBound, Error, - "cannot initialize array with dependent bound from a " - "list of initializers"); - context.emitter().Emit(value_loc_id, ArrayInitDependentBound); + if (target.diagnose) { + CARBON_DIAGNOSTIC(ArrayInitDependentBound, Error, + "cannot initialize array with dependent bound from a " + "list of initializers"); + context.emitter().Emit(value_loc_id, ArrayInitDependentBound); + } return SemIR::ErrorInst::SingletonInstId; } if (tuple_elem_types.size() != array_bound) { - CARBON_DIAGNOSTIC( - ArrayInitFromLiteralArgCountMismatch, Error, - "cannot initialize array of {0} element{0:s} from {1} initializer{1:s}", - IntAsSelect, IntAsSelect); - CARBON_DIAGNOSTIC(ArrayInitFromExprArgCountMismatch, Error, - "cannot initialize array of {0} element{0:s} from tuple " - "with {1} element{1:s}", - IntAsSelect, IntAsSelect); - context.emitter().Emit(value_loc_id, - literal_elems.empty() - ? ArrayInitFromExprArgCountMismatch - : ArrayInitFromLiteralArgCountMismatch, - *array_bound, tuple_elem_types.size()); + if (target.diagnose) { + CARBON_DIAGNOSTIC(ArrayInitFromLiteralArgCountMismatch, Error, + "cannot initialize array of {0} element{0:s} from {1} " + "initializer{1:s}", + IntAsSelect, IntAsSelect); + CARBON_DIAGNOSTIC( + ArrayInitFromExprArgCountMismatch, Error, + "cannot initialize array of {0} element{0:s} from tuple " + "with {1} element{1:s}", + IntAsSelect, IntAsSelect); + context.emitter().Emit(value_loc_id, + literal_elems.empty() + ? ArrayInitFromExprArgCountMismatch + : ArrayInitFromLiteralArgCountMismatch, + *array_bound, tuple_elem_types.size()); + } return SemIR::ErrorInst::SingletonInstId; } @@ -348,12 +353,15 @@ static auto ConvertTupleToTuple(Context& context, SemIR::TupleType src_type, // Check that the tuples are the same size. if (src_elem_types.size() != dest_elem_types.size()) { - CARBON_DIAGNOSTIC(TupleInitElementCountMismatch, Error, - "cannot initialize tuple of {0} element{0:s} from tuple " - "with {1} element{1:s}", - IntAsSelect, IntAsSelect); - context.emitter().Emit(value_loc_id, TupleInitElementCountMismatch, - dest_elem_types.size(), src_elem_types.size()); + if (target.diagnose) { + CARBON_DIAGNOSTIC( + TupleInitElementCountMismatch, Error, + "cannot initialize tuple of {0} element{0:s} from tuple " + "with {1} element{1:s}", + IntAsSelect, IntAsSelect); + context.emitter().Emit(value_loc_id, TupleInitElementCountMismatch, + dest_elem_types.size(), src_elem_types.size()); + } return SemIR::ErrorInst::SingletonInstId; } @@ -445,14 +453,16 @@ static auto ConvertStructToStructOrClass(Context& context, // TODO: If not, include the name of the first source field that doesn't // exist in the destination or vice versa in the diagnostic. if (src_elem_fields.size() != dest_elem_fields_size) { - CARBON_DIAGNOSTIC( - StructInitElementCountMismatch, Error, - "cannot initialize {0:class|struct} with {1} field{1:s} from struct " - "with {2} field{2:s}", - BoolAsSelect, IntAsSelect, IntAsSelect); - context.emitter().Emit(value_loc_id, StructInitElementCountMismatch, - ToClass, dest_elem_fields_size, - src_elem_fields.size()); + if (target.diagnose) { + CARBON_DIAGNOSTIC( + StructInitElementCountMismatch, Error, + "cannot initialize {0:class|struct} with {1} field{1:s} from struct " + "with {2} field{2:s}", + BoolAsSelect, IntAsSelect, IntAsSelect); + context.emitter().Emit(value_loc_id, StructInitElementCountMismatch, + ToClass, dest_elem_fields_size, + src_elem_fields.size()); + } return SemIR::ErrorInst::SingletonInstId; } @@ -511,21 +521,24 @@ static auto ConvertStructToStructOrClass(Context& context, if (auto lookup = src_field_indexes.Lookup(dest_field.name_id)) { src_field_index = lookup.value(); } else { - if (literal_elems_id.has_value()) { - CARBON_DIAGNOSTIC( - StructInitMissingFieldInLiteral, Error, - "missing value for field `{0}` in struct initialization", - SemIR::NameId); - context.emitter().Emit(value_loc_id, StructInitMissingFieldInLiteral, - dest_field.name_id); - } else { - CARBON_DIAGNOSTIC(StructInitMissingFieldInConversion, Error, - "cannot convert from struct type {0} to {1}: " - "missing field `{2}` in source type", - TypeOfInstId, SemIR::TypeId, SemIR::NameId); - context.emitter().Emit(value_loc_id, - StructInitMissingFieldInConversion, value_id, - target.type_id, dest_field.name_id); + if (target.diagnose) { + if (literal_elems_id.has_value()) { + CARBON_DIAGNOSTIC( + StructInitMissingFieldInLiteral, Error, + "missing value for field `{0}` in struct initialization", + SemIR::NameId); + context.emitter().Emit(value_loc_id, + StructInitMissingFieldInLiteral, + dest_field.name_id); + } else { + CARBON_DIAGNOSTIC(StructInitMissingFieldInConversion, Error, + "cannot convert from struct type {0} to {1}: " + "missing field `{2}` in source type", + TypeOfInstId, SemIR::TypeId, SemIR::NameId); + context.emitter().Emit(value_loc_id, + StructInitMissingFieldInConversion, value_id, + target.type_id, dest_field.name_id); + } } return SemIR::ErrorInst::SingletonInstId; } @@ -863,12 +876,11 @@ static auto PerformBuiltinConversion(Context& context, SemIR::LocId loc_id, foundation_value_id = PerformBuiltinConversion(context, loc_id, foundation_value_id, - { - .kind = target.kind, - .type_id = foundation_type_id, - .init_id = foundation_init_id, - .init_block = target.init_block, - }); + {.kind = target.kind, + .type_id = foundation_type_id, + .init_id = foundation_init_id, + .init_block = target.init_block, + .diagnose = target.diagnose}); if (foundation_value_id == SemIR::ErrorInst::SingletonInstId) { return SemIR::ErrorInst::SingletonInstId; } @@ -891,10 +903,10 @@ static auto PerformBuiltinConversion(Context& context, SemIR::LocId loc_id, // necessary. if (SemIR::GetExprCategory(context.sem_ir(), value_id) == SemIR::ExprCategory::Mixed) { - value_id = PerformBuiltinConversion( - context, loc_id, value_id, - ConversionTarget{.kind = ConversionTarget::Value, - .type_id = value_type_id}); + value_id = PerformBuiltinConversion(context, loc_id, value_id, + {.kind = ConversionTarget::Value, + .type_id = value_type_id, + .diagnose = target.diagnose}); } return AddInst( context, loc_id, {.type_id = target.type_id, .source_id = value_id}); @@ -979,7 +991,9 @@ static auto PerformBuiltinConversion(Context& context, SemIR::LocId loc_id, sem_ir.inst_blocks().Get(tuple_literal->elements_id)) { // TODO: This call recurses back into conversion. Switch to an // iterative approach. - type_ids.push_back(ExprAsType(context, loc_id, tuple_inst_id).type_id); + type_ids.push_back( + ExprAsType(context, loc_id, tuple_inst_id, target.diagnose) + .type_id); } auto tuple_type_id = GetTupleType(context, type_ids); return sem_ir.types().GetInstId(tuple_type_id); @@ -1075,7 +1089,7 @@ static auto PerformBuiltinConversion(Context& context, SemIR::LocId loc_id, // Given a value expression, form a corresponding initializer that copies from // that value, if it is possible to do so. -static auto PerformCopy(Context& context, SemIR::InstId expr_id) +static auto PerformCopy(Context& context, SemIR::InstId expr_id, bool diagnose) -> SemIR::InstId { auto expr = context.insts().Get(expr_id); auto type_id = expr.type_id(); @@ -1091,9 +1105,11 @@ static auto PerformCopy(Context& context, SemIR::InstId expr_id) // TODO: We don't yet have rules for whether and when a class type is // copyable, or how to perform the copy. - CARBON_DIAGNOSTIC(CopyOfUncopyableType, Error, - "cannot copy value of type {0}", TypeOfInstId); - context.emitter().Emit(expr_id, CopyOfUncopyableType, expr_id); + if (diagnose) { + CARBON_DIAGNOSTIC(CopyOfUncopyableType, Error, + "cannot copy value of type {0}", TypeOfInstId); + context.emitter().Emit(expr_id, CopyOfUncopyableType, expr_id); + } return SemIR::ErrorInst::SingletonInstId; } @@ -1114,9 +1130,11 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id, // TODO: We currently encounter this for use of namespaces and functions. // We should provide a better diagnostic for inappropriate use of // namespace names, and allow use of functions as values. - CARBON_DIAGNOSTIC(UseOfNonExprAsValue, Error, - "expression cannot be used as a value"); - context.emitter().Emit(expr_id, UseOfNonExprAsValue); + if (target.diagnose) { + CARBON_DIAGNOSTIC(UseOfNonExprAsValue, Error, + "expression cannot be used as a value"); + context.emitter().Emit(expr_id, UseOfNonExprAsValue); + } return SemIR::ErrorInst::SingletonInstId; } @@ -1127,6 +1145,9 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id, CARBON_CHECK(!target.is_initializer(), "Initialization of incomplete types is expected to be " "caught elsewhere."); + if (!target.diagnose) { + return context.emitter().BuildSuppressed(); + } CARBON_DIAGNOSTIC(IncompleteTypeInValueConversion, Error, "forming value of incomplete type {0}", SemIR::TypeId); @@ -1141,12 +1162,12 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id, target.type_id); }, [&] { + if (!target.diagnose || !target.is_initializer()) { + return context.emitter().BuildSuppressed(); + } CARBON_DIAGNOSTIC(AbstractTypeInInit, Error, "initialization of abstract type {0}", SemIR::TypeId); - if (!target.is_initializer()) { - return context.emitter().BuildSuppressed(); - } return context.emitter().Build(loc_id, AbstractTypeInInit, target.type_id); })) { @@ -1171,6 +1192,9 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id, .op_name = "Convert", }; expr_id = BuildUnaryOperator(context, loc_id, op, expr_id, [&] { + if (!target.diagnose) { + return context.emitter().BuildSuppressed(); + } CARBON_DIAGNOSTIC(ImplicitAsConversionFailure, Error, "cannot implicitly convert from {0} to {1}", TypeOfInstId, SemIR::TypeId); @@ -1256,7 +1280,7 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id, case SemIR::ExprCategory::Value: // When initializing from a value, perform a copy. if (target.is_initializer()) { - expr_id = PerformCopy(context, expr_id); + expr_id = PerformCopy(context, expr_id, target.diagnose); } break; } @@ -1314,6 +1338,16 @@ auto ConvertToValueOrRefOfType(Context& context, SemIR::LocId loc_id, {.kind = ConversionTarget::ValueOrRef, .type_id = type_id}); } +// Like ConvertToValueOfType but failure to convert does not result in +// diagnostics. An ErrorInst instruction is still returned on failure. +auto TryConvertToValueOfType(Context& context, SemIR::LocId loc_id, + SemIR::InstId expr_id, SemIR::TypeId type_id) + -> SemIR::InstId { + return Convert( + context, loc_id, expr_id, + {.kind = ConversionTarget::Value, .type_id = type_id, .diagnose = false}); +} + auto ConvertToBoolValue(Context& context, SemIR::LocId loc_id, SemIR::InstId value_id) -> SemIR::InstId { return ConvertToValueOfType( @@ -1362,8 +1396,8 @@ auto ConvertCallArgs(Context& context, SemIR::LocId call_loc_id, self_id, arg_refs, return_slot_arg_id); } -auto ExprAsType(Context& context, SemIR::LocId loc_id, SemIR::InstId value_id) - -> TypeExpr { +auto ExprAsType(Context& context, SemIR::LocId loc_id, SemIR::InstId value_id, + bool diagnose) -> TypeExpr { auto type_inst_id = ConvertToValueOfType(context, loc_id, value_id, SemIR::TypeType::SingletonTypeId); if (type_inst_id == SemIR::ErrorInst::SingletonInstId) { @@ -1373,9 +1407,11 @@ auto ExprAsType(Context& context, SemIR::LocId loc_id, SemIR::InstId value_id) auto type_const_id = context.constant_values().Get(type_inst_id); if (!type_const_id.is_constant()) { - CARBON_DIAGNOSTIC(TypeExprEvaluationFailure, Error, - "cannot evaluate type expression"); - context.emitter().Emit(loc_id, TypeExprEvaluationFailure); + if (diagnose) { + CARBON_DIAGNOSTIC(TypeExprEvaluationFailure, Error, + "cannot evaluate type expression"); + context.emitter().Emit(loc_id, TypeExprEvaluationFailure); + } return {.inst_id = SemIR::ErrorInst::SingletonInstId, .type_id = SemIR::ErrorInst::SingletonTypeId}; } diff --git a/toolchain/check/convert.h b/toolchain/check/convert.h index f056229865302..14c77ef78933e 100644 --- a/toolchain/check/convert.h +++ b/toolchain/check/convert.h @@ -43,6 +43,10 @@ struct ConversionTarget { // form the value of `init_id`, and that can be discarded if no // initialization is needed. PendingBlock* init_block = nullptr; + // Whether failure of conversion is an error and is diagnosed to the user. + // When looking for a possible conversion but with graceful fallback, diagnose + // should be false. + bool diagnose = true; // Are we converting this value into an initializer for an object? auto is_initializer() const -> bool { @@ -80,6 +84,13 @@ auto ConvertToValueOrRefOfType(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id, SemIR::TypeId type_id) -> SemIR::InstId; +// Attempted to convert `expr_id` to a value expression of type `type_id`, with +// graceful failure, which does not result in diagnostics. An ErrorInst +// instruction is still returned on failure. +auto TryConvertToValueOfType(Context& context, SemIR::LocId loc_id, + SemIR::InstId expr_id, SemIR::TypeId type_id) + -> SemIR::InstId; + // Converts `value_id` to a value expression of type `bool`. auto ConvertToBoolValue(Context& context, SemIR::LocId loc_id, SemIR::InstId value_id) -> SemIR::InstId; @@ -109,11 +120,15 @@ struct TypeExpr { }; // Converts an expression for use as a type. +// +// If `diagnose` is true, errors are diagnosed to the user. Set it to false when +// looking to see if a conversion is possible but with graceful fallback. +// // TODO: Most of the callers of this function discard the `inst_id` and lose // track of the conversion. In most cases we should be retaining that as the // operand of some downstream instruction. -auto ExprAsType(Context& context, SemIR::LocId loc_id, SemIR::InstId value_id) - -> TypeExpr; +auto ExprAsType(Context& context, SemIR::LocId loc_id, SemIR::InstId value_id, + bool diagnose = true) -> TypeExpr; } // namespace Carbon::Check diff --git a/toolchain/check/deduce.cpp b/toolchain/check/deduce.cpp index e13fa65466ede..e0adbf41236be 100644 --- a/toolchain/check/deduce.cpp +++ b/toolchain/check/deduce.cpp @@ -338,7 +338,10 @@ auto DeductionContext::Deduce() -> bool { // TODO: The call logic should reuse the conversion here (if any) instead // of doing the same conversion again. At the moment we throw away the // converted arg_id. - arg_id = ConvertToValueOfType(context(), loc_id_, arg_id, param_type_id); + arg_id = diagnose_ ? ConvertToValueOfType(context(), loc_id_, arg_id, + param_type_id) + : TryConvertToValueOfType(context(), loc_id_, arg_id, + param_type_id); if (arg_id == SemIR::ErrorInst::SingletonInstId) { return false; } @@ -554,26 +557,31 @@ auto DeductionContext::CheckDeductionIsComplete() -> bool { binding_type_id = context().types().GetTypeIdForTypeConstantId(param_type_const_id); - // TODO: Suppress diagnostics here if `diagnose_` is false. DiagnosticAnnotationScope annotate_diagnostics( &context().emitter(), [&](auto& builder) { NoteInitializingParam(binding_id, builder); }); - auto converted_arg_id = ConvertToValueOfType( - context(), loc_id_, deduced_arg_id, binding_type_id); + auto converted_arg_id = + diagnose_ ? ConvertToValueOfType(context(), loc_id_, deduced_arg_id, + binding_type_id) + : TryConvertToValueOfType(context(), loc_id_, + deduced_arg_id, binding_type_id); // Replace the deduced arg with its value converted to the parameter // type. The conversion of the argument type must produce a constant value // to be used in deduction. if (context().constant_values().Get(converted_arg_id).is_constant()) { deduced_arg_id = converted_arg_id; } else { - CARBON_DIAGNOSTIC(RuntimeConversionDuringCompTimeDeduction, Error, - "compile-time value requires runtime conversion, " - "constructing value of type {0}", - SemIR::TypeId); - auto diag = context().emitter().Build( - loc_id_, RuntimeConversionDuringCompTimeDeduction, binding_type_id); - NoteGenericHere(context(), generic_id_, diag); - diag.Emit(); + if (diagnose_) { + CARBON_DIAGNOSTIC(RuntimeConversionDuringCompTimeDeduction, Error, + "compile-time value requires runtime conversion, " + "constructing value of type {0}", + SemIR::TypeId); + auto diag = context().emitter().Build( + loc_id_, RuntimeConversionDuringCompTimeDeduction, + binding_type_id); + NoteGenericHere(context(), generic_id_, diag); + diag.Emit(); + } deduced_arg_id = SemIR::ErrorInst::SingletonInstId; } } diff --git a/toolchain/check/testdata/impl/no_prelude/impl_cycle.carbon b/toolchain/check/testdata/impl/no_prelude/impl_cycle.carbon index b25b03abcfa52..17070472c290f 100644 --- a/toolchain/check/testdata/impl/no_prelude/impl_cycle.carbon +++ b/toolchain/check/testdata/impl/no_prelude/impl_cycle.carbon @@ -145,7 +145,7 @@ fn F() { ({} as Wraps(C)) as (Wraps(C) as ComparableTo(Wraps(D))); } -// --- fail_todo_impl_recurse_with_simpler_type_in_generic_param_bidirectional_no_cycle.carbon +// --- impl_recurse_with_simpler_type_in_generic_param_bidirectional_no_cycle.carbon library "[[@TEST_NAME]]"; import Core; @@ -166,10 +166,6 @@ impl C as ComparableTo(D) {} // T is always ComparableWith(T). impl forall [T:! type] T as ComparableWith(T) {} // If U is ComparableTo(T), U is ComparableWith(T). -// -// TODO: When we look for the impl of `D as ComparableTo(C)` here, we find that -// it's not implemented. We then go to the next impl which does match, but -// deduction generates an error here regardless. impl forall [T:! type, U:! ComparableTo(T)] U as ComparableWith(T) {} // If U is ComparableTo(T), T is ComparableWith(U). impl forall [T:! type, U:! ComparableTo(T)] T as ComparableWith(U) {} @@ -178,19 +174,6 @@ fn Compare[T:! type, U:! ComparableWith(T)](t: T, u: U) {} fn F() { Compare({} as C, {} as C); - // CHECK:STDERR: fail_todo_impl_recurse_with_simpler_type_in_generic_param_bidirectional_no_cycle.carbon:[[@LINE+13]]:3: error: cannot implicitly convert from `type` to `ComparableTo(C)` [ImplicitAsConversionFailure] - // CHECK:STDERR: Compare({} as C, {} as D); - // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~ - // CHECK:STDERR: fail_todo_impl_recurse_with_simpler_type_in_generic_param_bidirectional_no_cycle.carbon:[[@LINE+10]]:3: note: type `type` does not implement interface `Core.ImplicitAs(ComparableTo(C))` [MissingImplInMemberAccessNote] - // CHECK:STDERR: Compare({} as C, {} as D); - // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~ - // CHECK:STDERR: fail_todo_impl_recurse_with_simpler_type_in_generic_param_bidirectional_no_cycle.carbon:[[@LINE-14]]:1: note: while deducing parameters of generic declared here [DeductionGenericHere] - // CHECK:STDERR: impl forall [T:! type, U:! ComparableTo(T)] U as ComparableWith(T) {} - // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - // CHECK:STDERR: fail_todo_impl_recurse_with_simpler_type_in_generic_param_bidirectional_no_cycle.carbon:[[@LINE-13]]:1: note: while deducing parameters of generic declared here [DeductionGenericHere] - // CHECK:STDERR: fn Compare[T:! type, U:! ComparableWith(T)](t: T, u: U) {} - // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - // CHECK:STDERR: Compare({} as C, {} as D); Compare({} as D, {} as C); }