Thorsten Ball created
Closes #16876
We only ever showed parsing errors, but not if something failed to
deserialize.
Basically, if you had a stray `,` somewhere, we'd show a notification
for user errors, but only squiggly lines if you had a `[]` instead of a
`{}`.
The squiggly lines would only show up when there were schema errors.
In the case of `formatter` settings, for example, if someone put in a
`{}` instead of `[]`, we'd never show anything.
With this change we always show a notification if parsing user or
project settings fails.
(Right now, the error message might still be bad, but that's a separate
change)
Release Notes:
- Added a notification to warn users if their user settings or
project-local settings failed to deserialize.
Demo:
https://github.com/user-attachments/assets/e5c48165-f2f7-4b5c-9c6d-6ea74f678683crates/language/src/language_settings.rs | 7 +
crates/project/src/project.rs | 26 ++++++
crates/project/src/project_settings.rs | 45 ++++++++++-
crates/settings/src/settings.rs | 4
crates/settings/src/settings_store.rs | 95 +++++++++++++++++--------
crates/workspace/src/workspace.rs | 19 ++++
crates/zed/src/main.rs | 28 +++++--
7 files changed, 173 insertions(+), 51 deletions(-)
@@ -1152,6 +1152,13 @@ mod tests {
);
}
+ #[test]
+ fn test_formatter_deserialization_invalid() {
+ let raw_auto = "{\"formatter\": {}}";
+ let result: Result<LanguageSettingsContent, _> = serde_json::from_str(raw_auto);
+ assert!(result.is_err());
+ }
+
#[test]
pub fn test_resolve_language_servers() {
fn language_server_names(names: &[&str]) -> Vec<LanguageServerName> {
@@ -59,12 +59,14 @@ use node_runtime::NodeRuntime;
use parking_lot::{Mutex, RwLock};
use paths::{local_tasks_file_relative_path, local_vscode_tasks_file_relative_path};
pub use prettier_store::PrettierStore;
-use project_settings::{ProjectSettings, SettingsObserver};
+use project_settings::{ProjectSettings, SettingsObserver, SettingsObserverEvent};
use remote::SshSession;
use rpc::{proto::SSH_PROJECT_ID, AnyProtoClient, ErrorCode};
use search::{SearchInputKind, SearchQuery, SearchResult};
use search_history::SearchHistory;
-use settings::{watch_config_file, Settings, SettingsLocation, SettingsStore};
+use settings::{
+ watch_config_file, InvalidSettingsError, Settings, SettingsLocation, SettingsStore,
+};
use smol::channel::Receiver;
use snippet::Snippet;
use snippet_provider::SnippetProvider;
@@ -230,6 +232,7 @@ pub enum Event {
LanguageServerRemoved(LanguageServerId),
LanguageServerLog(LanguageServerId, LanguageServerLogType, String),
Notification(String),
+ LocalSettingsUpdated(Result<(), InvalidSettingsError>),
LanguageServerPrompt(LanguageServerPromptRequest),
LanguageNotFound(Model<Buffer>),
ActiveEntryChanged(Option<ProjectEntryId>),
@@ -644,6 +647,8 @@ impl Project {
let settings_observer = cx.new_model(|cx| {
SettingsObserver::new_local(fs.clone(), worktree_store.clone(), cx)
});
+ cx.subscribe(&settings_observer, Self::on_settings_observer_event)
+ .detach();
let environment = ProjectEnvironment::new(&worktree_store, env, cx);
let lsp_store = cx.new_model(|cx| {
@@ -729,6 +734,8 @@ impl Project {
let settings_observer = cx.new_model(|cx| {
SettingsObserver::new_ssh(ssh.clone().into(), worktree_store.clone(), cx)
});
+ cx.subscribe(&settings_observer, Self::on_settings_observer_event)
+ .detach();
let environment = ProjectEnvironment::new(&worktree_store, None, cx);
let lsp_store = cx.new_model(|cx| {
@@ -913,6 +920,8 @@ impl Project {
cx.subscribe(&buffer_store, Self::on_buffer_store_event)
.detach();
cx.subscribe(&lsp_store, Self::on_lsp_store_event).detach();
+ cx.subscribe(&settings_observer, Self::on_settings_observer_event)
+ .detach();
let mut this = Self {
buffer_ordered_messages_tx: tx,
@@ -2058,6 +2067,19 @@ impl Project {
}
}
+ fn on_settings_observer_event(
+ &mut self,
+ _: Model<SettingsObserver>,
+ event: &SettingsObserverEvent,
+ cx: &mut ModelContext<Self>,
+ ) {
+ match event {
+ SettingsObserverEvent::LocalSettingsUpdated(error) => {
+ cx.emit(Event::LocalSettingsUpdated(error.clone()))
+ }
+ }
+ }
+
fn on_worktree_store_event(
&mut self,
_: Model<WorktreeStore>,
@@ -1,11 +1,11 @@
use collections::HashMap;
use fs::Fs;
-use gpui::{AppContext, AsyncAppContext, BorrowAppContext, Model, ModelContext};
+use gpui::{AppContext, AsyncAppContext, BorrowAppContext, EventEmitter, Model, ModelContext};
use paths::local_settings_file_relative_path;
use rpc::{proto, AnyProtoClient, TypedEnvelope};
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
-use settings::{Settings, SettingsSources, SettingsStore};
+use settings::{InvalidSettingsError, Settings, SettingsSources, SettingsStore};
use std::{
path::{Path, PathBuf},
sync::Arc,
@@ -176,6 +176,13 @@ pub enum SettingsObserverMode {
Remote,
}
+#[derive(Clone, Debug, PartialEq)]
+pub enum SettingsObserverEvent {
+ LocalSettingsUpdated(Result<(), InvalidSettingsError>),
+}
+
+impl EventEmitter<SettingsObserverEvent> for SettingsObserver {}
+
pub struct SettingsObserver {
mode: SettingsObserverMode,
downstream_client: Option<AnyProtoClient>,
@@ -415,11 +422,16 @@ impl SettingsObserver {
) {
let worktree_id = worktree.read(cx).id();
let remote_worktree_id = worktree.read(cx).id();
- cx.update_global::<SettingsStore, _>(|store, cx| {
+
+ let result = cx.update_global::<SettingsStore, anyhow::Result<()>>(|store, cx| {
for (directory, file_content) in settings_contents {
- store
- .set_local_settings(worktree_id, directory.clone(), file_content.as_deref(), cx)
- .log_err();
+ store.set_local_settings(
+ worktree_id,
+ directory.clone(),
+ file_content.as_deref(),
+ cx,
+ )?;
+
if let Some(downstream_client) = &self.downstream_client {
downstream_client
.send(proto::UpdateWorktreeSettings {
@@ -431,6 +443,25 @@ impl SettingsObserver {
.log_err();
}
}
- })
+ anyhow::Ok(())
+ });
+
+ match result {
+ Err(error) => {
+ if let Ok(error) = error.downcast::<InvalidSettingsError>() {
+ if let InvalidSettingsError::LocalSettings {
+ ref path,
+ ref message,
+ } = error
+ {
+ log::error!("Failed to set local settings in {:?}: {:?}", path, message);
+ cx.emit(SettingsObserverEvent::LocalSettingsUpdated(Err(error)));
+ }
+ }
+ }
+ Ok(()) => {
+ cx.emit(SettingsObserverEvent::LocalSettingsUpdated(Ok(())));
+ }
+ }
}
}
@@ -13,7 +13,9 @@ pub use editable_setting_control::*;
pub use json_schema::*;
pub use keymap_file::KeymapFile;
pub use settings_file::*;
-pub use settings_store::{Settings, SettingsLocation, SettingsSources, SettingsStore};
+pub use settings_store::{
+ InvalidSettingsError, Settings, SettingsLocation, SettingsSources, SettingsStore,
+};
#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash, PartialOrd, Ord)]
pub struct WorktreeId(usize);
@@ -3,6 +3,7 @@ use collections::{btree_map, hash_map, BTreeMap, HashMap};
use fs::Fs;
use futures::{channel::mpsc, future::LocalBoxFuture, FutureExt, StreamExt};
use gpui::{AppContext, AsyncAppContext, BorrowAppContext, Global, Task, UpdateGlobal};
+use paths::local_settings_file_relative_path;
use schemars::{gen::SchemaGenerator, schema::RootSchema, JsonSchema};
use serde::{de::DeserializeOwned, Deserialize as _, Serialize};
use smallvec::SmallVec;
@@ -10,7 +11,7 @@ use std::{
any::{type_name, Any, TypeId},
fmt::Debug,
ops::Range,
- path::Path,
+ path::{Path, PathBuf},
str,
sync::{Arc, LazyLock},
};
@@ -694,9 +695,14 @@ impl SettingsStore {
.deserialize_setting(&self.raw_extension_settings)
.log_err();
- let user_settings = setting_value
- .deserialize_setting(&self.raw_user_settings)
- .log_err();
+ let user_settings = match setting_value.deserialize_setting(&self.raw_user_settings) {
+ Ok(settings) => Some(settings),
+ Err(error) => {
+ return Err(anyhow!(InvalidSettingsError::UserSettings {
+ message: error.to_string()
+ }));
+ }
+ };
let mut release_channel_settings = None;
if let Some(release_settings) = &self
@@ -746,34 +752,43 @@ impl SettingsStore {
break;
}
- if let Some(local_settings) =
- setting_value.deserialize_setting(local_settings).log_err()
- {
- paths_stack.push(Some((*root_id, path.as_ref())));
- project_settings_stack.push(local_settings);
-
- // If a local settings file changed, then avoid recomputing local
- // settings for any path outside of that directory.
- if changed_local_path.map_or(false, |(changed_root_id, changed_local_path)| {
- *root_id != changed_root_id || !path.starts_with(changed_local_path)
- }) {
- continue;
- }
-
- if let Some(value) = setting_value
- .load_setting(
- SettingsSources {
- default: &default_settings,
- extensions: extension_settings.as_ref(),
- user: user_settings.as_ref(),
- release_channel: release_channel_settings.as_ref(),
- project: &project_settings_stack.iter().collect::<Vec<_>>(),
+ match setting_value.deserialize_setting(local_settings) {
+ Ok(local_settings) => {
+ paths_stack.push(Some((*root_id, path.as_ref())));
+ project_settings_stack.push(local_settings);
+
+ // If a local settings file changed, then avoid recomputing local
+ // settings for any path outside of that directory.
+ if changed_local_path.map_or(
+ false,
+ |(changed_root_id, changed_local_path)| {
+ *root_id != changed_root_id || !path.starts_with(changed_local_path)
},
- cx,
- )
- .log_err()
- {
- setting_value.set_local_value(*root_id, path.clone(), value);
+ ) {
+ continue;
+ }
+
+ if let Some(value) = setting_value
+ .load_setting(
+ SettingsSources {
+ default: &default_settings,
+ extensions: extension_settings.as_ref(),
+ user: user_settings.as_ref(),
+ release_channel: release_channel_settings.as_ref(),
+ project: &project_settings_stack.iter().collect::<Vec<_>>(),
+ },
+ cx,
+ )
+ .log_err()
+ {
+ setting_value.set_local_value(*root_id, path.clone(), value);
+ }
+ }
+ Err(error) => {
+ return Err(anyhow!(InvalidSettingsError::LocalSettings {
+ path: path.join(local_settings_file_relative_path()),
+ message: error.to_string()
+ }));
}
}
}
@@ -782,6 +797,24 @@ impl SettingsStore {
}
}
+#[derive(Debug, Clone, PartialEq)]
+pub enum InvalidSettingsError {
+ LocalSettings { path: PathBuf, message: String },
+ UserSettings { message: String },
+}
+
+impl std::fmt::Display for InvalidSettingsError {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ InvalidSettingsError::LocalSettings { message, .. }
+ | InvalidSettingsError::UserSettings { message } => {
+ write!(f, "{}", message)
+ }
+ }
+ }
+}
+impl std::error::Error for InvalidSettingsError {}
+
impl Debug for SettingsStore {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("SettingsStore")
@@ -64,7 +64,7 @@ use project::{
use remote::{SshConnectionOptions, SshSession};
use serde::Deserialize;
use session::AppSession;
-use settings::Settings;
+use settings::{InvalidSettingsError, Settings};
use shared_screen::SharedScreen;
use sqlez::{
bindable::{Bind, Column, StaticColumnCount},
@@ -832,6 +832,23 @@ impl Workspace {
}
}
+ project::Event::LocalSettingsUpdated(result) => {
+ struct LocalSettingsUpdated;
+ let id = NotificationId::unique::<LocalSettingsUpdated>();
+
+ match result {
+ Err(InvalidSettingsError::LocalSettings { message, path }) => {
+ let full_message =
+ format!("Failed to set local settings in {:?}:\n{}", path, message);
+ this.show_notification(id, cx, |cx| {
+ cx.new_view(|_| MessageNotification::new(full_message.clone()))
+ })
+ }
+ Err(_) => {}
+ Ok(_) => this.dismiss_notification(&id, cx),
+ }
+ }
+
project::Event::Notification(message) => {
struct ProjectNotification;
@@ -34,7 +34,9 @@ use parking_lot::Mutex;
use recent_projects::open_ssh_project;
use release_channel::{AppCommitSha, AppVersion};
use session::{AppSession, Session};
-use settings::{handle_settings_file_changes, watch_config_file, Settings, SettingsStore};
+use settings::{
+ handle_settings_file_changes, watch_config_file, InvalidSettingsError, Settings, SettingsStore,
+};
use simplelog::ConfigBuilder;
use smol::process::Command;
use std::{
@@ -626,20 +628,28 @@ fn handle_settings_changed(error: Option<anyhow::Error>, cx: &mut AppContext) {
for workspace in workspace::local_workspace_windows(cx) {
workspace
- .update(cx, |workspace, cx| match &error {
- Some(error) => {
- workspace.show_notification(id.clone(), cx, |cx| {
- cx.new_view(|_| {
- MessageNotification::new(format!("Invalid settings file\n{error}"))
+ .update(cx, |workspace, cx| {
+ match error
+ .as_ref()
+ .and_then(|error| error.downcast_ref::<InvalidSettingsError>())
+ {
+ Some(InvalidSettingsError::UserSettings { message }) => {
+ workspace.show_notification(id.clone(), cx, |cx| {
+ cx.new_view(|_| {
+ MessageNotification::new(format!(
+ "Invalid user settings file\n{message}"
+ ))
.with_click_message("Open settings file")
.on_click(|cx| {
cx.dispatch_action(zed_actions::OpenSettings.boxed_clone());
cx.emit(DismissEvent);
})
- })
- });
+ })
+ });
+ }
+ None => workspace.dismiss_notification(&id, cx),
+ _ => {}
}
- None => workspace.dismiss_notification(&id, cx),
})
.log_err();
}