Repository navigation
feat(tests): add compile_with_modules helper to Tester - #420
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds module-aware compilation to the test harness: Tester now records module sources, scans and topologically orders module dependencies, emits per-module PCM files to temporary paths, then compiles content using an overlay VFS and the generated PCMs. Tests were updated to use this helper instead of managing .pcm files manually. Changes
Sequence DiagramsequenceDiagram
participant Test as Test Case
participant Tester
participant Scanner as Module Scanner
participant Compiler as Clang -cc1
participant OVFS as Overlay VFS
participant FS as Temp PCM Storage
Test->>Tester: add module sources + main
Test->>Tester: compile_with_modules()
Tester->>Scanner: scan sources for module names & deps
Scanner-->>Tester: module names & dependency graph
Tester->>Tester: topo-sort modules (DFS, detect cycles)
loop for each module (topo order)
Tester->>Compiler: compile ModuleInterface
Compiler-->>FS: emit PCM (temp path)
Tester->>OVFS: register PCM path & mapping
end
Tester->>Compiler: compile main using OVFS + PCMs
Compiler-->>Tester: compilation result
Tester-->>Test: return success/failure
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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test/tester.cpp`:
- Around line 219-243: A temporary PCM created via createTemporaryFile
(pcm_path) is only recorded into pcm_paths after successful compile, so failures
leak files; update the code around the compile sequence to delete/unlink the
temporary file whenever pcm_path.has_value() and compilation fails or an early
return occurs (log errors if unlink fails), and ensure built_pcms entries are
not left pointing at removed files; additionally, modify clear() to iterate
pcm_paths, delete/unlink each file before calling pcm_paths.clear() (and remove
corresponding entries in built_pcms) so all temporary PCM files are cleaned up
on both failure paths and when clearing state; reference symbols: pcm_path,
pcm_paths, built_pcms, createTemporaryFile, compile(builder.params), and
clear().
🪄 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: 3f9ddb24-f250-40a2-ad72-7b5219cb6f4d
📒 Files selected for processing (3)
tests/unit/feature/semantic_tokens_tests.cpptests/unit/test/tester.cpptests/unit/test/tester.h
16bit-ykiko
force-pushed
the
feat/tester-compile-with-modules
branch
2 times, most recently
from
April 9, 2026 16:10
06e118f to
9077c31
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/unit/test/tester.cpp (1)
202-206:⚠️ Potential issue | 🟠 MajorTemporary PCM files still leak on failure and
clear().At Line 202, a temp PCM can be created and then returned early on failure (Line 217) before it is tracked; and Line 373 clears
pcm_pathswithout deleting files. This leaves orphaned temp files.💡 Proposed fix
@@ auto pcm_path = fs::createTemporaryFile("clice", "pcm"); if(!pcm_path) { LOG_ERROR("{}", pcm_path.error().message()); return false; } + pcm_paths.push_back(*pcm_path); @@ auto built = clice::compile(builder.params); if(!built.completed()) { for(auto& diag: built.diagnostics()) { LOG_ERROR("{}", diag.message); } return false; } built_pcms.try_emplace(mod.module_name, *pcm_path); - pcm_paths.push_back(*pcm_path); @@ void Tester::clear() { @@ - module_files.clear(); - pcm_paths.clear(); + for(const auto& path: pcm_paths) { + fs::remove(path); + } + pcm_paths.clear(); + module_files.clear(); }Also applies to: 217-226, 364-373
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test/tester.cpp` around lines 202 - 206, The temporary PCM file created by fs::createTemporaryFile (pcm_path) can leak because you return early after logging (LOG_ERROR) before adding it to pcm_paths, and pcm_paths.clear() is called later without deleting files; modify the logic in the test functions that use pcm_path and pcm_paths so that: 1) immediately register the created file path with the tracker (pcm_paths) as soon as createTemporaryFile returns successfully or ensure you delete the file before any early return (references: pcm_path, LOG_ERROR, createTemporaryFile); 2) replace pcm_paths.clear() with a proper cleanup routine that iterates pcm_paths and deletes each file (or use an RAII helper to auto-remove files) so clearing does not leave orphaned temp files (references: pcm_paths, clear()).
🤖 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/test/tester.cpp`:
- Around line 202-206: The temporary PCM file created by fs::createTemporaryFile
(pcm_path) can leak because you return early after logging (LOG_ERROR) before
adding it to pcm_paths, and pcm_paths.clear() is called later without deleting
files; modify the logic in the test functions that use pcm_path and pcm_paths so
that: 1) immediately register the created file path with the tracker (pcm_paths)
as soon as createTemporaryFile returns successfully or ensure you delete the
file before any early return (references: pcm_path, LOG_ERROR,
createTemporaryFile); 2) replace pcm_paths.clear() with a proper cleanup routine
that iterates pcm_paths and deletes each file (or use an RAII helper to
auto-remove files) so clearing does not leave orphaned temp files (references:
pcm_paths, clear()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 26c44833-b838-4c80-b943-e879f1c3ff7c
📒 Files selected for processing (3)
tests/unit/feature/semantic_tokens_tests.cpptests/unit/test/tester.cpptests/unit/test/tester.h
✅ Files skipped from review due to trivial changes (1)
- tests/unit/feature/semantic_tokens_tests.cpp
Add add_module() and compile_with_modules() to the Tester framework, enabling concise module tests via either separate add_module() calls or single-string #[filename] syntax with add_files(). The helper automatically scans for module dependencies, topologically sorts them, builds PCMs, and compiles the main file. Temporary PCM files are cleaned up in the destructor. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
16bit-ykiko
force-pushed
the
feat/tester-compile-with-modules
branch
from
April 9, 2026 23:04
9077c31 to
62aa5dd
Compare
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/test/tester.cpp`:
- Around line 161-164: The loop that populates name_to_index from modules
currently overwrites previous entries when modules[i].module_name is duplicated;
change the insertion to detect duplicates (e.g., use name_to_index.find(...) or
try_emplace/insert and check the returned pair) and if a duplicate module_name
is found, fail the test or assert (emit an error/ADD_FAILURE/abort) instead of
silently overwriting; update the loop that iterates modules and use the
duplicate-detection branch to report the colliding module_name and the two
indices so dependency resolution problems are caught early.
- Around line 156-159: scan_precise can return non-module/non-interface units
but the code always pushes an entry that later gets compiled as
CompilationKind::ModuleInterface; change the logic around the modules.push_back
call that uses mod.filename/mod.content/result.module_name/result.modules so
that you only enqueue when scan_precise actually identified a real module
interface (e.g., check a returned flag or that result.module_name is non-empty
or result.modules indicates an interface unit) and skip pushing otherwise;
update the conditional around the push to guard by that check so only true
interface modules are added and later compiled as ModuleInterface.
🪄 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: dd2b73c7-1266-4590-97ea-4ff326247a1d
📒 Files selected for processing (3)
tests/unit/feature/semantic_tokens_tests.cpptests/unit/test/tester.cpptests/unit/test/tester.h
✅ Files skipped from review due to trivial changes (1)
- tests/unit/feature/semantic_tokens_tests.cpp
…ster Clear buffers before Phase 1 remap in compile_driver_with_pch to avoid prepare_driver's try_emplace silently ignoring preamble-bounded remapping. Also reuse try_compile() in compile_with_modules and move base_cc1_args to anonymous namespace. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/unit/test/tester.cpp (2)
160-163:⚠️ Potential issue | 🟠 MajorOnly enqueue true interface modules from
scan_precise().Right now every scanned candidate is pushed, including non-module/non-interface units. Those are later compiled as
CompilationKind::ModuleInterface, which can fail incorrectly and pollute dependency indexing.Suggested fix
auto result = scan_precise(argv, TestVFS::root(), {}, nullptr, vfs); - modules.push_back( - {mod.filename, mod.content, result.module_name, std::move(result.modules)}); + if(result.module_name.empty() || !result.is_interface_unit) { + continue; + } + modules.push_back( + {mod.filename, mod.content, result.module_name, std::move(result.modules)});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test/tester.cpp` around lines 160 - 163, scan_precise() currently returns candidates of various kinds but the code always does modules.push_back(...) which enqueues non-interface units and later forces them to be compiled as CompilationKind::ModuleInterface; change the push site to only push when the scan result indicates an actual interface module (e.g. check result.is_interface or result.module_kind == ModuleInterface/Interface) before calling modules.push_back({mod.filename, mod.content, result.module_name, std::move(result.modules)}); this keeps non-module/non-interface results out of the modules list and prevents incorrect CompilationKind::ModuleInterface compilation and bad dependency indexing.
165-168:⚠️ Potential issue | 🟠 MajorDetect duplicate module names instead of overwriting map entries.
name_to_index[modules[i].module_name] = i;silently replaces prior modules with the same name, making dependency resolution unstable and potentially binding imports to the wrong PCM.Suggested fix
llvm::StringMap<std::size_t> name_to_index; for(std::size_t i = 0; i < modules.size(); ++i) { - name_to_index[modules[i].module_name] = i; + auto [it, inserted] = name_to_index.try_emplace(modules[i].module_name, i); + if(!inserted) { + LOG_ERROR("Duplicate module name '{}' in '{}' and '{}'", + modules[i].module_name, + modules[it->second].filename, + modules[i].filename); + return false; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test/tester.cpp` around lines 165 - 168, The loop that builds name_to_index currently overwrites entries for duplicate modules (name_to_index[modules[i].module_name] = i), which hides duplicate module names; change the logic in the loop that iterates modules to detect existing keys (use name_to_index.find or contains on modules[i].module_name) and handle duplicates explicitly—e.g., record an error/throw/abort or append indices to a vector instead of replacing the prior entry; update references that expect a single index (name_to_index) accordingly so dependency resolution uses the duplicate-aware behavior.
🤖 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/test/tester.cpp`:
- Around line 160-163: scan_precise() currently returns candidates of various
kinds but the code always does modules.push_back(...) which enqueues
non-interface units and later forces them to be compiled as
CompilationKind::ModuleInterface; change the push site to only push when the
scan result indicates an actual interface module (e.g. check result.is_interface
or result.module_kind == ModuleInterface/Interface) before calling
modules.push_back({mod.filename, mod.content, result.module_name,
std::move(result.modules)}); this keeps non-module/non-interface results out of
the modules list and prevents incorrect CompilationKind::ModuleInterface
compilation and bad dependency indexing.
- Around line 165-168: The loop that builds name_to_index currently overwrites
entries for duplicate modules (name_to_index[modules[i].module_name] = i), which
hides duplicate module names; change the logic in the loop that iterates modules
to detect existing keys (use name_to_index.find or contains on
modules[i].module_name) and handle duplicates explicitly—e.g., record an
error/throw/abort or append indices to a vector instead of replacing the prior
entry; update references that expect a single index (name_to_index) accordingly
so dependency resolution uses the duplicate-aware behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e615a4b3-3459-4675-8886-774a43c1542b
📒 Files selected for processing (2)
tests/unit/test/tester.cpptests/unit/test/tester.h
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
add_module()andcompile_with_modules()to theTestertest frameworkadd_module()calls and single-string#[filename]syntax viaadd_files()scan_precise, topologically sorts, builds PCMs in order, then compiles the main fileModuleImportandModuleReexportsemantic tokens tests to use the new APITest plan
🤖 Generated with Claude Code
Summary by CodeRabbit