debugger: Allow locators to generate full debug scenarios (#30014)

Piotr Osiewicz , Anthony , and Remco Smits created

Closes #ISSUE

Release Notes:

- N/A

---------

Co-authored-by: Anthony <anthony@zed.dev>
Co-authored-by: Remco Smits <djsmits12@gmail.com>

Change summary

crates/dap/src/registry.rs                    | 16 ++-
crates/debugger_ui/src/session/running.rs     | 86 ++++++++++++++++----
crates/editor/src/editor.rs                   |  5 
crates/languages/src/rust.rs                  | 41 ---------
crates/project/src/debugger/dap_store.rs      | 81 +++++--------------
crates/project/src/debugger/locators/cargo.rs | 72 +++++++++++++---
crates/proto/proto/debugger.proto             |  1 
crates/task/src/debug_format.rs               | 16 +++
crates/task/src/lib.rs                        | 25 ++++-
9 files changed, 197 insertions(+), 146 deletions(-)

Detailed changes

crates/dap/src/registry.rs 🔗

@@ -1,9 +1,9 @@
 use anyhow::Result;
 use async_trait::async_trait;
 use collections::FxHashMap;
-use gpui::{App, Global};
+use gpui::{App, Global, SharedString};
 use parking_lot::RwLock;
-use task::{DebugRequest, SpawnInTerminal};
+use task::{DebugRequest, DebugScenario, SpawnInTerminal, TaskTemplate};
 
 use crate::adapters::{DebugAdapter, DebugAdapterName};
 use std::{collections::BTreeMap, sync::Arc};
@@ -11,15 +11,17 @@ use std::{collections::BTreeMap, sync::Arc};
 /// Given a user build configuration, locator creates a fill-in debug target ([DebugRequest]) on behalf of the user.
 #[async_trait]
 pub trait DapLocator: Send + Sync {
+    fn name(&self) -> SharedString;
     /// Determines whether this locator can generate debug target for given task.
-    fn accepts(&self, build_config: &SpawnInTerminal) -> bool;
+    fn create_scenario(&self, build_config: &TaskTemplate, adapter: &str) -> Option<DebugScenario>;
+
     async fn run(&self, build_config: SpawnInTerminal) -> Result<DebugRequest>;
 }
 
 #[derive(Default)]
 struct DapRegistryState {
     adapters: BTreeMap<DebugAdapterName, Arc<dyn DebugAdapter>>,
-    locators: FxHashMap<String, Arc<dyn DapLocator>>,
+    locators: FxHashMap<SharedString, Arc<dyn DapLocator>>,
 }
 
 #[derive(Clone, Default)]
@@ -48,15 +50,15 @@ impl DapRegistry {
         );
     }
 
-    pub fn add_locator(&self, name: String, locator: Arc<dyn DapLocator>) {
-        let _previous_value = self.0.write().locators.insert(name, locator);
+    pub fn add_locator(&self, locator: Arc<dyn DapLocator>) {
+        let _previous_value = self.0.write().locators.insert(locator.name(), locator);
         debug_assert!(
             _previous_value.is_none(),
             "Attempted to insert a new debug locator when one is already registered"
         );
     }
 
-    pub fn locators(&self) -> FxHashMap<String, Arc<dyn DapLocator>> {
+    pub fn locators(&self) -> FxHashMap<SharedString, Arc<dyn DapLocator>> {
         self.0.read().locators.clone()
     }
 

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

@@ -38,8 +38,8 @@ use serde_json::Value;
 use settings::Settings;
 use stack_frame_list::StackFrameList;
 use task::{
-    DebugScenario, LaunchRequest, TaskContext, substitute_variables_in_map,
-    substitute_variables_in_str,
+    BuildTaskDefinition, DebugScenario, LaunchRequest, ShellBuilder, SpawnInTerminal, TaskContext,
+    substitute_variables_in_map, substitute_variables_in_str,
 };
 use terminal_view::TerminalView;
 use ui::{
@@ -696,6 +696,7 @@ impl RunningState {
         let task_store = project.read(cx).task_store().downgrade();
         let weak_project = project.downgrade();
         let weak_workspace = workspace.downgrade();
+        let is_local = project.read(cx).is_local();
         cx.spawn_in(window, async move |this, cx| {
             let DebugScenario {
                 adapter,
@@ -706,27 +707,68 @@ impl RunningState {
                 tcp_connection,
                 stop_on_entry,
             } = scenario;
-            let request = if let Some(request) = request {
-                request
-            } else if let Some(build) = build {
-                let Some(task) = task_store.update(cx, |this, cx| {
-                    this.task_inventory().and_then(|inventory| {
-                        inventory
-                            .read(cx)
-                            .task_template_by_label(buffer, worktree_id, &build, cx)
-                    })
-                })?
-                else {
-                    anyhow::bail!("Couldn't find task template for {:?}", build)
+            let build_output = if let Some(build) = build {
+                let (task, locator_name) = match build {
+                    BuildTaskDefinition::Template {
+                        task_template,
+                        locator_name,
+                    } => (task_template, locator_name),
+                    BuildTaskDefinition::ByName(ref label) => {
+                        let Some(task) = task_store.update(cx, |this, cx| {
+                            this.task_inventory().and_then(|inventory| {
+                                inventory.read(cx).task_template_by_label(
+                                    buffer,
+                                    worktree_id,
+                                    &label,
+                                    cx,
+                                )
+                            })
+                        })?
+                        else {
+                            anyhow::bail!("Couldn't find task template for {:?}", build)
+                        };
+                        (task, None)
+                    }
+                };
+                let locator_name = if let Some(locator_name) = locator_name {
+                    debug_assert!(request.is_none());
+                    Some(locator_name)
+                } else if request.is_none() {
+                    dap_store
+                        .update(cx, |this, cx| {
+                            this.debug_scenario_for_build_task(task.clone(), adapter.clone(), cx)
+                                .and_then(|scenario| match scenario.build {
+                                    Some(BuildTaskDefinition::Template {
+                                        locator_name, ..
+                                    }) => locator_name,
+                                    _ => None,
+                                })
+                        })
+                        .ok()
+                        .flatten()
+                } else {
+                    None
                 };
+
                 let Some(task) = task.resolve_task("debug-build-task", &task_context) else {
                     anyhow::bail!("Could not resolve task variables within a debug scenario");
                 };
 
+                let builder = ShellBuilder::new(is_local, &task.resolved.shell);
+                let command_label = builder.command_label(&task.resolved.command_label);
+                let (command, args) =
+                    builder.build(task.resolved.command.clone(), &task.resolved.args);
+
+                let task_with_shell = SpawnInTerminal {
+                    command_label,
+                    command,
+                    args,
+                    ..task.resolved.clone()
+                };
                 let terminal = project
                     .update_in(cx, |project, window, cx| {
                         project.create_terminal(
-                            TerminalKind::Task(task.resolved.clone()),
+                            TerminalKind::Task(task_with_shell.clone()),
                             window.window_handle(),
                             cx,
                         )
@@ -761,9 +803,19 @@ impl RunningState {
                 if !exit_status.success() {
                     anyhow::bail!("Build failed");
                 }
-
+                Some((task.resolved.clone(), locator_name))
+            } else {
+                None
+            };
+            let request = if let Some(request) = request {
+                request
+            } else if let Some((task, locator_name)) = build_output {
+                let locator_name = locator_name
+                    .ok_or_else(|| anyhow!("Could not find a valid locator for a build task"))?;
                 dap_store
-                    .update(cx, |this, cx| this.run_debug_locator(task.resolved, cx))?
+                    .update(cx, |this, cx| {
+                        this.run_debug_locator(&locator_name, task, cx)
+                    })?
                     .await?
             } else {
                 return Err(anyhow!("No request or build provided"));

crates/editor/src/editor.rs 🔗

@@ -5216,10 +5216,7 @@ impl Editor {
                                         for (_, task) in &resolved_tasks.templates {
                                             if let Some(scenario) = this
                                                 .debug_scenario_for_build_task(
-                                                    task.resolved.clone(),
-                                                    SharedString::from(
-                                                        task.original_task().label.clone(),
-                                                    ),
+                                                    task.original_task().clone(),
                                                     debug_adapter.clone(),
                                                     cx,
                                                 )

crates/languages/src/rust.rs 🔗

@@ -651,11 +651,6 @@ impl ContextProvider for RustContextProvider {
         } else {
             vec!["run".into()]
         };
-        let build_task_args = if let Some(package_to_run) = package_to_run {
-            vec!["build".into(), "-p".into(), package_to_run]
-        } else {
-            vec!["build".into()]
-        };
         let mut task_templates = vec![
             TaskTemplate {
                 label: format!(
@@ -689,8 +684,8 @@ impl ContextProvider for RustContextProvider {
                     "test".into(),
                     "-p".into(),
                     RUST_PACKAGE_TASK_VARIABLE.template_value(),
-                    RUST_TEST_NAME_TASK_VARIABLE.template_value(),
                     "--".into(),
+                    RUST_TEST_NAME_TASK_VARIABLE.template_value(),
                     "--nocapture".into(),
                 ],
                 tags: vec!["rust-test".to_owned()],
@@ -709,9 +704,9 @@ impl ContextProvider for RustContextProvider {
                     "--doc".into(),
                     "-p".into(),
                     RUST_PACKAGE_TASK_VARIABLE.template_value(),
-                    RUST_DOC_TEST_NAME_TASK_VARIABLE.template_value(),
                     "--".into(),
                     "--nocapture".into(),
+                    RUST_DOC_TEST_NAME_TASK_VARIABLE.template_value(),
                 ],
                 tags: vec!["rust-doc-test".to_owned()],
                 cwd: Some("$ZED_DIRNAME".to_owned()),
@@ -728,6 +723,7 @@ impl ContextProvider for RustContextProvider {
                     "test".into(),
                     "-p".into(),
                     RUST_PACKAGE_TASK_VARIABLE.template_value(),
+                    "--".into(),
                     RUST_TEST_FRAGMENT_TASK_VARIABLE.template_value(),
                 ],
                 tags: vec!["rust-mod-test".to_owned()],
@@ -783,37 +779,6 @@ impl ContextProvider for RustContextProvider {
                 cwd: Some("$ZED_DIRNAME".to_owned()),
                 ..TaskTemplate::default()
             },
-            TaskTemplate {
-                label: format!(
-                    "Build {} {} (package: {})",
-                    RUST_BIN_KIND_TASK_VARIABLE.template_value(),
-                    RUST_BIN_NAME_TASK_VARIABLE.template_value(),
-                    RUST_PACKAGE_TASK_VARIABLE.template_value(),
-                ),
-                cwd: Some("$ZED_DIRNAME".to_owned()),
-                command: "cargo".into(),
-                args: build_task_args,
-                tags: vec!["rust-main".to_owned()],
-                ..TaskTemplate::default()
-            },
-            TaskTemplate {
-                label: format!(
-                    "Build Test '{}' (package: {})",
-                    RUST_TEST_NAME_TASK_VARIABLE.template_value(),
-                    RUST_PACKAGE_TASK_VARIABLE.template_value(),
-                ),
-                command: "cargo".into(),
-                args: vec![
-                    "test".into(),
-                    "-p".into(),
-                    RUST_PACKAGE_TASK_VARIABLE.template_value(),
-                    RUST_TEST_NAME_TASK_VARIABLE.template_value(),
-                    "--no-run".into(),
-                ],
-                tags: vec!["rust-test".to_owned()],
-                cwd: Some("$ZED_DIRNAME".to_owned()),
-                ..TaskTemplate::default()
-            },
         ];
 
         if let Some(custom_target_dir) = custom_target_dir {

crates/project/src/debugger/dap_store.rs 🔗

@@ -46,7 +46,7 @@ use std::{
     path::{Path, PathBuf},
     sync::{Arc, Once},
 };
-use task::{DebugScenario, SpawnInTerminal};
+use task::{DebugScenario, SpawnInTerminal, TaskTemplate};
 use util::{ResultExt as _, merge_json_value_into};
 use worktree::Worktree;
 
@@ -99,8 +99,7 @@ impl DapStore {
     pub fn init(client: &AnyProtoClient, cx: &mut App) {
         static ADD_LOCATORS: Once = Once::new();
         ADD_LOCATORS.call_once(|| {
-            DapRegistry::global(cx)
-                .add_locator("cargo".into(), Arc::new(locators::cargo::CargoLocator {}))
+            DapRegistry::global(cx).add_locator(Arc::new(locators::cargo::CargoLocator {}))
         });
         client.add_entity_request_handler(Self::handle_run_debug_locator);
         client.add_entity_request_handler(Self::handle_get_debug_adapter_binary);
@@ -282,78 +281,38 @@ impl DapStore {
 
     pub fn debug_scenario_for_build_task(
         &self,
-        mut build: SpawnInTerminal,
-        unresoved_label: SharedString,
+        build: TaskTemplate,
         adapter: SharedString,
         cx: &mut App,
     ) -> Option<DebugScenario> {
-        build.args = build
-            .args
-            .into_iter()
-            .map(|arg| {
-                if arg.starts_with("$") {
-                    arg.strip_prefix("$")
-                        .and_then(|arg| build.env.get(arg).map(ToOwned::to_owned))
-                        .unwrap_or_else(|| arg)
-                } else {
-                    arg
-                }
-            })
-            .collect();
-
         DapRegistry::global(cx)
             .locators()
             .values()
-            .find(|locator| locator.accepts(&build))
-            .map(|_| DebugScenario {
-                adapter,
-                label: format!("Debug `{}`", build.label).into(),
-                build: Some(unresoved_label),
-                request: None,
-                initialize_args: None,
-                tcp_connection: None,
-                stop_on_entry: None,
-            })
+            .find_map(|locator| locator.create_scenario(&build, &adapter))
     }
 
     pub fn run_debug_locator(
         &mut self,
-        mut build_command: SpawnInTerminal,
+        locator_name: &str,
+        build_command: SpawnInTerminal,
         cx: &mut Context<Self>,
     ) -> Task<Result<DebugRequest>> {
         match &self.mode {
             DapStoreMode::Local(_) => {
                 // Pre-resolve args with existing environment.
-                build_command.args = build_command
-                    .args
-                    .into_iter()
-                    .map(|arg| {
-                        if arg.starts_with("$") {
-                            arg.strip_prefix("$")
-                                .and_then(|arg| build_command.env.get(arg).map(ToOwned::to_owned))
-                                .unwrap_or_else(|| arg)
-                        } else {
-                            arg
-                        }
-                    })
-                    .collect();
-                let locators = DapRegistry::global(cx)
-                    .locators()
-                    .values()
-                    .filter(|locator| locator.accepts(&build_command))
-                    .cloned()
-                    .collect::<Vec<_>>();
-                if !locators.is_empty() {
+                let locators = DapRegistry::global(cx).locators();
+                let locator = locators.get(locator_name);
+
+                if let Some(locator) = locator.cloned() {
                     cx.background_spawn(async move {
-                        for locator in locators {
-                            let result = locator
-                                .run(build_command.clone())
-                                .await
-                                .log_with_level(log::Level::Error);
-                            if let Some(result) = result {
-                                return Ok(result);
-                            }
+                        let result = locator
+                            .run(build_command.clone())
+                            .await
+                            .log_with_level(log::Level::Error);
+                        if let Some(result) = result {
+                            return Ok(result);
                         }
+
                         Err(anyhow!(
                             "None of the locators for task `{}` completed successfully",
                             build_command.label
@@ -370,6 +329,7 @@ impl DapStore {
                 let request = ssh.upstream_client.request(proto::RunDebugLocators {
                     project_id: ssh.upstream_project_id,
                     build_command: Some(build_command.to_proto()),
+                    locator: locator_name.to_owned(),
                 });
                 cx.background_spawn(async move {
                     let response = request.await?;
@@ -789,8 +749,11 @@ impl DapStore {
             .build_command
             .ok_or_else(|| anyhow!("missing definition"))?;
         let build_task = SpawnInTerminal::from_proto(task);
+        let locator = envelope.payload.locator;
         let request = this
-            .update(&mut cx, |this, cx| this.run_debug_locator(build_task, cx))?
+            .update(&mut cx, |this, cx| {
+                this.run_debug_locator(&locator, build_task, cx)
+            })?
             .await?;
 
         Ok(request.to_proto())

crates/project/src/debugger/locators/cargo.rs 🔗

@@ -1,12 +1,13 @@
 use anyhow::{Result, anyhow};
 use async_trait::async_trait;
 use dap::{DapLocator, DebugRequest};
+use gpui::SharedString;
 use serde_json::Value;
 use smol::{
     io::AsyncReadExt,
     process::{Command, Stdio},
 };
-use task::SpawnInTerminal;
+use task::{BuildTaskDefinition, DebugScenario, ShellBuilder, SpawnInTerminal, TaskTemplate};
 
 pub(crate) struct CargoLocator;
 
@@ -37,18 +38,51 @@ async fn find_best_executable(executables: &[String], test_name: &str) -> Option
 }
 #[async_trait]
 impl DapLocator for CargoLocator {
-    fn accepts(&self, build_config: &SpawnInTerminal) -> bool {
+    fn name(&self) -> SharedString {
+        SharedString::new_static("rust-cargo-locator")
+    }
+    fn create_scenario(&self, build_config: &TaskTemplate, adapter: &str) -> Option<DebugScenario> {
         if build_config.command != "cargo" {
-            return false;
+            return None;
         }
-        let Some(command) = build_config.args.first().map(|s| s.as_str()) else {
-            return false;
-        };
-        if matches!(command, "check" | "run") {
-            return false;
+        let mut task_template = build_config.clone();
+        let cargo_action = task_template.args.first_mut()?;
+        if cargo_action == "check" {
+            return None;
         }
-        !matches!(command, "test" | "bench")
-            || build_config.args.iter().any(|arg| arg == "--no-run")
+
+        match cargo_action.as_ref() {
+            "run" => {
+                *cargo_action = "build".to_owned();
+            }
+            "test" | "bench" => {
+                let delimiter = task_template
+                    .args
+                    .iter()
+                    .position(|arg| arg == "--")
+                    .unwrap_or(task_template.args.len());
+                if !task_template.args[..delimiter]
+                    .iter()
+                    .any(|arg| arg == "--no-run")
+                {
+                    task_template.args.insert(delimiter, "--no-run".to_owned());
+                }
+            }
+            _ => {}
+        }
+        let label = format!("Debug `{}`", build_config.label);
+        Some(DebugScenario {
+            adapter: adapter.to_owned().into(),
+            label: SharedString::from(label),
+            build: Some(BuildTaskDefinition::Template {
+                task_template,
+                locator_name: Some(self.name()),
+            }),
+            request: None,
+            initialize_args: None,
+            tcp_connection: None,
+            stop_on_entry: None,
+        })
     }
 
     async fn run(&self, build_config: SpawnInTerminal) -> Result<DebugRequest> {
@@ -57,10 +91,19 @@ impl DapLocator for CargoLocator {
                 "Couldn't get cwd from debug config which is needed for locators"
             ));
         };
-
-        let mut child = Command::new("cargo")
-            .args(&build_config.args)
-            .arg("--message-format=json")
+        let builder = ShellBuilder::new(true, &build_config.shell).non_interactive();
+        let (program, args) = builder.build(
+            "cargo".into(),
+            &build_config
+                .args
+                .iter()
+                .cloned()
+                .take_while(|arg| arg != "--")
+                .chain(Some("--message-format=json".to_owned()))
+                .collect(),
+        );
+        let mut child = Command::new(program)
+            .args(args)
             .envs(build_config.env.iter().map(|(k, v)| (k.clone(), v.clone())))
             .current_dir(cwd)
             .stdout(Stdio::piped())
@@ -89,7 +132,6 @@ impl DapLocator for CargoLocator {
         if executables.is_empty() {
             return Err(anyhow!("Couldn't get executable in cargo locator"));
         };
-
         let is_test = build_config.args.first().map_or(false, |arg| arg == "test");
 
         let mut test_name = None;

crates/proto/proto/debugger.proto 🔗

@@ -580,6 +580,7 @@ message DebugAdapterBinary {
 message RunDebugLocators {
     uint64 project_id = 1;
     SpawnInTerminal build_command = 2;
+    string locator = 3;
 }
 
 message DebugRequest {

crates/task/src/debug_format.rs 🔗

@@ -6,6 +6,8 @@ use serde::{Deserialize, Serialize};
 use std::path::PathBuf;
 use std::{net::Ipv4Addr, path::Path};
 
+use crate::TaskTemplate;
+
 /// Represents the host information of the debug adapter
 #[derive(Default, Deserialize, Serialize, PartialEq, Eq, JsonSchema, Clone, Debug)]
 pub struct TcpArgumentsTemplate {
@@ -171,6 +173,18 @@ impl From<AttachRequest> for DebugRequest {
     }
 }
 
+#[derive(Deserialize, Serialize, PartialEq, Eq, JsonSchema, Clone, Debug)]
+#[serde(untagged)]
+#[allow(clippy::large_enum_variant)]
+pub enum BuildTaskDefinition {
+    ByName(SharedString),
+    Template {
+        #[serde(flatten)]
+        task_template: TaskTemplate,
+        #[serde(skip)]
+        locator_name: Option<SharedString>,
+    },
+}
 /// This struct represent a user created debug task
 #[derive(Deserialize, Serialize, PartialEq, Eq, JsonSchema, Clone, Debug)]
 #[serde(rename_all = "snake_case")]
@@ -180,7 +194,7 @@ pub struct DebugScenario {
     pub label: SharedString,
     /// A task to run prior to spawning the debuggee.
     #[serde(default)]
-    pub build: Option<SharedString>,
+    pub build: Option<BuildTaskDefinition>,
     #[serde(flatten)]
     pub request: Option<DebugRequest>,
     /// Additional initialization arguments to be sent on DAP initialization

crates/task/src/lib.rs 🔗

@@ -16,7 +16,8 @@ use std::path::PathBuf;
 use std::str::FromStr;
 
 pub use debug_format::{
-    AttachRequest, DebugRequest, DebugScenario, DebugTaskFile, LaunchRequest, TcpArgumentsTemplate,
+    AttachRequest, BuildTaskDefinition, DebugRequest, DebugScenario, DebugTaskFile, LaunchRequest,
+    TcpArgumentsTemplate,
 };
 pub use task_template::{
     DebugArgsRequest, HideStrategy, RevealStrategy, TaskTemplate, TaskTemplates,
@@ -341,6 +342,7 @@ enum WindowsShellType {
 pub struct ShellBuilder {
     program: String,
     args: Vec<String>,
+    interactive: bool,
 }
 
 pub static DEFAULT_REMOTE_SHELL: &str = "\"${SHELL:-sh}\"";
@@ -359,7 +361,15 @@ impl ShellBuilder {
             Shell::Program(shell) => (shell.clone(), Vec::new()),
             Shell::WithArguments { program, args, .. } => (program.clone(), args.clone()),
         };
-        Self { program, args }
+        Self {
+            program,
+            args,
+            interactive: true,
+        }
+    }
+    pub fn non_interactive(mut self) -> Self {
+        self.interactive = false;
+        self
     }
 }
 
@@ -367,7 +377,8 @@ impl ShellBuilder {
 impl ShellBuilder {
     /// Returns the label to show in the terminal tab
     pub fn command_label(&self, command_label: &str) -> String {
-        format!("{} -i -c '{}'", self.program, command_label)
+        let interactivity = self.interactive.then_some("-i ").unwrap_or_default();
+        format!("{} {interactivity}-c '{}'", self.program, command_label)
     }
 
     /// Returns the program and arguments to run this task in a shell.
@@ -379,8 +390,12 @@ impl ShellBuilder {
                 command.push_str(&arg);
                 command
             });
-        self.args
-            .extend(["-i".to_owned(), "-c".to_owned(), combined_command]);
+        self.args.extend(
+            self.interactive
+                .then(|| "-i".to_owned())
+                .into_iter()
+                .chain(["-c".to_owned(), combined_command]),
+        );
 
         (self.program, self.args)
     }