C++ Interop: Fix access calculation to handle private member of base classes correctly (#6238)

Always take into account both the lookup access specifier and the
declaration. When set, lookup access specifier takes precedence. When
not set, we have two use cases:
1. This is not a record member, so no access is specified at all. Treat
this as public.
2. This is a record member of a base class. Treat this as private.
[Reference](https://github.com/llvm/llvm-project/blob/4b1d7827c07381610ad4fa7bd9d1a9659008b963/clang/include/clang/AST/DeclCXX.h#L1724).

Also, deduplicate access mapping between import and overload resolution.

Background:
https://github.com/carbon-language/carbon-lang/pull/6221#issuecomment-3407981790

Part of #5859.

toolchain/check/BUILD

@@ -21,6 +21,7 @@ cc_library(
         "context.cpp",
         "control_flow.cpp",
         "convert.cpp",
+        "cpp/access.cpp",
         "cpp/call.cpp",
         "cpp/custom_type_mapping.cpp",
         "cpp/import.cpp",
@@ -72,6 +73,7 @@ cc_library(
         "context.h",
         "control_flow.h",
         "convert.h",
+        "cpp/access.h",
         "cpp/call.h",
         "cpp/custom_type_mapping.h",
         "cpp/import.h",

toolchain/check/cpp/access.cpp

@@ -0,0 +1,47 @@
+// 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
+
+#include "toolchain/check/cpp/access.h"
+
+namespace Carbon::Check {
+
+static auto CalculateEffectiveAccess(clang::DeclAccessPair access_pair)
+    -> clang::AccessSpecifier {
+  // Note that we use `.getAccess()` here, not `->getAccess()`, which is
+  // equivalent to `.getDecl()->getAccess()`, because we want to consider the
+  // lookup access and not the lexical access.
+  switch (access_pair.getAccess()) {
+    // Lookup access takes precedence.
+    case clang::AS_public:
+    case clang::AS_protected:
+    case clang::AS_private:
+      return access_pair.getAccess();
+    case clang::AS_none:
+      // No access specified meaning depends on the declaration. For non class
+      // members, it means there's no access associated with this function so we
+      // treat it as public. For class members it means we lost access along the
+      // inheritance path, and the difference between `none` and `private` only
+      // matters when the access check is performed within a friend or member of
+      // the naming class. Because the naming class is a C++ class, and we don't
+      // yet have a mechanism for a C++ class to befriend a Carbon class, we can
+      // safely map `none` to `private` for now.
+      return access_pair->isCXXClassMember() ? clang::AS_private
+                                             : clang::AS_public;
+  }
+}
+
+auto MapCppAccess(clang::DeclAccessPair access_pair) -> SemIR::AccessKind {
+  switch (CalculateEffectiveAccess(access_pair)) {
+    case clang::AS_public:
+      return SemIR::AccessKind::Public;
+    case clang::AS_protected:
+      return SemIR::AccessKind::Protected;
+    case clang::AS_private:
+      return SemIR::AccessKind::Private;
+    case clang::AS_none:
+      CARBON_FATAL("Couldn't convert access");
+  }
+}
+
+}  // namespace Carbon::Check

toolchain/check/cpp/access.h

@@ -0,0 +1,18 @@
+// 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
+
+#ifndef CARBON_TOOLCHAIN_CHECK_CPP_ACCESS_H_
+#define CARBON_TOOLCHAIN_CHECK_CPP_ACCESS_H_
+
+#include "toolchain/sem_ir/name_scope.h"
+
+namespace Carbon::Check {
+
+// Calculates the effective access kind from the given (declaration, lookup
+// access) pair.
+auto MapCppAccess(clang::DeclAccessPair access_pair) -> SemIR::AccessKind;
+
+}  // namespace Carbon::Check
+
+#endif  // CARBON_TOOLCHAIN_CHECK_CPP_ACCESS_H_

toolchain/check/cpp/import.cpp

@@ -4,6 +4,7 @@
 
 #include "toolchain/check/cpp/import.h"
 
+#include <algorithm>
 #include <memory>
 #include <optional>
 #include <string>
@@ -34,6 +35,7 @@
 #include "toolchain/check/context.h"
 #include "toolchain/check/control_flow.h"
 #include "toolchain/check/convert.h"
+#include "toolchain/check/cpp/access.h"
 #include "toolchain/check/cpp/custom_type_mapping.h"
 #include "toolchain/check/cpp/thunk.h"
 #include "toolchain/check/diagnostic_helpers.h"
@@ -2014,33 +2016,18 @@ auto ImportCppFunctionDecl(Context& context, SemIR::LocId loc_id,
       SemIR::ClangDeclKey::ForFunctionDecl(clang_decl, num_params));
 }
 
-// Maps `clang::AccessSpecifier` to `SemIR::AccessKind`.
-static auto MapAccess(clang::AccessSpecifier access_specifier)
-    -> SemIR::AccessKind {
-  switch (access_specifier) {
-    case clang::AS_public:
-    case clang::AS_none:
-      return SemIR::AccessKind::Public;
-    case clang::AS_protected:
-      return SemIR::AccessKind::Protected;
-    case clang::AS_private:
-      return SemIR::AccessKind::Private;
-  }
-}
-
 // Imports a Clang declaration into Carbon and adds that name into the
 // `NameScope`.
 static auto ImportNameDeclIntoScope(Context& context, SemIR::LocId loc_id,
                                     SemIR::NameScopeId scope_id,
                                     SemIR::NameId name_id,
                                     SemIR::ClangDeclKey key,
-                                    clang::AccessSpecifier access)
+                                    SemIR::AccessKind access_kind)
     -> SemIR::ScopeLookupResult {
   SemIR::InstId inst_id = ImportDeclAndDependencies(context, loc_id, key);
   if (!inst_id.has_value()) {
     return SemIR::ScopeLookupResult::MakeNotFound();
   }
-  SemIR::AccessKind access_kind = MapAccess(access);
   AddNameToScope(context, scope_id, name_id, access_kind, inst_id);
   return SemIR::ScopeLookupResult::MakeWrappedLookupResult(inst_id,
                                                            access_kind);
@@ -2130,16 +2117,14 @@ auto ImportCppOverloadSet(
 // 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) {
-    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();
+  SemIR::AccessKind access_kind = SemIR::AccessKind::Private;
+  for (clang::DeclAccessPair overload : overload_set.pairs()) {
+    access_kind = std::min(access_kind, MapCppAccess(overload));
+    if (access_kind == SemIR::AccessKind::Public) {
+      break;
     }
   }
-  return MapAccess(access);
+  return access_kind;
 }
 
 // Imports an overload set from Clang to Carbon and adds the name into the
@@ -2260,7 +2245,7 @@ auto ImportNameFromCpp(Context& context, SemIR::LocId loc_id,
   }
   auto key = SemIR::ClangDeclKey::ForNonFunctionDecl(lookup->getFoundDecl());
   return ImportNameDeclIntoScope(context, loc_id, scope_id, name_id, key,
-                                 lookup->begin().getAccess());
+                                 MapCppAccess(lookup->begin().getPair()));
 }
 
 auto ImportClassDefinitionForClangDecl(Context& context, SemIR::LocId loc_id,

toolchain/check/cpp/overload_resolution.cpp

@@ -8,6 +8,7 @@
 #include "clang/Sema/Overload.h"
 #include "clang/Sema/Sema.h"
 #include "toolchain/base/kind_switch.h"
+#include "toolchain/check/cpp/access.h"
 #include "toolchain/check/cpp/import.h"
 #include "toolchain/check/cpp/location.h"
 #include "toolchain/check/cpp/operators.h"
@@ -88,22 +89,9 @@ 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;
-    }
+  SemIR::AccessKind member_access_kind = MapCppAccess(overload);
+  if (member_access_kind == SemIR::AccessKind::Public) {
+    return;
   }
 
   auto name_scope_const_id = context.constant_values().Get(