mirror of
https://github.com/zed-industries/zed.git
synced 2026-06-01 03:14:56 +07:00
tasks: Show error for unknown ZED_ variables (#45621)
Validate task variable names when the file is saved, immediately displaying an error toast if any invalid `ZED_*` variables are found. Valid tasks in the same file are still loaded and work normally. - Add `task::task_template::TaskTemplate::unknown_variables()` to detect invalid `ZED_` variable names - Add `project::ToastLink` struct and optional `link` field to `project::Event::Toast` - Show documentation link in the error toast Closes #23275 Release Notes: - Fixed user-defined tasks with unresolved `ZED_*` variables being silently omitted --------- Co-authored-by: dino <dinojoaocosta@gmail.com>
This commit is contained in:
Karl-Erik Enkelmann
GitHub
parent
b74b1977d4
commit
9d5f0f5aaf
8 changed files with 250 additions and 14 deletions
|
|
@ -619,6 +619,7 @@ impl GitBlame {
|
|||
cx.emit(project::Event::Toast {
|
||||
notification_id: "git-blame".into(),
|
||||
message: notification,
|
||||
link: None,
|
||||
});
|
||||
} else {
|
||||
// If we weren't triggered by a user, we just log errors in the background, instead of sending
|
||||
|
|
@ -786,7 +787,8 @@ mod tests {
|
|||
project::Event::Toast {
|
||||
notification_id: "git-blame".into(),
|
||||
message: "Failed to blame \"file.txt\": failed to get blame for \"file.txt\""
|
||||
.to_string()
|
||||
.to_string(),
|
||||
link: None
|
||||
}
|
||||
);
|
||||
|
||||
|
|
|
|||
|
|
@ -305,6 +305,13 @@ enum ProjectClientState {
|
|||
},
|
||||
}
|
||||
|
||||
/// A link to display in a toast notification, useful to point to documentation.
|
||||
#[derive(PartialEq, Debug, Clone)]
|
||||
pub struct ToastLink {
|
||||
pub label: &'static str,
|
||||
pub url: &'static str,
|
||||
}
|
||||
|
||||
#[derive(Clone, Debug, PartialEq)]
|
||||
pub enum Event {
|
||||
LanguageServerAdded(LanguageServerId, LanguageServerName, Option<WorktreeId>),
|
||||
|
|
@ -326,6 +333,8 @@ pub enum Event {
|
|||
Toast {
|
||||
notification_id: SharedString,
|
||||
message: String,
|
||||
/// Optional link to display as a button in the toast.
|
||||
link: Option<ToastLink>,
|
||||
},
|
||||
HideToast {
|
||||
notification_id: SharedString,
|
||||
|
|
@ -3215,6 +3224,7 @@ impl Project {
|
|||
cx.emit(Event::Toast {
|
||||
notification_id: "dap".into(),
|
||||
message: message.clone(),
|
||||
link: None,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
|
@ -3345,6 +3355,7 @@ impl Project {
|
|||
LspStoreEvent::Notification(message) => cx.emit(Event::Toast {
|
||||
notification_id: "lsp".into(),
|
||||
message: message.clone(),
|
||||
link: None,
|
||||
}),
|
||||
LspStoreEvent::SnippetEdit {
|
||||
buffer_id,
|
||||
|
|
@ -3395,6 +3406,7 @@ impl Project {
|
|||
let message = format!("Failed to set local settings in {path:?}:\n{message}");
|
||||
cx.emit(Event::Toast {
|
||||
notification_id: format!("local-settings-{path:?}").into(),
|
||||
link: None,
|
||||
message,
|
||||
});
|
||||
}
|
||||
|
|
@ -3408,6 +3420,10 @@ impl Project {
|
|||
let message = format!("Failed to set local tasks in {path:?}:\n{message}");
|
||||
cx.emit(Event::Toast {
|
||||
notification_id: format!("local-tasks-{path:?}").into(),
|
||||
link: Some(ToastLink {
|
||||
label: "Open Tasks Documentation",
|
||||
url: "https://zed.dev/docs/tasks",
|
||||
}),
|
||||
message,
|
||||
});
|
||||
}
|
||||
|
|
@ -3422,6 +3438,7 @@ impl Project {
|
|||
format!("Failed to set local debug scenarios in {path:?}:\n{message}");
|
||||
cx.emit(Event::Toast {
|
||||
notification_id: format!("local-debug-scenarios-{path:?}").into(),
|
||||
link: None,
|
||||
message,
|
||||
});
|
||||
}
|
||||
|
|
@ -4888,6 +4905,7 @@ impl Project {
|
|||
cx.emit(Event::Toast {
|
||||
notification_id: envelope.payload.notification_id.into(),
|
||||
message: envelope.payload.message,
|
||||
link: None,
|
||||
});
|
||||
Ok(())
|
||||
})
|
||||
|
|
|
|||
|
|
@ -512,6 +512,7 @@ impl Inventory {
|
|||
let new_resolved_tasks = worktree_tasks
|
||||
.flat_map(|(kind, task)| {
|
||||
let id_base = kind.to_id_base();
|
||||
|
||||
if let TaskSourceKind::Worktree { id, .. } = &kind {
|
||||
None.or_else(|| {
|
||||
let (_, _, item_context) =
|
||||
|
|
@ -660,8 +661,31 @@ impl Inventory {
|
|||
});
|
||||
}
|
||||
};
|
||||
|
||||
let mut validation_errors = Vec::new();
|
||||
let new_templates = raw_tasks.into_iter().filter_map(|raw_template| {
|
||||
serde_json::from_value::<TaskTemplate>(raw_template).log_err()
|
||||
let template = serde_json::from_value::<TaskTemplate>(raw_template).log_err()?;
|
||||
|
||||
// Validate the variable names used in the `TaskTemplate`.
|
||||
let unknown_variables = template.unknown_variables();
|
||||
if !unknown_variables.is_empty() {
|
||||
let variables_list = unknown_variables
|
||||
.iter()
|
||||
.map(|variable| format!("${variable}"))
|
||||
.collect::<Vec<_>>()
|
||||
.join(", ");
|
||||
|
||||
validation_errors.push(format!(
|
||||
"Task '{}' uses unknown variables: {}",
|
||||
template.label, variables_list
|
||||
));
|
||||
|
||||
// Skip this template, since it uses unknown variable names, but
|
||||
// continue processing others.
|
||||
return None;
|
||||
}
|
||||
|
||||
Some(template)
|
||||
});
|
||||
|
||||
let parsed_templates = &mut self.templates_from_settings;
|
||||
|
|
@ -710,6 +734,18 @@ impl Inventory {
|
|||
}
|
||||
}
|
||||
|
||||
if !validation_errors.is_empty() {
|
||||
return Err(InvalidSettingsError::Tasks {
|
||||
path: match &location {
|
||||
TaskSettingsLocation::Global(path) => path.to_path_buf(),
|
||||
TaskSettingsLocation::Worktree(location) => {
|
||||
location.path.as_std_path().join(task_file_name())
|
||||
}
|
||||
},
|
||||
message: validation_errors.join("\n"),
|
||||
});
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -68,10 +68,12 @@ use settings::SettingsStore;
|
|||
#[cfg(not(windows))]
|
||||
use std::os;
|
||||
use std::{
|
||||
cell::RefCell,
|
||||
env, mem,
|
||||
num::NonZeroU32,
|
||||
ops::Range,
|
||||
path::{Path, PathBuf},
|
||||
rc::Rc,
|
||||
str::FromStr,
|
||||
sync::{Arc, OnceLock},
|
||||
task::Poll,
|
||||
|
|
@ -1112,6 +1114,64 @@ async fn test_managing_project_specific_settings(cx: &mut gpui::TestAppContext)
|
|||
);
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_invalid_local_tasks_shows_toast_with_doc_link(cx: &mut gpui::TestAppContext) {
|
||||
init_test(cx);
|
||||
TaskStore::init(None);
|
||||
|
||||
// We need to start with a valid `.zed/tasks.json` file as otherwise the
|
||||
// event is emitted before we havd a chance to setup the event subscription.
|
||||
let fs = FakeFs::new(cx.executor());
|
||||
fs.insert_tree(
|
||||
path!("/dir"),
|
||||
json!({
|
||||
".zed": {
|
||||
"tasks.json": r#"[{ "label": "valid task", "command": "echo" }]"#,
|
||||
},
|
||||
"file.rs": ""
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
|
||||
let project = Project::test(fs.clone(), [path!("/dir").as_ref()], cx).await;
|
||||
let saw_toast = Rc::new(RefCell::new(false));
|
||||
|
||||
// Update the `.zed/tasks.json` file with an invalid variable, so we can
|
||||
// later assert that the `Event::Toast` even is emitted.
|
||||
fs.save(
|
||||
path!("/dir/.zed/tasks.json").as_ref(),
|
||||
&r#"[{ "label": "test $ZED_FOO", "command": "echo" }]"#.into(),
|
||||
Default::default(),
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
project.update(cx, |_, cx| {
|
||||
let saw_toast = saw_toast.clone();
|
||||
|
||||
cx.subscribe(&project, move |_, _, event: &Event, _| match event {
|
||||
Event::Toast {
|
||||
notification_id,
|
||||
message,
|
||||
link: Some(ToastLink { url, .. }),
|
||||
} => {
|
||||
assert!(notification_id.starts_with("local-tasks-"));
|
||||
assert!(message.contains("ZED_FOO"));
|
||||
assert_eq!(*url, "https://zed.dev/docs/tasks");
|
||||
*saw_toast.borrow_mut() = true;
|
||||
}
|
||||
_ => {}
|
||||
})
|
||||
.detach();
|
||||
});
|
||||
|
||||
cx.run_until_parked();
|
||||
assert!(
|
||||
*saw_toast.borrow(),
|
||||
"Expected `Event::Toast` was never emitted"
|
||||
);
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_fallback_to_single_worktree_tasks(cx: &mut gpui::TestAppContext) {
|
||||
init_test(cx);
|
||||
|
|
|
|||
|
|
@ -1834,6 +1834,7 @@ impl ProjectPanel {
|
|||
),
|
||||
abs_path
|
||||
),
|
||||
link: None,
|
||||
})
|
||||
});
|
||||
None
|
||||
|
|
|
|||
|
|
@ -128,8 +128,6 @@ impl TaskTemplates {
|
|||
|
||||
impl TaskTemplate {
|
||||
/// Replaces all `VariableName` task variables in the task template string fields.
|
||||
/// If any replacement fails or the new string substitutions still have [`ZED_VARIABLE_NAME_PREFIX`],
|
||||
/// `None` is returned.
|
||||
///
|
||||
/// Every [`ResolvedTask`] gets a [`TaskId`], based on the `id_base` (to avoid collision with various task sources),
|
||||
/// and hashes of its template and [`TaskContext`], see [`ResolvedTask`] fields' documentation for more details.
|
||||
|
|
@ -275,6 +273,53 @@ impl TaskTemplate {
|
|||
},
|
||||
})
|
||||
}
|
||||
|
||||
/// Validates that all `$ZED_*` variables used in this template are known
|
||||
/// variable names, returning a vector with all of the unique unknown
|
||||
/// variables.
|
||||
///
|
||||
/// Note that `$ZED_CUSTOM_*` variables are never considered to be invalid
|
||||
/// since those are provided dynamically by extensions.
|
||||
pub fn unknown_variables(&self) -> Vec<String> {
|
||||
let mut variables = HashSet::default();
|
||||
|
||||
Self::collect_unknown_variables(&self.label, &mut variables);
|
||||
Self::collect_unknown_variables(&self.command, &mut variables);
|
||||
|
||||
self.args
|
||||
.iter()
|
||||
.for_each(|arg| Self::collect_unknown_variables(arg, &mut variables));
|
||||
|
||||
self.env
|
||||
.values()
|
||||
.for_each(|value| Self::collect_unknown_variables(value, &mut variables));
|
||||
|
||||
if let Some(cwd) = &self.cwd {
|
||||
Self::collect_unknown_variables(cwd, &mut variables);
|
||||
}
|
||||
|
||||
variables.into_iter().collect()
|
||||
}
|
||||
|
||||
fn collect_unknown_variables(template: &str, unknown: &mut HashSet<String>) {
|
||||
shellexpand::env_with_context_no_errors(template, |variable| {
|
||||
// It's possible that the variable has a default defined, which is
|
||||
// separated by a `:`, for example, `${ZED_FILE:default_value} so we
|
||||
// ensure that we're only looking at the variable name itself.
|
||||
let colon_position = variable.find(':').unwrap_or(variable.len());
|
||||
let variable_name = &variable[..colon_position];
|
||||
|
||||
if variable_name.starts_with(ZED_VARIABLE_NAME_PREFIX)
|
||||
&& let without_prefix = &variable_name[ZED_VARIABLE_NAME_PREFIX.len()..]
|
||||
&& !without_prefix.starts_with("CUSTOM_")
|
||||
&& variable_name.parse::<VariableName>().is_err()
|
||||
{
|
||||
unknown.insert(variable_name.to_string());
|
||||
}
|
||||
|
||||
None::<&str>
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
const MAX_DISPLAY_VARIABLE_LENGTH: usize = 15;
|
||||
|
|
@ -327,7 +372,9 @@ fn substitute_all_template_variables_in_str<A: AsRef<str>>(
|
|||
substituted_variables: &mut HashSet<VariableName>,
|
||||
) -> Option<String> {
|
||||
let substituted_string = shellexpand::env_with_context(template_str, |var| {
|
||||
// Colons denote a default value in case the variable is not set. We want to preserve that default, as otherwise shellexpand will substitute it for us.
|
||||
// Colons denote a default value in case the variable is not set. We
|
||||
// want to preserve that default, as otherwise shellexpand will
|
||||
// substitute it for us.
|
||||
let colon_position = var.find(':').unwrap_or(var.len());
|
||||
let (variable_name, default) = var.split_at(colon_position);
|
||||
if let Some(name) = task_variables.get(variable_name) {
|
||||
|
|
@ -346,15 +393,19 @@ fn substitute_all_template_variables_in_str<A: AsRef<str>>(
|
|||
}
|
||||
}
|
||||
// This is an unknown variable.
|
||||
// We should not error out, as they may come from user environment (e.g. $PATH). That means that the variable substitution might not be perfect.
|
||||
// If there's a default, we need to return the string verbatim as otherwise shellexpand will apply that default for us.
|
||||
// We should not error out, as they may come from user environment (e.g.
|
||||
// $PATH). That means that the variable substitution might not be
|
||||
// perfect. If there's a default, we need to return the string verbatim
|
||||
// as otherwise shellexpand will apply that default for us.
|
||||
if !default.is_empty() {
|
||||
return Ok(Some(format!("${{{var}}}")));
|
||||
}
|
||||
|
||||
// Else we can just return None and that variable will be left as is.
|
||||
Ok(None)
|
||||
})
|
||||
.ok()?;
|
||||
|
||||
Some(substituted_string.into_owned())
|
||||
}
|
||||
|
||||
|
|
@ -374,6 +425,7 @@ fn substitute_all_template_variables_in_vec(
|
|||
)?;
|
||||
expanded.push(new_value);
|
||||
}
|
||||
|
||||
Some(expanded)
|
||||
}
|
||||
|
||||
|
|
@ -424,6 +476,7 @@ fn substitute_all_template_variables_in_map(
|
|||
)?;
|
||||
new_map.insert(new_key, new_value);
|
||||
}
|
||||
|
||||
Some(new_map)
|
||||
}
|
||||
|
||||
|
|
@ -696,8 +749,8 @@ mod tests {
|
|||
project_env: HashMap::default(),
|
||||
},
|
||||
);
|
||||
assert_eq!(
|
||||
resolved_task_attempt, None,
|
||||
assert!(
|
||||
matches!(resolved_task_attempt, None),
|
||||
"If any of the Zed task variables is not substituted, the task should not be resolved, but got some resolution without the variable {removed_variable:?} (index {i})"
|
||||
);
|
||||
}
|
||||
|
|
@ -971,4 +1024,54 @@ mod tests {
|
|||
"Should fail when ZED variable has no default and doesn't exist"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_unknown_variables() {
|
||||
// Variable names starting with `ZED_` that are not valid should be
|
||||
// reported.
|
||||
let label = "test unknown variables".to_string();
|
||||
let command = "$ZED_UNKNOWN".to_string();
|
||||
let task = TaskTemplate {
|
||||
label,
|
||||
command,
|
||||
..TaskTemplate::default()
|
||||
};
|
||||
|
||||
assert_eq!(task.unknown_variables(), vec!["ZED_UNKNOWN".to_string()]);
|
||||
|
||||
// Variable names starting with `ZED_CUSTOM_` should never be reported,
|
||||
// as those are dynamically provided by extensions.
|
||||
let label = "test custom variables".to_string();
|
||||
let command = "$ZED_CUSTOM_UNKNOWN".to_string();
|
||||
let task = TaskTemplate {
|
||||
label,
|
||||
command,
|
||||
..TaskTemplate::default()
|
||||
};
|
||||
|
||||
assert!(task.unknown_variables().is_empty());
|
||||
|
||||
// Unknown variable names with defaults should still be reported,
|
||||
// otherwise the default would always be silently used.
|
||||
let label = "test custom variables".to_string();
|
||||
let command = "${ZED_UNKNOWN:default_value}".to_string();
|
||||
let task = TaskTemplate {
|
||||
label,
|
||||
command,
|
||||
..TaskTemplate::default()
|
||||
};
|
||||
|
||||
assert_eq!(task.unknown_variables(), vec!["ZED_UNKNOWN".to_string()]);
|
||||
|
||||
// Valid variable names are not reported.
|
||||
let label = "test custom variables".to_string();
|
||||
let command = "$ZED_FILE".to_string();
|
||||
let task = TaskTemplate {
|
||||
label,
|
||||
command,
|
||||
..TaskTemplate::default()
|
||||
};
|
||||
|
||||
assert!(task.unknown_variables().is_empty());
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -124,7 +124,7 @@ impl TasksModalDelegate {
|
|||
|
||||
pub struct TasksModal {
|
||||
pub picker: Entity<Picker<TasksModalDelegate>>,
|
||||
_subscription: [Subscription; 2],
|
||||
_subscriptions: [Subscription; 2],
|
||||
}
|
||||
|
||||
impl TasksModal {
|
||||
|
|
@ -139,13 +139,18 @@ impl TasksModal {
|
|||
) -> Self {
|
||||
let picker = cx.new(|cx| {
|
||||
Picker::uniform_list(
|
||||
TasksModalDelegate::new(task_store, task_contexts, task_overrides, workspace),
|
||||
TasksModalDelegate::new(
|
||||
task_store.clone(),
|
||||
task_contexts,
|
||||
task_overrides,
|
||||
workspace.clone(),
|
||||
),
|
||||
window,
|
||||
cx,
|
||||
)
|
||||
.modal(is_modal)
|
||||
});
|
||||
let _subscription = [
|
||||
let mut _subscriptions = [
|
||||
cx.subscribe(&picker, |_, _, _: &DismissEvent, cx| {
|
||||
cx.emit(DismissEvent);
|
||||
}),
|
||||
|
|
@ -155,9 +160,10 @@ impl TasksModal {
|
|||
});
|
||||
}),
|
||||
];
|
||||
|
||||
Self {
|
||||
picker,
|
||||
_subscription,
|
||||
_subscriptions,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1346,10 +1346,20 @@ impl Workspace {
|
|||
project::Event::Toast {
|
||||
notification_id,
|
||||
message,
|
||||
link,
|
||||
} => this.show_notification(
|
||||
NotificationId::named(notification_id.clone()),
|
||||
cx,
|
||||
|cx| cx.new(|cx| MessageNotification::new(message.clone(), cx)),
|
||||
|cx| {
|
||||
let mut notification = MessageNotification::new(message.clone(), cx);
|
||||
if let Some(link) = link {
|
||||
notification = notification
|
||||
.more_info_message(link.label)
|
||||
.more_info_url(link.url);
|
||||
}
|
||||
|
||||
cx.new(|_| notification)
|
||||
},
|
||||
),
|
||||
|
||||
project::Event::HideToast { notification_id } => {
|
||||
|
|
|
|||
Loading…
Reference in a new issue