Skip to content

fix(modules): build test PCMs and use AST import tokens #429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Perdixky wants to merge 2 commits into from
Closed

fix(modules): build test PCMs and use AST import tokens #429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9515b5a
fix(modules): build test PCMs and use AST import tokens
Perdixky Apr 18, 2026
ef15b61
test: Add a module partation semantic tokens test
Perdixky Apr 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions src/compile/directive.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,18 +141,6 @@ class DirectiveCollector : public clang::PPCallbacks {
}
}

void moduleImport(clang::SourceLocation import_location,
clang::ModuleIdPath names,
const clang::Module*) override {
auto fid = unit.file_id(unit.expansion_location(import_location));
auto& import = unit->directives[fid].imports.emplace_back();
import.location = import_location;
for(auto name: names) {
import.name += name.getIdentifierInfo()->getName();
import.name_locations.emplace_back(name.getLoc());
}
}

void HasInclude(clang::SourceLocation location,
llvm::StringRef,
bool,
Expand Down
13 changes: 0 additions & 13 deletions src/compile/directive.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,18 +117,6 @@ struct Pragma {
clang::SourceLocation loc;
};

struct Import {
/// The name of imported module.
std::string name;

/// The location of import keyword, may comes from macro expansion.
clang::SourceLocation location;

/// The locations of tokens that make up the token name, may comes
/// from macro expansion.
std::vector<clang::SourceLocation> name_locations;
};

/// Information about `#embed` directive.
struct Embed {
/// The file name in the embed directive, not including quotes or angle brackets.
Expand Down Expand Up @@ -168,7 +156,6 @@ struct Directive {
std::vector<Condition> conditions;
std::vector<MacroRef> macros;
std::vector<Pragma> pragmas;
std::vector<Import> imports;
std::vector<Embed> embeds;
std::vector<HasEmbed> has_embeds;
};
Expand Down
18 changes: 7 additions & 11 deletions src/feature/semantic_tokens.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,13 @@ class SemanticTokensCollector : public SemanticVisitor<SemanticTokensCollector>
add_token(location, SymbolKind::Macro, modifiers);
}

// handleModuleOccurrence
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);
}
}
Comment on lines +245 to +251

@coderabbitai coderabbitai Bot Apr 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


// handleRelation

Expand Down Expand Up @@ -296,16 +302,6 @@ class SemanticTokensCollector : public SemanticVisitor<SemanticTokensCollector>
void highlight_modules() {
auto interested = unit.interested_file();

auto directives_it = unit.directives().find(interested);
if(directives_it != unit.directives().end()) {
for(const auto& import: directives_it->second.imports) {
add_token(import.location, SymbolKind::Keyword, 0);
for(auto loc: import.name_locations) {
add_token(loc, SymbolKind::Module, 0);
}
}
}

auto* mod = unit.context().getCurrentNamedModule();
if(!mod) {
return;
Expand Down
38 changes: 11 additions & 27 deletions src/semantic/semantic_visitor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,11 @@ class SemanticVisitor : public FilteredASTVisitor<SemanticVisitor<Derived>> {

/// Invoked when a module occurrence is seen in source code.
/// @param keyword The location of the `module` or `import` keyword.
/// @param identifiers Tokens that make up the module name.
/// @param identifiers Source locations of identifiers that make up the module name.
void handleModuleOccurrence(clang::SourceLocation keyword,
llvm::ArrayRef<clang::syntax::Token> identifiers) {
llvm::ArrayRef<clang::SourceLocation> identifiers) {
assert(keyword.isValid() && keyword.isFileID() && "Invalid keyword location");

Copilot AI Apr 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
assert(keyword.isValid() && keyword.isFileID() && "Invalid keyword location");
assert(keyword.isValid() && "Invalid keyword location");

Copilot uses AI. Check for mistakes.

/// FIXME: Check whether identifiers are valid.

if constexpr(!std::same_as<decltype(&SemanticVisitor::handleModuleOccurrence),
decltype(&Derived::handleModuleOccurrence)>) {
getDerived().handleModuleOccurrence(keyword, identifiers);
Expand Down Expand Up @@ -145,30 +143,16 @@ class SemanticVisitor : public FilteredASTVisitor<SemanticVisitor<Derived>> {
#define VISIT_TYPELOC(type) bool Visit##type(clang::type loc)

VISIT_DECL(ImportDecl) {
/// FIXME:
// auto tokens = TB.expandedTokens(decl->getSourceRange());
//
// assert(tokens.size() >= 2 && tokens[0].kind() == clang::tok::identifier &&
// tokens[0].text(SM) == "import" && "Invalid import declaration");
// assert([&]() {
// auto range = tokens.drop_front(1);
// for(auto iter = range.begin(); iter != range.end(); ++iter) {
// if(iter->kind() == clang::tok::identifier) {
// if(auto next = iter + 1;
// next != range.end() && (next->kind() == clang::tok::coloncolon ||
// next->kind() == clang::tok::period)) {
// continue;
// }
// break;
// } else {
// return false;
// }
// }
// return true;
//}() && "Invalid import declaration");
//
// handleModuleOccurrence(tokens[0].location(), tokens.drop_front(1));
auto keyword = decl->getLocation();
auto tokens = unit.expanded_tokens(decl->getSourceRange());
for(const auto& token: tokens) {
if(token.text(unit.context().getSourceManager()) == "import") {

Copilot AI Apr 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
if(token.text(unit.context().getSourceManager()) == "import") {
if(token.kind() == clang::tok::kw_import) {

Copilot uses AI. Check for mistakes.
keyword = token.location();
break;
}
}

handleModuleOccurrence(keyword, decl->getIdentifierLocs());
return true;
}

Expand Down
18 changes: 18 additions & 0 deletions tests/unit/feature/semantic_tokens_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,24 @@ export @kw[import] @mod[foo];
EXPECT_TOKEN("mod", SymbolKind::Module);
}

TEST_CASE(ModulePartitionImport) {
add_files("main.cppm", R"(
#[part.cppm]
export module foo:part;
export int x = 42;

#[main.cppm]
export module foo;
@kw[import] :@mod[part];
)");
ASSERT_TRUE(compile_with_modules());
tokens = feature::semantic_tokens(*unit, feature::PositionEncoding::UTF8);
decoded = decode_utf8_tokens(unit->interested_content(), tokens);

EXPECT_TOKEN("kw", SymbolKind::Keyword);
EXPECT_TOKEN("mod", SymbolKind::Module);
}

TEST_CASE(GlobalModuleFragment) {
add_main("main.cpp", R"cpp(
module;
Expand Down
10 changes: 8 additions & 2 deletions tests/unit/test/tester.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,10 +218,16 @@ bool Tester::compile_with_modules(llvm::StringRef standard) {
builder.params.vfs = overlay;
builder.params.pcms = built_pcms;

if(!builder.try_compile())
PCMInfo info;
auto built = clice::compile(builder.params, info);
if(!built.completed()) {
for(auto& diag: built.diagnostics()) {
LOG_ERROR("{}", diag.message);
}
return false;
}

built_pcms.try_emplace(mod.module_name, *pcm_path);
built_pcms.try_emplace(mod.module_name, info.path);
}

prepare(standard);
Expand Down
Loading