Skip to content

refactor(server, tests): improve error feedback, logging, and test infrastructure #436

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

refactor(server, tests): improve error feedback, logging, and test infrastructure #436

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
5 changes: 5 additions & 0 deletions src/compile/compilation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ CompilationStatus CompilationUnitRef::Self::run_clang(
std::unique_ptr diagnostic_consumer = self.create_diagnostic();
std::unique_ptr invocation = self.create_invocation(params, diagnostic_consumer.get());
if(!invocation) {
LOG_WARN("run_clang: invocation creation failed");
return CompilationStatus::SetupFail;
}

Expand All @@ -256,6 +257,7 @@ CompilationStatus CompilationUnitRef::Self::run_clang(
}

if(!instance.createTarget()) {
LOG_WARN("run_clang: target creation failed");
return CompilationStatus::SetupFail;
}

Expand All @@ -270,6 +272,7 @@ CompilationStatus CompilationUnitRef::Self::run_clang(
/// But if we fail to `BeginSourceFile` we don't need to call `EndSourceFile`. So just
/// reset it.
self.action.reset();
LOG_WARN("run_clang: BeginSourceFile failed");
return CompilationStatus::SetupFail;
}

Expand Down Expand Up @@ -303,6 +306,8 @@ CompilationStatus CompilationUnitRef::Self::run_clang(
/// in crash frequently. So forbidden it here and return as error.
if(!instance.getFrontendOpts().OutputFile.empty() &&
instance.getDiagnostics().hasErrorOccurred()) {
LOG_WARN("run_clang: errors during PCH/PCM generation, output={}",
instance.getFrontendOpts().OutputFile);
return CompilationStatus::FatalError;
}

Expand Down
79 changes: 60 additions & 19 deletions src/server/compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,11 @@ void Compiler::init_compile_graph() {

auto result = co_await pool.send_stateless(bp);
if(!result.has_value() || !result.value().success) {
LOG_WARN("BuildPCM failed for module {}: {}",
mod_it->second,
result.has_value() ? result.value().error : result.error().message);
auto error_msg = result.has_value() ? result.value().error : result.error().message;
LOG_WARN("BuildPCM failed for module {}: {}", mod_it->second, error_msg);
peer.send_notification(protocol::LogMessageParams{
protocol::MessageType::Warning,
std::format("PCM build failed for module {}: {}", mod_it->second, error_msg)});
co_return false;
}

Expand Down Expand Up @@ -171,6 +173,10 @@ bool Compiler::fill_compile_args(llvm::StringRef path,
auto& cmd = results.front();
directory = cmd.resolved.directory.str();
arguments = cmd.to_string_argv();
LOG_DEBUG("fill_compile_args: CDB match for {} (dir={}, {} args)",
path,
directory,
arguments.size());
return true;
}

Expand Down Expand Up @@ -521,9 +527,11 @@ kota::task<bool> Compiler::ensure_pch(Session& session,
auto result = co_await pool.send_stateless(bp);

if(!result.has_value() || !result.value().success) {
LOG_WARN("PCH build failed for {}: {}",
path,
result.has_value() ? result.value().error : result.error().message);
auto error_msg = result.has_value() ? result.value().error : result.error().message;
LOG_WARN("PCH build failed for {}: {}", path, error_msg);
peer.send_notification(protocol::LogMessageParams{
protocol::MessageType::Warning,
std::format("PCH build failed for {}: {}", path, error_msg)});
workspace.pch_cache[path_id].building.reset();
completion->set();
co_return false;
Expand Down Expand Up @@ -730,13 +738,20 @@ kota::task<bool> Compiler::ensure_compiled(Session& session) {
params.version = sess->version;
params.text = sess->text;
if(!self->fill_compile_args(file_path, params.directory, params.arguments, sess)) {
LOG_WARN("ensure_compiled: no compile args for {}", uri_str);
self->peer.send_notification(protocol::LogMessageParams{
protocol::MessageType::Warning,
std::format("No compile arguments available for {}", file_path)});
finish_compile();
co_return;
}

if(!co_await self
->ensure_deps(*sess, params.directory, params.arguments, params.pch, params.pcms)) {
LOG_WARN("Dependency preparation failed for {}, skipping compile", uri_str);
self->peer.send_notification(protocol::LogMessageParams{
protocol::MessageType::Warning,
std::format("Dependency preparation failed for {}", file_path)});
finish_compile();
co_return;
}
Expand Down Expand Up @@ -768,6 +783,9 @@ kota::task<bool> Compiler::ensure_compiled(Session& session) {

if(!result.has_value()) {
LOG_WARN("Compile failed for {}: {}", uri_str, result.error().message);
self->peer.send_notification(protocol::LogMessageParams{
protocol::MessageType::Error,
std::format("Compilation failed for {}: {}", file_path, result.error().message)});
self->clear_diagnostics(uri_str);
finish_compile();
co_return;
Expand Down Expand Up @@ -816,11 +834,17 @@ Compiler::RawResult Compiler::forward_query(worker::QueryKind kind,
auto text = session.text;

if(!co_await ensure_compiled(session)) {
co_return serde_raw{"null"};
LOG_WARN("forward_query: compilation failed for {}", path);
co_await kota::fail("Compilation failed");
}

auto sit = sessions.find(path_id);
if(sit == sessions.end() || sit->second.ast_dirty) {
if(sit == sessions.end()) {
LOG_WARN("forward_query: session lost after compile for {}", path);
co_await kota::fail("Document was closed during compilation");
}
if(sit->second.ast_dirty) {
LOG_DEBUG("forward_query: still dirty after compile for {} (concurrent edit)", path);
co_return serde_raw{"null"};
}
Comment on lines 836 to 849

@coderabbitai coderabbitai Bot Apr 23, 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

Don’t report concurrent edits as compilation failures.

ensure_compiled() returns false when the session remains dirty, including the generation-mismatch path for edits during compilation. Because Line 838 fails before the Line 846 dirty check, that branch is effectively unreachable for this case and normal rapid edits can surface as "Compilation failed".

Return a richer compile status, or re-check the current session state before converting the result into an LSP error.

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

In `@src/server/compiler.cpp` around lines 836 - 849, The current forward_query
treats any false from ensure_compiled(session) as a compilation failure and
returns an LSP error, which misclassifies generation-mismatch/concurrent edits;
change the logic so that when ensure_compiled(session) returns false you
re-check the live session state (find sessions via sessions.find(path_id) and
inspect sit->second.ast_dirty) before emitting an error: if the session was
closed, keep the fail("Document was closed during compilation") path; if the
session is still present but ast_dirty is true (concurrent edit), return the
benign serde_raw{"null"} result instead of a compilation failure; alternatively,
have ensure_compiled return a richer status (e.g., enum {Ok, Dirty, Closed,
Error}) and switch on that in forward_query to map Dirty->serde_raw{"null"},
Closed->fail(...), and Error->fail("Compilation failed").


Expand All @@ -832,8 +856,13 @@ Compiler::RawResult Compiler::forward_query(worker::QueryKind kind,

if(position) {
auto offset = mapper.to_offset(*position);
if(!offset)
co_return serde_raw{"null"};
if(!offset) {
LOG_WARN("forward_query: invalid position {}:{} for {}",
position->line,
position->character,
path);
co_await kota::fail("Invalid position: failed to convert to byte offset");
}
wp.offset = *offset;
}

Expand All @@ -847,7 +876,8 @@ Compiler::RawResult Compiler::forward_query(worker::QueryKind kind,

auto result = co_await pool.send_stateful(path_id, wp);
if(!result.has_value()) {
co_return serde_raw{};
LOG_WARN("forward_query: worker failed for {}: {}", path, result.error().message);
co_await kota::fail(result.error().message);
}
co_return std::move(result.value());
}
Expand All @@ -866,27 +896,36 @@ Compiler::RawResult Compiler::forward_build(worker::BuildKind kind,
wp.version = session.version;
wp.text = session.text;
if(!fill_compile_args(path, wp.directory, wp.arguments, &session)) {
co_return serde_raw{};
LOG_WARN("forward_build: compile args not available for {}", path);
co_await kota::fail("Compile arguments not available");
}

if(!co_await ensure_deps(session, wp.directory, wp.arguments, wp.pch, wp.pcms)) {
co_return serde_raw{};
LOG_WARN("forward_build: dependency preparation failed for {}", path);
co_await kota::fail("Dependency preparation failed");
}

// After co_await, verify session still exists.
if(sessions.find(path_id) == sessions.end()) {
co_return serde_raw{};
LOG_WARN("forward_build: session lost after co_await for {}", path);
co_await kota::fail("Document was closed during compilation");
}

lsp::PositionMapper mapper(wp.text, lsp::PositionEncoding::UTF16);
auto offset = mapper.to_offset(position);
if(!offset)
co_return serde_raw{"null"};
if(!offset) {
LOG_WARN("forward_build: invalid position {}:{} for {}",
position.line,
position.character,
path);
co_await kota::fail("Invalid position: failed to convert to byte offset");
}
wp.offset = *offset;

auto result = co_await pool.send_stateless(wp);
if(!result.has_value()) {
co_return serde_raw{};
LOG_WARN("forward_build: worker failed for {}: {}", path, result.error().message);
co_await kota::fail(result.error().message);
}
co_return std::move(result.value().result_json);
}
Expand All @@ -904,8 +943,10 @@ Compiler::RawResult Compiler::handle_completion(const protocol::Position& positi
pctx.kind == CompletionContext::IncludeAngled) {
std::string directory;
std::vector<std::string> arguments;
if(!fill_compile_args(path, directory, arguments))
co_return serde_raw{"[]"};
if(!fill_compile_args(path, directory, arguments)) {
LOG_WARN("handle_completion: compile args not available for {}", path);
co_await kota::fail("Compile arguments not available for include completion");
}

std::vector<const char*> args_ptrs;
args_ptrs.reserve(arguments.size());
Expand Down
5 changes: 4 additions & 1 deletion src/server/config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ std::optional<Config> Config::load_from_json(llvm::StringRef json, llvm::StringR
return config;
}

Config Config::load_from_workspace(llvm::StringRef workspace_root) {
Config Config::load_from_workspace(llvm::StringRef workspace_root, std::string* warning) {
if(!workspace_root.empty()) {
for(auto* name: {"clice.toml", ".clice/config.toml"}) {
auto config_path = path::join(workspace_root, name);
Expand All @@ -179,6 +179,9 @@ Config Config::load_from_workspace(llvm::StringRef workspace_root) {
// Present but malformed: fall through to defaults, but surface
// the situation clearly so users know their config wasn't applied.
LOG_WARN("Falling back to default configuration because {} is invalid", config_path);
if(warning)
*warning = std::format("Configuration file {} is invalid, falling back to defaults",
config_path);
}
}

Expand Down
5 changes: 4 additions & 1 deletion src/server/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,10 @@ struct Config {

/// Load config from the workspace, trying standard locations.
/// Returns a default config (with apply_defaults) if no file is found.
static Config load_from_workspace(llvm::StringRef workspace_root);
/// If `warning` is non-null and a config file was found but malformed,
/// the warning message is written there.
static Config load_from_workspace(llvm::StringRef workspace_root,
std::string* warning = nullptr);
};

} // namespace clice
Loading
Loading