From 6e9e871b74561fb4ad145295e448d260b75c1273 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Fri, 31 Jul 2026 13:09:03 -0700 Subject: [PATCH] Fix handling of recursive macros. (#7593) Inject the name of a macro rather than its contents when computing its expansion. If the macro refers to itself, it will not expand within its own body, rather than expanding once. Switching from `EnterTokenStream` to `EnterToken` exposed that our Clang preprocessing environment was a little broken -- we reached the end of the primary source file and starting tearing stuff down before we actually finished parsing, which we were mostly getting away with before but aren't any more. Enabled Clang's incremental processing mode to fix this. This causes Clang to remain in the main source file when it reaches EOF instead of popping it. This also causes the diagnostics for invalid `module;` declarations to change, but in a way that seems not really any worse than before. Also slightly changes the diagnostics produced from macro expansion failures. The new diagnostics are a bit more precise -- they now capture the outermost level of macro expansion -- but we don't do a good job of rendering the Clang snippet attached to the "in macro expansion" context note yet, so the context looks a bit weird: we get two different snippets attached to the same diagnostic. --- toolchain/check/cpp/generate_ast.cpp | 3 +- toolchain/check/cpp/import.cpp | 6 ++- toolchain/check/cpp/macros.cpp | 52 +++++++++---------- toolchain/check/cpp/macros.h | 5 +- toolchain/check/diagnostic_emitter.cpp | 2 + .../interop/cpp/basics/inline/modules.carbon | 28 +++++++--- .../testdata/interop/cpp/macros/macros.carbon | 41 ++++++++++----- 7 files changed, 86 insertions(+), 51 deletions(-) diff --git a/toolchain/check/cpp/generate_ast.cpp b/toolchain/check/cpp/generate_ast.cpp index 31b0353fa5e0..ff3efedeacc2 100644 --- a/toolchain/check/cpp/generate_ast.cpp +++ b/toolchain/check/cpp/generate_ast.cpp @@ -681,8 +681,6 @@ class GenerateASTAction : public clang::ASTFrontendAction { auto BeginSourceFileAction(clang::CompilerInstance& /*clang_instance*/) -> bool override { - // TODO: `clang.getPreprocessor().enableIncrementalProcessing();` to avoid - // the TU scope getting torn down before we're done parsing macros. return true; } @@ -699,6 +697,7 @@ class GenerateASTAction : public clang::ASTFrontendAction { clang_instance.getSema(), /*SkipFunctionBodies=*/false); + clang_instance.getPreprocessor().enableIncrementalProcessing(); clang_instance.getPreprocessor().EnterMainSourceFile(); parser_->Initialize(); diff --git a/toolchain/check/cpp/import.cpp b/toolchain/check/cpp/import.cpp index 90fcc9041efe..8281bf66e4d8 100644 --- a/toolchain/check/cpp/import.cpp +++ b/toolchain/check/cpp/import.cpp @@ -2502,9 +2502,10 @@ static auto IsIncompleteClass(Context& context, SemIR::NameScopeId scope_id) // TODO: Add support for other macro types and non-integer literal values. static auto ImportMacro(Context& context, SemIR::LocId loc_id, SemIR::NameScopeId scope_id, SemIR::NameId name_id, + clang::IdentifierInfo* identifier_info, clang::MacroInfo* macro_info) -> SemIR::ScopeLookupResult { - auto inst_id = TryEvaluateMacro(context, loc_id, name_id, macro_info); + auto inst_id = TryEvaluateMacro(context, loc_id, identifier_info, macro_info); if (inst_id == SemIR::ErrorInst::InstId) { return SemIR::ScopeLookupResult::MakeNotFound(); } @@ -2570,7 +2571,8 @@ auto ImportNameFromCpp(Context& context, SemIR::LocId loc_id, if (clang::MacroInfo* macro_info = LookupMacro(context, scope_id, identifier_info)) { - return ImportMacro(context, loc_id, scope_id, name_id, macro_info); + return ImportMacro(context, loc_id, scope_id, name_id, identifier_info, + macro_info); } auto lookup = ClangLookupName(context, scope_id, identifier_info); if (!lookup) { diff --git a/toolchain/check/cpp/macros.cpp b/toolchain/check/cpp/macros.cpp index 8267af39c8cd..a344e21fd0a8 100644 --- a/toolchain/check/cpp/macros.cpp +++ b/toolchain/check/cpp/macros.cpp @@ -11,7 +11,9 @@ #include "clang/Sema/Sema.h" #include "common/check.h" #include "toolchain/check/cpp/constant.h" +#include "toolchain/check/cpp/generate_ast.h" #include "toolchain/check/cpp/import.h" +#include "toolchain/check/cpp/location.h" #include "toolchain/check/literal.h" namespace Carbon::Check { @@ -46,52 +48,49 @@ static auto MapConstant(Context& context, SemIR::LocId loc_id, } auto TryEvaluateMacro(Context& context, SemIR::LocId loc_id, - SemIR::NameId name_id, clang::MacroInfo* macro_info) - -> SemIR::InstId { - auto name_str_opt = context.names().GetAsStringIfIdentifier(name_id); + clang::IdentifierInfo* identifier_info, + clang::MacroInfo* macro_info) -> SemIR::InstId { CARBON_CHECK(macro_info, "macro info missing"); - if (macro_info->getNumTokens() == 0) { context.TODO(loc_id, "Unsupported: macro with 0 replacement tokens"); return SemIR::ErrorInst::InstId; } - clang::Sema& sema = context.clang_sema(); - clang::Preprocessor& preprocessor = sema.getPreprocessor(); + auto& ast_context = context.cpp_context()->ast_context(); auto& parser = context.cpp_context()->parser(); + auto& preprocessor = context.cpp_context()->sema().getPreprocessor(); - llvm::SmallVector tokens(macro_info->tokens().begin(), - macro_info->tokens().end()); - - clang::Token current_token = parser.getCurToken(); - - // Add eof token - clang::Token eof; - eof.startToken(); - eof.setKind(clang::tok::eof); - eof.setLocation(current_token.getEndLoc()); - tokens.push_back(eof); - - tokens.push_back(current_token); - - preprocessor.EnterTokenStream(tokens, /*DisableMacroExpansion=*/false, + // Enter the macro name, not the macro body, so we properly suppress recursive + // expansion. + clang::Token macro_name[1]; + macro_name[0].startToken(); + macro_name[0].setKind(clang::tok::identifier); + macro_name[0].setIdentifierInfo(identifier_info); + macro_name[0].setLocation(GetCppLocation(context, loc_id)); + preprocessor.EnterTokenStream(macro_name, /*DisableMacroExpansion=*/false, /*IsReinject=*/false); - parser.ConsumeAnyToken(true); + + CARBON_CHECK(parser.getCurToken().is(clang::tok::eof)); + parser.ConsumeToken(); clang::ExprResult result = parser.ParseConstantExpression(); - clang::Expr* result_expr = result.get(); + // Consume any remaining tokens to advance the preprocessor and parser past + // the end of the macro. bool success = !result.isInvalid() && parser.getCurToken().is(clang::tok::eof); + while (!parser.getCurToken().is(clang::tok::eof)) { + parser.ConsumeAnyToken(true); + } if (!success) { - parser.SkipUntil(clang::tok::eof); CARBON_DIAGNOSTIC( InCppMacroEvaluation, Error, "failed to parse macro Cpp.{0} to a valid constant expression", std::string); - context.emitter().Emit(loc_id, InCppMacroEvaluation, (*name_str_opt).str()); + context.emitter().Emit(loc_id, InCppMacroEvaluation, + identifier_info->getName().str()); return SemIR::ErrorInst::InstId; } @@ -103,8 +102,7 @@ auto TryEvaluateMacro(Context& context, SemIR::LocId loc_id, } clang::Expr::EvalResult evaluated_result; - if (!result_expr->EvaluateAsConstantExpr(evaluated_result, - sema.getASTContext())) { + if (!result_expr->EvaluateAsConstantExpr(evaluated_result, ast_context)) { CARBON_FATAL("failed to evaluate macro as constant expression"); } diff --git a/toolchain/check/cpp/macros.h b/toolchain/check/cpp/macros.h index dbf046770852..c68129d6e102 100644 --- a/toolchain/check/cpp/macros.h +++ b/toolchain/check/cpp/macros.h @@ -8,6 +8,7 @@ #include "toolchain/check/context.h" namespace clang { +class IdentifierInfo; class MacroInfo; } // namespace clang @@ -17,8 +18,8 @@ namespace Carbon::Check { // constant if possible. Returns an `InstId` on success or // `SemIR::ErrorInst::InstId` otherwise. auto TryEvaluateMacro(Context& context, SemIR::LocId loc_id, - SemIR::NameId name_id, clang::MacroInfo* macro_info) - -> SemIR::InstId; + clang::IdentifierInfo* identifier_info, + clang::MacroInfo* macro_info) -> SemIR::InstId; } // namespace Carbon::Check diff --git a/toolchain/check/diagnostic_emitter.cpp b/toolchain/check/diagnostic_emitter.cpp index d7152e6d009c..0c2070167fa6 100644 --- a/toolchain/check/diagnostic_emitter.cpp +++ b/toolchain/check/diagnostic_emitter.cpp @@ -43,6 +43,8 @@ auto DiagnosticEmitter::ConvertLoc(LocIdForDiagnostics loc_id, break; case Carbon::SemIR::DiagnosticLocConverter::ImportLoc::CppMacroExpansion: // TODO: Include the macro name in the note. + // TODO: Include the Clang-generated snippet here rather than with the + // main diagnostic. context_fn(import.loc, InCppMacroExpansion); break; } diff --git a/toolchain/check/testdata/interop/cpp/basics/inline/modules.carbon b/toolchain/check/testdata/interop/cpp/basics/inline/modules.carbon index d5d2b2886784..5620d4464558 100644 --- a/toolchain/check/testdata/interop/cpp/basics/inline/modules.carbon +++ b/toolchain/check/testdata/interop/cpp/basics/inline/modules.carbon @@ -17,8 +17,12 @@ library "[[@TEST_NAME]]"; // TODO: This diagnostic isn't very good. import Cpp inline ''' +// CHECK:STDERR: fail_export_module_in_inline_cpp.carbon:[[@LINE+11]]:8: error: module declaration must not come from an #include directive [CppInteropParseError] +// CHECK:STDERR: 17 | export module Foo; +// CHECK:STDERR: | ^~~~~~~~~~~ +// CHECK:STDERR: // CHECK:STDERR: fail_export_module_in_inline_cpp.carbon:[[@LINE+7]]:8: error: module declaration must occur at the start of the translation unit [CppInteropParseError] -// CHECK:STDERR: 13 | export module Foo; +// CHECK:STDERR: 17 | export module Foo; // CHECK:STDERR: | ^ // CHECK:STDERR: :1:1: note: add 'module;' to the start of the file to introduce a global module fragment [CppInteropParseNote] // CHECK:STDERR: 1 | ; @@ -33,15 +37,19 @@ library "[[@TEST_NAME]]"; // TODO: This diagnostic isn't very good. import Cpp inline ''' +// CHECK:STDERR: fail_module_in_inline_cpp.carbon:[[@LINE+15]]:1: error: module declaration must not come from an #include directive [CppInteropParseError] +// CHECK:STDERR: 21 | module Foo; +// CHECK:STDERR: | ^~~~~~~~~~~ +// CHECK:STDERR: // CHECK:STDERR: fail_module_in_inline_cpp.carbon:[[@LINE+11]]:1: error: module declaration must occur at the start of the translation unit [CppInteropParseError] -// CHECK:STDERR: 17 | module Foo; +// CHECK:STDERR: 21 | module Foo; // CHECK:STDERR: | ^ // CHECK:STDERR: :1:1: note: add 'module;' to the start of the file to introduce a global module fragment [CppInteropParseNote] // CHECK:STDERR: 1 | ; // CHECK:STDERR: | ^ // CHECK:STDERR: // CHECK:STDERR: fail_module_in_inline_cpp.carbon:[[@LINE+4]]:8: error: module 'Foo' not found [CppInteropParseError] -// CHECK:STDERR: 17 | module Foo; +// CHECK:STDERR: 21 | module Foo; // CHECK:STDERR: | ^~~~ // CHECK:STDERR: module Foo; @@ -53,23 +61,31 @@ library "[[@TEST_NAME]]"; // TODO: This diagnostic isn't very good. import Cpp inline ''' +// CHECK:STDERR: fail_global_module_in_inline_cpp.carbon:[[@LINE+8]]:1: error: module declaration must not come from an #include directive [CppInteropParseError] +// CHECK:STDERR: 14 | module; +// CHECK:STDERR: | ^~~~~~~ +// CHECK:STDERR: // CHECK:STDERR: fail_global_module_in_inline_cpp.carbon:[[@LINE+4]]:1: error: 'module;' introducing a global module fragment can appear only at the start of the translation unit [CppInteropParseError] -// CHECK:STDERR: 10 | module; +// CHECK:STDERR: 14 | module; // CHECK:STDERR: | ^~~~~~~ // CHECK:STDERR: module; int n; +// CHECK:STDERR: fail_global_module_in_inline_cpp.carbon:[[@LINE+15]]:1: error: module declaration must not come from an #include directive [CppInteropParseError] +// CHECK:STDERR: 33 | module Foo; +// CHECK:STDERR: | ^~~~~~~~~~~ +// CHECK:STDERR: // CHECK:STDERR: fail_global_module_in_inline_cpp.carbon:[[@LINE+11]]:1: error: module declaration must occur at the start of the translation unit [CppInteropParseError] -// CHECK:STDERR: 25 | module Foo; +// CHECK:STDERR: 33 | module Foo; // CHECK:STDERR: | ^ // CHECK:STDERR: :1:1: note: add 'module;' to the start of the file to introduce a global module fragment [CppInteropParseNote] // CHECK:STDERR: 1 | ; // CHECK:STDERR: | ^ // CHECK:STDERR: // CHECK:STDERR: fail_global_module_in_inline_cpp.carbon:[[@LINE+4]]:8: error: module 'Foo' not found [CppInteropParseError] -// CHECK:STDERR: 25 | module Foo; +// CHECK:STDERR: 33 | module Foo; // CHECK:STDERR: | ^~~~ // CHECK:STDERR: module Foo; diff --git a/toolchain/check/testdata/interop/cpp/macros/macros.carbon b/toolchain/check/testdata/interop/cpp/macros/macros.carbon index c778a3b188ba..3712ffa1ace3 100644 --- a/toolchain/check/testdata/interop/cpp/macros/macros.carbon +++ b/toolchain/check/testdata/interop/cpp/macros/macros.carbon @@ -40,19 +40,21 @@ fn F() { } // --- bad_suffix.h +// CHECK:STDERR: ./bad_suffix.h:[[@LINE+1]]:24: in expansion of macro defined here [InCppMacroExpansion] #define CONFIG_VALUE 42f // --- fail_import_bad_suffix.carbon library "[[@TEST_NAME]]"; -// CHECK:STDERR: fail_import_bad_suffix.carbon:[[@LINE+4]]:10: in file included here [InCppInclude] -// CHECK:STDERR: ./bad_suffix.h:1:24: error: invalid digit 'f' in decimal constant [CppInteropParseError] -// CHECK:STDERR: 1 | #define CONFIG_VALUE 42f -// CHECK:STDERR: | ^ import Cpp library "bad_suffix.h"; fn F() { + // CHECK:STDERR: fail_import_bad_suffix.carbon:[[@LINE+20]]:6: error: invalid digit 'f' in decimal constant [CppInteropParseError] + // CHECK:STDERR: 27 | Cpp.CONFIG_VALUE; + // CHECK:STDERR: | ^ + // CHECK:STDERR: 2 | #define CONFIG_VALUE 42f + // CHECK:STDERR: | ^ // CHECK:STDERR: fail_import_bad_suffix.carbon:[[@LINE+15]]:3: note: in `Cpp` name lookup for `CONFIG_VALUE` [InCppNameLookup] // CHECK:STDERR: Cpp.CONFIG_VALUE; // CHECK:STDERR: ^~~~~~~~~~~~~~~~ @@ -72,19 +74,21 @@ fn F() { } // --- integer_literal_too_big.h +// CHECK:STDERR: ./integer_literal_too_big.h:[[@LINE+1]]:22: in expansion of macro defined here [InCppMacroExpansion] #define CONFIG_VALUE 18446744073709551616 // --- fail_import_integer_literal_too_big.carbon library "[[@TEST_NAME]]"; -// CHECK:STDERR: fail_import_integer_literal_too_big.carbon:[[@LINE+4]]:10: in file included here [InCppInclude] -// CHECK:STDERR: ./integer_literal_too_big.h:1:22: error: integer literal is too large to be represented in any integer type [CppInteropParseError] -// CHECK:STDERR: 1 | #define CONFIG_VALUE 18446744073709551616 -// CHECK:STDERR: | ^ import Cpp library "integer_literal_too_big.h"; fn F() { + // CHECK:STDERR: fail_import_integer_literal_too_big.carbon:[[@LINE+9]]:6: error: integer literal is too large to be represented in any integer type [CppInteropParseError] + // CHECK:STDERR: 16 | Cpp.CONFIG_VALUE; + // CHECK:STDERR: | ^ + // CHECK:STDERR: 2 | #define CONFIG_VALUE 18446744073709551616 + // CHECK:STDERR: | ^ // CHECK:STDERR: fail_import_integer_literal_too_big.carbon:[[@LINE+4]]:3: note: in `Cpp` name lookup for `CONFIG_VALUE` [InCppNameLookup] // CHECK:STDERR: Cpp.CONFIG_VALUE; // CHECK:STDERR: ^~~~~~~~~~~~~~~~ @@ -453,19 +457,21 @@ fn F() { // --- multiple_characters.h +// CHECK:STDERR: ./multiple_characters.h:[[@LINE+1]]:24: in expansion of macro defined here [InCppMacroExpansion] #define MULTIPLE_CHARS 'AB' // --- import_multiple_characters.carbon library "[[@TEST_NAME]]"; -// CHECK:STDERR: import_multiple_characters.carbon:[[@LINE+4]]:10: in file included here [InCppInclude] -// CHECK:STDERR: ./multiple_characters.h:2:24: warning: multi-character character constant [CppInteropParseWarning] -// CHECK:STDERR: 2 | #define MULTIPLE_CHARS 'AB' -// CHECK:STDERR: | ^ import Cpp library "multiple_characters.h"; fn F() { + // CHECK:STDERR: import_multiple_characters.carbon:[[@LINE+9]]:25: warning: multi-character character constant [CppInteropParseWarning] + // CHECK:STDERR: 16 | let unused a: i32 = Cpp.MULTIPLE_CHARS; + // CHECK:STDERR: | ^ + // CHECK:STDERR: 3 | #define MULTIPLE_CHARS 'AB' + // CHECK:STDERR: | ^ // CHECK:STDERR: import_multiple_characters.carbon:[[@LINE+4]]:22: note: in `Cpp` name lookup for `MULTIPLE_CHARS` [InCppNameLookup] // CHECK:STDERR: let unused a: i32 = Cpp.MULTIPLE_CHARS; // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~ @@ -971,6 +977,17 @@ fn F() { Cpp.m = 2; } +// --- self_expand.carbon + +import Cpp inline ''' +const int COUNT = 0; +#define COUNT COUNT + 1 +'''; + +class C(N: i32) {} +// Expansion of `COUNT` should be suppressed inside itself. +let _: C(1) = {} as C(Cpp.COUNT); + // CHECK:STDOUT: --- import_integer_literal_replacement_token.carbon // CHECK:STDOUT: // CHECK:STDOUT: constants {