From a305eda8d159dd13160c689963818480c3c4d357 Mon Sep 17 00:00:00 2001 From: Piotr Osiewicz <24362066+osiewicz@users.noreply.github.com> Date: Fri, 30 May 2025 23:08:41 +0200 Subject: [PATCH] debugger: Relax implementation of `validate_config` to not run validation (#31785) 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. --- 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 ++- .../src/tests/new_session_modal.rs | 2 +- 11 files changed, 35 insertions(+), 147 deletions(-) diff --git a/crates/dap/src/adapters.rs b/crates/dap/src/adapters.rs index 331917592060bf6dd150f0ad32cb9e4de79de6e5..ac15d6fefa3a81ffc7bec3cbedf8e27a008faa84 100644 --- a/crates/dap/src/adapters.rs +++ b/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 { - 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 { @@ -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(), }, }) diff --git a/crates/dap/src/dap.rs b/crates/dap/src/dap.rs index 2363fc2242f301f2e1c9ad2759183554c4b2a4bc..7ab500d53b1fa0ab0335249ec59c25caeca36bca 100644 --- a/crates/dap/src/dap.rs +++ b/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); diff --git a/crates/dap_adapters/src/codelldb.rs b/crates/dap_adapters/src/codelldb.rs index fbe2e49147925463da629cb818bfb2ffa2fca744..33727e617a6ee8062a86b4d99c6062b6596b4ae3 100644 --- a/crates/dap_adapters/src/codelldb.rs +++ b/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 { - 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 { let mut configuration = json!({ "request": match zed_scenario.request { diff --git a/crates/dap_adapters/src/gdb.rs b/crates/dap_adapters/src/gdb.rs index 376f62a752efa20f6009ddbac71b8a0f17408e21..1915787b10977006f52b6137380753be053b760f 100644 --- a/crates/dap_adapters/src/gdb.rs +++ b/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(), }; diff --git a/crates/dap_adapters/src/go.rs b/crates/dap_adapters/src/go.rs index 29501c75444ef67fdcd6e59be70ca865b9a01334..85919c0dc9498693b72435082dc57c1f3dc6f117 100644 --- a/crates/dap_adapters/src/go.rs +++ b/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 { - 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 { 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)?, }, }) } diff --git a/crates/dap_adapters/src/javascript.rs b/crates/dap_adapters/src/javascript.rs index 74ef5feccfe4a9b1734baab7e333adb3c2327062..ba0f0ccff3efa114ce90ef0d66d74e77210d6368 100644 --- a/crates/dap_adapters/src/javascript.rs +++ b/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 { - 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 { let mut args = json!({ "type": "pwa-node", diff --git a/crates/dap_adapters/src/php.rs b/crates/dap_adapters/src/php.rs index c7a429b6ec74e969d1013a8d4750430818d4acd3..5065cbbbb1ffc1f56be5d46deaeb4e425b824f7a 100644 --- a/crates/dap_adapters/src/php.rs +++ b/crates/dap_adapters/src/php.rs @@ -94,7 +94,7 @@ impl PhpDebugAdapter { envs: HashMap::default(), request_args: StartDebuggingRequestArguments { configuration: task_definition.config.clone(), - request: ::validate_config(self, &task_definition.config)?, + request: ::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 { + fn request_kind(&self, _: &serde_json::Value) -> Result { Ok(StartDebuggingRequestArgumentsRequest::Launch) } diff --git a/crates/dap_adapters/src/python.rs b/crates/dap_adapters/src/python.rs index a00736f4a744223c94ceb2e1d92205b907efffad..343f999aa1c055112235a2cde18aae61f0d312d0 100644 --- a/crates/dap_adapters/src/python.rs +++ b/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 { - 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 { - 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": { diff --git a/crates/dap_adapters/src/ruby.rs b/crates/dap_adapters/src/ruby.rs index a67e1da602c2d546e1774efa9136410842f8e87e..65a342ade9e495dcdb9721e091047bacd7a66272 100644 --- a/crates/dap_adapters/src/ruby.rs +++ b/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(), }, }) diff --git a/crates/debugger_ui/src/session/running.rs b/crates/debugger_ui/src/session/running.rs index 22216ab78a4ec4c77e5a12f3bdb7057beb162731..a7b058e3320d20c1925d4853a31d8ff90d56bcd2 100644 --- a/crates/debugger_ui/src/session/running.rs +++ b/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 { diff --git a/crates/debugger_ui/src/tests/new_session_modal.rs b/crates/debugger_ui/src/tests/new_session_modal.rs index 11dc9a7370721556c02033bcec96b3aecdbb65aa..ad9f6f63da1cf48e2afe470dac2d29743fa11afe 100644 --- a/crates/debugger_ui/src/tests/new_session_modal.rs +++ b/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",