Repository navigation
Conversation
Build module test inputs through the PCM path instead of syntax-only compilation. Take import semantic tokens from ImportDecl locations so export import is highlighted from the AST.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe diff removes directive-side tracking of module imports (deleting Changes
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 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the modules test harness to build real module PCMs (instead of syntax-only compilation), and updates semantic token generation to derive module import tokens from AST ImportDecl locations so export import highlights the import keyword correctly.
Changes:
- Build module test inputs through
compile(params, PCMInfo&)to generate PCMs and feed them viaPrebuiltModuleFiles. - Collect module import semantic tokens from AST
ImportDecl(using identifier source locations) instead of preprocessor directive tracking. - Remove the now-unused
Directive::importscollection and related preprocessor callback plumbing.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test/tester.cpp | Switches module test setup to actually generate PCMs and reuse them for dependent compilations. |
| src/semantic/semantic_visitor.h | Adds AST-based ImportDecl handling and changes module occurrence callback to use identifier source locations. |
| src/feature/semantic_tokens.cpp | Implements handleModuleOccurrence to emit module/import semantic tokens; removes directive-based module highlighting. |
| src/compile/directive.h | Removes Import directive structure and Directive::imports storage. |
| src/compile/directive.cpp | Removes PPCallbacks::moduleImport collection into directives. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void handleModuleOccurrence(clang::SourceLocation keyword, | ||
| llvm::ArrayRef<clang::syntax::Token> identifiers) { | ||
| llvm::ArrayRef<clang::SourceLocation> identifiers) { | ||
| assert(keyword.isValid() && keyword.isFileID() && "Invalid keyword location"); |
There was a problem hiding this comment.
handleModuleOccurrence asserts keyword.isFileID(), but module/import keywords can legally come from macro expansions (MacroID). This assertion becomes reachable now that VisitImportDecl calls handleModuleOccurrence, and it can crash on macro-expanded imports. Consider relaxing the assertion (e.g., allow MacroID and/or normalize via spelling/expansion location) consistent with SemanticTokensCollector::add_token() which already handles MacroID locations.
| assert(keyword.isValid() && keyword.isFileID() && "Invalid keyword location"); | |
| assert(keyword.isValid() && "Invalid keyword location"); |
| auto keyword = decl->getLocation(); | ||
| auto tokens = unit.expanded_tokens(decl->getSourceRange()); | ||
| for(const auto& token: tokens) { | ||
| if(token.text(unit.context().getSourceManager()) == "import") { |
There was a problem hiding this comment.
VisitImportDecl searches for the import keyword by comparing token.text(...) == "import". This is more fragile and slower than checking the token kind (e.g., token.kind() == clang::tok::kw_import), and avoids depending on SourceManager text extraction for each token.
| if(token.text(unit.context().getSourceManager()) == "import") { | |
| if(token.kind() == clang::tok::kw_import) { |
![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 `@src/feature/semantic_tokens.cpp`:
- Around line 245-251: In handleModuleOccurrence, avoid emitting a duplicate
Keyword token for the import keyword or ensure merge preserves identical kinds:
either skip calling add_token(keyword, SymbolKind::Keyword, 0) when a Keyword
token for the same source range already exists (check the token store via the
existing token lookup/contains method), or modify the merge_tokens logic to
treat same-range tokens with identical SymbolKind as non-conflicting (preserve
the existing SymbolKind instead of converting to Conflict); update references to
handleModuleOccurrence, add_token, and merge_tokens accordingly.
🪄 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: 44278dd2-ae1c-43bd-92fa-83ffc35e54ac
📒 Files selected for processing (5)
src/compile/directive.cppsrc/compile/directive.hsrc/feature/semantic_tokens.cppsrc/semantic/semantic_visitor.htests/unit/test/tester.cpp
💤 Files with no reviewable changes (2)
- src/compile/directive.cpp
- src/compile/directive.h
| void handleModuleOccurrence(clang::SourceLocation keyword, | ||
| llvm::ArrayRef<clang::SourceLocation> identifiers) { | ||
| add_token(keyword, SymbolKind::Keyword, 0); | ||
| for(auto loc: identifiers) { | ||
| add_token(loc, SymbolKind::Module, 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid turning duplicate import keyword tokens into Conflict.
Line 247 can emit the same Keyword range already produced by the lexical pass; merge_tokens() then treats same-range non-directive duplicates as SymbolKind::Conflict. Preserve identical kinds when resolving conflicts, or avoid emitting the duplicate keyword here.
🐛 Proposed fix: preserve identical token kinds during merge
static void resolve_conflict(RawToken& last, const RawToken& current) {
if(last.kind == SymbolKind::Conflict) {
return;
}
+ if(last.kind == current.kind) {
+ last.modifiers |= current.modifiers;
+ return;
+ }
// Directive is a low-priority lexical kind; semantic tokens override it.
if(last.kind == SymbolKind::Directive) {
last = current;
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/feature/semantic_tokens.cpp` around lines 245 - 251, In
handleModuleOccurrence, avoid emitting a duplicate Keyword token for the import
keyword or ensure merge preserves identical kinds: either skip calling
add_token(keyword, SymbolKind::Keyword, 0) when a Keyword token for the same
source range already exists (check the token store via the existing token
lookup/contains method), or modify the merge_tokens logic to treat same-range
tokens with identical SymbolKind as non-conflicting (preserve the existing
SymbolKind instead of converting to Conflict); update references to
handleModuleOccurrence, add_token, and merge_tokens accordingly.
|
Now this PR is ready for review, I hope you could take a glance at it and leave any suggestions. |
|
So why were these changes made? Are there any cases that the old method couldn't handle? |
|
The import expression is parsed as an |
|
The situation is a bit complicated, but at least keeping the import preprocessor directive is the right call; we might have other uses for it in the future. I don't think the current approach needs to be changed unless you can find an error case that it can't handle properly. |
|
Now I have figured out a suitable case to demonstrate the incorrect behavior of the legacy implementation. For non-partition modules: For import :part; // PPCallbacks returns name='foo:part' loc='main.cppm:2:8' -> Wrong Additionally, there are other cases that do not even trigger this callback. See: We do use additional logic to handle these cases, but that would not be elegant. I've added the corresponding test cases, and the |
|
OK, that makes sense. But keep the directive collection, and if possible, figure out why it ignores the partition modules. Resolve all review comments, otherwise merging is blocked. |
|
I still believe we shouldn't restore the directive collection. A bloated implementation will be misleading for contributors, especially LLMs. It may create a gap between the AST trace and the directive trace. However, I respect your opinion—if you insist, I'll keep it. |
|
Thanks! This was fixed independently along the way — the test harness now builds real PCMs for module tests instead of silently taking the syntax-only path. Closing as superseded. |
Build module test inputs through the PCM path instead of syntax-only compilation. Take import semantic tokens from ImportDecl locations so export import is highlighted from AST.
Summary by CodeRabbit
Refactor
Tests