feat(completion): smart paren detection to avoid duplicate parens#415
Open
16bit-ykiko wants to merge 1 commit intomainfrom
Open
feat(completion): smart paren detection to avoid duplicate parens#41516bit-ykiko wants to merge 1 commit intomainfrom
16bit-ykiko wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughThe PR enhances code completion snippet generation by implementing smart parentheses detection. When the completion cursor is immediately followed by a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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 |
When the next non-whitespace character after the cursor is already '(',
skip inserting parentheses and parameter placeholders in the snippet.
This prevents duplicate parens when completing a function name that
already has arguments written after it.
Uses the original file content (not Clang's internal buffer) for
lookahead to correctly detect the next token.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16bit-ykiko
force-pushed
the
feat/completion-smart-parens
branch
from
680086a to
8e9d4fa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/feature/code_completion.cpp (1)
232-236: Misleading comment.The guard is unconditional on
placeholder_index == 0— it fires whether or notskip_parenswas set (e.g.void foo()withskip_parens=falsealso returns{}). The comment implies a combined condition. Either tighten the comment or make the check match it.📝 Suggested comment tweak
- // If no placeholders were generated and parens were skipped, - // return empty to signal plain text. + // If no placeholders were generated, signal plain text by returning empty + // (callers fall back to the label). This covers no-arg functions and the + // skip_parens=true case where all placeholders were suppressed. if(placeholder_index == 0) { return {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/feature/code_completion.cpp` around lines 232 - 236, The comment incorrectly states the guard depends on skip_parens but the code only checks placeholder_index; update the conditional to match the comment by changing the guard to check both placeholder_index == 0 and skip_parens (e.g., if(placeholder_index == 0 && skip_parens) { return {}; }) or alternatively change the comment to say the return is unconditional when no placeholders were generated; locate the check around placeholder_index and skip_parens in the function in src/feature/code_completion.cpp and apply one of these two fixes to keep code and comment consistent.tests/unit/feature/code_completion_tests.cpp (1)
338-374: Consider adding a whitespace-between-cursor-and-paren test.Both new tests are well-targeted, but neither exercises the whitespace-skipping branch of
next_token_char(the whole reason we iterate instead of just peekingcontent[offset]). An extra case likeint z = fo$(pos) (1, 2.0f);(with spaces/newlines before the() would lock in that behavior and prevent accidental regressions if someone later simplifies the lookahead to a single-char check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/feature/code_completion_tests.cpp` around lines 338 - 374, Add a new unit test that mirrors SmartParensSkip but places whitespace/newlines between the cursor and the following '(' to exercise the whitespace-skipping branch used by next_token_char; call it e.g. SmartParensSkipWithWhitespace, set feature::CodeCompletionOptions the same way as the other tests (bundle_overloads=false, enable_function_arguments_snippet=true), invoke code_complete with a snippet like "int foooo(...); int z = fo$(pos) (1, 2.0f);" and then locate the completion via find_item("foooo") and assert (like SmartParensSkip) that the resolved protocol::TextEdit new_text does not contain "(" so the snippet degrades to plain text.
🤖 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/code_completion.cpp`:
- Around line 161-171: next_token_char currently only skips whitespace and will
return '/' when a comment follows the cursor, breaking smart-paren logic; update
next_token_char to also skip C++-style comments by detecting '/' then peeking
the next char: if "//" advance i until newline or end, if "/*" advance i until
the matching "*/" or end (being careful with bounds), otherwise treat '/' as a
token; continue the outer search after skipping comments and return '\0' on EOF.
Ensure you reference and update the loop and index handling in next_token_char
to correctly advance past multi-char comment sequences and avoid out-of-bounds
peeks.
- Around line 262-266: The constructor's member initializer list initializes
members in the wrong order causing -Wreorder: reorder the initializers to match
the declaration order (offset, encoding, output, original_content, options,
info) — specifically swap the positions of options and original_content in the
initializer list so original_content is initialized before options (keep info
last as std::make_shared<clang::GlobalCodeCompletionAllocator>()).
---
Nitpick comments:
In `@src/feature/code_completion.cpp`:
- Around line 232-236: The comment incorrectly states the guard depends on
skip_parens but the code only checks placeholder_index; update the conditional
to match the comment by changing the guard to check both placeholder_index == 0
and skip_parens (e.g., if(placeholder_index == 0 && skip_parens) { return {}; })
or alternatively change the comment to say the return is unconditional when no
placeholders were generated; locate the check around placeholder_index and
skip_parens in the function in src/feature/code_completion.cpp and apply one of
these two fixes to keep code and comment consistent.
In `@tests/unit/feature/code_completion_tests.cpp`:
- Around line 338-374: Add a new unit test that mirrors SmartParensSkip but
places whitespace/newlines between the cursor and the following '(' to exercise
the whitespace-skipping branch used by next_token_char; call it e.g.
SmartParensSkipWithWhitespace, set feature::CodeCompletionOptions the same way
as the other tests (bundle_overloads=false,
enable_function_arguments_snippet=true), invoke code_complete with a snippet
like "int foooo(...); int z = fo$(pos) (1, 2.0f);" and then locate the
completion via find_item("foooo") and assert (like SmartParensSkip) that the
resolved protocol::TextEdit new_text does not contain "(" so the snippet
degrades to plain text.
🪄 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: f457ba2a-a0b1-4533-9555-12f585fab066
📒 Files selected for processing (2)
src/feature/code_completion.cpptests/unit/feature/code_completion_tests.cpp
Comment on lines
+161
to
+171
| /// Find the first non-whitespace character after the given offset in content. | ||
| /// Returns '\0' if none found (end of content). | ||
| auto next_token_char(llvm::StringRef content, std::uint32_t offset) -> char { | ||
| for(auto i = offset; i < content.size(); ++i) { | ||
| char c = content[i]; | ||
| if(c != ' ' && c != '\t' && c != '\n' && c != '\r') { | ||
| return c; | ||
| } | ||
| } | ||
| return '\0'; | ||
| } |
There was a problem hiding this comment.
Lookahead does not skip comments.
next_token_char only skips whitespace. A cursor followed by a comment before the (, such as foo|/* comment */(args) or foo| // comment\n(args), will return / and fail to trigger smart-paren skipping, causing duplicate parens in those (uncommon but real) cases. If you want this to be robust, consider skipping // line comments and /* */ block comments too. Otherwise this is an acceptable limitation worth a doc note.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/feature/code_completion.cpp` around lines 161 - 171, next_token_char
currently only skips whitespace and will return '/' when a comment follows the
cursor, breaking smart-paren logic; update next_token_char to also skip
C++-style comments by detecting '/' then peeking the next char: if "//" advance
i until newline or end, if "/*" advance i until the matching "*/" or end (being
careful with bounds), otherwise treat '/' as a token; continue the outer search
after skipping comments and return '\0' on EOF. Ensure you reference and update
the loop and index handling in next_token_char to correctly advance past
multi-char comment sequences and avoid out-of-bounds peeks.
Comment on lines
+262
to
+266
| const CodeCompletionOptions& options, | ||
| llvm::StringRef original_content) : | ||
| clang::CodeCompleteConsumer({}), offset(offset), encoding(encoding), output(output), | ||
| options(options), info(std::make_shared<clang::GlobalCodeCompletionAllocator>()) {} | ||
| options(options), original_content(original_content), | ||
| info(std::make_shared<clang::GlobalCodeCompletionAllocator>()) {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the member declaration order vs initializer order in the file under review.
rg -nP -C1 '^\s*(std::uint32_t offset;|PositionEncoding encoding;|std::vector<protocol::CompletionItem>& output;|llvm::StringRef original_content;|const CodeCompletionOptions& options;|clang::CodeCompletionTUInfo info;)' src/feature/code_completion.cpp
rg -nP -C2 'clang::CodeCompleteConsumer\(\{\}\), offset\(offset\)' src/feature/code_completion.cppRepository: clice-io/clice
Length of output: 881
Member initializer order does not match declaration order (-Wreorder).
The data members are declared as offset (528), encoding (529), output (530), original_content (531), options (532), but initialized as offset, encoding, output, options, original_content (lines 264-265). C++ requires member initialization in declaration order; this mismatch triggers -Wreorder, which will fail with -Werror.
🔧 Proposed fix
private:
std::uint32_t offset;
PositionEncoding encoding;
std::vector<protocol::CompletionItem>& output;
- llvm::StringRef original_content;
const CodeCompletionOptions& options;
+ llvm::StringRef original_content;
clang::CodeCompletionTUInfo info;Or swap the initializers on lines 265-266 to match declaration order.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/feature/code_completion.cpp` around lines 262 - 266, The constructor's
member initializer list initializes members in the wrong order causing
-Wreorder: reorder the initializers to match the declaration order (offset,
encoding, output, original_content, options, info) — specifically swap the
positions of options and original_content in the initializer list so
original_content is initialized before options (keep info last as
std::make_shared<clang::GlobalCodeCompletionAllocator>()).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
(, skip inserting parentheses and parameter placeholders in the snippetfo|(args)→ inserts justfoooo, notfoooo(${1:...})Test plan
SmartParensSkip— next char is(, no parens insertedSmartParensInsert— next char is;, parens inserted normallypixi run formatcleanStacked on #412.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests