settings: accept trailing commas (#2606)

Z-2357

I've found a crate that handles both comments and trailing commas in
JSON. It is a fork of `serde_json`, but it is maintained & up-to-date.
Sadly RawValue seems to not play nicely with it; I've ran into
deserialisation issues around use of RawValue. For this PR I've migrated
to `Value` API.

Obviously this is just a point of discussion, not something I'd merge
straight away. There may be better solutions to this particular problem.

I've also noticed that `serde_json_lenient` does not handle trailing
commas after bindings array. I'm not sure how big of an issue that is.

Release Notes:
- Improved handling of trailing commas in settings files.
[#1322](https://github.com/zed-industries/community/issues/1322)

Cargo.lock

@@ -593,7 +593,7 @@ dependencies = [
  "http",
  "http-body",
  "hyper",
- "itoa",
+ "itoa 1.0.6",
  "matchit",
  "memchr",
  "mime",
@@ -3011,7 +3011,7 @@ checksum = "bd6effc99afb63425aff9b05836f029929e345a6148a14b7ecd5ab67af944482"
 dependencies = [
  "bytes 1.4.0",
  "fnv",
- "itoa",
+ "itoa 1.0.6",
 ]
 
 [[package]]
@@ -3070,7 +3070,7 @@ dependencies = [
  "http-body",
  "httparse",
  "httpdate",
- "itoa",
+ "itoa 1.0.6",
  "pin-project-lite 0.2.9",
  "socket2",
  "tokio",
@@ -3336,6 +3336,12 @@ dependencies = [
  "either",
 ]
 
+[[package]]
+name = "itoa"
+version = "0.4.8"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "b71991ff56294aa922b450139ee08b3bfc70982c6b2c7562771375cf73542dd4"
+
 [[package]]
 name = "itoa"
 version = "1.0.6"
@@ -3396,12 +3402,6 @@ dependencies = [
  "wasm-bindgen",
 ]
 
-[[package]]
-name = "json_comments"
-version = "0.2.1"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "41ee439ee368ba4a77ac70d04f14015415af8600d6c894dc1f11bd79758c57d5"
-
 [[package]]
 name = "jwt"
 version = "0.16.0"
@@ -5667,7 +5667,7 @@ dependencies = [
  "bitflags",
  "errno 0.2.8",
  "io-lifetimes 0.5.3",
- "itoa",
+ "itoa 1.0.6",
  "libc",
  "linux-raw-sys 0.0.42",
  "once_cell",
@@ -6099,7 +6099,19 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "057d394a50403bcac12672b2b18fb387ab6d289d957dab67dd201875391e52f1"
 dependencies = [
  "indexmap",
- "itoa",
+ "itoa 1.0.6",
+ "ryu",
+ "serde",
+]
+
+[[package]]
+name = "serde_json_lenient"
+version = "0.1.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "7d7b9ce5b0a63c6269b9623ed828b39259545a6ec0d8a35d6135ad6af6232add"
+dependencies = [
+ "indexmap",
+ "itoa 0.4.8",
  "ryu",
  "serde",
 ]
@@ -6122,7 +6134,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "d3491c14715ca2294c4d6a88f15e84739788c1d030eed8c110436aafdaa2f3fd"
 dependencies = [
  "form_urlencoded",
- "itoa",
+ "itoa 1.0.6",
  "ryu",
  "serde",
 ]
@@ -6148,7 +6160,7 @@ dependencies = [
  "fs",
  "futures 0.3.28",
  "gpui",
- "json_comments",
+ "indoc",
  "lazy_static",
  "postage",
  "pretty_assertions",
@@ -6157,6 +6169,7 @@ dependencies = [
  "serde",
  "serde_derive",
  "serde_json",
+ "serde_json_lenient",
  "smallvec",
  "sqlez",
  "staff_mode",
@@ -6507,7 +6520,7 @@ dependencies = [
  "hkdf",
  "hmac 0.12.1",
  "indexmap",
- "itoa",
+ "itoa 1.0.6",
  "libc",
  "libsqlite3-sys",
  "log",
@@ -6993,7 +7006,7 @@ version = "0.3.21"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "8f3403384eaacbca9923fa06940178ac13e4edb725486d70e8e15881d0c836cc"
 dependencies = [
- "itoa",
+ "itoa 1.0.6",
  "serde",
  "time-core",
  "time-macros",

crates/gpui/src/app.rs

@@ -445,7 +445,7 @@ type WindowBoundsCallback = Box<dyn FnMut(WindowBounds, Uuid, &mut WindowContext
 type KeystrokeCallback =
     Box<dyn FnMut(&Keystroke, &MatchResult, Option<&Box<dyn Action>>, &mut WindowContext) -> bool>;
 type ActiveLabeledTasksCallback = Box<dyn FnMut(&mut AppContext) -> bool>;
-type DeserializeActionCallback = fn(json: &str) -> anyhow::Result<Box<dyn Action>>;
+type DeserializeActionCallback = fn(json: serde_json::Value) -> anyhow::Result<Box<dyn Action>>;
 type WindowShouldCloseSubscriptionCallback = Box<dyn FnMut(&mut AppContext) -> bool>;
 
 pub struct AppContext {
@@ -624,14 +624,14 @@ impl AppContext {
     pub fn deserialize_action(
         &self,
         name: &str,
-        argument: Option<&str>,
+        argument: Option<serde_json::Value>,
     ) -> Result<Box<dyn Action>> {
         let callback = self
             .action_deserializers
             .get(name)
             .ok_or_else(|| anyhow!("unknown action {}", name))?
             .1;
-        callback(argument.unwrap_or("{}"))
+        callback(argument.unwrap_or_else(|| serde_json::Value::Object(Default::default())))
             .with_context(|| format!("invalid data for action {}", name))
     }
 
@@ -5573,7 +5573,7 @@ mod tests {
         let action1 = cx
             .deserialize_action(
                 "test::something::ComplexAction",
-                Some(r#"{"arg": "a", "count": 5}"#),
+                Some(serde_json::from_str(r#"{"arg": "a", "count": 5}"#).unwrap()),
             )
             .unwrap();
         let action2 = cx

crates/gpui/src/app/action.rs

@@ -11,7 +11,7 @@ pub trait Action: 'static {
     fn qualified_name() -> &'static str
     where
         Self: Sized;
-    fn from_json_str(json: &str) -> anyhow::Result<Box<dyn Action>>
+    fn from_json_str(json: serde_json::Value) -> anyhow::Result<Box<dyn Action>>
     where
         Self: Sized;
 }
@@ -38,7 +38,7 @@ macro_rules! actions {
             $crate::__impl_action! {
                 $namespace,
                 $name,
-                fn from_json_str(_: &str) -> $crate::anyhow::Result<Box<dyn $crate::Action>> {
+                fn from_json_str(_: $crate::serde_json::Value) -> $crate::anyhow::Result<Box<dyn $crate::Action>> {
                     Ok(Box::new(Self))
                 }
             }
@@ -58,8 +58,8 @@ macro_rules! impl_actions {
             $crate::__impl_action! {
                 $namespace,
                 $name,
-                fn from_json_str(json: &str) -> $crate::anyhow::Result<Box<dyn $crate::Action>> {
+                fn from_json_str(json: $crate::serde_json::Value) -> $crate::anyhow::Result<Box<dyn $crate::Action>> {
-                    Ok(Box::new($crate::serde_json::from_str::<Self>(json)?))
+                    Ok(Box::new($crate::serde_json::from_value::<Self>(json)?))
                 }
             }
         )*

crates/gpui/src/app/window.rs

@@ -394,7 +394,7 @@ impl<'a> WindowContext<'a> {
             .iter()
             .filter_map(move |(name, (type_id, deserialize))| {
                 if let Some(action_depth) = handler_depths_by_action_type.get(type_id).copied() {
-                    let action = deserialize("{}").ok()?;
+                    let action = deserialize(serde_json::Value::Object(Default::default())).ok()?;
                     let bindings = self
                         .keystroke_matcher
                         .bindings_for_action_type(*type_id)

crates/settings/Cargo.toml

@@ -21,7 +21,7 @@ util = { path = "../util" }
 
 anyhow.workspace = true
 futures.workspace = true
-json_comments = "0.2"
+serde_json_lenient = {version = "0.1", features = ["preserve_order", "raw_value"]}
 lazy_static.workspace = true
 postage.workspace = true
 rust-embed.workspace = true
@@ -37,6 +37,6 @@ tree-sitter-json = "*"
 [dev-dependencies]
 gpui = { path = "../gpui", features = ["test-support"] }
 fs = { path = "../fs", features = ["test-support"] }
-
+indoc.workspace = true
 pretty_assertions = "1.3.0"
 unindent.workspace = true

crates/settings/src/keymap_file.rs

@@ -1,5 +1,5 @@
 use crate::{settings_store::parse_json_with_comments, SettingsAssets};
-use anyhow::{Context, Result};
+use anyhow::{anyhow, Context, Result};
 use collections::BTreeMap;
 use gpui::{keymap_matcher::Binding, AppContext};
 use schemars::{
@@ -8,7 +8,7 @@ use schemars::{
     JsonSchema,
 };
 use serde::Deserialize;
-use serde_json::{value::RawValue, Value};
+use serde_json::Value;
 use util::{asset_str, ResultExt};
 
 #[derive(Deserialize, Default, Clone, JsonSchema)]
@@ -24,7 +24,7 @@ pub struct KeymapBlock {
 
 #[derive(Deserialize, Default, Clone)]
 #[serde(transparent)]
-pub struct KeymapAction(Box<RawValue>);
+pub struct KeymapAction(Value);
 
 impl JsonSchema for KeymapAction {
     fn schema_name() -> String {
@@ -37,11 +37,12 @@ impl JsonSchema for KeymapAction {
 }
 
 #[derive(Deserialize)]
-struct ActionWithData(Box<str>, Box<RawValue>);
+struct ActionWithData(Box<str>, Value);
 
 impl KeymapFile {
     pub fn load_asset(asset_path: &str, cx: &mut AppContext) -> Result<()> {
         let content = asset_str::<SettingsAssets>(asset_path);
+
         Self::parse(content.as_ref())?.add_to_cx(cx)
     }
 
@@ -54,18 +55,27 @@ impl KeymapFile {
             let bindings = bindings
                 .into_iter()
                 .filter_map(|(keystroke, action)| {
-                    let action = action.0.get();
+                    let action = action.0;
 
                     // This is a workaround for a limitation in serde: serde-rs/json#497
                     // We want to deserialize the action data as a `RawValue` so that we can
                     // deserialize the action itself dynamically directly from the JSON
                     // string. But `RawValue` currently does not work inside of an untagged enum.
-                    if action.starts_with('[') {
+                    if let Value::Array(items) = action {
-                        let ActionWithData(name, data) = serde_json::from_str(action).log_err()?;
+                        let Ok([name, data]): Result<[serde_json::Value; 2], _> = items.try_into() else {
-                        cx.deserialize_action(&name, Some(data.get()))
+                            return Some(Err(anyhow!("Expected array of length 2")));
+                        };
+                        let serde_json::Value::String(name) = name else {
+                            return Some(Err(anyhow!("Expected first item in array to be a string.")))
+                        };
+                        cx.deserialize_action(
+                            &name,
+                            Some(data),
+                        )
+                    } else if let Value::String(name) = action {
+                        cx.deserialize_action(&name, None)
                     } else {
-                        let name = serde_json::from_str(action).log_err()?;
+                        return Some(Err(anyhow!("Expected two-element array, got {:?}", action)));
-                        cx.deserialize_action(name, None)
                     }
                     .with_context(|| {
                         format!(
@@ -118,3 +128,24 @@ impl KeymapFile {
         serde_json::to_value(root_schema).unwrap()
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::KeymapFile;
+
+    #[test]
+    fn can_deserialize_keymap_with_trailing_comma() {
+        let json = indoc::indoc! {"[
+              // Standard macOS bindings
+              {
+                \"bindings\": {
+                  \"up\": \"menu::SelectPrev\",
+                },
+              },
+            ]
+                  "
+
+        };
+        KeymapFile::parse(json).unwrap();
+    }
+}

crates/settings/src/settings_store.rs

@@ -834,11 +834,8 @@ fn to_pretty_json(value: &impl Serialize, indent_size: usize, indent_prefix_len:
 }
 
 pub fn parse_json_with_comments<T: DeserializeOwned>(content: &str) -> Result<T> {
-    Ok(serde_json::from_reader(
+    Ok(serde_json_lenient::from_str(content)?)
-        json_comments::CommentSettings::c_style().strip_comments(content.as_bytes()),
-    )?)
 }
-
 #[cfg(test)]
 mod tests {
     use super::*;