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.

Author: zygoloid

explorer/interpreter/type_checker.cpp

@@ -795,6 +795,18 @@ auto TypeChecker::BuildBuiltinMethodCall(const ImplScope& impl_scope,
   CARBON_ASSIGN_OR_RETURN(Nonnull<const InterfaceType*> 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<Expression*> iface_expr = arena_->New<ValueLiteral>(
       source_loc, iface_type, arena_->New<TypeType>(), 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<const Value*> expected,
-                             Nonnull<const Value*> actual,
-                             const ImplScope& impl_scope) const
-    -> ErrorOr<Success> {
-  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<Expression*> e,
             Nonnull<const ConstraintType*> 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<Expression*> e,
           op.set_static_type(&cast<PointerType>(*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<TypeType>(), ts[0],
-                                            impl_scope));
+        case Operator::Ptr: {
+          auto* type_type = arena_->New<TypeType>();
+          CARBON_ASSIGN_OR_RETURN(
+              Nonnull<Expression*> converted,
+              ImplicitlyConvert("pointee type", impl_scope, op.arguments()[0],
+                                type_type));
+          op.arguments()[0] = converted;
           op.set_static_type(arena_->New<TypeType>());
           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<Expression*> 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<BoolType>(), &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<StringType>(), &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<ChoiceType>(*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<Nonnull<const AlternativeSignature*>> signature =
           choice_type.declaration().FindAlternative(
               alternative.alternative_name());
@@ -4098,6 +4097,7 @@ auto TypeChecker::TypeCheckPattern(
       auto& expression = cast<ExpressionPattern>(*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<const Value*> expr_value,
                               InterpExp(&expression, arena_, trace_stream_));
       p->set_value(expr_value);
@@ -4299,7 +4299,10 @@ auto TypeChecker::TypeCheckStmt(Nonnull<Statement*> s,
             TypeCheckPattern(&for_stmt.variable_declaration(),
                              &cast<StaticArrayType>(rhs).element_type(),
                              inner_impl_scope, ValueCategory::Var));
-
+        CARBON_RETURN_IF_ERROR(ExpectExactType(
+            for_stmt.source_loc(), "`for` pattern",
+            &cast<StaticArrayType>(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;

explorer/interpreter/type_checker.h

@@ -410,15 +410,6 @@ class TypeChecker {
   auto IsSameType(Nonnull<const Value*> type1, Nonnull<const Value*> 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<const Value*> expected, Nonnull<const Value*> actual,
-                  const ImplScope& impl_scope) const -> ErrorOr<Success>;
-
   // 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,

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];
 }

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;
+}

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;
 }

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;
 }

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;
 }

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;
 }

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;
+}

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;
 }

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;
 }

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;
 }

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;
+}

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;
+}

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;
 }

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;
 }