Rework handling of C++ references. (#6268)

For now, map C++ reference types to const-qualified Carbon pointer types
rather than picking between a (non-const) pointer or a value type. This
fixes misbehavior in lowering for reference members in classes and
reference return types.

Update the special-case handling for references as function parameters
so that it continues to map const reference parameters to Carbon
pass-by-value, and unify the code paths for `self` parameters and other
parameters, which were mostly doing the same thing but had some subtle
differences.

Add references to the list of types that we can pass to and from C++
directly, without needing an additional layer of thunks.

Author: zygoloid

toolchain/check/cpp/import.cpp

@@ -1212,8 +1212,7 @@ static auto MapQualifiedType(Context& context, clang::QualType type,
 
   if (quals.hasConst()) {
     auto type_id = GetConstType(context, type_expr.inst_id);
-    type_expr = {.inst_id = context.types().GetInstId(type_id),
-                 .type_id = type_id};
+    type_expr = TypeExpr::ForUnsugared(context, type_id);
     quals.removeConst();
   }
 
@@ -1260,19 +1259,18 @@ static auto MapPointerType(Context& context, SemIR::LocId loc_id,
   return pointer_type_expr;
 }
 
-// Maps a C++ reference type to a Carbon type.
-// We map `T&` to `T*`, and `T&&` to `T`.
+// Maps a C++ reference type to a Carbon type. We map all references to
+// pointers for now. Note that when mapping function parameters and return
+// types, a different rule is used; see MapParameterType for details.
 // TODO: Revisit this and decide what we really want to do here.
 static auto MapReferenceType(Context& context, clang::QualType type,
                              TypeExpr referenced_type_expr) -> TypeExpr {
   CARBON_CHECK(type->isReferenceType());
-
-  if (!type->isLValueReferenceType()) {
-    return referenced_type_expr;
-  }
-
-  return TypeExpr::ForUnsugared(
-      context, GetPointerType(context, referenced_type_expr.inst_id));
+  SemIR::TypeId pointer_type_id =
+      GetPointerType(context, referenced_type_expr.inst_id);
+  pointer_type_id =
+      GetConstType(context, context.types().GetInstId(pointer_type_id));
+  return TypeExpr::ForUnsugared(context, pointer_type_id);
 }
 
 // Maps a C++ type to a Carbon type. `type` should not be canonicalized because
@@ -1318,6 +1316,81 @@ static auto MapType(Context& context, SemIR::LocId loc_id, clang::QualType type)
   return mapped;
 }
 
+namespace {
+// Information about how to map a C++ parameter type into Carbon.
+struct ParameterTypeInfo {
+  // The type to use for the Carbon parameter.
+  TypeExpr type;
+  // Whether to build an `addr` pattern.
+  bool want_addr_pattern;
+  // If building an `addr` pattern, the type matched by that pattern.
+  TypeExpr pointee_type;
+};
+}  // namespace
+
+// Given the type of a C++ function parameter, returns information about the
+// type to use for the corresponding Carbon parameter.
+//
+// Note that if the parameter has a type for which `IsSimpleAbiType` returns
+// true, we must produce a parameter type that has the same calling convention
+// as the C++ type.
+//
+// TODO: Use `ref` instead of `addr`.
+static auto MapParameterType(Context& context, SemIR::LocId loc_id,
+                             clang::QualType param_type) -> ParameterTypeInfo {
+  ParameterTypeInfo info = {.type = TypeExpr::None,
+                            .want_addr_pattern = false,
+                            .pointee_type = TypeExpr::None};
+
+  // Perform some custom mapping for parameters of reference type:
+  //
+  //   * `T& x` -> `addr x: T*`.
+  //   * `const T& x` -> `x: T`.
+  //   * `T&& x` -> `x: T`.
+  //
+  // TODO: For the `&&` mapping, we allow an rvalue reference to bind to a
+  // durable reference expression. This should not be allowed.
+  if (param_type->isReferenceType()) {
+    clang::QualType pointee_type = param_type->getPointeeType();
+    if (param_type->isLValueReferenceType()) {
+      if (pointee_type.isConstQualified()) {
+        // TODO: Consider only doing this if `const` is the only qualifier. For
+        // now, any other qualifier will fail when mapping the type.
+        auto split_type = pointee_type.getSplitUnqualifiedType();
+        split_type.Quals.removeConst();
+        pointee_type = context.ast_context().getQualifiedType(split_type);
+      } else {
+        // The reference will map to a pointer. Request an `addr` pattern.
+        info.want_addr_pattern = true;
+      }
+    }
+    param_type = pointee_type;
+  }
+
+  info.type = MapType(context, loc_id, param_type);
+  if (info.want_addr_pattern && info.type.inst_id.has_value()) {
+    info.pointee_type = info.type;
+    info.type = TypeExpr::ForUnsugared(
+        context, GetPointerType(context, info.pointee_type.inst_id));
+  }
+  return info;
+}
+
+// Finishes building the pattern to use for a function parameter, given the
+// binding pattern and information about how the parameter is being mapped into
+// Carbon.
+static auto FinishParameterPattern(Context& context, SemIR::InstId pattern_id,
+                                   ParameterTypeInfo info) -> SemIR::InstId {
+  if (!info.want_addr_pattern || pattern_id == SemIR::ErrorInst::InstId) {
+    return pattern_id;
+  }
+  return AddPatternInst(
+      context, {SemIR::LocId(pattern_id),
+                SemIR::AddrPattern({.type_id = GetPatternType(
+                                        context, info.pointee_type.type_id),
+                                    .inner_id = pattern_id})});
+}
+
 // Returns a block for the implicit parameters of the given function
 // declaration. Because function templates are not yet supported, this currently
 // only contains the `self` parameter. On error, produces a diagnostic and
@@ -1334,32 +1407,10 @@ static auto MakeImplicitParamPatternsBlockId(
   // Build a `self` parameter from the object parameter.
   BeginSubpattern(context);
 
-  // Perform some special-case mapping for the object parameter:
-  //
-  //  - If it's a const reference to T, produce a by-value `self: T` parameter.
-  //  - If it's a non-const reference to T, produce an `addr self: T*`
-  //    parameter.
-  //  - Otherwise, map it directly, which will currently fail for `&&`-qualified
-  //    methods.
-  //
-  // TODO: Some of this mapping should be performed for all parameters.
   clang::QualType param_type =
       method_decl->getFunctionObjectParameterReferenceType();
-  bool addr_self = false;
-  if (param_type->isLValueReferenceType()) {
-    param_type = param_type.getNonReferenceType();
-    if (param_type.isConstQualified()) {
-      // TODO: Consider only doing this if `const` is the only qualifier. For
-      // now, any other qualifier will fail when mapping the type.
-      auto split_type = param_type.getSplitUnqualifiedType();
-      split_type.Quals.removeConst();
-      param_type = method_decl->getASTContext().getQualifiedType(split_type);
-    } else {
-      addr_self = true;
-    }
-  }
-
-  auto [type_inst_id, type_id] = MapType(context, loc_id, param_type);
+  auto param_info = MapParameterType(context, loc_id, param_type);
+  auto [type_inst_id, type_id] = param_info.type;
   SemIR::ExprRegionId type_expr_region_id =
       EndSubpatternAsExpr(context, type_inst_id);
 
@@ -1372,10 +1423,8 @@ static auto MakeImplicitParamPatternsBlockId(
 
   // TODO: Fill in a location once available.
   auto pattern_id =
-      addr_self ? AddAddrSelfParamPattern(context, SemIR::LocId::None,
-                                          type_expr_region_id, type_inst_id)
-                : AddSelfParamPattern(context, SemIR::LocId::None,
-                                      type_expr_region_id, type_id);
+      AddSelfParamPattern(context, loc_id, type_expr_region_id, type_id);
+  pattern_id = FinishParameterPattern(context, pattern_id, param_info);
 
   return context.inst_blocks().Add({pattern_id});
 }
@@ -1409,12 +1458,11 @@ static auto MakeParamPatternsBlockId(Context& context, SemIR::LocId loc_id,
     // TODO: The presence of qualifiers here is probably a Clang bug.
     clang::QualType param_type = orig_param_type.getUnqualifiedType();
 
-    bool is_ref_param = param_type->isLValueReferenceType();
-
     // Mark the start of a region of insts, needed for the type expression
     // created later with the call of `EndSubpatternAsExpr()`.
     BeginSubpattern(context);
-    auto [orig_type_inst_id, type_id] = MapType(context, loc_id, param_type);
+    auto param_info = MapParameterType(context, loc_id, param_type);
+    auto [orig_type_inst_id, type_id] = param_info.type;
     // Type expression of the binding pattern - a single-entry/single-exit
     // region that allows control flow in the type expression e.g. fn F(x: if C
     // then i32 else i64).
@@ -1452,17 +1500,7 @@ static auto MakeParamPatternsBlockId(Context& context, SemIR::LocId loc_id,
                       {.type_id = context.insts().Get(pattern_id).type_id(),
                        .subpattern_id = pattern_id,
                        .index = SemIR::CallParamIndex::None})});
-    if (is_ref_param) {
-      // We map `T&` parameters to `addr param: T*`.
-      // TODO: Revisit this and decide what we really want to do here.
-      pattern_id = AddPatternInst(
-          context, {param_loc_id,
-                    SemIR::AddrPattern(
-                        {.type_id = GetPatternType(
-                             context, context.types().GetTypeIdForTypeInstId(
-                                          orig_type_inst_id)),
-                         .inner_id = pattern_id})});
-    }
+    pattern_id = FinishParameterPattern(context, pattern_id, param_info);
     params.push_back(pattern_id);
   }
   return context.inst_blocks().Add(params);
@@ -1476,6 +1514,9 @@ static auto GetReturnTypeExpr(Context& context, SemIR::LocId loc_id,
                               clang::FunctionDecl* clang_decl) -> TypeExpr {
   clang::QualType orig_ret_type = clang_decl->getReturnType();
   if (!orig_ret_type->isVoidType()) {
+    // TODO: We should eventually map reference returns to non-pointer types
+    // here. We should return by `ref` for `T&` return types once `ref` return
+    // is implemented.
     auto [orig_type_inst_id, type_id] = MapType(context, loc_id, orig_ret_type);
     if (!orig_type_inst_id.has_value()) {
       context.TODO(loc_id, llvm::formatv("Unsupported: return type: {0}",

toolchain/check/cpp/thunk.cpp

@@ -58,9 +58,20 @@ static auto IsSimpleAbiType(clang::ASTContext& ast_context,
     return true;
   }
 
-  if (!for_parameter && type->isLValueReferenceType()) {
-    // An lvalue reference return type maps to a pointer, which uses the same
-    // lowering rule.
+  if (type->isReferenceType()) {
+    if (for_parameter) {
+      // A reference parameter has a simple ABI if it's a non-const lvalue
+      // reference.  Otherwise, we map it to pass-by-value, and it's only simple
+      // if the type uses a pointer value representation.
+      //
+      // TODO: Check whether the pointee type maps to a Carbon type that uses a
+      // pointer value representation, and treat it as simple if so.
+      return type->isLValueReferenceType() &&
+             !type->getPointeeType().isConstQualified();
+    }
+
+    // A reference return type is always mapped to a Carbon pointer, which uses
+    // the same ABI rule as a C++ reference.
     return true;
   }
 
@@ -93,7 +104,8 @@ struct CalleeFunctionInfo {
     bool is_ctor = isa<clang::CXXConstructorDecl>(decl);
     has_object_parameter = method_decl && !method_decl->isStatic() && !is_ctor;
     if (has_object_parameter && method_decl->isImplicitObjectMemberFunction()) {
-      implicit_this_type = method_decl->getThisType();
+      implicit_object_parameter_type =
+          method_decl->getFunctionObjectParameterReferenceType();
     }
     effective_return_type =
         is_ctor ? ast_context.getCanonicalTagType(method_decl->getParent())
@@ -104,7 +116,7 @@ struct CalleeFunctionInfo {
 
   // Returns whether this callee has an implicit `this` parameter.
   auto has_implicit_object_parameter() const -> bool {
-    return !implicit_this_type.isNull();
+    return !implicit_object_parameter_type.isNull();
   }
 
   // Returns whether this callee has an explicit `this` parameter.
@@ -143,9 +155,9 @@ struct CalleeFunctionInfo {
   // implicit.
   bool has_object_parameter;
 
-  // If the callee has an implicit object parameter, the corresponding `this`
-  // type. Otherwise a null type.
-  clang::QualType implicit_this_type;
+  // If the callee has an implicit object parameter, the type of that parameter,
+  // which will always be a reference type. Otherwise a null type.
+  clang::QualType implicit_object_parameter_type;
 
   // The return type that the callee has when viewed from Carbon. This is the
   // C++ return type, except that constructors return the class type in Carbon
@@ -180,16 +192,10 @@ auto IsCppThunkRequired(Context& context, const SemIR::Function& function)
   }
 
   auto& ast_context = context.ast_context();
-  if (callee_info.has_implicit_object_parameter()) {
-    // TODO: The object parameter is a reference parameter, but we don't force a
-    // thunk here like we do for explicit reference parameters in the case where
-    // we would map the parameter to an `addr` parameter. We should make this
-    // behavior consistent.
-    auto* method_decl = cast<clang::CXXMethodDecl>(decl);
-    if (method_decl->getRefQualifier() == clang::RQ_RValue ||
-        method_decl->getMethodQualifiers().hasConst()) {
-      return true;
-    }
+  if (callee_info.has_implicit_object_parameter() &&
+      !IsSimpleAbiType(ast_context, callee_info.implicit_object_parameter_type,
+                       /*for_parameter=*/true)) {
+    return true;
   }
 
   const auto* function_type =
@@ -238,8 +244,7 @@ static auto BuildThunkParameterTypes(clang::ASTContext& ast_context,
   llvm::SmallVector<clang::QualType> thunk_param_types;
   thunk_param_types.reserve(callee_info.num_thunk_params());
   if (callee_info.has_implicit_object_parameter()) {
-    thunk_param_types.push_back(
-        GetNonnullType(ast_context, callee_info.implicit_this_type));
+    thunk_param_types.push_back(callee_info.implicit_object_parameter_type);
   }
 
   const auto* function_type =
@@ -371,23 +376,22 @@ static auto BuildThunkParamRef(clang::Sema& sema,
     clang::ExprResult deref_result =
         sema.BuildUnaryOp(nullptr, clang_loc, clang::UO_Deref, call_arg);
     CARBON_CHECK(deref_result.isUsable());
-
-    // Cast to an rvalue when initializing an rvalue reference. The validity of
-    // the initialization of the reference should be validated by the caller of
-    // the thunk.
-    //
-    // TODO: Consider inserting a cast to an rvalue in more cases. Note that we
-    // currently pass pointers to non-temporary objects as the argument when
-    // calling a thunk, so we'll need to either change that or generate
-    // different thunks depending on whether we're moving from each parameter.
-    if (type->isRValueReferenceType()) {
-      deref_result = clang::ImplicitCastExpr::Create(
-          sema.getASTContext(), deref_result.get()->getType(), clang::CK_NoOp,
-          deref_result.get(), nullptr, clang::ExprValueKind::VK_XValue,
-          clang::FPOptionsOverride());
-    }
     call_arg = deref_result.get();
   }
+
+  // Cast to an rvalue when initializing an rvalue reference. The validity of
+  // the initialization of the reference should be validated by the caller of
+  // the thunk.
+  //
+  // TODO: Consider inserting a cast to an rvalue in more cases. Note that we
+  // currently pass pointers to non-temporary objects as the argument when
+  // calling a thunk, so we'll need to either change that or generate
+  // different thunks depending on whether we're moving from each parameter.
+  if (!type.isNull() && type->isRValueReferenceType()) {
+    call_arg = clang::ImplicitCastExpr::Create(
+        sema.getASTContext(), call_arg->getType(), clang::CK_NoOp, call_arg,
+        nullptr, clang::ExprValueKind::VK_XValue, clang::FPOptionsOverride());
+  }
   return call_arg;
 }
 
@@ -443,14 +447,14 @@ static auto BuildThunkBody(clang::Sema& sema,
                            callee_info.has_explicit_object_parameter()
                                ? callee_info.decl->getParamDecl(0)->getType()
                                : clang::QualType());
-    bool is_arrow = callee_info.has_implicit_object_parameter();
+    constexpr bool IsArrow = false;
     auto object =
-        sema.PerformMemberExprBaseConversion(object_param_ref, is_arrow);
+        sema.PerformMemberExprBaseConversion(object_param_ref, IsArrow);
     if (object.isInvalid()) {
       return clang::StmtError();
     }
     callee = sema.BuildMemberExpr(
-        object.get(), is_arrow, clang_loc, clang::NestedNameSpecifierLoc(),
+        object.get(), IsArrow, clang_loc, clang::NestedNameSpecifierLoc(),
         clang::SourceLocation(), callee_info.decl,
         clang::DeclAccessPair::make(callee_info.decl, clang::AS_public),
         /*HadMultipleCandidates=*/false, clang::DeclarationNameInfo(),

toolchain/check/pattern.cpp

@@ -157,15 +157,4 @@ auto AddSelfParamPattern(Context& context, SemIR::LocId loc_id,
   return pattern_id;
 }
 
-auto AddAddrSelfParamPattern(Context& context, SemIR::LocId loc_id,
-                             SemIR::ExprRegionId type_expr_region_id,
-                             SemIR::TypeInstId type_inst_id) -> SemIR::InstId {
-  auto pattern_id = AddSelfParamPattern(context, loc_id, type_expr_region_id,
-                                        GetPointerType(context, type_inst_id));
-  return AddPatternInst<SemIR::AddrPattern>(
-      context, loc_id,
-      {.type_id = GetPatternType(context, SemIR::AutoType::TypeId),
-       .inner_id = pattern_id});
-}
-
 }  // namespace Carbon::Check

toolchain/check/pattern.h

@@ -59,11 +59,6 @@ auto AddSelfParamPattern(Context& context, SemIR::LocId loc_id,
                          SemIR::ExprRegionId type_expr_region_id,
                          SemIR::TypeId type_id) -> SemIR::InstId;
 
-// As the above, but for `addr self: Self*`.
-auto AddAddrSelfParamPattern(Context& context, SemIR::LocId loc_id,
-                             SemIR::ExprRegionId type_expr_region_id,
-                             SemIR::TypeInstId type_inst_id) -> SemIR::InstId;
-
 }  // namespace Carbon::Check
 
 #endif  // CARBON_TOOLCHAIN_CHECK_PATTERN_H_