debugger: Relax implementation of `validate_config` to not run validation (#31785)

Piotr Osiewicz created

When we moved to schema-based debug configs, we've added validate_config
- a trait method
that is supposed to both validate the configuration and determine
whether it is a launch configuration
or an attach configuration.

The validation bit is a bit problematic though - we received reports on
Discords about
scenarios not starting up properly; it turned out that Javascript's
implementation was overly strict.
Thus, I got rid of any code that tries to validate the config - let's
let the debug adapter itself
decide whether it can digest the configuration or not. validate_config
is now left unimplemented for most
DebugAdapter implementations (except for PHP), because all adapters use
`request`: 'launch'/'attach' for that.
Let's leave the trait method in place though, as nothing guarantees this
to be true for all adapters.

cc @Anthony-Eid

Release Notes:

- debugger: Improved error messages when the debug scenario is not
valid.
- debugger: Fixed cases where valid configs were rejected.

Change summary

crates/dap/src/adapters.rs                        | 26 ++++----
crates/dap/src/dap.rs                             |  2 
crates/dap_adapters/src/codelldb.rs               | 51 +----------------
crates/dap_adapters/src/gdb.rs                    |  2 
crates/dap_adapters/src/go.rs                     | 24 +-------
crates/dap_adapters/src/javascript.rs             | 32 +---------
crates/dap_adapters/src/php.rs                    |  7 -
crates/dap_adapters/src/python.rs                 | 27 +--------
crates/dap_adapters/src/ruby.rs                   |  2 
crates/debugger_ui/src/session/running.rs         |  7 +
crates/debugger_ui/src/tests/new_session_modal.rs |  2 
11 files changed, 35 insertions(+), 147 deletions(-)

Detailed changes

crates/dap/src/adapters.rs 🔗

@@ -370,21 +370,19 @@ pub trait DebugAdapter: 'static + Send + Sync {
         None
     }
 
-    fn validate_config(
+    /// Extracts the kind (attach/launch) of debug configuration from the given JSON config.
+    /// This method should only return error when the kind cannot be determined for a given configuration;
+    /// in particular, it *should not* validate whether the request as a whole is valid, because that's best left to the debug adapter itself to decide.
+    fn request_kind(
         &self,
         config: &serde_json::Value,
     ) -> Result<StartDebuggingRequestArgumentsRequest> {
-        let map = config.as_object().context("Config isn't an object")?;
-
-        let request_variant = map
-            .get("request")
-            .and_then(|val| val.as_str())
-            .context("request argument is not found or invalid")?;
-
-        match request_variant {
-            "launch" => Ok(StartDebuggingRequestArgumentsRequest::Launch),
-            "attach" => Ok(StartDebuggingRequestArgumentsRequest::Attach),
-            _ => Err(anyhow!("request must be either 'launch' or 'attach'")),
+        match config.get("request") {
+            Some(val) if val == "launch" => Ok(StartDebuggingRequestArgumentsRequest::Launch),
+            Some(val) if val == "attach" => Ok(StartDebuggingRequestArgumentsRequest::Attach),
+            _ => Err(anyhow!(
+                "missing or invalid `request` field in config. Expected 'launch' or 'attach'"
+            )),
         }
     }
 
@@ -414,7 +412,7 @@ impl DebugAdapter for FakeAdapter {
         serde_json::Value::Null
     }
 
-    fn validate_config(
+    fn request_kind(
         &self,
         config: &serde_json::Value,
     ) -> Result<StartDebuggingRequestArgumentsRequest> {
@@ -459,7 +457,7 @@ impl DebugAdapter for FakeAdapter {
             envs: HashMap::default(),
             cwd: None,
             request_args: StartDebuggingRequestArguments {
-                request: self.validate_config(&task_definition.config)?,
+                request: self.request_kind(&task_definition.config)?,
                 configuration: task_definition.config.clone(),
             },
         })

crates/dap/src/dap.rs 🔗

@@ -52,7 +52,7 @@ pub fn send_telemetry(scenario: &DebugScenario, location: TelemetrySpawnLocation
         return;
     };
     let kind = adapter
-        .validate_config(&scenario.config)
+        .request_kind(&scenario.config)
         .ok()
         .map(serde_json::to_value)
         .and_then(Result::ok);

crates/dap_adapters/src/codelldb.rs 🔗

@@ -1,11 +1,8 @@
 use std::{collections::HashMap, path::PathBuf, sync::OnceLock};
 
-use anyhow::{Context as _, Result, anyhow};
+use anyhow::{Context as _, Result};
 use async_trait::async_trait;
-use dap::{
-    StartDebuggingRequestArgumentsRequest,
-    adapters::{DebugTaskDefinition, latest_github_release},
-};
+use dap::adapters::{DebugTaskDefinition, latest_github_release};
 use futures::StreamExt;
 use gpui::AsyncApp;
 use serde_json::Value;
@@ -37,7 +34,7 @@ impl CodeLldbDebugAdapter {
                 Value::String(String::from(task_definition.label.as_ref())),
             );
 
-        let request = self.validate_config(&configuration)?;
+        let request = self.request_kind(&configuration)?;
 
         Ok(dap::StartDebuggingRequestArguments {
             request,
@@ -89,48 +86,6 @@ impl DebugAdapter for CodeLldbDebugAdapter {
         DebugAdapterName(Self::ADAPTER_NAME.into())
     }
 
-    fn validate_config(
-        &self,
-        config: &serde_json::Value,
-    ) -> Result<StartDebuggingRequestArgumentsRequest> {
-        let map = config
-            .as_object()
-            .ok_or_else(|| anyhow!("Config isn't an object"))?;
-
-        let request_variant = map
-            .get("request")
-            .and_then(|r| r.as_str())
-            .ok_or_else(|| anyhow!("request field is required and must be a string"))?;
-
-        match request_variant {
-            "launch" => {
-                // For launch, verify that one of the required configs exists
-                if !(map.contains_key("program")
-                    || map.contains_key("targetCreateCommands")
-                    || map.contains_key("cargo"))
-                {
-                    return Err(anyhow!(
-                        "launch request requires either 'program', 'targetCreateCommands', or 'cargo' field"
-                    ));
-                }
-                Ok(StartDebuggingRequestArgumentsRequest::Launch)
-            }
-            "attach" => {
-                // For attach, verify that either pid or program exists
-                if !(map.contains_key("pid") || map.contains_key("program")) {
-                    return Err(anyhow!(
-                        "attach request requires either 'pid' or 'program' field"
-                    ));
-                }
-                Ok(StartDebuggingRequestArgumentsRequest::Attach)
-            }
-            _ => Err(anyhow!(
-                "request must be either 'launch' or 'attach', got '{}'",
-                request_variant
-            )),
-        }
-    }
-
     fn config_from_zed_format(&self, zed_scenario: ZedDebugConfig) -> Result<DebugScenario> {
         let mut configuration = json!({
             "request": match zed_scenario.request {

crates/dap_adapters/src/gdb.rs 🔗

@@ -178,7 +178,7 @@ impl DebugAdapter for GdbDebugAdapter {
         let gdb_path = user_setting_path.unwrap_or(gdb_path?);
 
         let request_args = StartDebuggingRequestArguments {
-            request: self.validate_config(&config.config)?,
+            request: self.request_kind(&config.config)?,
             configuration: config.config.clone(),
         };
 

crates/dap_adapters/src/go.rs 🔗

@@ -1,6 +1,6 @@
-use anyhow::{Context as _, anyhow, bail};
+use anyhow::{Context as _, bail};
 use dap::{
-    StartDebuggingRequestArguments, StartDebuggingRequestArgumentsRequest,
+    StartDebuggingRequestArguments,
     adapters::{
         DebugTaskDefinition, DownloadedFileType, download_adapter_from_github,
         latest_github_release,
@@ -350,24 +350,6 @@ impl DebugAdapter for GoDebugAdapter {
         })
     }
 
-    fn validate_config(
-        &self,
-        config: &serde_json::Value,
-    ) -> Result<StartDebuggingRequestArgumentsRequest> {
-        let map = config.as_object().context("Config isn't an object")?;
-
-        let request_variant = map
-            .get("request")
-            .and_then(|val| val.as_str())
-            .context("request argument is not found or invalid")?;
-
-        match request_variant {
-            "launch" => Ok(StartDebuggingRequestArgumentsRequest::Launch),
-            "attach" => Ok(StartDebuggingRequestArgumentsRequest::Attach),
-            _ => Err(anyhow!("request must be either 'launch' or 'attach'")),
-        }
-    }
-
     fn config_from_zed_format(&self, zed_scenario: ZedDebugConfig) -> Result<DebugScenario> {
         let mut args = match &zed_scenario.request {
             dap::DebugRequest::Attach(attach_config) => {
@@ -488,7 +470,7 @@ impl DebugAdapter for GoDebugAdapter {
             connection: None,
             request_args: StartDebuggingRequestArguments {
                 configuration: task_definition.config.clone(),
-                request: self.validate_config(&task_definition.config)?,
+                request: self.request_kind(&task_definition.config)?,
             },
         })
     }

crates/dap_adapters/src/javascript.rs 🔗

@@ -1,9 +1,6 @@
 use adapters::latest_github_release;
-use anyhow::{Context as _, anyhow};
-use dap::{
-    StartDebuggingRequestArguments, StartDebuggingRequestArgumentsRequest,
-    adapters::DebugTaskDefinition,
-};
+use anyhow::Context as _;
+use dap::{StartDebuggingRequestArguments, adapters::DebugTaskDefinition};
 use gpui::AsyncApp;
 use std::{collections::HashMap, path::PathBuf, sync::OnceLock};
 use task::DebugRequest;
@@ -95,7 +92,7 @@ impl JsDebugAdapter {
             }),
             request_args: StartDebuggingRequestArguments {
                 configuration: task_definition.config.clone(),
-                request: self.validate_config(&task_definition.config)?,
+                request: self.request_kind(&task_definition.config)?,
             },
         })
     }
@@ -107,29 +104,6 @@ impl DebugAdapter for JsDebugAdapter {
         DebugAdapterName(Self::ADAPTER_NAME.into())
     }
 
-    fn validate_config(
-        &self,
-        config: &serde_json::Value,
-    ) -> Result<dap::StartDebuggingRequestArgumentsRequest> {
-        match config.get("request") {
-            Some(val) if val == "launch" => {
-                if config.get("program").is_none() && config.get("url").is_none() {
-                    return Err(anyhow!(
-                        "either program or url is required for launch request"
-                    ));
-                }
-                Ok(StartDebuggingRequestArgumentsRequest::Launch)
-            }
-            Some(val) if val == "attach" => {
-                if !config.get("processId").is_some_and(|val| val.is_u64()) {
-                    return Err(anyhow!("processId must be a number"));
-                }
-                Ok(StartDebuggingRequestArgumentsRequest::Attach)
-            }
-            _ => Err(anyhow!("missing or invalid request field in config")),
-        }
-    }
-
     fn config_from_zed_format(&self, zed_scenario: ZedDebugConfig) -> Result<DebugScenario> {
         let mut args = json!({
             "type": "pwa-node",

crates/dap_adapters/src/php.rs 🔗

@@ -94,7 +94,7 @@ impl PhpDebugAdapter {
             envs: HashMap::default(),
             request_args: StartDebuggingRequestArguments {
                 configuration: task_definition.config.clone(),
-                request: <Self as DebugAdapter>::validate_config(self, &task_definition.config)?,
+                request: <Self as DebugAdapter>::request_kind(self, &task_definition.config)?,
             },
         })
     }
@@ -282,10 +282,7 @@ impl DebugAdapter for PhpDebugAdapter {
         Some(SharedString::new_static("PHP").into())
     }
 
-    fn validate_config(
-        &self,
-        _: &serde_json::Value,
-    ) -> Result<StartDebuggingRequestArgumentsRequest> {
+    fn request_kind(&self, _: &serde_json::Value) -> Result<StartDebuggingRequestArgumentsRequest> {
         Ok(StartDebuggingRequestArgumentsRequest::Launch)
     }
 

crates/dap_adapters/src/python.rs 🔗

@@ -1,9 +1,6 @@
 use crate::*;
-use anyhow::{Context as _, anyhow};
-use dap::{
-    DebugRequest, StartDebuggingRequestArguments, StartDebuggingRequestArgumentsRequest,
-    adapters::DebugTaskDefinition,
-};
+use anyhow::Context as _;
+use dap::{DebugRequest, StartDebuggingRequestArguments, adapters::DebugTaskDefinition};
 use gpui::{AsyncApp, SharedString};
 use json_dotpath::DotPaths;
 use language::{LanguageName, Toolchain};
@@ -86,7 +83,7 @@ impl PythonDebugAdapter {
         &self,
         task_definition: &DebugTaskDefinition,
     ) -> Result<StartDebuggingRequestArguments> {
-        let request = self.validate_config(&task_definition.config)?;
+        let request = self.request_kind(&task_definition.config)?;
 
         let mut configuration = task_definition.config.clone();
         if let Ok(console) = configuration.dot_get_mut("console") {
@@ -254,24 +251,6 @@ impl DebugAdapter for PythonDebugAdapter {
         })
     }
 
-    fn validate_config(
-        &self,
-        config: &serde_json::Value,
-    ) -> Result<StartDebuggingRequestArgumentsRequest> {
-        let map = config.as_object().context("Config isn't an object")?;
-
-        let request_variant = map
-            .get("request")
-            .and_then(|val| val.as_str())
-            .context("request is not valid")?;
-
-        match request_variant {
-            "launch" => Ok(StartDebuggingRequestArgumentsRequest::Launch),
-            "attach" => Ok(StartDebuggingRequestArgumentsRequest::Attach),
-            _ => Err(anyhow!("request must be either 'launch' or 'attach'")),
-        }
-    }
-
     async fn dap_schema(&self) -> serde_json::Value {
         json!({
             "properties": {

crates/dap_adapters/src/ruby.rs 🔗

@@ -265,7 +265,7 @@ impl DebugAdapter for RubyDebugAdapter {
             cwd: None,
             envs: std::collections::HashMap::default(),
             request_args: StartDebuggingRequestArguments {
-                request: self.validate_config(&definition.config)?,
+                request: self.request_kind(&definition.config)?,
                 configuration: definition.config.clone(),
             },
         })

crates/debugger_ui/src/session/running.rs 🔗

@@ -812,7 +812,7 @@ impl RunningState {
             let request_type = dap_registry
                 .adapter(&adapter)
                 .ok_or_else(|| anyhow!("{}: is not a valid adapter name", &adapter))
-                .and_then(|adapter| adapter.validate_config(&config));
+                .and_then(|adapter| adapter.request_kind(&config));
 
             let config_is_valid = request_type.is_ok();
 
@@ -954,7 +954,10 @@ impl RunningState {
                 config = scenario.config;
                 Self::substitute_variables_in_config(&mut config, &task_context);
             } else {
-                anyhow::bail!("No request or build provided");
+                let Err(e) = request_type else {
+                    unreachable!();
+                };
+                anyhow::bail!("Zed cannot determine how to run this debug scenario. `build` field was not provided and Debug Adapter won't accept provided configuration because: {e}");
             };
 
             Ok(DebugTaskDefinition {

crates/debugger_ui/src/tests/new_session_modal.rs 🔗

@@ -322,7 +322,7 @@ async fn test_dap_adapter_config_conversion_and_validation(cx: &mut TestAppConte
         );
 
         let request_type = adapter
-            .validate_config(&debug_scenario.config)
+            .request_kind(&debug_scenario.config)
             .unwrap_or_else(|_| {
                 panic!(
                     "Adapter {} should validate the config successfully",