Add support for importing a trivial global C++ function #5033

bricknerb · 2025-02-27T15:57:02Z

ASTUnit is owned by CompileSubcommand, passed through Unit to be populated in ImportCppFiles() and used via SemIR::File.
When generating the AST, pass -x c++ args to compile C++ (temporary until we pass args properly).
Cpp namespace is marked as a special namespace and has dedicated logic in LookupNameInExactScope().
The logic for importing declarations from C++ to Carbon is in import_cpp.cpp, but we're likely to want to refactor this signfiicantly over time as it grows (perhaps a dedicated directory?).

Part of #4666.

ASTUnit is owned by `CompileSubcommand`, passed through `Unit` to be populated in `ImportCppFiles()` and used via `SemIR::File`. When generating the AST, pass `-x c++` args to compile C++ (temporary until we pass args properly). `Cpp` namespace is marked as a special namespace and has dedicated logic in `LookupNameInExactScope()`. The logic for importing declarations from C++ to Carbon is in `import_cpp.cpp`, but we're likely to want to refactor this signfiicantly over time as it grows (perhaps a dedicated directory?).

zygoloid

Very exciting, this looks like a great step :)

zygoloid · 2025-02-27T22:53:38Z

toolchain/check/import_cpp.cpp

+  clang::Sema& sema = ast->getSema();
+
+  auto name = context.names().GetAsStringIfIdentifier(name_id);
+  CARBON_CHECK(name);


What happens if we try to look up a non-identifier Carbon name, such as via Cpp.Self or Cpp.base? Perhaps we should produce a lookup failure for non-identifier names for now.

Added a test and now using GetIRBaseName().

My general understanding is we want to require raw keyword syntax in this sort of situation: i.e., Cpp.r#base, with Cpp.base being an error. So GetAsStringIfIdentifier was correct, rather than GetIRBaseName (which I think mainly exists for debug output).

jonmeow

Similarly, nice to see progress here! I realize I missed the publish button though.

toolchain/sem_ir/file.h

toolchain/check/name_lookup.cpp

toolchain/check/import_cpp.cpp

toolchain/sem_ir/file.h

toolchain/check/import_cpp.cpp

…ted, return error `InstId` to avoid diagnosing on name not found

It's not initialized everywhere and no warning is given. https://github.com/carbon-language/carbon-lang/blob/43b9969058b3de2e0b05896ef8e4489c1dc85cb6/toolchain/language_server/context.cpp#L146

toolchain/sem_ir/file.h

… value as a parameter

toolchain/check/testdata/interop/cpp/no_prelude/function_decl.carbon

jonmeow

If you want to address final comments and merge, that seems fine. Note I've suggested a significant change not to use GetIRBaseName (explanation: #5033 (comment))

jonmeow · 2025-02-28T15:58:59Z

toolchain/check/import_cpp.cpp

+      {// Parse C++ (and not C)
+       "-x", "c++"},


Suggested change

{// Parse C++ (and not C)

"-x", "c++"},

// Parse C++ (and not C).

{"-x", "c++"},

Per style: "Only use line comments (with //, not /* ... */), on a line by themselves"
https://github.com/carbon-language/carbon-lang/blob/trunk/docs/project/cpp_style_guide.md

jonmeow · 2025-02-28T16:04:02Z

toolchain/check/import_cpp.cpp

+  clang::Sema& sema = ast->getSema();
+
+  auto name = context.names().GetAsStringIfIdentifier(name_id);
+  CARBON_CHECK(name);


My general understanding is we want to require raw keyword syntax in this sort of situation: i.e., Cpp.r#base, with Cpp.base being an error. So GetAsStringIfIdentifier was correct, rather than GetIRBaseName (which I think mainly exists for debug output).

jonmeow · 2025-02-28T16:07:21Z

toolchain/check/import_cpp.cpp

+  return std::move(generated_ast);
+}
+
+// Lookups the given name in the Clang AST. Returns the lookup result if lookup


Suggested change

// Lookups the given name in the Clang AST. Returns the lookup result if lookup

// Looks up the given name in the Clang AST. Returns the lookup result if lookup

Small grammar thing.

toolchain/check/import_cpp.cpp

jonmeow · 2025-02-28T16:22:24Z

toolchain/check/import_cpp.cpp

+      lookup, ast->getASTContext().getTranslationUnitDecl());
+
+  if (lookup.isClassLookup()) {
+    // TODO: When support class lookup, also check access.


Suggested change

// TODO: When support class lookup, also check access.

// TODO: To support class lookup, also return the AccessKind for storage.

To be sure, access can't be checked here; that could lead to something like a protected member being incorrectly hidden, when the caller doesn't have access. Instead the AccessKind needs to be part of the returned information so that it can be stored in the lookup table (instead of AccessKind::Public), and then used by lookup to determine access.

(also, small grammar thing -- "to support" or "when supporting")

jonmeow · 2025-02-28T16:23:26Z

toolchain/check/import_cpp.cpp

+
+  if (lookup.isClassLookup()) {
+    // TODO: When support class lookup, also check access.
+    context.TODO(loc_id, "Unsupported: Lookup in Class");


Maybe (given the "unsupported" string):

Suggested change

context.TODO(loc_id, "Unsupported: Lookup in Class");

context.TODO(loc_id, "Unsupported: Lookup in Class");

return std::nullopt;

jonmeow · 2025-02-28T16:29:45Z

toolchain/check/import_cpp.cpp

+  CARBON_CHECK(ast);
+  clang::Sema& sema = ast->getSema();
+
+  llvm::StringRef name = context.names().GetIRBaseName(name_id);


Suggested change

llvm::StringRef name = context.names().GetIRBaseName(name_id);

std::optional<llvm::StringRef> name = context.names().GetAsStringIfIdentifier(name_id);

if (!name) {

// Special names never exist in C++ code.

return std::nullopt;

}

(see comment)

Done.
I thought of using a dedicated error for this use case to point out this is a special name (and maybe even tell the user how to fix), but keeping as is.
See 16867c9

…tead.

bricknerb · 2025-03-03T09:23:12Z

If you want to address final comments and merge, that seems fine. Note I've suggested a significant change not to use GetIRBaseName (explanation: #5033 (comment))

Thanks!
I've addressed the final comments and will try to merge.

github-actions bot added the toolchain label Feb 27, 2025

github-actions bot requested a review from zygoloid February 27, 2025 15:57

bricknerb changed the title ~~Add support for import a trivial global C++ function~~ Add support for importing a trivial global C++ function Feb 27, 2025

zygoloid approved these changes Feb 27, 2025

View reviewed changes

jonmeow reviewed Feb 27, 2025

View reviewed changes

bricknerb added 7 commits February 28, 2025 11:47

Fix formatting

afd4441

Style: Avoid typename in braced initialization

a1cf815

When finding a name in Cpp, but the found declaration is not suppor…

ddb345b

…ted, return error `InstId` to avoid diagnosing on name not found

Add a TODO to remove File::set_cpp_ast()

2ffec0e

TODO for isClassLookup()

b6b93aa

Add function comments

a216d6c

Initialize Unit.cpp_ast to null by default.

b12945e

It's not initialized everywhere and no warning is given. https://github.com/carbon-language/carbon-lang/blob/43b9969058b3de2e0b05896ef8e4489c1dc85cb6/toolchain/language_server/context.cpp#L146

bricknerb commented Feb 28, 2025

View reviewed changes

toolchain/sem_ir/file.h Show resolved Hide resolved

bricknerb added 6 commits February 28, 2025 15:10

Change ImportCppFiles() to return the AST instead of using a return…

b369be6

… value as a parameter

Improve File::cpp_ast_ comment

63701ab

Remove unnecessary CARBON_CHECK

3ec0ec8

Handle special names by using GetIRBaseName()

29e1548

Add a test for using a the wrong name when calling a Cpp function

30881c1

Update test name to include fail_

b7e5bc4

bricknerb commented Feb 28, 2025

View reviewed changes

toolchain/check/testdata/interop/cpp/no_prelude/function_decl.carbon Show resolved Hide resolved

bricknerb requested review from zygoloid and jonmeow February 28, 2025 14:28

jonmeow approved these changes Feb 28, 2025

View reviewed changes

bricknerb added 4 commits March 3, 2025 09:46

Improve class lookup TODO

0384049

Do not allow using special names in Cpp. Use raw keyword syntax ins…

2c59d1d

…tead.

Fix style and grammar.

4cf320d

Treat special names inside Cpp as not existing

16867c9

bricknerb enabled auto-merge March 3, 2025 09:23

bricknerb added this pull request to the merge queue Mar 3, 2025

Merged via the queue into carbon-language:trunk with commit 87b9cab Mar 3, 2025
8 checks passed

bricknerb deleted the interop2 branch March 3, 2025 11:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for importing a trivial global C++ function #5033

Add support for importing a trivial global C++ function #5033

bricknerb commented Feb 27, 2025 •

edited

Loading

zygoloid left a comment

zygoloid Feb 27, 2025

bricknerb Feb 28, 2025

jonmeow Feb 28, 2025

bricknerb Mar 3, 2025

jonmeow left a comment

jonmeow left a comment

jonmeow Feb 28, 2025

bricknerb Mar 3, 2025

jonmeow Feb 28, 2025

jonmeow Feb 28, 2025

bricknerb Mar 3, 2025

jonmeow Feb 28, 2025

bricknerb Mar 3, 2025

jonmeow Feb 28, 2025

bricknerb Mar 3, 2025

jonmeow Feb 28, 2025

bricknerb Mar 3, 2025

bricknerb commented Mar 3, 2025

	// Lookups the given name in the Clang AST. Returns the lookup result if lookup
	// Looks up the given name in the Clang AST. Returns the lookup result if lookup

	// TODO: When support class lookup, also check access.
	// TODO: To support class lookup, also return the AccessKind for storage.

	context.TODO(loc_id, "Unsupported: Lookup in Class");
	context.TODO(loc_id, "Unsupported: Lookup in Class");
	return std::nullopt;

-  llvm::StringRef name = context.names().GetIRBaseName(name_id);
+  std::optional<llvm::StringRef> name = context.names().GetAsStringIfIdentifier(name_id);
+  if (!name) {
+    // Special names never exist in C++ code.
+    return std::nullopt;
+  }

Add support for importing a trivial global C++ function #5033

Add support for importing a trivial global C++ function #5033

Conversation

bricknerb commented Feb 27, 2025 • edited Loading

zygoloid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow left a comment

Choose a reason for hiding this comment

jonmeow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bricknerb commented Mar 3, 2025

bricknerb commented Feb 27, 2025 •

edited

Loading