Reverse Interop: Nested namespace support by dwblaikie · Pull Request #6940 · carbon-language/carbon-lang

dwblaikie · 2026-03-19T22:17:47Z

Start recording the clang::DeclContext* -> InstId mapping for use in later operations.

The test update includes removing the initial fail_* test because I hadn't thought about the use of namespace aliases as a way to test for the presence of a namespace without the failure caused by not finding the thing inside the namespace.

toolchain/check/cpp/generate_ast.cpp

jonmeow · 2026-03-19T22:47:59Z

toolchain/check/cpp/generate_ast.cpp

  auto StartTranslationUnit(clang::ASTConsumer* consumer) -> void override;

 private:
+  auto MapInstIdToClangDecl(clang::DeclContext& decl_context,


Pardon if I ask a silly question... Should this class have comments? When is it used and what are its semantics?

Oh, sure. Added some comments in 2d9c34e

// Provides clang ASTs representing Carbon SemIR entities. class CarbonExternalASTSource : public clang::ExternalASTSource { public: explicit CarbonExternalASTSource(Context* context, clang::ASTContext* ast_context) : context_(context), ast_context_(ast_context) {} // Look up decls for `decl_name` inside `decl_context`, adding the decls to // `decl_context`. Returns true if any decls were added. auto FindExternalVisibleDeclsByName(

Commenting StartTranslationUnit didn't seem especially worthwhile - the contract is fairly self-explanatory, and it doesn't seem helpful to comment the current implementation compared to reading it & it'd get out of date/just encode whatever we do in there, not some broader contract that I can think of.

FWIW, maybe just "// See ExternalASTSource" -- I'd missed the override at the end

Sure bd9bd21

toolchain/check/cpp/generate_ast.cpp

jonmeow · 2026-03-19T22:58:52Z

toolchain/check/cpp/generate_ast.cpp

+  auto decl_context_inst_id =
+      scope_mapping.Lookup(const_cast<clang::DeclContext*>(decl_context));


I'm a bit confused by the call order here, can you document what the behavior is here?

A map lookup seems like it'd be expensive versus the logic on lines 400-408, am I thinking about the relative cost wrong? Why do a map lookup if the decl kind isn't a TU, and thus won't be inserted into the map?

The Insert below is inserting carbon_cpp_namespace, so it seems like there's nothing mapping to the decl_context being looked up here; what am I missing?

I'm a bit confused by the call order here, can you document what the behavior is here?

Added this comment:

// If the context doesn't already have a mapping between C++ and Carbon, // check if this is the root mapping (for the "Carbon" namespace in the // translation unit scope) and if so, create that mapping.

in f84df3c but maybe it needs more than that? (see how the rest of the words in this comment go - if some of those would be worth adding to the code, or different ones)

A map lookup seems like it'd be expensive versus the logic on lines 400-408, am I thinking about the relative cost wrong? Why do a map lookup if the decl kind isn't a TU, and thus won't be inserted into the map?

You're probably right about the relative cost - though this seemed to me like a clearer way to express the intent of the code in terms of readability. Not sure though, perhaps there's a similarly readable alternative.

The idea is that we only map things into contexts we've previously mapped - except for the root/"Carbon" namespace (it's the start of the inductive reasoning).

The Insert below is inserting carbon_cpp_namespace, so it seems like there's nothing mapping to the decl_context being looked up here; what am I missing?

Let's say you've got some C++ naming Carbon::Foo::Bar.

Initially Clang asks whether Carbon knows about anything called "Carbon" in the global namespace, we say yes, construct a new NamespaceDecl and record our internal mapping from Package to "Carbon" namespace.

Next Clang comes back and asks if we know anything about a "Foo" in the Carbon NamespaceDecl. We look up in the map, see that the Carbon NamespaceDecl has an associated InstId, so we use that InstId to lookup "Foo" on the carbon side, find it's another namespace, create another mapping from the new "Foo" NamespaceDecl to the carbon InstId for that namespace. Repeat as needed.

You're probably right about the relative cost - though this seemed to me like a clearer way to express the intent of the code in terms of readability. Not sure though, perhaps there's a similarly readable alternative.

What kinds of C++ name lookups would this occur in? That's really important for how heavily I'd suggest weighing performance. e.g. if somebody writes x (not namespace qualified), ideally that doesn't result in any map lookups here unless it's in some namespace Carbon { ... x ... }

Let's say you've got some C++ naming Carbon::Foo::Bar.

Thanks for this explanation, I was misunderstanding the code. :)

Sorry, one more connected thought -- if this is called frequently, then given an input C++ decl and a desire to avoid maps, I don't know if walking up parent contexts to see if Carbon is a parent would be faster than a map lookup on the whole. But that's kind of stray comment, maybe TODO to try territory

The name lookups we should get are for any scope we say could have external things - which is the global/TU scope, and the nested scopes we create. The nested ones we know will have a map lookup - but we can avoid the global one if we store whether we've already initialized the "Carbon" namespace (if we have, then any other query for any name in the global scope should fail - we don't have any other names to provide there)

Maybe 801785e is something roughly right for now?

Ultimately, yeah, we might end up doing a parent context walk (once we figure out how to handle Clang Modules support (that also uses an ExternalASTSource) along with Carbon - then we'd get more callbacks for other scopes that aren't the ones we care about (ones that have names in modules)) in the future, but not sure it's worth a TODO - could add one if you like.

toolchain/check/testdata/interop/cpp/reverse/simple.carbon

toolchain/check/cpp/generate_ast.cpp

This doesn't feel like the nicest way to do it (adding the check in 2 different places per lookup function) but since it has to wait until after the insertion callback has constructed the key, with the current structure I don't see a great alternative. The return operation could be restructured into a function that DCHECKs and then constructs the key value result type, and some of this code could be commoned into an intermediate base (between BaseImpl and MapBase/SetBase)? This only tests one of the 6 checks introduced in this patch, because death tests are especially expensive (partly innate, and partly due to the cost of symbolizing failures - if we figure out how to disable the symbolizer in death tests (llvm has this problem too/could benefit from a solution) then maybe we could more comprehensively test these paths) (the test could be made more explicit by using custom types that intentionally hash differently - but that seems more verbose/perhaps harder to follow that relying on the pointer conversion that was where the problem was initially observed) Discovered while working on carbon-language#6940

jonmeow

LG, thanks for the comments!

toolchain/check/cpp/generate_ast.cpp

jonmeow · 2026-03-23T22:31:14Z

toolchain/check/cpp/generate_ast.cpp

  auto StartTranslationUnit(clang::ASTConsumer* consumer) -> void override;

 private:
+  auto MapInstIdToClangDecl(clang::DeclContext& decl_context,


FWIW, maybe just "// See ExternalASTSource" -- I'd missed the override at the end

toolchain/check/cpp/generate_ast.cpp

jonmeow · 2026-03-23T22:47:29Z

toolchain/check/cpp/generate_ast.cpp

+  auto decl_context_inst_id =
+      scope_mapping.Lookup(const_cast<clang::DeclContext*>(decl_context));


You're probably right about the relative cost - though this seemed to me like a clearer way to express the intent of the code in terms of readability. Not sure though, perhaps there's a similarly readable alternative.

What kinds of C++ name lookups would this occur in? That's really important for how heavily I'd suggest weighing performance. e.g. if somebody writes x (not namespace qualified), ideally that doesn't result in any map lookups here unless it's in some namespace Carbon { ... x ... }

Let's say you've got some C++ naming Carbon::Foo::Bar.

Thanks for this explanation, I was misunderstanding the code. :)

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

zygoloid

My comments have been addressed; I'm happy whenever jonmeow is. (I see that dwblaikie rerequested review from Jon so I'm not just merging.)

jonmeow

LG, the CHECK on decl_context_inst_id especially helps me.

Generate class declarations for Carbon classes referenced from C++ Based on #6940, review from eb62070 onwards

Reverse Interop: Nested namespace support

38ae676

dwblaikie requested a review from a team as a code owner March 19, 2026 22:17

dwblaikie requested review from jonmeow and removed request for a team March 19, 2026 22:17

github-actions bot added the toolchain label Mar 19, 2026

zygoloid reviewed Mar 19, 2026

View reviewed changes

toolchain/check/cpp/generate_ast.cpp Outdated Show resolved Hide resolved

toolchain/check/cpp/generate_ast.cpp Outdated Show resolved Hide resolved

jonmeow reviewed Mar 19, 2026

View reviewed changes

toolchain/check/cpp/generate_ast.cpp Show resolved Hide resolved

dwblaikie mentioned this pull request Mar 23, 2026

Verify heterogeneous keys hash to the same value in set/map #6950

Open

dwblaikie added 10 commits March 23, 2026 06:55

Move comment from member function definition to declaration

85dd780

Document the null return from the mapping function

ba6e008

CHECK insertions insert

127ace8

Add comment to clarify the creation of the root "Carbon" namespace

f84df3c

Remove unnecessary const_cast

2669787

Use primary context for mapping

edbc565

Add test coverage for multiply nested namespaces

7042795

scope_mapping -> scope_map_

b2252e0

Comment scope_map_

2c189f0

Add some comments to CarbonExternalASTSource

2d9c34e

dwblaikie requested review from jonmeow and zygoloid March 23, 2026 22:06

jonmeow approved these changes Mar 23, 2026

View reviewed changes

dwblaikie and others added 4 commits March 23, 2026 16:44

Update toolchain/check/cpp/generate_ast.cpp

bf8c94f

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

Update toolchain/check/cpp/generate_ast.cpp

3d7883a

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

Add comment for StartTranslationUnit

bd9bd21

Avoid map lookups at the global scope

801785e

dwblaikie requested a review from jonmeow March 24, 2026 02:33

dwblaikie mentioned this pull request Mar 24, 2026

Reverse Interop: Class declarations #6955

Merged

zygoloid reviewed Mar 24, 2026

View reviewed changes

jonmeow approved these changes Mar 24, 2026

View reviewed changes

jonmeow added this pull request to the merge queue Mar 24, 2026

Merged via the queue into carbon-language:trunk with commit 2af5f97 Mar 24, 2026
8 checks passed

github-merge-queue bot pushed a commit that referenced this pull request Mar 25, 2026

Reverse Interop: Class declarations (#6955)

415cd6f

Generate class declarations for Carbon classes referenced from C++ Based on #6940, review from eb62070 onwards

		auto decl_context_inst_id =
		scope_mapping.Lookup(const_cast<clang::DeclContext*>(decl_context));

Conversation

dwblaikie commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonmeow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zygoloid left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonmeow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dwblaikie commented Mar 19, 2026 •

edited

Loading

zygoloid left a comment •

edited

Loading