Add warning when using history isolation with non-SQLite history format (#16151)

# Description
This PR depends on #16147, use `git diff 132ikl/shell-warning
132ikl/isolation-warn` to see only changes from this PR

People seem to get tripped up by this a lot, and it's not exactly
intuitive, so I added a warning if you try to set
`$env.config.history.isolation = true` when using the plaintext file
format:

    Warning: nu:🐚:invalid_config

      ⚠ Encountered 1 warnings(s) when updating config

    Warning: nu:🐚:incompatible_options

      ⚠ Incompatible options
       ╭─[source:1:33]
     1 │ $env.config.history.isolation = true
       ·                                 ──┬─
       ·                                   ╰── history isolation only compatible with SQLite format
       ╰────
      help: disable history isolation, or set $env.config.history.file_format = "sqlite"


# User-Facing Changes
* Added a warning when using history isolation without using SQLite
history.

# Tests + Formatting
Added a test

crates/nu-protocol/src/config/error.rs

@@ -1,10 +1,12 @@
 use super::ConfigPath;
-use crate::{Config, ConfigError, ShellError, Span, Type, Value};
+use crate::{Config, ConfigError, ConfigWarning, ShellError, ShellWarning, Span, Type, Value};
 
 #[derive(Debug)]
+#[must_use]
 pub(super) struct ConfigErrors<'a> {
     config: &'a Config,
     errors: Vec<ConfigError>,
+    warnings: Vec<ConfigWarning>,
 }
 
 impl<'a> ConfigErrors<'a> {
@@ -12,17 +14,26 @@ impl<'a> ConfigErrors<'a> {
         Self {
             config,
             errors: Vec::new(),
+            warnings: Vec::new(),
         }
     }
 
-    pub fn is_empty(&self) -> bool {
+    pub fn has_errors(&self) -> bool {
-        self.errors.is_empty()
+        !self.errors.is_empty()
+    }
+
+    pub fn has_warnings(&self) -> bool {
+        !self.warnings.is_empty()
     }
 
     pub fn error(&mut self, error: ConfigError) {
         self.errors.push(error);
     }
 
+    pub fn warn(&mut self, warning: ConfigWarning) {
+        self.warnings.push(warning);
+    }
+
     pub fn type_mismatch(&mut self, path: &ConfigPath, expected: Type, actual: &Value) {
         self.error(ConfigError::TypeMismatch {
             path: path.to_string(),
@@ -75,13 +86,15 @@ impl<'a> ConfigErrors<'a> {
         });
     }
 
-    pub fn into_shell_error(self) -> Option<ShellError> {
+    pub fn check(self) -> Result<Option<ShellWarning>, ShellError> {
-        if self.is_empty() {
+        match (self.has_errors(), self.has_warnings()) {
-            None
+            (true, _) => Err(ShellError::InvalidConfig {
-        } else {
-            Some(ShellError::InvalidConfig {
                 errors: self.errors,
-            })
+            }),
+            (false, true) => Ok(Some(ShellWarning::InvalidConfig {
+                warnings: self.warnings,
+            })),
+            (false, false) => Ok(None),
         }
     }
 }

crates/nu-protocol/src/config/history.rs

@@ -1,5 +1,5 @@
 use super::{config_update_string_enum, prelude::*};
-use crate as nu_protocol;
+use crate::{self as nu_protocol, ConfigWarning};
 
 #[derive(Clone, Copy, Debug, IntoValue, PartialEq, Eq, Serialize, Deserialize)]
 pub enum HistoryFileFormat {
@@ -77,15 +77,35 @@ impl UpdateFromValue for HistoryConfig {
             return;
         };
 
+        // might not be correct if file format was changed away from sqlite rather than isolation,
+        // but this is an edge case and the span of the relevant value here should be close enough
+        let mut isolation_span = value.span();
+
         for (col, val) in record.iter() {
             let path = &mut path.push(col);
             match col.as_str() {
-                "isolation" => self.isolation.update(val, path, errors),
+                "isolation" => {
+                    isolation_span = val.span();
+                    self.isolation.update(val, path, errors)
+                }
                 "sync_on_enter" => self.sync_on_enter.update(val, path, errors),
                 "max_size" => self.max_size.update(val, path, errors),
                 "file_format" => self.file_format.update(val, path, errors),
                 _ => errors.unknown_option(path, val),
             }
         }
+
+        // Listing all formats separately in case additional ones are added
+        match (self.isolation, self.file_format) {
+            (true, HistoryFileFormat::Plaintext) => {
+                errors.warn(ConfigWarning::IncompatibleOptions {
+                    label: "history isolation only compatible with SQLite format",
+                    span: isolation_span,
+                    help: r#"disable history isolation, or set $env.config.history.file_format = "sqlite""#,
+                });
+            }
+            (true, HistoryFileFormat::Sqlite) => (),
+            (false, _) => (),
+        }
     }
 }

crates/nu-protocol/src/config/mod.rs

@@ -1,7 +1,7 @@
 //! Module containing the internal representation of user configuration
 
-use crate as nu_protocol;
 use crate::FromValue;
+use crate::{self as nu_protocol};
 use helper::*;
 use prelude::*;
 use std::collections::HashMap;
@@ -219,7 +219,11 @@ impl UpdateFromValue for Config {
 }
 
 impl Config {
-    pub fn update_from_value(&mut self, old: &Config, value: &Value) -> Option<ShellError> {
+    pub fn update_from_value(
+        &mut self,
+        old: &Config,
+        value: &Value,
+    ) -> Result<Option<ShellWarning>, ShellError> {
         // Current behaviour is that config errors are displayed, but do not prevent the rest
         // of the config from being updated (fields with errors are skipped/not updated).
         // Errors are simply collected one-by-one and wrapped into a ShellError variant at the end.
@@ -228,6 +232,6 @@ impl Config {
 
         self.update(value, &mut path, &mut errors);
 
-        errors.into_shell_error()
+        errors.check()
     }
 }

crates/nu-protocol/src/config/plugin_gc.rs

@@ -147,7 +147,7 @@ mod tests {
         let mut errors = ConfigErrors::new(&config);
         let mut result = PluginGcConfigs::default();
         result.update(&input, &mut ConfigPath::new(), &mut errors);
-        assert!(errors.is_empty(), "errors: {errors:#?}");
+        assert!(!errors.has_errors(), "errors: {errors:#?}");
         assert_eq!(expected, result);
     }
 

crates/nu-protocol/src/config/prelude.rs

@@ -1,4 +1,4 @@
 pub(super) use super::{ConfigPath, UpdateFromValue, error::ConfigErrors};
-pub use crate::{IntoValue, ShellError, Span, Type, Value, record};
+pub use crate::{IntoValue, ShellError, ShellWarning, Span, Type, Value, record};
 pub use serde::{Deserialize, Serialize};
 pub use std::str::FromStr;

crates/nu-protocol/src/engine/stack.rs

@@ -4,6 +4,7 @@ use crate::{
         ArgumentStack, DEFAULT_OVERLAY_NAME, EngineState, ErrorHandlerStack, Redirection,
         StackCallArgGuard, StackCollectValueGuard, StackIoGuard, StackOutDest,
     },
+    report_shell_warning,
 };
 use nu_utils::IgnoreCaseExt;
 use std::{
@@ -212,18 +213,17 @@ impl Stack {
         if let Some(value) = self.get_env_var(engine_state, "config") {
             let old = self.get_config(engine_state);
             let mut config = (*old).clone();
-            let error = config.update_from_value(&old, value);
+            let result = config.update_from_value(&old, value);
             // The config value is modified by the update, so we should add it again
             self.add_env_var("config".into(), config.clone().into_value(value.span()));
             self.config = Some(config.into());
-            match error {
+            if let Some(warning) = result? {
-                None => Ok(()),
+                report_shell_warning(engine_state, &warning);
-                Some(err) => Err(err),
             }
         } else {
             self.config = None;
-            Ok(())
         }
+        Ok(())
     }
 
     pub fn add_var(&mut self, var_id: VarId, value: Value) {

crates/nu-protocol/src/errors/config.rs

@@ -1,3 +1,5 @@
+use std::hash::Hash;
+
 use crate::{ShellError, Span, Type};
 use miette::Diagnostic;
 use thiserror::Error;
@@ -54,3 +56,29 @@ pub enum ConfigError {
     #[diagnostic(transparent)]
     ShellError(#[from] ShellError),
 }
+
+/// Warnings which don't prevent config from being loaded, but we should inform the user about
+#[derive(Clone, Debug, PartialEq, Error, Diagnostic)]
+#[diagnostic(severity(Warning))]
+pub enum ConfigWarning {
+    #[error("Incompatible options")]
+    #[diagnostic(code(nu::shell::incompatible_options), help("{help}"))]
+    IncompatibleOptions {
+        label: &'static str,
+        #[label = "{label}"]
+        span: Span,
+        help: &'static str,
+    },
+}
+
+// To keep track of reported warnings
+impl Hash for ConfigWarning {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        match self {
+            ConfigWarning::IncompatibleOptions { label, help, .. } => {
+                label.hash(state);
+                help.hash(state);
+            }
+        }
+    }
+}

crates/nu-protocol/src/errors/mod.rs

@@ -1,6 +1,6 @@
 mod chained_error;
 mod compile_error;
-mod config_error;
+mod config;
 mod labeled_error;
 mod parse_error;
 mod parse_warning;
@@ -9,7 +9,7 @@ pub mod shell_error;
 pub mod shell_warning;
 
 pub use compile_error::CompileError;
-pub use config_error::ConfigError;
+pub use config::{ConfigError, ConfigWarning};
 pub use labeled_error::{ErrorLabel, LabeledError};
 pub use parse_error::{DidYouMean, ParseError};
 pub use parse_warning::ParseWarning;

crates/nu-protocol/src/errors/parse_warning.rs

@@ -1,14 +1,16 @@
 use crate::Span;
 use miette::Diagnostic;
-use serde::{Deserialize, Serialize};
 use std::hash::Hash;
 use thiserror::Error;
 
 use crate::{ReportMode, Reportable};
 
-#[derive(Clone, Debug, Error, Diagnostic, Serialize, Deserialize)]
+#[derive(Clone, Debug, Error, Diagnostic)]
 #[diagnostic(severity(Warning))]
 pub enum ParseWarning {
+    /// A parse-time deprectaion. Indicates that something will be removed in a future release.
+    ///
+    /// Use [`ShellWarning::Deprecated`] if this is a deprecation which is only detectable at run-time.
     #[error("{dep_type} deprecated.")]
     #[diagnostic(code(nu::parser::deprecated))]
     Deprecated {

crates/nu-protocol/src/errors/shell_warning.rs

@@ -1,14 +1,16 @@
 use crate::Span;
 use miette::Diagnostic;
-use serde::{Deserialize, Serialize};
 use std::hash::Hash;
 use thiserror::Error;
 
-use crate::{ReportMode, Reportable};
+use crate::{ConfigWarning, ReportMode, Reportable};
 
-#[derive(Clone, Debug, Error, Diagnostic, Serialize, Deserialize)]
+#[derive(Clone, Debug, Error, Diagnostic)]
 #[diagnostic(severity(Warning))]
 pub enum ShellWarning {
+    /// A parse-time deprectaion. Indicates that something will be removed in a future release.
+    ///
+    /// Use [`ParseWarning::Deprecated`] if this is a deprecation which is detectable at parse-time.
     #[error("{dep_type} deprecated.")]
     #[diagnostic(code(nu::shell::deprecated))]
     Deprecated {
@@ -20,20 +22,20 @@ pub enum ShellWarning {
         help: Option<String>,
         report_mode: ReportMode,
     },
-}
+    /// Warnings reported while updating the config
-
+    #[error("Encountered {} warnings(s) when updating config", warnings.len())]
-impl ShellWarning {
+    #[diagnostic(code(nu::shell::invalid_config))]
-    pub fn span(&self) -> Span {
+    InvalidConfig {
-        match self {
+        #[related]
-            ShellWarning::Deprecated { span, .. } => *span,
+        warnings: Vec<ConfigWarning>,
-        }
+    },
-    }
 }
 
 impl Reportable for ShellWarning {
     fn report_mode(&self) -> ReportMode {
         match self {
             ShellWarning::Deprecated { report_mode, .. } => *report_mode,
+            ShellWarning::InvalidConfig { .. } => ReportMode::FirstUse,
         }
     }
 }
@@ -48,6 +50,8 @@ impl Hash for ShellWarning {
                 dep_type.hash(state);
                 label.hash(state);
             }
+            // We always report config warnings, so no hash necessary
+            ShellWarning::InvalidConfig { .. } => (),
         }
     }
 }

tests/repl/test_config.rs

@@ -168,3 +168,11 @@ fn mutate_nu_config_plugin_gc_plugins() -> TestResult {
         "0sec",
     )
 }
+
+#[test]
+fn mutate_nu_config_history_warning() -> TestResult {
+    fail_test(
+        r#"$env.config.history.file_format = "plaintext"; $env.config.history.isolation = true"#,
+        "history isolation only compatible with SQLite format",
+    )
+}