feat: Run and report diagnostics from protoc upon save

README.md

@@ -82,8 +82,9 @@ Protols is configured using a `protols.toml` file, which you can place in any di
 include_paths = ["foobar", "bazbaaz"] # Include paths to look for protofiles during parsing
 disable_parse_diagnostics = true # Disable diagnostics for parsing
 
-[config.experimental] # Experimental configuration; this should be considered unsafe and not fully tested
-use_protoc_diagnostics = true # Use diagnostics from protoc
+[config.experimental] # experimental configuration; this should be considered unsafe and not fully tested
+use_protoc_diagnostics = true # use diagnostics from protoc
+protoc_path = "protoc" # Path to proto compiler (protoc)
 
 [formatter] # Formatter specific configuration
 clang_format_path = "/usr/bin/clang-format" # clang-format binary to execute in formatting
@@ -96,13 +97,14 @@ clang_format_path = "/usr/bin/clang-format" # clang-format binary to execute in
 The `[config]` section contains stable settings that should generally remain unchanged.
 
 - `include_paths`: Directories to search for `.proto` files. Absolute or relative to LSP workspace root.
-- `disable_parse_diagnostics`: Set to `true` to disable diagnostics during parsing.
+- `disable_parse_diagnostics`: Set to `true` to disable tree-sitter parse diagnostics during parsing.
 
 #### Experimental Configuration
 
 The `[config.experimental]` section contains settings that are in development or not fully tested.
 
 - `use_protoc_diagnostics`: Enable diagnostics from the `protoc` compiler when set to `true`.
+- `protoc_path`: Uses protoc from this path for diagnostics
 
 #### Formatter Configuration
 

protols.toml

@@ -1,2 +1,5 @@
 [config]
 include_paths = ["src/workspace/input"]
+
+[config.experimental] # experimental configuration; this should be considered unsafe and not fully tested
+use_protoc_diagnostics = true # use diagnostics from protoc

sample/simple.proto

@@ -2,6 +2,8 @@ syntax = "proto3";
 
 package com.book;
 
+import "google/protobuf/any.proto";
+
 // This is a book represeted by some comments that we like to address in the
 // review
 message Book {
@@ -24,6 +26,7 @@ message Book {
   }
 
   enum BookState {
+    UNSPECIFIED = 0;
     HARD_COVER = 1;
     SOFT_COVER = 2;
   }
@@ -52,10 +55,10 @@ service BookService {
 
 message BookStore {
   reserved 1;
-  Book book = 0;
-  string name = 1;
-  map<int64, string> books = 2;
-  EnumSample sample = 3;
+  Book book = 5;
+  string name = 2;
+  map<int64, string> books = 3;
+  EnumSample sample = 4;
 }
 
 // These are enum options representing some operation in the proto

src/config/mod.rs

@@ -6,6 +6,10 @@ fn default_clang_format_path() -> String {
     "clang-format".to_string()
 }
 
+fn default_protoc_path() -> String {
+    "protoc".to_string()
+}
+
 #[derive(Serialize, Deserialize, Debug, Clone, Default)]
 #[serde(default)]
 pub struct ProtolsConfig {
@@ -28,10 +32,11 @@ pub struct Config {
     pub experimental: ExperimentalConfig,
 }
 
-#[derive(Serialize, Deserialize, Debug, Clone, Default)]
+#[derive(Serialize, Deserialize, Debug, Clone)]
 #[serde(default)]
 pub struct ExperimentalConfig {
     pub use_protoc_diagnostics: bool,
+    pub protoc_path: String,
 }
 
 impl Default for FormatterConfig {
@@ -41,3 +46,12 @@ impl Default for FormatterConfig {
         }
     }
 }
+
+impl Default for ExperimentalConfig {
+    fn default() -> Self {
+        Self {
+            protoc_path: default_protoc_path(),
+            use_protoc_diagnostics: false,
+        }
+    }
+}

src/config/workspace.rs

@@ -21,15 +21,18 @@ pub struct WorkspaceProtoConfigs {
 
 impl WorkspaceProtoConfigs {
     pub fn new() -> Self {
+        // Try to find protobuf library and get its include paths
+        let protoc_include_prefix = Config::new()
+            .atleast_version("3.0.0")
+            .probe("protobuf")
+            .map(|lib| lib.include_paths)
+            .unwrap_or_default();
+
         Self {
-            workspaces: Default::default(),
-            formatters: Default::default(),
-            protoc_include_prefix: Config::new()
-                .atleast_version("3.0.0")
-                .probe("protobuf")
-                .map(|l| l.include_paths)
-                .unwrap_or_default(),
-            configs: Default::default(),
+            workspaces: HashSet::new(),
+            formatters: HashMap::new(),
+            configs: HashMap::new(),
+            protoc_include_prefix,
         }
     }
 

src/lsp.rs

@@ -392,7 +392,26 @@ impl LanguageServer for ProtoLanguageServer {
         Box::pin(async move { Ok(response) })
     }
 
-    fn did_save(&mut self, _: DidSaveTextDocumentParams) -> Self::NotifyResult {
+    fn did_save(&mut self, params: DidSaveTextDocumentParams) -> Self::NotifyResult {
+        let uri = params.text_document.uri;
+        let content = self.state.get_content(&uri);
+
+        let Some(ipath) = self.configs.get_include_paths(&uri) else {
+            return ControlFlow::Continue(());
+        };
+
+        let Some(pconf) = self.configs.get_config_for_uri(&uri) else {
+            return ControlFlow::Continue(());
+        };
+
+        if let Some(diagnostics) = self
+            .state
+            .upsert_file(&uri, content, &ipath, 8, &pconf.config)
+        {
+            if let Err(e) = self.client.publish_diagnostics(diagnostics) {
+                error!(error=%e, "failed to publish diagnostics")
+            }
+        }
         ControlFlow::Continue(())
     }
 
@@ -408,15 +427,14 @@ impl LanguageServer for ProtoLanguageServer {
             return ControlFlow::Continue(());
         };
 
-        let Some(diagnostics) = self.state.upsert_file(&uri, content.clone(), &ipath, 8) else {
-            return ControlFlow::Continue(());
-        };
-
         let Some(pconf) = self.configs.get_config_for_uri(&uri) else {
             return ControlFlow::Continue(());
         };
 
-        if !pconf.config.disable_parse_diagnostics {
+        if let Some(diagnostics) = self
+            .state
+            .upsert_file(&uri, content, &ipath, 8, &pconf.config)
+        {
             if let Err(e) = self.client.publish_diagnostics(diagnostics) {
                 error!(error=%e, "failed to publish diagnostics")
             }
@@ -432,20 +450,21 @@ impl LanguageServer for ProtoLanguageServer {
             return ControlFlow::Continue(());
         };
 
-        let Some(diagnostics) = self.state.upsert_file(&uri, content, &ipath, 2) else {
+        let Some(pconf) = self.configs.get_config_for_uri(&uri) else {
             return ControlFlow::Continue(());
         };
 
-        let Some(ws) = self.configs.get_config_for_uri(&uri) else {
-            return ControlFlow::Continue(());
-        };
-
-        if !ws.config.disable_parse_diagnostics {
+        // override config to disable protoc diagnostics during change
+        let mut pconf = pconf.config.clone();
+        pconf.experimental.use_protoc_diagnostics = false;
+        if let Some(diagnostics) = self
+            .state
+            .upsert_file(&uri, content, &ipath, 8, &pconf)
+        {
             if let Err(e) = self.client.publish_diagnostics(diagnostics) {
                 error!(error=%e, "failed to publish diagnostics")
             }
         }
-
         ControlFlow::Continue(())
     }
 

src/main.rs

@@ -15,6 +15,7 @@ mod formatter;
 mod lsp;
 mod nodekind;
 mod parser;
+mod protoc;
 mod server;
 mod state;
 mod utils;

src/protoc.rs

@@ -0,0 +1,85 @@
+use crate::utils::ts_to_lsp_position;
+use async_lsp::lsp_types::{Diagnostic, DiagnosticSeverity, Range};
+use std::process::Command;
+use tree_sitter::Point;
+
+pub struct ProtocDiagnostics {}
+
+impl ProtocDiagnostics {
+    pub fn new() -> Self {
+        Self {}
+    }
+
+    pub fn collect_diagnostics(
+        &self,
+        protoc_path: &str,
+        file_path: &str,
+        include_paths: &[String],
+    ) -> Vec<Diagnostic> {
+        let mut cmd = Command::new(protoc_path);
+
+        // Add include paths
+        for path in include_paths {
+            cmd.arg("-I").arg(path);
+        }
+
+        // Generate descriptor but discard its output
+        cmd.arg("-o")
+            .arg(if cfg!(windows) { "NUL" } else { "/dev/null" });
+
+        // Add the file to check
+        cmd.arg(file_path);
+
+        // Run protoc and capture output
+        match cmd.output() {
+            Ok(output) => {
+                if !output.status.success() {
+                    let error = String::from_utf8_lossy(&output.stderr);
+                    self.parse_protoc_output(&error)
+                } else {
+                    Vec::new()
+                }
+            }
+            Err(e) => {
+                tracing::error!(error=%e, "failed to run protoc");
+                Vec::new()
+            }
+        }
+    }
+
+    fn parse_protoc_output(&self, output: &str) -> Vec<Diagnostic> {
+        let mut diagnostics = Vec::new();
+
+        for line in output.lines() {
+            // Parse protoc error format: file:line:column: message
+            if let Some((file_info, message)) = line.split_once(": ") {
+                let parts: Vec<&str> = file_info.split(':').collect();
+                if parts.len() >= 3 {
+                    if let (Ok(line), Ok(col)) = (parts[1].parse::<u32>(), parts[2].parse::<u32>())
+                    {
+                        let point = Point {
+                            row: (line - 1) as usize,
+                            column: (col - 1) as usize,
+                        };
+                        let diagnostic = Diagnostic {
+                            range: Range {
+                                start: ts_to_lsp_position(&point),
+                                end: ts_to_lsp_position(&Point {
+                                    row: point.row,
+                                    column: point.column + 1,
+                                }),
+                            },
+                            severity: Some(DiagnosticSeverity::ERROR),
+                            source: Some("protoc".to_string()),
+                            message: message.to_string(),
+                            ..Default::default()
+                        };
+                        diagnostics.push(diagnostic);
+                    }
+                }
+            }
+        }
+
+        diagnostics
+    }
+}

src/state.rs

@@ -12,15 +12,19 @@ use tree_sitter::Node;
 use walkdir::WalkDir;
 
 use crate::{
+    config::Config,
     nodekind::NodeKind,
     parser::{ParsedTree, ProtoParser},
 };
 
+use crate::protoc::ProtocDiagnostics;
+
 pub struct ProtoLanguageState {
     documents: Arc<RwLock<HashMap<Url, String>>>,
     trees: Arc<RwLock<HashMap<Url, ParsedTree>>>,
     parser: Arc<Mutex<ProtoParser>>,
     parsed_workspaces: Arc<RwLock<HashSet<String>>>,
+    protoc_diagnostics: Arc<Mutex<ProtocDiagnostics>>,
 }
 
 impl ProtoLanguageState {
@@ -30,6 +34,7 @@ impl ProtoLanguageState {
             trees: Default::default(),
             parser: Arc::new(Mutex::new(ProtoParser::new())),
             parsed_workspaces: Arc::new(RwLock::new(HashSet::new())),
+            protoc_diagnostics: Arc::new(Mutex::new(ProtocDiagnostics::new())),
         }
     }
 
@@ -217,13 +222,34 @@ impl ProtoLanguageState {
         content: String,
         ipath: &[PathBuf],
         depth: usize,
+        config: &Config,
     ) -> Option<PublishDiagnosticsParams> {
         info!(%uri, %depth, "upserting file");
         let diag = self.upsert_content(uri, content.clone(), ipath, depth);
         self.get_tree(uri).map(|tree| {
-            let diag = tree.collect_import_diagnostics(content.as_ref(), diag);
-            let mut d = tree.collect_parse_diagnostics();
-            d.extend(diag);
+            let mut d = vec![];
+            if !config.disable_parse_diagnostics {
+                d.extend(tree.collect_parse_diagnostics());
+            }
+            d.extend(tree.collect_import_diagnostics(content.as_ref(), diag));
+
+            // Add protoc diagnostics if enabled
+            if config.experimental.use_protoc_diagnostics {
+                if let Ok(protoc_diagnostics) = self.protoc_diagnostics.lock() {
+                    if let Some(file_path) = uri.to_file_path().ok() {
+                        let protoc_diags = protoc_diagnostics.collect_diagnostics(
+                            &config.experimental.protoc_path,
+                            file_path.to_str().unwrap_or_default(),
+                            &ipath
+                                .iter()
+                                .map(|p| p.to_str().unwrap_or_default().to_string())
+                                .collect::<Vec<_>>(),
+                        );
+                        d.extend(protoc_diags);
+                    }
+                }
+            }
+
             PublishDiagnosticsParams {
                 uri: tree.uri.clone(),
                 diagnostics: d,

src/workspace/definition.rs

@@ -52,8 +52,8 @@ mod test {
 
     use insta::assert_yaml_snapshot;
 
+    use crate::config::Config;
     use crate::state::ProtoLanguageState;
-
     #[test]
     fn workspace_test_definition() {
         let ipath = vec![PathBuf::from("src/workspace/input")];
@@ -66,9 +66,9 @@ mod test {
         let c = include_str!("input/c.proto");
 
         let mut state: ProtoLanguageState = ProtoLanguageState::new();
-        state.upsert_file(&a_uri, a.to_owned(), &ipath, 2);
-        state.upsert_file(&b_uri, b.to_owned(), &ipath, 2);
-        state.upsert_file(&c_uri, c.to_owned(), &ipath, 2);
+        state.upsert_file(&a_uri, a.to_owned(), &ipath, 2, &Config::default());
+        state.upsert_file(&b_uri, b.to_owned(), &ipath, 2, &Config::default());
+        state.upsert_file(&c_uri, c.to_owned(), &ipath, 2, &Config::default());
 
         assert_yaml_snapshot!(state.definition(
             &ipath,

src/workspace/hover.rs

@@ -664,9 +664,9 @@ Included from {}"#,
 mod test {
     use insta::assert_yaml_snapshot;
 
+    use crate::config::Config;
     use crate::context::hoverable::Hoverables;
     use crate::state::ProtoLanguageState;
-
     #[test]
     fn workspace_test_hover() {
         let ipath = vec![std::env::current_dir().unwrap().join("src/workspace/input")];
@@ -679,9 +679,9 @@ mod test {
         let c = include_str!("input/c.proto");
 
         let mut state: ProtoLanguageState = ProtoLanguageState::new();
-        state.upsert_file(&a_uri, a.to_owned(), &ipath, 3);
-        state.upsert_file(&b_uri, b.to_owned(), &ipath, 2);
-        state.upsert_file(&c_uri, c.to_owned(), &ipath, 2);
+        state.upsert_file(&a_uri, a.to_owned(), &ipath, 3, &Config::default());
+        state.upsert_file(&b_uri, b.to_owned(), &ipath, 2, &Config::default());
+        state.upsert_file(&c_uri, c.to_owned(), &ipath, 2, &Config::default());
 
         assert_yaml_snapshot!(state.hover(
             &ipath,

src/workspace/rename.rs

@@ -70,6 +70,7 @@ mod test {
 
     use insta::assert_yaml_snapshot;
 
+    use crate::config::Config;
     use crate::state::ProtoLanguageState;
 
     #[test]
@@ -84,9 +85,9 @@ mod test {
         let c = include_str!("input/c.proto");
 
         let mut state: ProtoLanguageState = ProtoLanguageState::new();
-        state.upsert_file(&a_uri, a.to_owned(), &ipath, 2);
-        state.upsert_file(&b_uri, b.to_owned(), &ipath, 2);
-        state.upsert_file(&c_uri, c.to_owned(), &ipath, 2);
+        state.upsert_file(&a_uri, a.to_owned(), &ipath, 2, &Config::default());
+        state.upsert_file(&b_uri, b.to_owned(), &ipath, 2, &Config::default());
+        state.upsert_file(&c_uri, c.to_owned(), &ipath, 2, &Config::default());
 
         assert_yaml_snapshot!(state.rename_fields(
             "com.workspace",
@@ -123,9 +124,9 @@ mod test {
         let c = include_str!("input/c.proto");
 
         let mut state: ProtoLanguageState = ProtoLanguageState::new();
-        state.upsert_file(&a_uri, a.to_owned(), &ipath, 2);
-        state.upsert_file(&b_uri, b.to_owned(), &ipath, 2);
-        state.upsert_file(&c_uri, c.to_owned(), &ipath, 2);
+        state.upsert_file(&a_uri, a.to_owned(), &ipath, 2, &Config::default());
+        state.upsert_file(&b_uri, b.to_owned(), &ipath, 2, &Config::default());
+        state.upsert_file(&c_uri, c.to_owned(), &ipath, 2, &Config::default());
 
         assert_yaml_snapshot!(state.reference_fields(
             "com.workspace",