Skip to content

refactor(filtered ast visitor): refactor DocumentSymbolCollector to fit the CRTP design of RecursiveASTVisitor #427

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

Open
Perdixky wants to merge 4 commits into
base: main
Choose a base branch
from
Open

refactor(filtered ast visitor): refactor DocumentSymbolCollector to fit the CRTP design of RecursiveASTVisitor #427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b9dbe63
refactor(semantic): remove redundant AST traversal filters
Perdixky Apr 17, 2026
c13b79a
Remove reboundent interested-only processing
Perdixky Apr 18, 2026
358f024
Simplify switch by ternary operator
Perdixky Apr 18, 2026
0d606b7
refactor(document-symbols): use decl-specific traversal
Perdixky Apr 18, 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
82 changes: 43 additions & 39 deletions src/feature/document_symbols.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,65 +111,69 @@ class DocumentSymbolCollector : public FilteredASTVisitor<DocumentSymbolCollecto
public:
explicit DocumentSymbolCollector(CompilationUnitRef unit) : FilteredASTVisitor(unit, true) {}

bool on_traverse_decl(clang::Decl* decl, auto traverse) {
if(!is_interested(decl)) {
return (this->*traverse)(decl);
}

auto* named = llvm::dyn_cast<clang::NamedDecl>(decl);
if(!named) {
return (this->*traverse)(decl);
}
private:
auto push_symbol(clang::NamedDecl* decl) {
struct Guard {
std::vector<InternalSymbol>* previous;
std::vector<InternalSymbol>** cursor;

~Guard() {
if(previous) {
*cursor = previous;
}
}
};

auto [fid, selection_range] =
unit.decompose_range(unit.expansion_location(named->getLocation()));
auto [fid2, range] = unit.decompose_expansion_range(named->getSourceRange());
unit.decompose_range(unit.expansion_location(decl->getLocation()));
auto [fid2, range] = unit.decompose_expansion_range(decl->getSourceRange());
if(fid != fid2 || fid != unit.interested_file() || !selection_range.valid() ||
!range.valid()) {
return true;
return Guard{nullptr, nullptr};
}

auto* previous = result.cursor;
auto& symbol = result.cursor->emplace_back();
symbol.kind = SymbolKind::from(decl);
symbol.name = ast::display_name_of(named);
symbol.detail = symbol_detail(unit.context(), *named);
symbol.name = ast::display_name_of(decl);
symbol.detail = symbol_detail(unit.context(), *decl);
symbol.selection_range = selection_range;
symbol.range = range;

result.cursor = &symbol.children;
auto ok = (this->*traverse)(decl);
result.cursor = previous;
return ok;
return Guard{previous, &result.cursor};
}

public:
#define TRAVERSE_SYMBOL_DECL(type) \
bool Traverse##type##Decl(clang::type##Decl* decl) { \
auto guard = this->push_symbol(decl); \
return Base::Traverse##type##Decl(decl); \
}

TRAVERSE_SYMBOL_DECL(Namespace);
TRAVERSE_SYMBOL_DECL(Enum);
TRAVERSE_SYMBOL_DECL(EnumConstant);
TRAVERSE_SYMBOL_DECL(Function);
TRAVERSE_SYMBOL_DECL(CXXMethod);
TRAVERSE_SYMBOL_DECL(CXXConstructor);
TRAVERSE_SYMBOL_DECL(CXXDestructor);
TRAVERSE_SYMBOL_DECL(CXXConversion);
TRAVERSE_SYMBOL_DECL(CXXDeductionGuide);
TRAVERSE_SYMBOL_DECL(Record);
TRAVERSE_SYMBOL_DECL(CXXRecord);
TRAVERSE_SYMBOL_DECL(Field);
TRAVERSE_SYMBOL_DECL(Var);
TRAVERSE_SYMBOL_DECL(Binding);
TRAVERSE_SYMBOL_DECL(Concept);

#undef TRAVERSE_SYMBOL_DECL
Comment on lines +148 to +170

@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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Look for any tests asserting presence/absence of typedef/type-alias entries in document symbols
rg -nP --type=cpp -C3 'document_symbols|DocumentSymbol' -g '*test*'
rg -nP --type=cpp -C2 '\b(typedef|using .* =)' -g '*document*symbol*test*'

Repository: clice-io/clice

Length of output: 5174

🏁 Script executed:

# Find and read the document_symbols.cpp file to verify the TRAVERSE_SYMBOL_DECL macro
fd -t f 'document_symbols\.cpp' -x cat -n {}

Repository: clice-io/clice

Length of output: 9555

🏁 Script executed:

# Check what's in symbol_detail function around lines 87-90
fd -t f 'document_symbols\.cpp' -x sed -n '80,100p' {}

Repository: clice-io/clice

Length of output: 723

🏁 Script executed:

# Look at the test file for any typedef/type alias test cases
cat -n tests/unit/feature/document_symbol_tests.cpp | head -200

Repository: clice-io/clice

Length of output: 4423

🏁 Script executed:

# Search for TypedefDecl, TypeAliasDecl, TypeAliasTemplateDecl in the entire codebase
rg -n 'TypedefDecl|TypeAliasDecl|TypeAliasTemplateDecl' --type=cpp

Repository: clice-io/clice

Length of output: 1442

🏁 Script executed:

# Check what other Traverse methods exist in DocumentSymbolCollector or similar visitor classes
rg -A2 'Traverse.*Decl\(' src/feature/document_symbols.cpp

# Search for all type alias and typedef related declaration types in Clang
rg -n 'TypeAlias|TypedefDecl|TemplateDecl' --type=cpp src/ | grep -E '(Typedef|TypeAlias|Template.*Decl)' | head -20

Repository: clice-io/clice

Length of output: 2549

🏁 Script executed:

# Check if there are template specialization or other template-related types we might be missing
rg -n 'ClassTemplateDecl|FunctionTemplateDecl|VarTemplateDecl' --type=cpp | head -20

Repository: clice-io/clice

Length of output: 2166

🏁 Script executed:

# Verify that Typedef, TypeAlias, TypeAliasTemplate exist as Clang declaration types
# and check if there are any usages in the current codebase that would be missed
rg -n 'TypedefDecl|TypeAliasDecl|TypeAliasTemplateDecl' --type=cpp src/feature/document_symbols.cpp

Repository: clice-io/clice

Length of output: 40

🏁 Script executed:

# Check if there's a FilteredASTVisitor base class that might have these Traverse methods
fd -t f 'filtered_ast_visitor' -x cat -n {} | head -50

Repository: clice-io/clice

Length of output: 1942

🏁 Script executed:

# Check if RecursiveASTVisitor has Traverse methods for Typedef, TypeAlias, TypeAliasTemplate
# by searching Clang documentation or code patterns
rg -n 'TraverseTypedef|TraverseTypeAlias|Typedef.*Decl' --type=cpp src/ | head -10

Repository: clice-io/clice

Length of output: 934

🏁 Script executed:

# Look at git history to see if these were removed recently
git log --oneline --all -S 'TRAVERSE_SYMBOL_DECL(Typedef' -- src/feature/document_symbols.cpp 2>/dev/null | head -5

Repository: clice-io/clice

Length of output: 40

🏁 Script executed:

# Check if there's any pattern that shows how template declarations should be handled
rg -B5 -A5 'ClassTemplateDecl|FunctionTemplateDecl' src/feature/document_symbols.cpp

Repository: clice-io/clice

Length of output: 40

🏁 Script executed:

# Check symbol_kind.cpp to see how it recognizes TypedefNameDecl
sed -n '1,30p' src/semantic/symbol_kind.cpp

Repository: clice-io/clice

Length of output: 1457

🏁 Script executed:

# Check if code_completion handles TypedefDecl in TRAVERSE or similar
rg -B5 -A5 'TypedefNameDecl' src/feature/code_completion.cpp | head -30

Repository: clice-io/clice

Length of output: 515

Typedefs and type aliases are excluded from document symbols, leaving dead code in symbol_detail.

The TRAVERSE_SYMBOL_DECL macro (lines 154–168) explicitly lists only these declaration types, but excludes Typedef, TypeAlias, and TypeAliasTemplate. Meanwhile, symbol_detail (lines 87–88) contains a branch that prints "type alias" for TypedefNameDecl—code that will never execute because these nodes don't reach push_symbol.

Since symbol_kind.cpp already recognizes TypedefNameDecl as SymbolKind::Type, and other features like code_completion.cpp handle both TypedefNameDecl and TypeAliasTemplateDecl, users will incorrectly stop seeing typedef/using declarations in the document-symbols outline (and potentially template type aliases if they are also missing).

Either remove the stale TypedefNameDecl handling from symbol_detail, or extend the macro to include these types:

Suggested fix 
     TRAVERSE_SYMBOL_DECL(Binding);
     TRAVERSE_SYMBOL_DECL(Concept);
+    TRAVERSE_SYMBOL_DECL(Typedef);
+    TRAVERSE_SYMBOL_DECL(TypeAlias);
+    TRAVERSE_SYMBOL_DECL(TypeAliasTemplate);

Also add test coverage for typedef and type alias declarations to prevent regressions.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/feature/document_symbols.cpp` around lines 148 - 170, The
TRAVERSE_SYMBOL_DECL macro currently omits typedef/type alias nodes so
TypedefNameDecl handling in symbol_detail is dead: update the macro invocations
to also include Typedef, TypeAlias, and TypeAliasTemplate (i.e., add
TRAVERSE_SYMBOL_DECL(Typedef); TRAVERSE_SYMBOL_DECL(TypeAlias);
TRAVERSE_SYMBOL_DECL(TypeAliasTemplate);) so push_symbol is called for those
decls and symbol_detail's TypedefNameDecl branch is reachable; alternatively, if
you prefer removing dead code, delete the TypedefNameDecl/type alias branch from
symbol_detail. After making the change, add unit/integration tests that assert
typedef and using/type-alias-template declarations appear in the
document-symbols output.


auto collect() -> std::vector<InternalSymbol> {
TraverseDecl(unit.tu());
return std::move(result.symbols);
}

private:
static bool is_interested(clang::Decl* decl) {
switch(decl->getKind()) {
case clang::Decl::Namespace:
case clang::Decl::Enum:
case clang::Decl::EnumConstant:
case clang::Decl::Function:
case clang::Decl::CXXMethod:
case clang::Decl::CXXConstructor:
case clang::Decl::CXXDestructor:
case clang::Decl::CXXConversion:
case clang::Decl::CXXDeductionGuide:
case clang::Decl::Record:
case clang::Decl::CXXRecord:
case clang::Decl::Field:
case clang::Decl::Var:
case clang::Decl::Binding:
case clang::Decl::Concept: return true;
default: return false;
}
}

private:
SymbolFrame result;
};
Expand Down
31 changes: 3 additions & 28 deletions src/semantic/filtered_ast_visitor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ class FilteredASTVisitor : public clang::RecursiveASTVisitor<Derived> {
return true;
}

/// Clang doesn't visit implicit declarations and
/// implicit template instantiations by default.
if(llvm::isa<clang::TranslationUnitDecl>(decl)) {
if(interested_only) {
for(auto top: unit.top_level_decls()) {
Expand All @@ -46,34 +48,7 @@ class FilteredASTVisitor : public clang::RecursiveASTVisitor<Derived> {
return Base::TraverseDecl(decl);
}

if(decl->isImplicit()) {
return true;
}

/// We don't want to visit implicit instantiation.
if(auto SD = llvm::dyn_cast<clang::ClassTemplateSpecializationDecl>(decl)) {
if(SD->getSpecializationKind() == clang::TSK_ImplicitInstantiation) {
return true;
}
}

if(auto SD = llvm::dyn_cast<clang::FunctionDecl>(decl)) {
if(SD->getTemplateSpecializationKind() == clang::TSK_ImplicitInstantiation) {
return true;
}
}

if(auto SD = llvm::dyn_cast<clang::VarTemplateSpecializationDecl>(decl)) {
if(SD->getSpecializationKind() == clang::TSK_ImplicitInstantiation) {
return true;
}
}

if constexpr(requires { getDerived().on_traverse_decl(decl, &Base::TraverseDecl); }) {
return getDerived().on_traverse_decl(decl, &Base::TraverseDecl);
} else {
return Base::TraverseDecl(decl);
}
return Base::TraverseDecl(decl);
}

bool TraverseStmt(clang::Stmt* stmt) {
Expand Down
24 changes: 5 additions & 19 deletions src/semantic/semantic_visitor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#pragma once

#include "compile/directive.h"
#include "semantic/ast_utility.h"
#include "semantic/filtered_ast_visitor.h"
#include "semantic/relation_kind.h"
Expand Down Expand Up @@ -107,28 +108,13 @@ class SemanticVisitor : public FilteredASTVisitor<SemanticVisitor<Derived>> {
}

void run() {
if(Base::interested_only) {
for(auto decl: unit.top_level_decls()) {
Base::TraverseDecl(decl);
}
} else {
Base::TraverseAST(unit.context());
}
Base::TraverseAST(unit.context());

for(auto directive: unit.directives()) {
for(auto macro: directive.second.macros) {
switch(macro.kind) {
case MacroRef::Kind::Def: {
handleMacroOccurrence(macro.macro, RelationKind::Definition, macro.loc);
break;
}

case MacroRef::Kind::Ref:
case MacroRef::Kind::Undef: {
handleMacroOccurrence(macro.macro, RelationKind::Reference, macro.loc);
break;
}
}
auto kind = macro.kind == MacroRef::Kind::Def ? RelationKind::Definition
: RelationKind::Reference;
handleMacroOccurrence(macro.macro, kind, macro.loc);
}
}
}
Expand Down
Loading