feat: classify more semantic token modifiers

[Myriad-Dreamin]

Summary

• add declaration helpers to classify semantic token modifiers such as readonly, static, abstract, virtual, default library, and constructor/destructor
• unwrap template declarations before applying attribute-style modifiers so modifier checks hit the underlying declaration
• keep templated/dependent-name handling and expand emitted semantic token modifiers in semantic_tokens.cpp

Testing

• Not run (not requested in this turn)

Summary by CodeRabbit

• Chores
  • Enhanced semantic token analysis to improve code recognition and more accurate classification of declarations across C++ and Objective-C codebases.

[coderabbitai]

📝 Walkthrough

Walkthrough

Added helper predicates to classify declarations (default library, static, const, abstract, virtual) in semantic_tokens.cpp. Modified handleDeclOccurrence to unwrap template declarations and activate previously-commented symbol modifier computations for semantic token enrichment.

Changes

Cohort / File(s)	Summary
Semantic Token Modifiers Expansion src/feature/semantic_tokens.cpp	Added six new classification predicates: is_default_library, is_static (C++/Objective-C variants), is_const(QualType), is_const(Decl), is_abstract, and is_virtual. Enhanced handleDeclOccurrence to unwrap TemplateDecl and activate SymbolModifiers for Readonly, Static, Abstract, Virtual, DefaultLibrary, Deprecated, DependentName, and ConstructorOrDestructor.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• feat: classify dependent-name semantic token modifier #387: Directly overlaps with changes adding DependentName handling and expanding SymbolModifiers in handleDeclOccurrence.

Poem

🐰 With whiskers twitching, bright and keen,
New modifiers grace the semantic scene!
Static, const, virtual too,
Template unwrapping, fresh and new! ✨
Our tokens sing with richer detail,
A hoppy hop to glory's trail! 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat: classify more semantic token modifiers' accurately describes the main change: adding new helper predicates to classify semantic token modifiers (readonly, static, abstract, virtual, default library, constructor/destructor) in semantic_tokens.cpp.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch detect-semantic-token-modifiers

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/feature/semantic_tokens.cpp (2)

190-195: Consider using a separate variable for the unwrapped declaration.

Reassigning decl mid-function can be error-prone for maintainability. If future code is added above line 190 expecting the unwrapped decl, or vice versa, subtle bugs could occur.

♻️ Suggested refactor using a separate variable

-        // Apply attribute-style modifiers to the underlying declaration.
-        // The attribute tests don't want to look at the template.
-        if(const auto* template_decl = llvm::dyn_cast<clang::TemplateDecl>(decl)) {
-            if(const auto* templated_decl = template_decl->getTemplatedDecl())
-                decl = templated_decl;
-        }
+        // Apply attribute-style modifiers to the underlying declaration.
+        // The attribute tests don't want to look at the template.
+        const clang::NamedDecl* attr_decl = decl;
+        if(const auto* template_decl = llvm::dyn_cast<clang::TemplateDecl>(decl)) {
+            if(const auto* templated_decl = template_decl->getTemplatedDecl())
+                attr_decl = templated_decl;
+        }

Then use attr_decl for the modifier checks below.

🤖 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 190 - 195, The code reassigns
the local variable decl to the templated declaration which is fragile; instead
introduce a new variable (e.g., attr_decl or unwrapped_decl) to hold the result
of llvm::dyn_cast<clang::TemplateDecl>(decl)->getTemplatedDecl() when present
and use that new variable for the subsequent attribute/modifier checks, leaving
the original decl untouched; update references in the surrounding logic that
currently use decl for attribute-style modifier application to use attr_decl
(falling back to decl when no templated decl exists).

---

138-140: Questionable: Marking functions as readonly based on return type.

A function returning a const type (e.g., const int* foo()) isn't inherently "readonly"—it may still have side effects. This could lead to misleading highlighting where const int* getData() is marked readonly but void logAccess() (a pure side-effect function) isn't.

Consider whether this case should be removed, or if the intent is specifically to highlight functions that return something immutable (in which case a different modifier like Const might be more appropriate than Readonly).

🤖 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 138 - 140, The current logic in
semantic_tokens.cpp marks functions as readonly by checking the return type (the
dyn_cast<clang::FunctionDecl>(decl) branch returning
is_const(function->getReturnType())), which is misleading; update this by
removing that return-type-based readonly check or by mapping it to a distinct
modifier (e.g., "Const" rather than "Readonly"). Concretely: locate the
dyn_cast<clang::FunctionDecl>(decl) branch and either delete the return of
is_const(function->getReturnType()) so functions are not marked readonly based
on return type, or replace the behavior so that when
is_const(function->getReturnType()) is true you emit a different token/modifier
name (e.g., Const) and leave Readonly for true side-effect-free analysis; ensure
the token emission functions and modifier enums are updated accordingly (adjust
any uses of is_const, the FunctionDecl branch, and the token/modifier mapping).

🤖 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 85-87: Remove the clang::FunctionDecl special-case that returns
function->isStatic(): locate the llvm::dyn_cast<clang::FunctionDecl>(decl) block
(the one that does "if(const auto* function =
llvm::dyn_cast<clang::FunctionDecl>(decl)) { return function->isStatic(); }")
and delete it so namespace-scoped functions marked static are no longer treated
as having the 'static' modifier; allow the function to fall through to the
existing handling used for other decl kinds instead.

---

Nitpick comments:
In `@src/feature/semantic_tokens.cpp`:
- Around line 190-195: The code reassigns the local variable decl to the
templated declaration which is fragile; instead introduce a new variable (e.g.,
attr_decl or unwrapped_decl) to hold the result of
llvm::dyn_cast<clang::TemplateDecl>(decl)->getTemplatedDecl() when present and
use that new variable for the subsequent attribute/modifier checks, leaving the
original decl untouched; update references in the surrounding logic that
currently use decl for attribute-style modifier application to use attr_decl
(falling back to decl when no templated decl exists).
- Around line 138-140: The current logic in semantic_tokens.cpp marks functions
as readonly by checking the return type (the dyn_cast<clang::FunctionDecl>(decl)
branch returning is_const(function->getReturnType())), which is misleading;
update this by removing that return-type-based readonly check or by mapping it
to a distinct modifier (e.g., "Const" rather than "Readonly"). Concretely:
locate the dyn_cast<clang::FunctionDecl>(decl) branch and either delete the
return of is_const(function->getReturnType()) so functions are not marked
readonly based on return type, or replace the behavior so that when
is_const(function->getReturnType()) is true you emit a different token/modifier
name (e.g., Const) and leave Readonly for true side-effect-free analysis; ensure
the token emission functions and modifier enums are updated accordingly (adjust
any uses of is_const, the FunctionDecl branch, and the token/modifier mapping).

🪄 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: ec9a4842-4210-46a1-a769-d41a466fbdfa

📥 Commits

Reviewing files that changed from the base of the PR and between 8d4ad26 and b6ec5e5.

📒 Files selected for processing (1)

• src/feature/semantic_tokens.cpp