Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Oct 21, 2025
+1,971 −198
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
97eef9b
C++ Interop: Fix access deduction
bricknerb Oct 17, 2025
d18b5f4
Rebane `DeduceClangAccess()` to `ConvertCppAccess()`.
bricknerb Oct 20, 2025
c296cd8
Fix access calculation to support layered inheritance
bricknerb Oct 20, 2025
018dca8
Merge tests
bricknerb Oct 20, 2025
7bd83dc
Move coverage for public base class.
bricknerb Oct 20, 2025
311a45c
Move coverage for protected base class.
bricknerb Oct 20, 2025
6fc59fb
Move coverage for private base class.
bricknerb Oct 20, 2025
db3e0f1
Move coverage for public base class of a private base class.
bricknerb Oct 20, 2025
15318cb
Coverage for public base class of a protected base class.
bricknerb Oct 20, 2025
9dad5ab
Use `UnresolvedSetIterator::getPair()`.
bricknerb Oct 20, 2025
6bdde04
Simplify iterating over overload set to calculate access.
bricknerb Oct 20, 2025
37ee14b
Move the public shortcut when iterating over overload set to a separt…
bricknerb Oct 20, 2025
3bd7827
Iterate over `DeclAccessPair` and remove `MapAccess`.
bricknerb Oct 20, 2025
62d4dcb
Add a comment on why we use `.getAccess()` and not `->getAccess()`.
bricknerb Oct 20, 2025
a2a464f
Remove `Derived` suffix from class names to be consistent and concise…
bricknerb Oct 20, 2025
7593a35
Rename `foo()` to `Overload()`.
bricknerb Oct 20, 2025
531527d
Use explicit parameter types to distinguish between the `public`, `pr…
bricknerb Oct 20, 2025
41b3742
Rename `disallowed` to `denied`.
bricknerb Oct 20, 2025
dbb71ad
Merge branch 'main' into none
bricknerb Oct 20, 2025
d3b0cf6
Rename `ConvertCppAccess()` to `MapCppAccess()`.
bricknerb Oct 21, 2025
cbe8304
Clarify that the difference between private and none for class member…
bricknerb Oct 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions toolchain/check/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
44 changes: 44 additions & 0 deletions toolchain/check/cpp/access.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// 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::AccessSpecifier lookup_access,
clang::AccessSpecifier lexical_access)
-> clang::AccessSpecifier {
if (lookup_access != clang::AS_none) {
// Lookup access takes precedence.
return lookup_access;
}

if (lexical_access != clang::AS_none) {
// When a base class private member is accessed through a derived class, the
// lookup access would be set to `AS_none`.
CARBON_CHECK(lexical_access == clang::AS_private);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this CHECK correct? I've suggested a test which I think may hit it.

If it's not correct, perhaps this function could be replaced with something like:

auto DeduceClangAccess(clang::AccessSpecifier lookup_access,
                       clang::AccessSpecifier lexical_access)
    -> SemIR::AccessKind {
  // Lookup access takes precedence.
  switch (lookup_access != clang::AS_none ? lookup_access : lexical_access) {
    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:
      // This is not a record member.
      return SemIR::AccessKind::Public;
  }
}

That also is pretty short, and if you're not expecting many other access functions, perhaps could be inlined into the header to remove access.cpp.

Copy link
Contributor

@jonmeow jonmeow Oct 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, while equivalent [ed: for current tests, the suggested] tests might also suggest this still should have some TODOs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, this check was incorrect.
I've added more tests based on your suggestions and adjusted the logic to make it correct for these cases.

return lexical_access;
}

// No access specified means that this is not a record member, so we treat it
// as public.
return clang::AS_public;
}

auto DeduceClangAccess(clang::AccessSpecifier lookup_access,
clang::AccessSpecifier lexical_access)
-> SemIR::AccessKind {
switch (CalculateEffectiveAccess(lookup_access, lexical_access)) {
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 deduce access");
}
}

} // namespace Carbon::Check
20 changes: 20 additions & 0 deletions toolchain/check/cpp/access.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// 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 {

// Deduces the effective access kind from the given lookup and lexical access
// specifiers.
auto DeduceClangAccess(clang::AccessSpecifier lookup_access,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've generally been saying "Deduce" in the toolchain specifically for generic deduction. Also, skimming through the cpp headers, don't they typically say "Cpp" instead of "Clang"?

Perhaps there's a better term here, because it's just doing a combination of the two parameters. Had you considered something like "ConvertCppAccess"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

clang::AccessSpecifier lexical_access)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like both callers could pass in the access specifiers as a DeclAccessPair. Had you considered taking that here? That might even let you just remove MapAccess from import.cpp, since there's it.getPair()

Copy link
Contributor Author

@bricknerb bricknerb Oct 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

-> SemIR::AccessKind;

} // namespace Carbon::Check

#endif // CARBON_TOOLCHAIN_CHECK_CPP_ACCESS_H_
36 changes: 13 additions & 23 deletions toolchain/check/cpp/import.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "toolchain/check/cpp/import.h"

#include <algorithm>
#include <memory>
#include <optional>
#include <string>
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -2014,18 +2016,10 @@ 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)
// Deduces the access given UnresolvedSetIterator.
static auto MapAccess(clang::UnresolvedSetIterator iterator)
-> 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;
}
return DeduceClangAccess(iterator.getAccess(), iterator->getAccess());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference between .getAccess and ->getAccess seems subtle, perhaps document what's occurring here where you do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer relevant here anymore.
Added a comment in CalculateEffectiveAccess().

}

// Imports a Clang declaration into Carbon and adds that name into the
Expand All @@ -2034,13 +2028,12 @@ 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);
Expand Down Expand Up @@ -2130,16 +2123,13 @@ 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 = MapAccess(overload_set.begin());
for (auto it = overload_set.begin() + 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had you considered initializing access_kind to Private, so that you wouldn't need this begin() offset and could only call MapAccess from inside the loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

it != overload_set.end() && access_kind != SemIR::AccessKind::Public;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd typically expect a for loop to focus on comparing the value it's iterating over (it), with other comparisons inside the loop. What made you choose to do this comparison as part of the for loop parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the old code it made sense to me to avoid a break.
Moved the optimization to an explicit condition with a break.

++it) {
access_kind = std::min(access_kind, MapAccess(it));
}
return MapAccess(access);
return access_kind;
}

// Imports an overload set from Clang to Carbon and adds the name into the
Expand Down Expand Up @@ -2260,7 +2250,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());
MapAccess(lookup->begin()));
}

auto ImportClassDefinitionForClangDecl(Context& context, SemIR::LocId loc_id,
Expand Down
21 changes: 5 additions & 16 deletions toolchain/check/cpp/overload_resolution.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -88,22 +89,10 @@ 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 =
DeduceClangAccess(overload.getAccess(), overload->getAccess());
if (member_access_kind == SemIR::AccessKind::Public) {
return;
}

auto name_scope_const_id = context.constant_values().Get(
Expand Down
Loading
Loading