From c8141b59d97d4454c16b84d4a0ec1007035b15fb Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Thu, 2 Mar 2023 14:04:44 -0800 Subject: [PATCH] Remove ExpectType and some calls to IsImplicitlyConvertible. (#2647) These functions are dangerous, as they check whether conversions are possible without actually performing the conversions. In each case where they were used, explorer would crash in some cases if a user-defined conversion is required. This change moves us more towards implicit conversions being handled by a regular function call on an interface and away from them being magical builtins. Unfortunately, this exposes a pre-existing bug that a call of the form `x.(ImplicitAs(T).Convert)()` compiles even if `x` only has an explicit conversion to `T`. That's worked around here for now, but will need a proper fix later. --- explorer/interpreter/type_checker.cpp | 69 ++++++++++--------- explorer/interpreter/type_checker.h | 9 --- .../testdata/array/fail_size_mismatch.carbon | 2 +- explorer/testdata/assert/fail_convert.carbon | 25 +++++++ .../class/fail_direct_base_class_init.carbon | 2 +- .../testdata/class/fail_field_mismatch.carbon | 2 +- .../testdata/class/fail_field_missing.carbon | 2 +- .../class/fail_invalid_subtyping.carbon | 2 +- explorer/testdata/for/fail_convert.carbon | 30 ++++++++ .../generic_class/fail_point_equal.carbon | 2 +- .../fail_invalid_ptr_conversion1.carbon | 2 +- .../fail_invalid_ptr_conversion2.carbon | 2 +- .../pointer/pointer_to_type_like.carbon | 25 +++++++ .../testdata/pointer/tuple_pointer.carbon | 25 +++++++ .../fail_implicit_convert_with_as.carbon | 4 +- explorer/testdata/tuple/fail_to_array.carbon | 2 +- 16 files changed, 152 insertions(+), 53 deletions(-) create mode 100644 explorer/testdata/assert/fail_convert.carbon create mode 100644 explorer/testdata/for/fail_convert.carbon create mode 100644 explorer/testdata/pointer/pointer_to_type_like.carbon create mode 100644 explorer/testdata/pointer/tuple_pointer.carbon diff --git a/explorer/interpreter/type_checker.cpp b/explorer/interpreter/type_checker.cpp index ced0f5b68fe9..6ece912f6b54 100644 --- a/explorer/interpreter/type_checker.cpp +++ b/explorer/interpreter/type_checker.cpp @@ -795,6 +795,18 @@ auto TypeChecker::BuildBuiltinMethodCall(const ImplScope& impl_scope, CARBON_ASSIGN_OR_RETURN(Nonnull iface_type, GetBuiltinInterfaceType(source_loc, interface)); + if (interface.builtin == Builtins::ImplicitAs) { + // Type-checking the below expression resolves the member name to + // `As(Destination).Convert`, which allows both implicit and explicit + // conversions. So manually check that `ImplicitAs(Destination)` is + // actually implemented. + // TODO: This check should be performed as part of type-checking the + // compound member access expression below. This is a short-term + // workaround. + CARBON_RETURN_IF_ERROR(impl_scope.Resolve( + iface_type, &source->static_type(), source->source_loc(), *this)); + } + // Build an expression to perform the call `source.(interface.method)(args)`. Nonnull iface_expr = arena_->New( source_loc, iface_type, arena_->New(), ValueCategory::Let); @@ -839,23 +851,6 @@ auto TypeChecker::ExpectNonPlaceholderType(SourceLocation source_loc, CARBON_FATAL() << "unknown kind of placeholder type " << *type; } -auto TypeChecker::ExpectType(SourceLocation source_loc, - std::string_view context, - Nonnull expected, - Nonnull actual, - const ImplScope& impl_scope) const - -> ErrorOr { - if (!IsImplicitlyConvertible(actual, expected, impl_scope, - /*allow_user_defined_conversions=*/true)) { - return ProgramError(source_loc) - << "type error in " << context << ": " - << "'" << *actual << "' is not implicitly convertible to '" - << *expected << "'"; - } else { - return Success(); - } -} - // Argument deduction matches two values and attempts to find a set of // substitutions into deduced bindings in one of them that would result in the // other. @@ -2986,6 +2981,10 @@ auto TypeChecker::TypeCheckExp(Nonnull e, Nonnull iface_constraint, ConvertToConstraintType(access.source_loc(), "compound member access", *iface)); + // TODO: We should check that the base type implements the specified + // interface, not only the interface containing the member. + // `x.(ImplicitAs(T).Convert)()` should require that the type of `x` + // implements `ImplicitAs(T)`, not only `As(T)`. CARBON_ASSIGN_OR_RETURN(witness, impl_scope.Resolve(iface_constraint, *base_type, e->source_loc(), *this)); @@ -3271,13 +3270,17 @@ auto TypeChecker::TypeCheckExp(Nonnull e, op.set_static_type(&cast(*ts[0]).pointee_type()); op.set_value_category(ValueCategory::Var); return Success(); - case Operator::Ptr: - CARBON_RETURN_IF_ERROR(ExpectType(e->source_loc(), "*", - arena_->New(), ts[0], - impl_scope)); + case Operator::Ptr: { + auto* type_type = arena_->New(); + CARBON_ASSIGN_OR_RETURN( + Nonnull converted, + ImplicitlyConvert("pointee type", impl_scope, op.arguments()[0], + type_type)); + op.arguments()[0] = converted; op.set_static_type(arena_->New()); op.set_value_category(ValueCategory::Let); return Success(); + } case Operator::AddressOf: if (op.arguments()[0]->value_category() != ValueCategory::Var) { return ProgramError(op.arguments()[0]->source_loc()) @@ -3436,10 +3439,10 @@ auto TypeChecker::TypeCheckExp(Nonnull e, return ProgramError(e->source_loc()) << "__intrinsic_assert takes 2 arguments"; } - CARBON_RETURN_IF_ERROR(ExpectType( + CARBON_RETURN_IF_ERROR(ExpectExactType( e->source_loc(), "__intrinsic_assert argument 0", arena_->New(), &args[0]->static_type(), impl_scope)); - CARBON_RETURN_IF_ERROR(ExpectType( + CARBON_RETURN_IF_ERROR(ExpectExactType( e->source_loc(), "__intrinsic_assert argument 1", arena_->New(), &args[1]->static_type(), impl_scope)); e->set_static_type(TupleType::Empty()); @@ -3978,10 +3981,9 @@ auto TypeChecker::TypeCheckPattern( << "conversion to type succeeded but didn't produce a type, got " << *type; if (expected) { - if (IsConcreteType(type)) { - CARBON_RETURN_IF_ERROR(ExpectType(p->source_loc(), "name binding", - type, *expected, impl_scope)); - } else { + // TODO: Per proposal #2188, we should be performing conversions at + // this level rather than on the overall initializer. + if (!IsConcreteType(type)) { BindingMap generic_args; if (!PatternMatch(type, *expected, binding.type().source_loc(), std::nullopt, generic_args, trace_stream_, @@ -4063,11 +4065,8 @@ auto TypeChecker::TypeCheckPattern( << "alternative pattern does not name a choice type."; } const auto& choice_type = cast(*type); - if (expected) { - CARBON_RETURN_IF_ERROR(ExpectType(alternative.source_loc(), - "alternative pattern", &choice_type, - *expected, impl_scope)); - } + // TODO: Per proposal #2188, we should perform an implicit conversion on + // the scrutinee if a choice type is provided. std::optional> signature = choice_type.declaration().FindAlternative( alternative.alternative_name()); @@ -4098,6 +4097,7 @@ auto TypeChecker::TypeCheckPattern( auto& expression = cast(*p).expression(); CARBON_RETURN_IF_ERROR(TypeCheckExp(&expression, impl_scope)); p->set_static_type(&expression.static_type()); + // TODO: Per proposal #2188, we should form an `==` comparison here. CARBON_ASSIGN_OR_RETURN(Nonnull expr_value, InterpExp(&expression, arena_, trace_stream_)); p->set_value(expr_value); @@ -4299,7 +4299,10 @@ auto TypeChecker::TypeCheckStmt(Nonnull s, TypeCheckPattern(&for_stmt.variable_declaration(), &cast(rhs).element_type(), inner_impl_scope, ValueCategory::Var)); - + CARBON_RETURN_IF_ERROR(ExpectExactType( + for_stmt.source_loc(), "`for` pattern", + &cast(rhs).element_type(), + &for_stmt.variable_declaration().static_type(), impl_scope)); } else { return ProgramError(for_stmt.source_loc()) << "expected array type after in, found value of type " << rhs; diff --git a/explorer/interpreter/type_checker.h b/explorer/interpreter/type_checker.h index 06556ecf164e..b3f13d4c8728 100644 --- a/explorer/interpreter/type_checker.h +++ b/explorer/interpreter/type_checker.h @@ -410,15 +410,6 @@ class TypeChecker { auto IsSameType(Nonnull type1, Nonnull type2, const ImplScope& impl_scope) const -> bool; - // Check whether `actual` is implicitly convertible to `expected` - // and halt with a fatal compilation error if it is not. - // - // TODO: Does not actually perform the conversion if a user-defined - // conversion is needed. Should be used very rarely for that reason. - auto ExpectType(SourceLocation source_loc, std::string_view context, - Nonnull expected, Nonnull actual, - const ImplScope& impl_scope) const -> ErrorOr; - // Check whether `actual` is the same type as `expected` and halt with a // fatal compilation error if it is not. auto ExpectExactType(SourceLocation source_loc, std::string_view context, diff --git a/explorer/testdata/array/fail_size_mismatch.carbon b/explorer/testdata/array/fail_size_mismatch.carbon index 62e9e453d997..dac0903428b4 100644 --- a/explorer/testdata/array/fail_size_mismatch.carbon +++ b/explorer/testdata/array/fail_size_mismatch.carbon @@ -9,7 +9,7 @@ package ExplorerTest api; fn Main() -> i32 { - // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/array/fail_size_mismatch.carbon:[[@LINE+1]]: type error in name binding: '(i32, i32, i32)' is not implicitly convertible to '[i32; 2]' + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/array/fail_size_mismatch.carbon:[[@LINE+1]]: type error in initializer of variable: '(i32, i32, i32)' is not implicitly convertible to '[i32; 2]' var x: [i32; 2] = (0, 1, 2); return x[0]; } diff --git a/explorer/testdata/assert/fail_convert.carbon b/explorer/testdata/assert/fail_convert.carbon new file mode 100644 index 000000000000..53ee9099be93 --- /dev/null +++ b/explorer/testdata/assert/fail_convert.carbon @@ -0,0 +1,25 @@ +// 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 +// RUN: %{not} %{explorer-run} +// RUN: %{not} %{explorer-run-trace} + +package ExplorerTest api; + +class ConvertTo(T:! type) { + var v: T; + impl as ImplicitAs(T) { + fn Convert[self: Self]() -> T { return self.v; } + } +} + +fn Main() -> i32 { + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/assert/fail_convert.carbon:[[@LINE+3]]: type error in __intrinsic_assert argument 0 + // CHECK:STDERR: expected: bool + // CHECK:STDERR: actual: class ConvertTo(T = bool) + __intrinsic_assert({.v = true} as ConvertTo(bool), {.v = "Pass"} as ConvertTo(String)); + __intrinsic_assert({.v = false} as ConvertTo(bool), {.v = "Fail"} as ConvertTo(String)); + return 0; +} diff --git a/explorer/testdata/class/fail_direct_base_class_init.carbon b/explorer/testdata/class/fail_direct_base_class_init.carbon index f85de82afdd9..8f79d6b30d43 100644 --- a/explorer/testdata/class/fail_direct_base_class_init.carbon +++ b/explorer/testdata/class/fail_direct_base_class_init.carbon @@ -17,7 +17,7 @@ class B extends A { } fn Main() -> i32 { - // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_direct_base_class_init.carbon:[[@LINE+1]]: type error in name binding: '{.a: i32, .b: i32}' is not implicitly convertible to 'class B' + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_direct_base_class_init.carbon:[[@LINE+1]]: type error in initializer of variable: '{.a: i32, .b: i32}' is not implicitly convertible to 'class B' var b: B = {.a=0, .b=1}; return 0; } diff --git a/explorer/testdata/class/fail_field_mismatch.carbon b/explorer/testdata/class/fail_field_mismatch.carbon index 06f3184c7d10..afd6ab26ae99 100644 --- a/explorer/testdata/class/fail_field_mismatch.carbon +++ b/explorer/testdata/class/fail_field_mismatch.carbon @@ -14,7 +14,7 @@ class Point { } fn Main() -> i32 { - // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_field_mismatch.carbon:[[@LINE+1]]: type error in name binding: '{.x: i32, .z: i32}' is not implicitly convertible to 'class Point' + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_field_mismatch.carbon:[[@LINE+1]]: type error in initializer of variable: '{.x: i32, .z: i32}' is not implicitly convertible to 'class Point' var p: Point = {.x = 1, .z = 2}; return p.x - 1; } diff --git a/explorer/testdata/class/fail_field_missing.carbon b/explorer/testdata/class/fail_field_missing.carbon index 86d0efe0f28b..b0e4e8c06246 100644 --- a/explorer/testdata/class/fail_field_missing.carbon +++ b/explorer/testdata/class/fail_field_missing.carbon @@ -14,7 +14,7 @@ class Point { } fn Main() -> i32 { - // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_field_missing.carbon:[[@LINE+1]]: type error in name binding: '{.x: i32}' is not implicitly convertible to 'class Point' + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_field_missing.carbon:[[@LINE+1]]: type error in initializer of variable: '{.x: i32}' is not implicitly convertible to 'class Point' var p: Point = {.x = 1}; return p.x - 1; } diff --git a/explorer/testdata/class/fail_invalid_subtyping.carbon b/explorer/testdata/class/fail_invalid_subtyping.carbon index 2872d389d90a..7f86ce80d5e8 100644 --- a/explorer/testdata/class/fail_invalid_subtyping.carbon +++ b/explorer/testdata/class/fail_invalid_subtyping.carbon @@ -16,7 +16,7 @@ class D { fn Main() -> i32 { var d: D = {}; - // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_invalid_subtyping.carbon:[[@LINE+1]]: type error in name binding: 'class D*' is not implicitly convertible to 'class C*' + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/class/fail_invalid_subtyping.carbon:[[@LINE+1]]: type error in initializer of variable: 'class D*' is not implicitly convertible to 'class C*' var c: C* = &d; return 0; } diff --git a/explorer/testdata/for/fail_convert.carbon b/explorer/testdata/for/fail_convert.carbon new file mode 100644 index 000000000000..49e074c638ba --- /dev/null +++ b/explorer/testdata/for/fail_convert.carbon @@ -0,0 +1,30 @@ +// 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 +// RUN: %{not} %{explorer-run} +// RUN: %{not} %{explorer-run-trace} + +package ExplorerTest api; + +class IntLike { + var n: i32; +} + +impl i32 as ImplicitAs(IntLike) { + fn Convert[self: i32]() -> IntLike { return {.n = self}; } +} + +fn Main() -> i32 { + var arr: [i32; 4] = (0, 1, 2, 3); + // TODO: We should accept this, but currently have nowhere in our + // representation to describe the conversion. + for (x: IntLike in arr) { + Print("{0}", x.n); + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/for/fail_convert.carbon:[[@LINE+3]]: type error in `for` pattern + // CHECK:STDERR: expected: i32 + // CHECK:STDERR: actual: class IntLike + } + return 0; +} diff --git a/explorer/testdata/generic_class/fail_point_equal.carbon b/explorer/testdata/generic_class/fail_point_equal.carbon index 5dc6cebde095..f322cd7d1c6d 100644 --- a/explorer/testdata/generic_class/fail_point_equal.carbon +++ b/explorer/testdata/generic_class/fail_point_equal.carbon @@ -15,7 +15,7 @@ class Point(T:! type) { fn Main() -> i32 { var p: Point(i32) = {.x = 0, .y = 0}; - // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/generic_class/fail_point_equal.carbon:[[@LINE+1]]: type error in name binding: 'class Point(T = i32)' is not implicitly convertible to 'class Point(T = bool)' + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/generic_class/fail_point_equal.carbon:[[@LINE+1]]: type error in initializer of variable: 'class Point(T = i32)' is not implicitly convertible to 'class Point(T = bool)' var q: Point(bool) = p; return 0; } diff --git a/explorer/testdata/pointer/fail_invalid_ptr_conversion1.carbon b/explorer/testdata/pointer/fail_invalid_ptr_conversion1.carbon index 1287e266033b..aadef9cdb7a3 100644 --- a/explorer/testdata/pointer/fail_invalid_ptr_conversion1.carbon +++ b/explorer/testdata/pointer/fail_invalid_ptr_conversion1.carbon @@ -13,7 +13,7 @@ class A {} fn Main() -> i32 { var a: A = {}; var b: A* = &a; - // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/pointer/fail_invalid_ptr_conversion1.carbon:[[@LINE+1]]: type error in name binding: 'class A*' is not implicitly convertible to 'i32' + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/pointer/fail_invalid_ptr_conversion1.carbon:[[@LINE+1]]: type error in initializer of variable: 'class A*' is not implicitly convertible to 'i32' var c: i32 = b; return 1; } diff --git a/explorer/testdata/pointer/fail_invalid_ptr_conversion2.carbon b/explorer/testdata/pointer/fail_invalid_ptr_conversion2.carbon index b2710abd5c08..12729ef96723 100644 --- a/explorer/testdata/pointer/fail_invalid_ptr_conversion2.carbon +++ b/explorer/testdata/pointer/fail_invalid_ptr_conversion2.carbon @@ -13,7 +13,7 @@ class A {} fn Main() -> i32 { var a: i32 = 0; var b: i32* = &a; - // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/pointer/fail_invalid_ptr_conversion2.carbon:[[@LINE+1]]: type error in name binding: 'i32*' is not implicitly convertible to 'class A*' + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/pointer/fail_invalid_ptr_conversion2.carbon:[[@LINE+1]]: type error in initializer of variable: 'i32*' is not implicitly convertible to 'class A*' var c: A* = b; return 1; } diff --git a/explorer/testdata/pointer/pointer_to_type_like.carbon b/explorer/testdata/pointer/pointer_to_type_like.carbon new file mode 100644 index 000000000000..d74711cbe8e1 --- /dev/null +++ b/explorer/testdata/pointer/pointer_to_type_like.carbon @@ -0,0 +1,25 @@ +// 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 +// RUN: %{explorer-run} +// RUN: %{explorer-run-trace} +// CHECK:STDOUT: result: 1 + +package ExplorerTest api; + +class TypeLike { + var v: type; + impl as ImplicitAs(type) { + fn Convert[self: Self]() -> type { return i32; } + } +} + +fn Almosti32() -> TypeLike { return {.v = i32}; } + +fn Main() -> i32 { + var a: Almosti32() = 1; + var p: Almosti32()* = &a; + return *p; +} diff --git a/explorer/testdata/pointer/tuple_pointer.carbon b/explorer/testdata/pointer/tuple_pointer.carbon new file mode 100644 index 000000000000..5b6f9c15d8f3 --- /dev/null +++ b/explorer/testdata/pointer/tuple_pointer.carbon @@ -0,0 +1,25 @@ +// 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 +// RUN: %{explorer-run} +// RUN: %{explorer-run-trace} +// CHECK:STDOUT: 3 +// CHECK:STDOUT: 4 +// CHECK:STDOUT: result: 0 + +package ExplorerTest api; + +fn Main() -> i32 { + var a: (i32, i32) = (1, 2); + var p: (i32, i32)* = &a; + + a[0] = 3; + Print("{0}", (*p)[0]); + + (*p)[1] = 4; + Print("{0}", a[1]); + + return 0; +} diff --git a/explorer/testdata/tuple/fail_implicit_convert_with_as.carbon b/explorer/testdata/tuple/fail_implicit_convert_with_as.carbon index 16649c777462..b1e2d19ab561 100644 --- a/explorer/testdata/tuple/fail_implicit_convert_with_as.carbon +++ b/explorer/testdata/tuple/fail_implicit_convert_with_as.carbon @@ -21,7 +21,7 @@ external impl A as As(B) { fn Main() -> i32 { var a: (i32, A) = (1, {.a = 2}); - // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/tuple/fail_implicit_convert_with_as.carbon:[[@LINE+1]]: type error in name binding: '(i32, class A)' is not implicitly convertible to '(i32, class B)' + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/tuple/fail_implicit_convert_with_as.carbon:[[@LINE+1]]: type error in initializer of variable: '(i32, class A)' is not implicitly convertible to '(i32, class B)' var b: (i32, B) = a; - return 0; + return b[1].b; } diff --git a/explorer/testdata/tuple/fail_to_array.carbon b/explorer/testdata/tuple/fail_to_array.carbon index 0241c3710111..381f3e3b10f5 100644 --- a/explorer/testdata/tuple/fail_to_array.carbon +++ b/explorer/testdata/tuple/fail_to_array.carbon @@ -10,6 +10,6 @@ package ExplorerTest api; fn Main() -> i32 { var t: auto = (1, 2); - // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/tuple/fail_to_array.carbon:[[@LINE+1]]: type error in name binding: '(i32, i32)' is not implicitly convertible to '[i32; 3]' + // CHECK:STDERR: COMPILATION ERROR: {{.*}}/explorer/testdata/tuple/fail_to_array.carbon:[[@LINE+1]]: type error in initializer of variable: '(i32, i32)' is not implicitly convertible to '[i32; 3]' var a: [i32; 3] = t; }