Skip to content

fix(semantic-tokens): filter ineligible highlight references#434

Open
Myriad-Dreamin wants to merge 7 commits intomainfrom
requests/highlight-name-filter/d9eec958-acd5-4df7-92cc--c074498e6e1badd9/a1-proposal-1
Open

fix(semantic-tokens): filter ineligible highlight references#434
Myriad-Dreamin wants to merge 7 commits intomainfrom
requests/highlight-name-filter/d9eec958-acd5-4df7-92cc--c074498e6e1badd9/a1-proposal-1

Conversation

@Myriad-Dreamin
Copy link
Copy Markdown
Contributor

@Myriad-Dreamin Myriad-Dreamin commented Apr 21, 2026

Add a reusable declaration-name eligibility helper that mirrors clangd's canHighlightName, use it to suppress unsupported reference tokens in the semantic-token collector, and cover the change with focused semantic-token regression tests plus a constructor/destructor positive case.

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced semantic token filtering to prevent certain declaration references from being highlighted when they cannot be properly highlighted, while maintaining support for constructors and destructors.
  • Tests

    • Added test coverage for operator reference suppression and constructor/destructor name highlighting behavior.
  • Chores

    • Updated .gitignore configuration.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 21, 2026

📝 Walkthrough

Walkthrough

This PR adds suppression logic for semantic tokens on references with non-highlightable declaration names. A new utility function classifies declaration names (Identifiers, constructors, destructors) that can be highlighted. The semantic token collector now early-returns for references with non-highlightable names, preventing token emission.

Changes

Cohort / File(s) Summary
Configuration
.gitignore
Updated ignore patterns to ignore .codex and removed -openspec/ entry.
Utility Functions
src/semantic/ast_utility.h, src/semantic/ast_utility.cpp
Added new can_highlight_name() function that classifies declaration name kinds, returning true for Identifiers, constructors, and destructors; false for operators, conversion functions, and other ineligible kinds. Includes exhaustiveness check via std::unreachable().
Feature Implementation
src/feature/semantic_tokens.cpp
Added early-return in handleDeclOccurrence when relation indicates a reference and the referenced declaration name cannot be highlighted, preventing token emission for ineligible references.
Tests
tests/unit/feature/semantic_tokens_tests.cpp
Added EXPECT_NO_TOKEN() assertion helper and two test cases: IneligibleOperatorReferenceIsSuppressed validates operator references are skipped; ConstructorAndDestructorNamesRemainHighlighted verifies constructor/destructor identifiers retain semantic tokens with appropriate modifiers.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • 16bit-ykiko

Poem

🐰 A filter for tokens, so spry and so bright,
We highlight what matters and hide what's not right,
Constructors shine golden, operators take flight—
Now references filter with surgical might! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: filtering ineligible highlight references in semantic tokens, which is the primary objective evident from the code changes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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 requests/highlight-name-filter/d9eec958-acd5-4df7-92cc--c074498e6e1badd9/a1-proposal-1

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

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

@Myriad-Dreamin Myriad-Dreamin marked this pull request as ready for review April 21, 2026 18:54
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
tests/unit/feature/semantic_tokens_tests.cpp (1)

285-307: Add a constructor reference case to exercise the new guard.

Line 303 and Line 305 assert constructor declaration/definition tokens, but the new filter only runs for references. If CXXConstructorName were accidentally removed from can_highlight_name, these constructor assertions would still pass. Add a constructor-call/reference assertion alongside the destructor reference.

Suggested coverage addition
 void use(S* value) {
+    (void)@ctor_ref[S]();
     value->@dtor_ref[~]S();
 }
 )cpp");
@@
     EXPECT_TOKEN("ctor_decl", SymbolKind::Method, declaration | special_member);
     EXPECT_TOKEN("dtor_decl", SymbolKind::Method, declaration | special_member);
     EXPECT_TOKEN("ctor_def", SymbolKind::Method, definition | special_member);
+    EXPECT_TOKEN("ctor_ref", SymbolKind::Method, special_member);
     EXPECT_TOKEN("dtor_ref", SymbolKind::Method, special_member);
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/feature/semantic_tokens_tests.cpp` around lines 285 - 307, Add a
constructor-reference case in the ConstructorAndDestructorNamesRemainHighlighted
test by inserting a constructor call annotated like new `@ctor_ref`[S](); (or
equivalent call site) inside the run_utf8 sample, and then add an assertion
EXPECT_TOKEN("ctor_ref", SymbolKind::Method, special_member) alongside the
existing dtor_ref assertion; this ensures the new guard that runs only for
references (can_highlight_name) is exercised for constructors as well. Ensure
you use the same modifier_mask special_member and the unique token name
"ctor_ref" so the test locates the annotation and validates the constructor
reference highlighting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/unit/feature/semantic_tokens_tests.cpp`:
- Around line 285-307: Add a constructor-reference case in the
ConstructorAndDestructorNamesRemainHighlighted test by inserting a constructor
call annotated like new `@ctor_ref`[S](); (or equivalent call site) inside the
run_utf8 sample, and then add an assertion EXPECT_TOKEN("ctor_ref",
SymbolKind::Method, special_member) alongside the existing dtor_ref assertion;
this ensures the new guard that runs only for references (can_highlight_name) is
exercised for constructors as well. Ensure you use the same modifier_mask
special_member and the unique token name "ctor_ref" so the test locates the
annotation and validates the constructor reference highlighting.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 58aa4fe0-cee0-48a2-ab29-e854b9931069

📥 Commits

Reviewing files that changed from the base of the PR and between 17e6801 and f1c2841.

📒 Files selected for processing (5)
  • .gitignore
  • src/feature/semantic_tokens.cpp
  • src/semantic/ast_utility.cpp
  • src/semantic/ast_utility.h
  • tests/unit/feature/semantic_tokens_tests.cpp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant