Implement support for mixed-access overload sets. (#6137)

zygoloid · danakj · web-flow · commit 3b6d202730b8 · 2025-09-26T23:18:16.000Z
This turns out to be quite important, as several important standard
library types (such as `std::string`) have mixed-access overload sets
for their constructors as an implementation detail. The overall approach
here is:

- Use the most permissive access to determine the access of the overload
set itself. This affects whether name lookup finds the member name at
all.
- After overload resolution, re-check the access of the selected member,
if it's protected or private.

---------

Co-authored-by: Dana Jansens &lt;danakj@orodu.net&gt;
diff --git a/toolchain/check/cpp/import.cpp b/toolchain/check/cpp/import.cpp
@@ -508,18 +508,6 @@ static auto GetDeclContext(Context& context, SemIR::NameScopeId scope_id)
       context.clang_decls().Get(scope_clang_decl_context_id).key.decl);
 }
 
-// Looks up for constructors in the class scope and returns the lookup result.
-static auto ClangConstructorLookup(Context& context,
-                                   SemIR::NameScopeId scope_id)
-    -> clang::DeclContextLookupResult {
-  const SemIR::NameScope& scope = context.name_scopes().Get(scope_id);
-
-  clang::Sema& sema = context.clang_sema();
-  clang::Decl* decl =
-      context.clang_decls().Get(scope.clang_decl_context_id()).key.decl;
-  return sema.LookupConstructors(cast<clang::CXXRecordDecl>(decl));
-}
-
 // Returns true if the given Clang declaration is the implicit injected class
 // name within the class.
 static auto IsDeclInjectedClassName(Context& context,
@@ -2074,12 +2062,14 @@ static auto LookupBuiltinTypes(Context& context, SemIR::LocId loc_id,
 
 auto ImportCppOverloadSet(Context& context, SemIR::NameScopeId scope_id,
                           SemIR::NameId name_id,
-                          const clang::UnresolvedSet<4>& overload_set)
+                          clang::CXXRecordDecl* naming_class,
+                          clang::UnresolvedSet<4>&& overload_set)
     -> SemIR::InstId {
   SemIR::CppOverloadSetId overload_set_id = context.cpp_overload_sets().Add(
       SemIR::CppOverloadSet{.name_id = name_id,
                             .parent_scope_id = scope_id,
-                            .candidate_functions = overload_set});
+                            .naming_class = naming_class,
+                            .candidate_functions = std::move(overload_set)});
 
   auto overload_set_inst_id =
       // TODO: Add a location.
@@ -2093,65 +2083,65 @@ auto ImportCppOverloadSet(Context& context, SemIR::NameScopeId scope_id,
   return overload_set_inst_id;
 }
 
-// Gets the access for an overloaded function set. Returns std::nullopt
-// if the access is not the same for all functions in the overload set.
-// TODO: Fix to support functions with different access levels.
-static auto GetOverloadSetAccess(Context& context, SemIR::LocId loc_id,
-                                 const clang::UnresolvedSet<4>& overload_set)
-    -> std::optional<SemIR::AccessKind> {
+// Gets the best access for an overloaded function set. This is the access that
+// we use for the overload set as a whole. More fine-grained checking is done
+// after overload resolution.
+static auto GetOverloadSetAccess(const clang::UnresolvedSet<4>& overload_set)
+    -> SemIR::AccessKind {
   clang::AccessSpecifier access = overload_set.begin().getAccess();
   for (auto it = overload_set.begin() + 1; it != overload_set.end(); ++it) {
-    if (it.getAccess() != access) {
-      context.TODO(loc_id, "Unsupported: Overloaded set with mixed access");
-      return std::nullopt;
+    CARBON_CHECK(
+        (it.getAccess() == clang::AS_none) == (access == clang::AS_none),
+        "Unexpected mixture of members and non-members");
+    if (it.getAccess() < access) {
+      access = it->getAccess();
     }
   }
   return MapAccess(access);
 }
 
 // Imports an overload set from Clang to Carbon and adds the name into the
 // `NameScope`.
-static auto ImportOverloadSetIntoScope(
-    Context& context, SemIR::LocId loc_id, SemIR::NameScopeId scope_id,
-    SemIR::NameId name_id, const clang::UnresolvedSet<4>& overload_set)
+static auto ImportOverloadSetIntoScope(Context& context,
+                                       SemIR::NameScopeId scope_id,
+                                       SemIR::NameId name_id,
+                                       clang::CXXRecordDecl* naming_class,
+                                       clang::UnresolvedSet<4>&& overload_set)
     -> SemIR::ScopeLookupResult {
-  std::optional<SemIR::AccessKind> access_kind =
-      GetOverloadSetAccess(context, loc_id, overload_set);
-  if (!access_kind.has_value()) {
-    return SemIR::ScopeLookupResult::MakeError();
-  }
-
-  SemIR::InstId inst_id =
-      ImportCppOverloadSet(context, scope_id, name_id, overload_set);
-  AddNameToScope(context, scope_id, name_id, access_kind.value(), inst_id);
+  SemIR::AccessKind access_kind = GetOverloadSetAccess(overload_set);
+  SemIR::InstId inst_id = ImportCppOverloadSet(
+      context, scope_id, name_id, naming_class, std::move(overload_set));
+  AddNameToScope(context, scope_id, name_id, access_kind, inst_id);
   return SemIR::ScopeLookupResult::MakeWrappedLookupResult(inst_id,
-                                                           access_kind.value());
+                                                           access_kind);
 }
 
 // Imports the constructors for a given class name. The found constructors are
 // imported as part of an overload set into the scope. Currently copy/move
 // constructors are not imported.
-static auto ImportConstructorsIntoScope(Context& context, SemIR::LocId loc_id,
+static auto ImportConstructorsIntoScope(Context& context,
                                         SemIR::NameScopeId scope_id,
                                         SemIR::NameId name_id)
     -> SemIR::ScopeLookupResult {
+  auto* naming_class =
+      cast<clang::CXXRecordDecl>(GetDeclContext(context, scope_id));
   clang::DeclContextLookupResult constructors_lookup =
-      ClangConstructorLookup(context, scope_id);
+      context.clang_sema().LookupConstructors(naming_class);
 
   clang::UnresolvedSet<4> overload_set;
   for (auto* decl : constructors_lookup) {
     auto info = clang::getConstructorInfo(decl);
     if (!info.Constructor || info.Constructor->isCopyOrMoveConstructor()) {
       continue;
     }
-    overload_set.addDecl(info.FoundDecl, info.FoundDecl->getAccess());
+    overload_set.addDecl(info.FoundDecl, info.FoundDecl.getAccess());
   }
   if (overload_set.empty()) {
     return SemIR::ScopeLookupResult::MakeNotFound();
   }
 
-  return ImportOverloadSetIntoScope(context, loc_id, scope_id, name_id,
-                                    overload_set);
+  return ImportOverloadSetIntoScope(context, scope_id, name_id, naming_class,
+                                    std::move(overload_set));
 }
 
 // Imports a builtin type from Clang to Carbon and adds the name into the
@@ -2207,8 +2197,9 @@ auto ImportNameFromCpp(Context& context, SemIR::LocId loc_id,
        lookup->getFoundDecl()->isFunctionOrFunctionTemplate())) {
     clang::UnresolvedSet<4> overload_set;
     overload_set.append(lookup->begin(), lookup->end());
-    return ImportOverloadSetIntoScope(context, loc_id, scope_id, name_id,
-                                      overload_set);
+    return ImportOverloadSetIntoScope(context, scope_id, name_id,
+                                      lookup->getNamingClass(),
+                                      std::move(overload_set));
   }
   if (!lookup->isSingleResult()) {
     // Clang will diagnose ambiguous lookup results for us.
@@ -2224,7 +2215,7 @@ auto ImportNameFromCpp(Context& context, SemIR::LocId loc_id,
   }
   if (IsDeclInjectedClassName(context, scope_id, name_id,
                               lookup->getFoundDecl())) {
-    return ImportConstructorsIntoScope(context, loc_id, scope_id, name_id);
+    return ImportConstructorsIntoScope(context, scope_id, name_id);
   }
   auto key = SemIR::ClangDeclKey::ForNonFunctionDecl(lookup->getFoundDecl());
   return ImportNameDeclIntoScope(context, loc_id, scope_id, name_id, key,
diff --git a/toolchain/check/cpp/import.h b/toolchain/check/cpp/import.h
@@ -34,7 +34,8 @@ auto ImportCppFunctionDecl(Context& context, SemIR::LocId loc_id,
 // Imports an overloaded function set from Clang to Carbon.
 auto ImportCppOverloadSet(Context& context, SemIR::NameScopeId scope_id,
                           SemIR::NameId name_id,
-                          const clang::UnresolvedSet<4>& overload_set)
+                          clang::CXXRecordDecl* naming_class,
+                          clang::UnresolvedSet<4>&& overload_set)
     -> SemIR::InstId;
 
 // Looks up the given name in the Clang AST generated when importing C++ code
diff --git a/toolchain/check/cpp/operators.cpp b/toolchain/check/cpp/operators.cpp
@@ -206,7 +206,8 @@ auto LookupCppOperator(Context& context, SemIR::LocId loc_id, Operator op,
   }
 
   return ImportCppOverloadSet(context, SemIR::NameScopeId::None,
-                              SemIR::NameId::CppOperator, functions);
+                              SemIR::NameId::CppOperator,
+                              /*naming_class=*/nullptr, std::move(functions));
 }
 
 }  // namespace Carbon::Check
diff --git a/toolchain/check/cpp/overload_resolution.cpp b/toolchain/check/cpp/overload_resolution.cpp
@@ -11,9 +11,12 @@
 #include "toolchain/check/cpp/import.h"
 #include "toolchain/check/cpp/location.h"
 #include "toolchain/check/cpp/type_mapping.h"
+#include "toolchain/check/member_access.h"
+#include "toolchain/check/name_lookup.h"
 #include "toolchain/diagnostics/diagnostic_emitter.h"
 #include "toolchain/sem_ir/function.h"
 #include "toolchain/sem_ir/ids.h"
+#include "toolchain/sem_ir/name_scope.h"
 #include "toolchain/sem_ir/typed_insts.h"
 
 namespace Carbon::Check {
@@ -79,6 +82,40 @@ static auto AddOverloadCandidataes(clang::Sema& sema,
   }
 }
 
+// Checks whether a selected overload is accessible and diagnoses if not.
+static auto CheckOverloadAccess(Context& context, SemIR::LocId loc_id,
+                                const SemIR::CppOverloadSet& overload_set,
+                                clang::DeclAccessPair overload,
+                                SemIR::InstId overload_inst_id) -> void {
+  SemIR::AccessKind member_access_kind;
+  switch (overload->getAccess()) {
+    case clang::AS_none:
+    case clang::AS_public: {
+      return;
+    }
+
+    case clang::AS_protected: {
+      member_access_kind = SemIR::AccessKind::Protected;
+      break;
+    }
+
+    case clang::AS_private: {
+      member_access_kind = SemIR::AccessKind::Private;
+      break;
+    }
+  }
+
+  auto name_scope_const_id = context.constant_values().Get(
+      context.name_scopes().Get(overload_set.parent_scope_id).inst_id());
+  SemIR::AccessKind allowed_access_kind =
+      GetHighestAllowedAccess(context, loc_id, name_scope_const_id);
+  CheckAccess(context, loc_id, SemIR::LocId(overload_inst_id),
+              overload_set.name_id, member_access_kind,
+              /*is_parent_access=*/false,
+              {.constant_id = name_scope_const_id,
+               .highest_allowed_access = allowed_access_kind});
+}
+
 // Returns whether the decl is an operator member function.
 static auto IsOperatorMethodDecl(clang::Decl* decl) -> bool {
   auto* clang_method_decl = dyn_cast<clang::CXXMethodDecl>(decl);
@@ -136,12 +173,14 @@ static auto ResolveCalleeId(Context& context, SemIR::LocId loc_id,
       // TODO: Handle the cases when Function is null.
       CARBON_CHECK(best_viable_fn->Function);
       sema.MarkFunctionReferenced(loc, best_viable_fn->Function);
-      SemIR::InstId result = ImportCppFunctionDecl(
+      SemIR::InstId result_id = ImportCppFunctionDecl(
           context, loc_id, best_viable_fn->Function,
           // If this is an operator method, the first arg will be used as self.
           arg_exprs.size() -
               (IsOperatorMethodDecl(best_viable_fn->Function) ? 1 : 0));
-      return result;
+      CheckOverloadAccess(context, loc_id, overload_set,
+                          best_viable_fn->FoundDecl, result_id);
+      return result_id;
     }
     case clang::OverloadingResult::OR_No_Viable_Function: {
       candidate_set.NoteCandidates(
diff --git a/toolchain/check/member_access.cpp b/toolchain/check/member_access.cpp
@@ -96,10 +96,8 @@ static auto IsInstanceType(Context& context, SemIR::TypeId type_id) -> bool {
   return false;
 }
 
-// Returns the highest allowed access. For example, if this returns `Protected`
-// then only `Public` and `Protected` accesses are allowed--not `Private`.
-static auto GetHighestAllowedAccess(Context& context, SemIR::LocId loc_id,
-                                    SemIR::ConstantId name_scope_const_id)
+auto GetHighestAllowedAccess(Context& context, SemIR::LocId loc_id,
+                             SemIR::ConstantId name_scope_const_id)
     -> SemIR::AccessKind {
   SemIR::ScopeLookupResult lookup_result =
       LookupUnqualifiedName(context, loc_id, SemIR::NameId::SelfType,
diff --git a/toolchain/check/member_access.h b/toolchain/check/member_access.h
@@ -10,6 +10,13 @@
 
 namespace Carbon::Check {
 
+// Returns the highest allowed access for members of `name_scope_const_id`. For
+// example, if this returns `Protected` then only `Public` and `Protected`
+// accesses are allowed -- not `Private`.
+auto GetHighestAllowedAccess(Context& context, SemIR::LocId loc_id,
+                             SemIR::ConstantId name_scope_const_id)
+    -> SemIR::AccessKind;
+
 // Creates SemIR to perform a member access with base expression `base_id` and
 // member name `name_id`. When `required`, failing to find the name is a
 // diagnosed error; otherwise, `None` is returned. Returns the result of the
diff --git a/toolchain/check/name_lookup.cpp b/toolchain/check/name_lookup.cpp
@@ -195,7 +195,7 @@ auto LookupNameInExactScope(Context& context, SemIR::LocId loc_id,
 
 // Prints diagnostics on invalid qualified name access.
 static auto DiagnoseInvalidQualifiedNameAccess(
-    Context& context, SemIR::LocId loc_id, SemIR::InstId scope_result_id,
+    Context& context, SemIR::LocId loc_id, SemIR::LocId member_loc_id,
     SemIR::NameId name_id, SemIR::AccessKind access_kind, bool is_parent_access,
     AccessInfo access_info) -> void {
   auto class_type = context.insts().TryGetAs<SemIR::ClassType>(
@@ -231,7 +231,7 @@ static auto DiagnoseInvalidQualifiedNameAccess(
   context.emitter()
       .Build(loc_id, ClassInvalidMemberAccess,
              access_kind == SemIR::AccessKind::Private, name_id, parent_type_id)
-      .Note(scope_result_id, ClassMemberDeclaration)
+      .Note(member_loc_id, ClassMemberDeclaration)
       .Emit();
 }
 
@@ -255,6 +255,17 @@ static auto IsAccessProhibited(std::optional<AccessInfo> access_info,
   }
 }
 
+auto CheckAccess(Context& context, SemIR::LocId loc_id,
+                 SemIR::LocId member_loc_id, SemIR::NameId name_id,
+                 SemIR::AccessKind access_kind, bool is_parent_access,
+                 AccessInfo access_info) -> void {
+  if (IsAccessProhibited(access_info, access_kind, is_parent_access)) {
+    DiagnoseInvalidQualifiedNameAccess(context, loc_id, member_loc_id, name_id,
+                                       access_kind, is_parent_access,
+                                       access_info);
+  }
+}
+
 // Information regarding a prohibited access.
 struct ProhibitedAccessInfo {
   // The resulting inst of the lookup.
@@ -475,9 +486,9 @@ auto LookupQualifiedName(Context& context, SemIR::LocId loc_id,
 
         // Note, `access_info` is guaranteed to have a value here, since
         // `prohibited_accesses` is non-empty.
-        DiagnoseInvalidQualifiedNameAccess(context, loc_id, scope_result_id,
-                                           name_id, access_kind,
-                                           is_parent_access, *access_info);
+        DiagnoseInvalidQualifiedNameAccess(
+            context, loc_id, SemIR::LocId(scope_result_id), name_id,
+            access_kind, is_parent_access, *access_info);
       }
     }
 
diff --git a/toolchain/check/name_lookup.h b/toolchain/check/name_lookup.h
@@ -99,6 +99,13 @@ auto LookupQualifiedName(Context& context, SemIR::LocId loc_id,
 auto LookupNameInCore(Context& context, SemIR::LocId loc_id,
                       llvm::StringRef name) -> SemIR::InstId;
 
+// Checks whether a name is accessible in the given access context. Produces a
+// diagnostic if not.
+auto CheckAccess(Context& context, SemIR::LocId loc_id,
+                 SemIR::LocId member_loc_id, SemIR::NameId name_id,
+                 SemIR::AccessKind access_kind, bool is_parent_access,
+                 AccessInfo access_info) -> void;
+
 // Prints a diagnostic for a duplicate name.
 auto DiagnoseDuplicateName(Context& context, SemIR::NameId name_id,
                            SemIR::LocId dup_def, SemIR::LocId prev_def) -> void;
diff --git a/toolchain/check/testdata/interop/cpp/class/access.carbon b/toolchain/check/testdata/interop/cpp/class/access.carbon
diff --git a/toolchain/check/testdata/interop/cpp/function/overloads.carbon b/toolchain/check/testdata/interop/cpp/function/overloads.carbon
diff --git a/toolchain/sem_ir/cpp_overload_set.h b/toolchain/sem_ir/cpp_overload_set.h

Original file line number	Diff line number	Diff line change
`@@ -206,7 +206,8 @@ auto LookupCppOperator(Context& context, SemIR::LocId loc_id, Operator op,`
`206`	`206`	`}`
`207`	`207`
`208`	`208`	`return ImportCppOverloadSet(context, SemIR::NameScopeId::None,`
`209`		`- SemIR::NameId::CppOperator, functions);`
	`209`	`+ SemIR::NameId::CppOperator,`
	`210`	`+ /naming_class=/nullptr, std::move(functions));`
`210`	`211`	`}`
`211`	`212`
`212`	`213`	`} // namespace Carbon::Check`