Load all key bindings that parse and use markdown in error notificati…

…ons (#23113)

* Collects and reports all parse errors

* Shares parsed `KeyBindingContextPredicate` among the actions.

* Updates gpui keybinding and action parsing to return structured
errors.

* Renames "block" to "section" to match the docs, as types like
`KeymapSection` are shown in `json-language-server` hovers.

* Removes wrapping of `context` and `use_key_equivalents` fields so that
`json-language-server` auto-inserts `""` and `false` instead of `null`.

* Updates `add_to_cx` to take `&self`, so that the user keymap doesn't
get unnecessarily cloned.

In retrospect I wish I'd just switched to using TreeSitter to do the
parsing and provide proper diagnostics. This is tracked in #23333

Release Notes:

- Improved handling of errors within the user keymap file. Parse errors
within context, keystrokes, or actions no longer prevent loading the key
bindings that do parse.

crates/collab/src/tests/test_server.rs

@@ -304,7 +304,9 @@ impl TestServer {
             collab_ui::init(&app_state, cx);
             file_finder::init(cx);
             menu::init();
-            settings::KeymapFile::load_asset(os_keymap, cx).unwrap();
+            cx.bind_keys(
+                settings::KeymapFile::load_asset_allow_partial_failure(os_keymap, cx).unwrap(),
+            );
             language_model::LanguageModelRegistry::test(cx);
             assistant::context_store::init(&client.clone().into());
         });

crates/command_palette/src/command_palette.rs

@@ -15,7 +15,6 @@ use gpui::{
     ParentElement, Render, Styled, Task, UpdateGlobal, View, ViewContext, VisualContext, WeakView,
 };
 use picker::{Picker, PickerDelegate};
-
 use postage::{sink::Sink, stream::Stream};
 use settings::Settings;
 use ui::{h_flex, prelude::*, v_flex, HighlightedLabel, KeyBinding, ListItem, ListItemSpacing};
@@ -635,7 +634,7 @@ mod tests {
             workspace::init(app_state.clone(), cx);
             init(cx);
             Project::init_settings(cx);
-            KeymapFile::parse(
+            cx.bind_keys(KeymapFile::load_panic_on_failure(
                 r#"[
                     {
                         "bindings": {
@@ -645,10 +644,8 @@ mod tests {
                         }
                     }
                 ]"#,
-            )
-            .unwrap()
-            .add_to_cx(cx)
-            .unwrap();
+                cx,
+            ));
             app_state
         })
     }

crates/docs_preprocessor/src/docs_preprocessor.rs

@@ -32,8 +32,8 @@ impl PreprocessorContext {
             _ => return None,
         };
 
-        keymap.blocks().iter().find_map(|block| {
-            block.bindings().iter().find_map(|(keystroke, a)| {
+        keymap.sections().find_map(|section| {
+            section.bindings().find_map(|(keystroke, a)| {
                 if a.to_string() == action {
                     Some(keystroke.to_string())
                 } else {

crates/gpui/src/action.rs

@@ -1,9 +1,12 @@
 use crate::SharedString;
-use anyhow::{anyhow, Context, Result};
+use anyhow::{anyhow, Result};
 use collections::HashMap;
 pub use no_action::{is_no_action, NoAction};
 use serde_json::json;
-use std::any::{Any, TypeId};
+use std::{
+    any::{Any, TypeId},
+    fmt::Display,
+};
 
 /// Actions are used to implement keyboard-driven UI.
 /// When you declare an action, you can bind keys to the action in the keymap and
@@ -97,6 +100,47 @@ impl dyn Action {
     }
 }
 
+/// Error type for `Keystroke::parse`. This is used instead of `anyhow::Error` so that Zed can use
+/// markdown to display it.
+#[derive(Debug)]
+pub enum ActionBuildError {
+    /// Indicates that an action with this name has not been registered.
+    NotFound {
+        /// Name of the action that was not found.
+        name: String,
+    },
+    /// Indicates that an error occurred while building the action, typically a JSON deserialization
+    /// error.
+    BuildError {
+        /// Name of the action that was attempting to be built.
+        name: String,
+        /// Error that occurred while building the action.
+        error: anyhow::Error,
+    },
+}
+
+impl std::error::Error for ActionBuildError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        match self {
+            ActionBuildError::NotFound { .. } => None,
+            ActionBuildError::BuildError { error, .. } => error.source(),
+        }
+    }
+}
+
+impl Display for ActionBuildError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            ActionBuildError::NotFound { name } => {
+                write!(f, "Didn't find an action named \"{name}\"")
+            }
+            ActionBuildError::BuildError { name, error } => {
+                write!(f, "Error while building action \"{name}\": {error}")
+            }
+        }
+    }
+}
+
 type ActionBuilder = fn(json: serde_json::Value) -> anyhow::Result<Box<dyn Action>>;
 
 pub(crate) struct ActionRegistry {
@@ -201,22 +245,28 @@ impl ActionRegistry {
             .ok_or_else(|| anyhow!("no action type registered for {:?}", type_id))?
             .clone();
 
-        self.build_action(&name, None)
+        Ok(self.build_action(&name, None)?)
     }
 
     /// Construct an action based on its name and optional JSON parameters sourced from the keymap.
     pub fn build_action(
         &self,
         name: &str,
         params: Option<serde_json::Value>,
-    ) -> Result<Box<dyn Action>> {
+    ) -> std::result::Result<Box<dyn Action>, ActionBuildError> {
         let build_action = self
             .by_name
             .get(name)
-            .ok_or_else(|| anyhow!("No action type registered for {}", name))?
+            .ok_or_else(|| ActionBuildError::NotFound {
+                name: name.to_owned(),
+            })?
             .build;
-        (build_action)(params.unwrap_or_else(|| json!({})))
-            .with_context(|| format!("Attempting to build action {}", name))
+        (build_action)(params.unwrap_or_else(|| json!({}))).map_err(|e| {
+            ActionBuildError::BuildError {
+                name: name.to_owned(),
+                error: e,
+            }
+        })
     }
 
     pub fn all_action_names(&self) -> &[SharedString] {

crates/gpui/src/app.rs

@@ -32,11 +32,11 @@ pub use test_context::*;
 use util::ResultExt;
 
 use crate::{
-    current_platform, hash, init_app_menus, Action, ActionRegistry, Any, AnyView, AnyWindowHandle,
-    Asset, AssetSource, BackgroundExecutor, Bounds, ClipboardItem, Context, DispatchPhase,
-    DisplayId, Entity, EventEmitter, FocusHandle, FocusId, ForegroundExecutor, Global, KeyBinding,
-    Keymap, Keystroke, LayoutId, Menu, MenuItem, OwnedMenu, PathPromptOptions, Pixels, Platform,
-    PlatformDisplay, Point, PromptBuilder, PromptHandle, PromptLevel, Render,
+    current_platform, hash, init_app_menus, Action, ActionBuildError, ActionRegistry, Any, AnyView,
+    AnyWindowHandle, Asset, AssetSource, BackgroundExecutor, Bounds, ClipboardItem, Context,
+    DispatchPhase, DisplayId, Entity, EventEmitter, FocusHandle, FocusId, ForegroundExecutor,
+    Global, KeyBinding, Keymap, Keystroke, LayoutId, Menu, MenuItem, OwnedMenu, PathPromptOptions,
+    Pixels, Platform, PlatformDisplay, Point, PromptBuilder, PromptHandle, PromptLevel, Render,
     RenderablePromptHandle, Reservation, ScreenCaptureSource, SharedString, SubscriberSet,
     Subscription, SvgRenderer, Task, TextSystem, View, ViewContext, Window, WindowAppearance,
     WindowContext, WindowHandle, WindowId,
@@ -1220,7 +1220,7 @@ impl AppContext {
         &self,
         name: &str,
         data: Option<serde_json::Value>,
-    ) -> Result<Box<dyn Action>> {
+    ) -> std::result::Result<Box<dyn Action>, ActionBuildError> {
         self.actions.build_action(name, data)
     }
 

crates/gpui/src/keymap/binding.rs

@@ -1,14 +1,15 @@
+use std::rc::Rc;
+
 use collections::HashMap;
 
-use crate::{Action, KeyBindingContextPredicate, Keystroke};
-use anyhow::Result;
+use crate::{Action, InvalidKeystrokeError, KeyBindingContextPredicate, Keystroke};
 use smallvec::SmallVec;
 
 /// A keybinding and its associated metadata, from the keymap.
 pub struct KeyBinding {
     pub(crate) action: Box<dyn Action>,
     pub(crate) keystrokes: SmallVec<[Keystroke; 2]>,
-    pub(crate) context_predicate: Option<KeyBindingContextPredicate>,
+    pub(crate) context_predicate: Option<Rc<KeyBindingContextPredicate>>,
 }
 
 impl Clone for KeyBinding {
@@ -22,28 +23,27 @@ impl Clone for KeyBinding {
 }
 
 impl KeyBinding {
-    /// Construct a new keybinding from the given data.
-    pub fn new<A: Action>(keystrokes: &str, action: A, context_predicate: Option<&str>) -> Self {
+    /// Construct a new keybinding from the given data. Panics on parse error.
+    pub fn new<A: Action>(keystrokes: &str, action: A, context: Option<&str>) -> Self {
+        let context_predicate = if let Some(context) = context {
+            Some(KeyBindingContextPredicate::parse(context).unwrap().into())
+        } else {
+            None
+        };
         Self::load(keystrokes, Box::new(action), context_predicate, None).unwrap()
     }
 
     /// Load a keybinding from the given raw data.
     pub fn load(
         keystrokes: &str,
         action: Box<dyn Action>,
-        context: Option<&str>,
+        context_predicate: Option<Rc<KeyBindingContextPredicate>>,
         key_equivalents: Option<&HashMap<char, char>>,
-    ) -> Result<Self> {
-        let context = if let Some(context) = context {
-            Some(KeyBindingContextPredicate::parse(context)?)
-        } else {
-            None
-        };
-
+    ) -> std::result::Result<Self, InvalidKeystrokeError> {
         let mut keystrokes: SmallVec<[Keystroke; 2]> = keystrokes
             .split_whitespace()
             .map(Keystroke::parse)
-            .collect::<Result<_>>()?;
+            .collect::<std::result::Result<_, _>>()?;
 
         if let Some(equivalents) = key_equivalents {
             for keystroke in keystrokes.iter_mut() {
@@ -58,7 +58,7 @@ impl KeyBinding {
         Ok(Self {
             keystrokes,
             action,
-            context_predicate: context,
+            context_predicate,
         })
     }
 
@@ -88,8 +88,8 @@ impl KeyBinding {
     }
 
     /// Get the predicate used to match this binding
-    pub fn predicate(&self) -> Option<&KeyBindingContextPredicate> {
-        self.context_predicate.as_ref()
+    pub fn predicate(&self) -> Option<Rc<KeyBindingContextPredicate>> {
+        self.context_predicate.as_ref().map(|rc| rc.clone())
     }
 }
 

crates/gpui/src/keymap/context.rs

@@ -244,7 +244,7 @@ impl KeyBindingContextPredicate {
         let source = skip_whitespace(source);
         let (predicate, rest) = Self::parse_expr(source, 0)?;
         if let Some(next) = rest.chars().next() {
-            Err(anyhow!("unexpected character {next:?}"))
+            Err(anyhow!("unexpected character '{next:?}'"))
         } else {
             Ok(predicate)
         }
@@ -333,7 +333,7 @@ impl KeyBindingContextPredicate {
         let next = source
             .chars()
             .next()
-            .ok_or_else(|| anyhow!("unexpected eof"))?;
+            .ok_or_else(|| anyhow!("unexpected end"))?;
         match next {
             '(' => {
                 source = skip_whitespace(&source[1..]);
@@ -369,7 +369,7 @@ impl KeyBindingContextPredicate {
                     source,
                 ))
             }
-            _ => Err(anyhow!("unexpected character {next:?}")),
+            _ => Err(anyhow!("unexpected character '{next:?}'")),
         }
     }
 
@@ -389,15 +389,15 @@ impl KeyBindingContextPredicate {
         if let (Self::Identifier(left), Self::Identifier(right)) = (self, other) {
             Ok(Self::Equal(left, right))
         } else {
-            Err(anyhow!("operands must be identifiers"))
+            Err(anyhow!("operands of == must be identifiers"))
         }
     }
 
     fn new_neq(self, other: Self) -> Result<Self> {
         if let (Self::Identifier(left), Self::Identifier(right)) = (self, other) {
             Ok(Self::NotEqual(left, right))
         } else {
-            Err(anyhow!("operands must be identifiers"))
+            Err(anyhow!("operands of != must be identifiers"))
         }
     }
 }
@@ -504,7 +504,7 @@ mod tests {
             KeyBindingContextPredicate::parse("c == !d")
                 .unwrap_err()
                 .to_string(),
-            "operands must be identifiers"
+            "operands of == must be identifiers"
         );
     }