Repository navigation
feat: add scan_module_decl() fallback for conditional module declarations - #373
Merged
Merged
Conversation
…ions When `scan()` detects a module declaration inside `#if`/`#ifdef` (need_preprocess=true), the module name cannot be extracted by the lexer-based scanner alone. This adds `scan_module_decl()` — a lightweight preprocessor-based fallback that runs clang's preprocessor to evaluate conditions and extract the actual module name. It stops lexing as soon as the module declaration is found, so it only processes the file preamble. - Add `scan_module_decl()` in scan.h/scan.cpp - Integrate fallback into `scan_dependency_graph()` for wave 0 files - Add comprehensive scan() tests for all cppref module declaration forms - Add scan_module_decl() tests for conditional modules (ifdef, if expr, GMF) - Add scan_precise() tests for module import semantics (named, dotted, partition, export-import, GMF, mixed includes/imports) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughDuring Phase 2+3 per-file processing, when a file has Changes
Sequence DiagramsequenceDiagram
participant DG as Dependency Graph
participant CDB as Compilation DB
participant SMD as scan_module_decl()
participant CI as CompilerInstance
participant PP as Preprocessor
participant Cache as Scan Cache
DG->>DG: Phase 2+3: file flagged need_preprocess && wave_num==0
DG->>CDB: lookup compile args & directory for file
CDB-->>DG: return args, directory
DG->>SMD: call scan_module_decl(args, directory, content?, cache?, vfs?)
SMD->>CI: create CompilerInstance with args
CI->>PP: attach ScanDirectivesGetter / PreprocessOnlyAction
SMD->>PP: lex tokens until pp.isInNamedModule() becomes true
PP-->>SMD: return module_name, is_interface_unit
SMD-->>DG: ScanResult with module info
alt module_name non-empty
DG->>Cache: update cached entry (module_name, is_interface_unit, need_preprocess=false) if ext_cache present
DG->>DG: map module/interface into dependency graph
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/syntax/scan_tests.cpp (1)
433-443: Consider using an exact match assertion for consistency.This test uses
find("mylib")which is weaker than the exact matchEXPECT_EQ(result.module_name, "mylib:core")used in thePartitionInterfacetest (line 175). If Clang consistently represents partitions as"mylib:core", consider using an exact match here too for stronger validation.auto result = scan_module_decl(args, TestVFS::root(), {}, nullptr, vfs); - // clang represents partition as "mylib:core". - EXPECT_TRUE(result.module_name.find("mylib") != std::string::npos); + EXPECT_EQ(result.module_name, "mylib:core"); EXPECT_TRUE(result.is_interface_unit);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/syntax/scan_tests.cpp` around lines 433 - 443, The ModuleDeclPartition test uses a weak substring check on result.module_name; change the assertion to assert the exact expected partition string by replacing the find-based EXPECT_TRUE with an exact equality check (e.g. EXPECT_EQ(result.module_name, "mylib:core")) while leaving the EXPECT_TRUE(result.is_interface_unit) unchanged; update the assertion in the TEST_CASE named ModuleDeclPartition that verifies result.module_name produced by scan_module_decl to use exact match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/syntax/scan_tests.cpp`:
- Around line 433-443: The ModuleDeclPartition test uses a weak substring check
on result.module_name; change the assertion to assert the exact expected
partition string by replacing the find-based EXPECT_TRUE with an exact equality
check (e.g. EXPECT_EQ(result.module_name, "mylib:core")) while leaving the
EXPECT_TRUE(result.is_interface_unit) unchanged; update the assertion in the
TEST_CASE named ModuleDeclPartition that verifies result.module_name produced by
scan_module_decl to use exact match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12b15940-6d8b-45cc-9b13-1c70df12c06c
📒 Files selected for processing (4)
src/syntax/dependency_graph.cppsrc/syntax/scan.cppsrc/syntax/scan.htests/unit/syntax/scan_tests.cpp
Move all module-related scan tests (scan() module declarations, scan_module_decl() fallback, scan_precise() module imports) from scan_tests.cpp into module_scan_tests.cpp. Apply clang-format. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/syntax/module_scan_tests.cpp (2)
248-257: Strengthen assertion to check exact partition name.This test uses substring matching (
find("mylib")) while the analogousPartitionInterfacetest inModuleScan(line 46) uses exact matching withEXPECT_EQ(result.module_name, "mylib:core"). Ifscan_module_decl()should return the same format asscan(), use an exact assertion for consistency and to catch regressions in partition name handling.Proposed fix
auto args = std::vector<const char*>{"clang++", "-std=c++20", main_path.c_str()}; auto result = scan_module_decl(args, TestVFS::root(), {}, nullptr, vfs); - EXPECT_TRUE(result.module_name.find("mylib") != std::string::npos); + EXPECT_EQ(result.module_name, "mylib:core"); EXPECT_TRUE(result.is_interface_unit);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/syntax/module_scan_tests.cpp` around lines 248 - 257, The test "Partition" currently uses substring matching on result.module_name which is too weak; update the assertion in the TEST_CASE Partition to assert the exact partition name by replacing the EXPECT_TRUE(result.module_name.find("mylib") != std::string::npos) with an exact equality check EXPECT_EQ(result.module_name, "mylib:core"), keeping the existing EXPECT_TRUE(result.is_interface_unit) assertion; locate this change around the TEST_CASE Partition and the call to scan_module_decl(...) to ensure consistency with the PartitionInterface test and catch regressions in partition name handling.
330-342: Consider verifying actual imported module content for partition tests.
PartitionImportandExportImportPartitiontests only assertASSERT_GE(result.modules.size(), 1u)without checking the actual imported module name. Other tests likeNamedImportandMultipleImportsverify exact content (e.g.,EXPECT_EQ(result.modules[0], "other")).If the expected format for partition imports (
:core) is well-defined, adding content verification would catch regressions. If the format intentionally varies, a brief comment explaining why would help maintainability.Example: add content verification
TEST_CASE(PartitionImport) { // ... ASSERT_GE(result.modules.size(), 1u); + // Partition imports are recorded as ":core" (or "mylib:core" depending on implementation) + EXPECT_TRUE(result.modules[0].find("core") != std::string::npos); }Also applies to: 359-371
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/syntax/module_scan_tests.cpp` around lines 330 - 342, Update the PartitionImport and ExportImportPartition tests to verify the actual imported module entry instead of just asserting count: determine the canonical string that scan_precise produces for a partition import (what appears in result.modules for the import `:core`) and replace the generic ASSERT_GE(result.modules.size(), 1u) with a specific equality check like EXPECT_EQ(result.modules[0], "<expected_partition_name>"); if the representation intentionally varies, add a short comment in the test (PartitionImport / ExportImportPartition) explaining why the exact content is not asserted. Ensure you reference result.modules and scan_precise when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/syntax/module_scan_tests.cpp`:
- Around line 248-257: The test "Partition" currently uses substring matching on
result.module_name which is too weak; update the assertion in the TEST_CASE
Partition to assert the exact partition name by replacing the
EXPECT_TRUE(result.module_name.find("mylib") != std::string::npos) with an exact
equality check EXPECT_EQ(result.module_name, "mylib:core"), keeping the existing
EXPECT_TRUE(result.is_interface_unit) assertion; locate this change around the
TEST_CASE Partition and the call to scan_module_decl(...) to ensure consistency
with the PartitionInterface test and catch regressions in partition name
handling.
- Around line 330-342: Update the PartitionImport and ExportImportPartition
tests to verify the actual imported module entry instead of just asserting
count: determine the canonical string that scan_precise produces for a partition
import (what appears in result.modules for the import `:core`) and replace the
generic ASSERT_GE(result.modules.size(), 1u) with a specific equality check like
EXPECT_EQ(result.modules[0], "<expected_partition_name>"); if the representation
intentionally varies, add a short comment in the test (PartitionImport /
ExportImportPartition) explaining why the exact content is not asserted. Ensure
you reference result.modules and scan_precise when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 528ffbd1-7214-4c0c-8669-2007e2a82b82
📒 Files selected for processing (2)
src/syntax/dependency_graph.cpptests/unit/syntax/module_scan_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/syntax/dependency_graph.cpp
Replace repetitive VFS + args setup in ModuleDeclFallback and ModuleImportScan tests with a shared ModuleScanFixture helper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/unit/syntax/module_scan_tests.cpp (4)
12-26: Avoid persisting argv as raw pointers in the fixture.On Line 25,
main_path.c_str()is stored inargs; this is fragile ifModuleScanFixtureis ever copied/moved. Either make the fixture non-copyable/non-movable or store owned strings and build argv at call time.♻️ Suggested minimal hardening (non-copyable fixture)
struct ModuleScanFixture { llvm::IntrusiveRefCntPtr<TestVFS> vfs = llvm::makeIntrusiveRefCnt<TestVFS>(); std::string main_path; std::vector<const char*> args; + + ModuleScanFixture(const ModuleScanFixture&) = delete; + ModuleScanFixture& operator=(const ModuleScanFixture&) = delete; + ModuleScanFixture(ModuleScanFixture&&) = delete; + ModuleScanFixture& operator=(ModuleScanFixture&&) = delete;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/syntax/module_scan_tests.cpp` around lines 12 - 26, The fixture stores main_path.c_str() in args (ModuleScanFixture, args and main_path), which leaves dangling pointers when the fixture is copied/moved; fix by owning the strings instead of persisting raw char* — e.g., add a std::vector<std::string> (or a member like owned_args) to hold each argument including main_path, push_back owned strings in the ModuleScanFixture constructor, and only create a temporary vector<const char*> from owned_args when you need to call into the API; alternatively, make ModuleScanFixture non-copyable/non-movable (delete copy/move constructors and assignment) if you prefer to keep args as const char*.
272-277:NoModulefallback case should assert empty dependency list too.This case currently validates name/interface only. Add a
modules.empty()assertion to lock the full no-module contract.🧪 Small assertion improvement
TEST_CASE(NoModule) { ModuleScanFixture f("main.cpp", "int main() { return 0; }"); auto result = f.decl(); EXPECT_TRUE(result.module_name.empty()); EXPECT_FALSE(result.is_interface_unit); + EXPECT_TRUE(result.modules.empty()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/syntax/module_scan_tests.cpp` around lines 272 - 277, The NoModule test's validation is incomplete: after calling ModuleScanFixture::decl() in the TEST_CASE(NoModule) you must also assert that the dependency list is empty; add an assertion checking result.modules.empty() (or EXPECT_TRUE(result.modules.empty())) alongside the existing checks of result.module_name.empty() and result.is_interface_unit to enforce the full no-module contract.
265-270: Partition declaration assertion is too permissive.Line 268 only checks substring presence, so malformed values can still pass. Prefer asserting the exact expected module name for this input.
🎯 Tighten the assertion
TEST_CASE(Partition) { ModuleScanFixture f("main.cppm", "export module mylib:core;"); auto result = f.decl(); - EXPECT_TRUE(result.module_name.find("mylib") != std::string::npos); + EXPECT_EQ(result.module_name, "mylib:core"); EXPECT_TRUE(result.is_interface_unit); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/syntax/module_scan_tests.cpp` around lines 265 - 270, The test Partition uses ModuleScanFixture with "export module mylib:core;" but asserts only that result.module_name contains "mylib", which is too permissive; replace the substring check on result.module_name with an exact equality assertion (e.g., EXPECT_EQ or equivalent) against the full expected module name "mylib:core" so the test verifies the precise parsed name while keeping the existing is_interface_unit assertion.
324-331: Partition import tests should verify more than “at least one”.These checks can pass even when the wrong dependency is captured. Tighten to exact count plus a non-empty value (or canonical expected value if defined by scanner contract).
✅ Make regressions easier to catch
TEST_CASE(PartitionImport) { @@ auto result = f.precise(); - ASSERT_GE(result.modules.size(), 1u); + ASSERT_EQ(result.modules.size(), 1u); + EXPECT_FALSE(result.modules[0].empty()); } @@ TEST_CASE(ExportImportPartition) { @@ auto result = f.precise(); - ASSERT_GE(result.modules.size(), 1u); + ASSERT_EQ(result.modules.size(), 1u); + EXPECT_FALSE(result.modules[0].empty()); }Also applies to: 343-350
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/syntax/module_scan_tests.cpp` around lines 324 - 331, The test PartitionImport (TEST_CASE PartitionImport) currently only asserts result.modules.size() >= 1 which is too weak; change the assertions on the output of ModuleScanFixture::precise() (result from f.precise()) to assert the exact expected number of captured modules (use ASSERT_EQ(result.modules.size(), <expected_count>)) and then verify at least one module has a non-empty/canonical name (e.g., ASSERT_FALSE(result.modules[0].name.empty()) or compare to the expected module name "mylib" or its canonical form). Apply the same tightening to the similar test block around lines 343-350 to assert exact counts and non-empty/expected module names rather than just >= 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/syntax/module_scan_tests.cpp`:
- Around line 12-26: The fixture stores main_path.c_str() in args
(ModuleScanFixture, args and main_path), which leaves dangling pointers when the
fixture is copied/moved; fix by owning the strings instead of persisting raw
char* — e.g., add a std::vector<std::string> (or a member like owned_args) to
hold each argument including main_path, push_back owned strings in the
ModuleScanFixture constructor, and only create a temporary vector<const char*>
from owned_args when you need to call into the API; alternatively, make
ModuleScanFixture non-copyable/non-movable (delete copy/move constructors and
assignment) if you prefer to keep args as const char*.
- Around line 272-277: The NoModule test's validation is incomplete: after
calling ModuleScanFixture::decl() in the TEST_CASE(NoModule) you must also
assert that the dependency list is empty; add an assertion checking
result.modules.empty() (or EXPECT_TRUE(result.modules.empty())) alongside the
existing checks of result.module_name.empty() and result.is_interface_unit to
enforce the full no-module contract.
- Around line 265-270: The test Partition uses ModuleScanFixture with "export
module mylib:core;" but asserts only that result.module_name contains "mylib",
which is too permissive; replace the substring check on result.module_name with
an exact equality assertion (e.g., EXPECT_EQ or equivalent) against the full
expected module name "mylib:core" so the test verifies the precise parsed name
while keeping the existing is_interface_unit assertion.
- Around line 324-331: The test PartitionImport (TEST_CASE PartitionImport)
currently only asserts result.modules.size() >= 1 which is too weak; change the
assertions on the output of ModuleScanFixture::precise() (result from
f.precise()) to assert the exact expected number of captured modules (use
ASSERT_EQ(result.modules.size(), <expected_count>)) and then verify at least one
module has a non-empty/canonical name (e.g.,
ASSERT_FALSE(result.modules[0].name.empty()) or compare to the expected module
name "mylib" or its canonical form). Apply the same tightening to the similar
test block around lines 343-350 to assert exact counts and non-empty/expected
module names rather than just >= 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5df1a363-9a88-40d1-b33a-5428b7c84666
📒 Files selected for processing (1)
tests/unit/syntax/module_scan_tests.cpp
…ases - Verify partition import values: clang returns fully-qualified names (e.g., "mylib:core" for `import :core;` inside `module mylib;`) - Add tests for: implementation partition import, multiple partition imports, mixed named+partition imports, partition-to-partition imports (both interface and implementation units) - Remove header unit import tests (hang in VFS-only context) - Document that header units are not testable without real compilation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/syntax/module_scan_tests.cpp`:
- Around line 469-476: The test uses substring checks on result.module_name
which can hide regressions; update the assertions in the
PartitionInterfaceImportingPartition test (and the related partition test) to
assert exact equality of the full partition names instead of substring
membership: replace EXPECT_TRUE(result.module_name.find("mylib") !=
std::string::npos) with an equality check against "mylib:ui" in the
PartitionInterfaceImportingPartition test, and similarly assert equality against
"mylib:detail" in the other partition test; keep the existing check of
result.is_interface_unit as-is.
- Around line 265-269: The test's module-name assertion is too permissive;
change the check in TEST_CASE Partition to assert exact equality on
result.module_name (from ModuleScanFixture f / result) against the full expected
partition name "mylib:core" instead of using find("mylib") != npos, so the test
fails for malformed names while keeping the existing is_interface_unit
assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67227653-442c-446c-89f5-228ede28833f
📒 Files selected for processing (1)
tests/unit/syntax/module_scan_tests.cpp
- Use exact EXPECT_EQ for partition module_name ("mylib:ui", "mylib:detail")
instead of fuzzy find() checks
- Add macro-expanded import tests: inline #define, command-line -D,
and GMF header-defined macro — clang correctly expands all three
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/unit/syntax/module_scan_tests.cpp (1)
265-269:⚠️ Potential issue | 🟡 MinorTighten partition module-name assertion to exact match.
At Line 268, substring matching is still too permissive and can pass malformed values. Assert the full expected partition name.
Proposed fix
TEST_CASE(Partition) { ModuleScanFixture f("main.cppm", "export module mylib:core;"); auto result = f.decl(); - EXPECT_TRUE(result.module_name.find("mylib") != std::string::npos); + EXPECT_EQ(result.module_name, "mylib:core"); EXPECT_TRUE(result.is_interface_unit); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/syntax/module_scan_tests.cpp` around lines 265 - 269, The test currently uses a substring check on result.module_name in the TEST_CASE "Partition" which is too permissive; update the assertion to check exact equality for the partition name by replacing the substring expectation with a strict equality comparison against "mylib" (locate the test in TEST_CASE Partition, the ModuleScanFixture f("main.cppm", "export module mylib:core;") and the result variable returned by f.decl() and change the assertion that references result.module_name to assert exact match).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/unit/syntax/module_scan_tests.cpp`:
- Around line 265-269: The test currently uses a substring check on
result.module_name in the TEST_CASE "Partition" which is too permissive; update
the assertion to check exact equality for the partition name by replacing the
substring expectation with a strict equality comparison against "mylib" (locate
the test in TEST_CASE Partition, the ModuleScanFixture f("main.cppm", "export
module mylib:core;") and the result variable returned by f.decl() and change the
assertion that references result.module_name to assert exact match).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 645e1814-e5eb-4135-b30d-a378b98c414f
📒 Files selected for processing (1)
tests/unit/syntax/module_scan_tests.cpp
- Extract ModuleScanFixture to shared header (module_scan_fixture.h) - Split module import tests into separate file (module_import_tests.cpp) - Make fixture non-copyable/non-movable to prevent dangling c_str() pointers - Use exact EXPECT_EQ for partition names instead of substring find() - Add modules.empty() assertion to NoModule fallback test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
@coderrabbit review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
scan_module_decl()— a lightweight preprocessor-based fallback that resolves module declarations inside#if/#ifdefconditionals. Whenscan()detectsneed_preprocess=true, this function runs clang's preprocessor to evaluate conditions and extract the actual module name. It stops lexing as soon as the module declaration is found, making it much cheaper thanscan_precise().scan_dependency_graph()for wave 0 source files, so conditional module declarations (e.g.#ifdef USE_MODULES / export module M; / #endif) are correctly registered in the dependency graph.scan_module_decl()tests for conditional resolution andscan_precise()tests for module import semantics.Test plan
scan()tests cover: primary interface, implementation, dotted names, partitions, GMF, conditional module declarations, private module fragmentscan_module_decl()tests cover: basic, conditional with-D, conditional with#ifexpression, GMF with conditional, implementation unit, dotted name, partition, no-module filescan_precise()tests cover: named import, multiple imports, dotted import, partition import, export-import, export-import partition, implementation import, GMF with import, mixed includes/imports, no-module file🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests