[clang-tools-extra] [clangd] [C++ Modules] Enable content validation for module input files (PR #187653)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clangd

@llvm/pr-subscribers-clang-tools-extra

Author: Chuanqi Xu (ChuanqiXu9)

<details>
<summary>Changes</summary>

The IsModuleFileUpToDate function was not properly validating input files for C++20 modules. By default, ASTReader skips input file validation for StandardCXXModule files unless ForceCheckCXX20ModulesInputFiles and ValidateASTInputFilesContent are both set.

This change:
- Passes ValidateASTInputFilesContent=true to ASTReader constructor
- Uses ARR_OutOfDate flag for cleaner error handling
- Simplifies the validation logic (ReadAST already validates internally)
- Adds a test to verify header changes in module units are detected

Assised with AI.

---
Full diff: https://github.com/llvm/llvm-project/pull/187653.diff


2 Files Affected:

- (modified) clang-tools-extra/clangd/ModulesBuilder.cpp (+16-17) 
- (modified) clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp (+70) 


``````````diff
diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp
index 950392b91ac7c..387fe7ce0895a 100644
--- a/clang-tools-extra/clangd/ModulesBuilder.cpp
+++ b/clang-tools-extra/clangd/ModulesBuilder.cpp
@@ -269,28 +269,27 @@ bool IsModuleFileUpToDate(PathRef ModuleFilePath,
   PCHContainerOperations PCHOperations;
   CodeGenOptions CodeGenOpts;
   ASTReader Reader(PP, *ModCache, /*ASTContext=*/nullptr,
-                   PCHOperations.getRawReader(), CodeGenOpts, {});
+                   PCHOperations.getRawReader(), CodeGenOpts, {},
+                   /*isysroot=*/"",
+                   /*DisableValidationKind=*/DisableValidationForModuleKind::None,
+                   /*AllowASTWithCompilerErrors=*/false,
+                   /*AllowConfigurationMismatch=*/false,
+                   /*ValidateSystemInputs=*/false,
+                   /*ForceValidateUserInputs=*/true,
+                   /*ValidateASTInputFilesContent=*/true);
 
   // We don't need any listener here. By default it will use a validator
   // listener.
   Reader.setListener(nullptr);
 
-  if (Reader.ReadAST(ModuleFileName::makeExplicit(ModuleFilePath),
-                     serialization::MK_MainFile, SourceLocation(),
-                     ASTReader::ARR_None) != ASTReader::Success)
-    return false;
-
-  bool UpToDate = true;
-  Reader.getModuleManager().visit([&](serialization::ModuleFile &MF) -> bool {
-    Reader.visitInputFiles(
-        MF, /*IncludeSystem=*/false, /*Complain=*/false,
-        [&](const serialization::InputFile &IF, bool isSystem) {
-          if (!IF.getFile() || IF.isOutOfDate())
-            UpToDate = false;
-        });
-    return !UpToDate;
-  });
-  return UpToDate;
+  // Use ARR_OutOfDate so that ReadAST returns OutOfDate instead of Failure
+  // when input files are modified. This allows us to detect staleness
+  // without treating it as a hard error.
+  // ReadAST will validate all input files internally and return OutOfDate
+  // if any file is modified.
+  return Reader.ReadAST(ModuleFileName::makeExplicit(ModuleFilePath),
+                        serialization::MK_MainFile, SourceLocation(),
+                        ASTReader::ARR_OutOfDate) == ASTReader::Success;
 }
 
 bool IsModuleFilesUpToDate(
diff --git a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp
index 3132959a5967c..7cda291d3d3b7 100644
--- a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp
+++ b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp
@@ -644,6 +644,76 @@ import M;
   EXPECT_EQ(CDB.getGlobalScanningCount(), 1u);
 }
 
+// Test that canReuse detects changes to headers included in module units.
+// This verifies that the ASTReader correctly tracks header file dependencies
+// in BMI files and that IsModuleFileUpToDate correctly validates them.
+TEST_F(PrerequisiteModulesTests, CanReuseWithHeadersInModuleUnit) {
+  MockDirectoryCompilationDatabase CDB(TestDir, FS);
+
+  // Create a header file that will be included in a module unit
+  CDB.addFile("header1.h", R"cpp(
+inline int getValue() { return 42; }
+  )cpp");
+
+  // Module M includes header1.h in the global module fragment
+  CDB.addFile("M.cppm", R"cpp(
+module;
+#include "header1.h"
+export module M;
+export int m_value = getValue();
+  )cpp");
+
+  // Module N imports M (similar structure to ReusabilityTest)
+  CDB.addFile("N.cppm", R"cpp(
+export module N;
+import :Part;
+import M;
+  )cpp");
+
+  // Add a module partition (similar to ReusabilityTest)
+  CDB.addFile("N-part.cppm", R"cpp(
+export module N:Part;
+  )cpp");
+
+  ModulesBuilder Builder(CDB);
+
+  // Build prerequisite modules for N (which depends on M)
+  auto NInfo = Builder.buildPrerequisiteModulesFor(getFullPath("N.cppm"), FS);
+  EXPECT_TRUE(NInfo);
+
+  ParseInputs NInput = getInputs("N.cppm", CDB);
+  std::unique_ptr<CompilerInvocation> Invocation =
+      buildCompilerInvocation(NInput, DiagConsumer);
+
+  // Initially, canReuse should return true
+  EXPECT_TRUE(NInfo->canReuse(*Invocation, FS.view(TestDir)));
+
+  // Test 1: Modify header1.h (included by M)
+  // canReuse should detect this change since M's BMI records header1.h as input
+  CDB.addFile("header1.h", R"cpp(
+inline int getValue() { return 43; }
+  )cpp");
+  EXPECT_FALSE(NInfo->canReuse(*Invocation, FS.view(TestDir)));
+
+  // Rebuild and verify canReuse returns true again
+  NInfo = Builder.buildPrerequisiteModulesFor(getFullPath("N.cppm"), FS);
+  EXPECT_TRUE(NInfo->canReuse(*Invocation, FS.view(TestDir)));
+
+  // Test 2: Modify the module source file itself
+  CDB.addFile("M.cppm", R"cpp(
+module;
+#include "header1.h"
+export module M;
+export int m_value = getValue();
+export int m_new_value = 10;
+  )cpp");
+  EXPECT_FALSE(NInfo->canReuse(*Invocation, FS.view(TestDir)));
+
+  // Rebuild after module source change
+  NInfo = Builder.buildPrerequisiteModulesFor(getFullPath("N.cppm"), FS);
+  EXPECT_TRUE(NInfo->canReuse(*Invocation, FS.view(TestDir)));
+}
+
 TEST_F(PrerequisiteModulesTests, PrebuiltModuleFileTest) {
   MockDirectoryCompilationDatabase CDB(TestDir, FS);
 

``````````

</details>


https://github.com/llvm/llvm-project/pull/187653