Skip to content

feat(completion): smart paren detection to avoid duplicate parens #415

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
16bit-ykiko wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(completion): smart paren detection to avoid duplicate parens #415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
74 changes: 58 additions & 16 deletions src/feature/code_completion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,12 +158,28 @@ auto extract_signature(const clang::CodeCompletionString& ccs) -> std::string {
return signature;
}

/// 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';
}
Comment on lines +161 to +171

@coderabbitai coderabbitai Bot Apr 18, 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 | 🟡 Minor

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.


/// Build a snippet string from a CodeCompletionString.
/// Produces e.g. "funcName(${1:int x}, ${2:float y})" for functions,
/// or "ClassName<${1:T}>" for class templates.
auto build_snippet(const clang::CodeCompletionString& ccs) -> std::string {
/// If skip_parens is true, omits everything from '(' onward (when the next
/// token after the cursor is already '(').
auto build_snippet(const clang::CodeCompletionString& ccs, bool skip_parens = false)
-> std::string {
std::string snippet;
unsigned placeholder_index = 0;
bool in_parens = false;

for(const auto& chunk: ccs) {
using CK = clang::CodeCompletionString::ChunkKind;
Expand All @@ -174,33 +190,47 @@ auto build_snippet(const clang::CodeCompletionString& ccs) -> std::string {
}
break;
case CK::CK_Placeholder:
if(in_parens && skip_parens) {
break;
}
if(chunk.Text) {
snippet += std::format("${{{0}:{1}}}", ++placeholder_index, chunk.Text);
}
break;
case CK::CK_LeftParen: snippet += '('; break;
case CK::CK_RightParen: snippet += ')'; break;
case CK::CK_LeftParen:
in_parens = true;
if(!skip_parens) {
snippet += '(';
}
break;
case CK::CK_RightParen:
in_parens = false;
if(!skip_parens) {
snippet += ')';
}
break;
case CK::CK_LeftAngle: snippet += '<'; break;
case CK::CK_RightAngle: snippet += '>'; break;
case CK::CK_Comma: snippet += ", "; break;
case CK::CK_Comma:
if(!(in_parens && skip_parens)) {
snippet += ", ";
}
break;
case CK::CK_Text:
if(chunk.Text) {
if(!(in_parens && skip_parens) && chunk.Text) {
snippet += chunk.Text;
}
break;
case CK::CK_Optional:
// Optional chunks contain default arguments — skip for snippet.
break;
case CK::CK_Optional: break;
case CK::CK_Informative:
case CK::CK_ResultType:
case CK::CK_CurrentParameter:
// Display-only chunks, not part of insertion.
break;
case CK::CK_CurrentParameter: break;
default: break;
}
}

// If no placeholders were generated, return empty to signal plain text.
// If no placeholders were generated and parens were skipped,
// return empty to signal plain text.
if(placeholder_index == 0) {
return {};
}
Expand Down Expand Up @@ -229,9 +259,11 @@ class CodeCompletionCollector final : public clang::CodeCompleteConsumer {
CodeCompletionCollector(std::uint32_t offset,
PositionEncoding encoding,
std::vector<protocol::CompletionItem>& output,
const CodeCompletionOptions& options) :
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>()) {}
Comment on lines +262 to +266

@coderabbitai coderabbitai Bot Apr 18, 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
# 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.cpp

Repository: 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>()).


clang::CodeCompletionAllocator& getAllocator() final {
return info.getAllocator();
Expand Down Expand Up @@ -425,7 +457,8 @@ class CodeCompletionCollector final : public clang::CodeCompleteConsumer {
// Generate snippet for non-bundled callables.
if(is_callable && !options.bundle_overloads &&
options.enable_function_arguments_snippet) {
snippet = build_snippet(*ccs);
bool next_is_paren = next_token_char(original_content, offset) == '(';
snippet = build_snippet(*ccs, /*skip_parens=*/next_is_paren);
}
}

Expand Down Expand Up @@ -495,6 +528,7 @@ class CodeCompletionCollector final : public clang::CodeCompleteConsumer {
std::uint32_t offset;
PositionEncoding encoding;
std::vector<protocol::CompletionItem>& output;
llvm::StringRef original_content;
const CodeCompletionOptions& options;
clang::CodeCompletionTUInfo info;
};
Expand All @@ -509,7 +543,15 @@ auto code_complete(CompilationParams& params,
auto& [file, offset] = params.completion;
(void)file;

auto* consumer = new CodeCompletionCollector(offset, encoding, items, options);
// Get the original file content for lookahead (smart parens detection).
llvm::StringRef original_content;
auto buf_it = params.buffers.find(file);
if(buf_it != params.buffers.end()) {
original_content = buf_it->second->getBuffer();
}

auto* consumer =
new CodeCompletionCollector(offset, encoding, items, options, original_content);
auto unit = complete(params, consumer);
(void)unit;

Expand Down
38 changes: 38 additions & 0 deletions tests/unit/feature/code_completion_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,44 @@ int z = fo$(pos)
*it->insert_text_format == protocol::InsertTextFormat::PlainText);
}

TEST_CASE(SmartParensSkip) {
// When next token after cursor is '(', snippet should not insert parens.
feature::CodeCompletionOptions opts;
opts.bundle_overloads = false;
opts.enable_function_arguments_snippet = true;
code_complete(R"cpp(
int foooo(int x, float y);
int z = fo$(pos)(1, 2.0f);
)cpp",
opts);

auto it = find_item("foooo");
ASSERT_TRUE(it != items.end());
// With parens already present, snippet should degrade to plain text
// (no placeholders → build_snippet returns empty → label used).
auto& edit = std::get<protocol::TextEdit>(*it->text_edit);
ASSERT_TRUE(edit.new_text.find("(") == std::string::npos);
}

TEST_CASE(SmartParensInsert) {
// When next token is NOT '(', snippet should include parens normally.
feature::CodeCompletionOptions opts;
opts.bundle_overloads = false;
opts.enable_function_arguments_snippet = true;
code_complete(R"cpp(
int foooo(int x, float y);
int z = fo$(pos);
)cpp",
opts);

auto it = find_item("foooo");
ASSERT_TRUE(it != items.end());
auto& edit = std::get<protocol::TextEdit>(*it->text_edit);
// Should contain '(' since there's no existing paren.
ASSERT_TRUE(edit.new_text.find("(") != std::string::npos);
ASSERT_TRUE(edit.new_text.find("${1:") != std::string::npos);
}

TEST_CASE(SnippetBundleMode) {
// In bundle mode, snippets should NOT be generated even if enabled.
feature::CodeCompletionOptions opts;
Expand Down
Loading