zed-industries / zed

Code at the speed of thought – Zed is a high-performance, multiplayer code editor from the creators of Atom and Tree-sitter.
https://zed.dev
Other
49.15k stars 2.97k forks source link

Code actions import the wrong headers for C functions #16221

Open Nickname064 opened 2 months ago

Nickname064 commented 2 months ago

Check for existing issues

Describe the bug / provide steps to reproduce it

When writing a C program, if you try to use a function that has an equivalent C++ header, and use code actions to try to auto-import the corresponding C header, it will import the C++ version instead.

Ex: instead of importing errno.h, it will import cerrno

Environment

Zed: v0.147.2 (Zed) OS: Linux Wayland arch unknown Memory: 14.9 GiB Architecture: x86_64 GPU: AMD Radeon 610M (RADV RAPHAEL_MENDOCINO) || radv || Mesa 24.1.5-arch1.2

If applicable, add mockups / screenshots to help explain present your vision of the feature

No response

If applicable, attach your Zed.log file to this issue.

Zed.log


SomeoneToIgnore commented 2 months ago

This is due to clangd using C++ by default: https://github.com/zed-industries/zed/issues/14411#issuecomment-2227324916

I'm very curious to know which initializationOptions json can we pass into the server to treat is a "generic C" (C11?) to have some reasonable default.

Then we'll able to do something like

diff --git a/crates/languages/src/c.rs b/crates/languages/src/c.rs
index ea11b4e0d0..b53e5bf9e6 100644
--- a/crates/languages/src/c.rs
+++ b/crates/languages/src/c.rs
@@ -6,19 +6,26 @@ use http_client::github::{latest_github_release, GitHubLspBinaryVersion};
 pub use language::*;
 use lsp::LanguageServerBinary;
 use project::project_settings::{BinarySettings, ProjectSettings};
+use serde_json::json;
 use settings::Settings;
 use smol::fs::{self, File};
 use std::{any::Any, env::consts, path::PathBuf, sync::Arc};
 use util::{fs::remove_matching, maybe, ResultExt};

-pub struct CLspAdapter;
+pub struct ClangdArapter {
+    use_cpp: bool,
+}

-impl CLspAdapter {
+impl ClangdArapter {
     const SERVER_NAME: &'static str = "clangd";
+
+    pub fn new(use_cpp: bool) -> Self {
+        Self { use_cpp }
+    }
 }

 #[async_trait(?Send)]
-impl super::LspAdapter for CLspAdapter {
+impl super::LspAdapter for ClangdArapter {
     fn name(&self) -> LanguageServerName {
         LanguageServerName(Self::SERVER_NAME.into())
     }
@@ -299,6 +306,21 @@ impl super::LspAdapter for CLspAdapter {
             filter_range,
         })
     }
+
+    async fn initialization_options(
+        self: Arc<Self>,
+        _: &Arc<dyn LspAdapterDelegate>,
+    ) -> Result<Option<serde_json::Value>> {
+        if self.use_cpp {
+            Ok(None)
+        } else {
+            Ok(Some(json!({
+                "configSettings": {
+                    "clangd.arguments": ["-std=c++17"]
+                }
+            })))
+        }
+    }
 }

 async fn get_cached_server_binary(container_dir: PathBuf) -> Option<LanguageServerBinary> {
diff --git a/crates/languages/src/lib.rs b/crates/languages/src/lib.rs
index a499444c44..0e15192737 100644
--- a/crates/languages/src/lib.rs
+++ b/crates/languages/src/lib.rs
@@ -103,8 +103,11 @@ pub fn init(
         };
     }
     language!("bash", Vec::new(), bash_task_context());
-    language!("c", vec![Arc::new(c::CLspAdapter) as Arc<dyn LspAdapter>]);
-    language!("cpp", vec![Arc::new(c::CLspAdapter)]);
+    language!(
+        "c",
+        vec![Arc::new(c::ClangdArapter::new(false)) as Arc<dyn LspAdapter>]
+    );
+    language!("cpp", vec![Arc::new(c::ClangdArapter::new(true))]);
     language!(
         "css",
         vec![Arc::new(css::CssLspAdapter::new(node_runtime.clone())),]

https://clang.llvm.org/extra/doxygen/structclang_1_1clangd_1_1InitializationOptions.html seems like a good start for this


Alternatively, instead of providing the initialization options, maybe one could go and alter binary arguments (seems that clangd prefers this way?) similar to https://zed.dev/docs/languages/cpp#arguments

One caveat here would be to understand how to apply the "use C" arguments so that it does not override custom user arguments + finding the right incarnation for the command arguments. Feels that we could use the "use C" approach when the argument list is empty, otherwise allow the user to have his overrides.

Enapiuz commented 6 days ago

Having similar issue just trying to type bool and expect it to suggest stdbool.h to include, which doesn't happen instead it suggests very C++ish stuff even when I type it in a *.c file:

image
SomeoneToIgnore commented 6 days ago

@Enapiuz , yes, we are aware that everyone will have a similar issue, just as written right above:

This is due to clangd using C++ by default: https://github.com/zed-industries/zed/issues/14411#issuecomment-2227324916

Enapiuz commented 6 days ago

@SomeoneToIgnore sorry, didn't provide full context and thanks for that link, already configured clangd this way, saved my day 🙂

that's how it looks in neovim, with pretty much zero additional config aside from what comes with kickstart.nvim (and I see nothing specific there)

image

and this is how it looks in Zed

image

Technically it's not wrong, but I'm curious why this difference is here and it doesn't want to include header that contains this symbol

SomeoneToIgnore commented 6 days ago

it doesn't want to include header that contains this symbol

To elaborate, do you expect the completion menu to "include"/show this, or when the completion applies it should add that include into the file?

Given

Technically it's not wrong

I assume the former? If so, that is either due to this piece of code in Zed: https://github.com/zed-industries/zed/blob/291ca2c32cc0e82283fd4a20591a440585b195ab/crates/languages/src/c.rs#L116 or something relate to the way LSP server sends back its completion data.

If the latter, that might be Zed's bug with the way it handles completion LSP responses and resolved them, but for that we also need the completion data to see.

That data could be seen inside debug: open language server logs on the RPC messages tab: image

That

// Send:
{"jsonrpc":"2.0","id":165,"method":"textDocument/completion", ...

for Rust had been replied with

// Receive:
{"jsonrpc":"2.0","id":165, result: ...

response that contained some completion data, which might (or not) contain label details and other things with the import data to display. Or it might be the "method":"completionItem/resolve" for the corresponding completion item.

Either way, label discrepancies and completion results seem to be a standalone issue, if those are now look like C completions. This issue is about turning CPP completions into C ones for C projects, so let's continue in a separate issue if you see some issue in those labels/completion applications.