refactor: abbrevation expansion trait logic (nushell#1094)

Author: casedami

src/engine.rs

@@ -36,9 +36,9 @@ use {
             semantic_prompt::{Osc133ClickEventsMarkers, SemanticPromptMarkers},
         },
         utils::text_manipulation,
-        EditCommand, ExampleHighlighter, Highlighter, LineBuffer, Menu, MenuEvent, MouseButton,
-        Prompt, PromptHistorySearch, ReedlineMenu, Signal, UndoBehavior, ValidationResult,
-        Validator,
+        AbbrExpandContext, EditCommand, ExampleHighlighter, Highlighter, LineBuffer, Menu,
+        MenuEvent, MouseButton, Prompt, PromptHistorySearch, ReedlineMenu, Signal, UndoBehavior,
+        ValidationResult, Validator,
     },
     crossterm::{
         cursor::{SetCursorStyle, Show},
@@ -626,8 +626,8 @@ impl Reedline {
     ///
     /// Overwrites any existing abbreviations with the same key.
     ///
-    /// Note, by default abbreviations are expanded within string literals. To change this behavior
-    /// override the `is_inside_string_literal` function defined by [`Highlighter`].
+    /// Note, by default abbreviations are expanded everywhere. To suppress expansion in certain
+    /// syntactic positions (e.g. string literals), override [`Highlighter::should_expand_abbr`].
     pub fn with_abbreviations(mut self, abbreviations: HashMap<String, String>) -> Self {
         self.abbreviations.extend(abbreviations);
         self
@@ -1758,10 +1758,8 @@ impl Reedline {
 
     /// Expands an abbreviation at the word before the cursor, if any exists
     ///
-    /// Note, this method uses the `is_inside_string_literal` function defined by [`Highlighter`]
-    /// to decide whether to expand an abbreviation when the cursor is inside a string literal.
-    /// Unless overridden, `is_inside_string_literal` returns `false`, resulting in abbreviations
-    /// being expanded even when inside a string literal.
+    /// Calls [`Highlighter::should_expand_abbr`] with [`AbbrExpandContext::WordAbbreviation`]
+    /// to decide whether expansion is permitted at the cursor position
     fn try_expand_abbreviation_at_cursor(&mut self, submitted: bool) -> Option<ReedlineEvent> {
         let buffer = self.editor.get_buffer();
         let cursor_position_in_buffer = self.editor.insertion_point();
@@ -1787,10 +1785,11 @@ impl Reedline {
             // The first char in the buffer is a space or there are consecutive spaces
             return None;
         }
-        if self
-            .highlighter
-            .is_inside_string_literal(buffer, word_start)
-        {
+        if !self.highlighter.should_expand_abbr(
+            buffer,
+            word_start,
+            AbbrExpandContext::WordAbbreviation,
+        ) {
             return None;
         }
 
@@ -1826,10 +1825,11 @@ impl Reedline {
             }
         }
 
-        if self
-            .highlighter
-            .is_inside_string_literal(buffer, parsed.remainder.len())
-        {
+        if !self.highlighter.should_expand_abbr(
+            buffer,
+            parsed.remainder.len(),
+            AbbrExpandContext::BangExpansion,
+        ) {
             return None;
         }
 
@@ -2564,7 +2564,7 @@ mod tests {
         set_buffer_at_end(&mut reedline, buffer);
         assert!(
             reedline.try_expand_abbreviation_at_cursor(true).is_some(),
-            "must expand when highlighter does not override is_inside_string_literal"
+            "must expand when highlighter does not override should_expand_abbr"
         );
     }
 
@@ -2648,7 +2648,7 @@ mod tests {
         set_buffer_at_end(&mut reedline, buffer);
         assert!(
             reedline.parse_bang_command().is_some(),
-            "must expand when highlighter does not override is_inside_string_literal"
+            "must expand when highlighter does not override should_expand_abbr"
         );
     }
 }

src/highlighter/example.rs

@@ -1,4 +1,4 @@
-use crate::highlighter::Highlighter;
+use crate::highlighter::{AbbrExpandContext, Highlighter};
 use crate::StyledText;
 use nu_ansi_term::{Color, Style};
 
@@ -15,9 +15,10 @@ pub struct ExampleHighlighter {
 }
 
 impl Highlighter for ExampleHighlighter {
-    fn is_inside_string_literal(&self, line: &str, cursor: usize) -> bool {
+    // A simple example of disabling abbreviation expansion within string literals
+    fn should_expand_abbr(&self, line: &str, cursor: usize, _context: AbbrExpandContext) -> bool {
         if line.is_empty() || cursor == 0 {
-            return false;
+            return true;
         }
         let mut in_single = false;
         let mut in_double = false;
@@ -40,7 +41,7 @@ impl Highlighter for ExampleHighlighter {
             }
             byte_pos += 1;
         }
-        in_single || in_double
+        !(in_single || in_double)
     }
 
     fn highlight(&self, line: &str, _cursor: usize) -> StyledText {

src/highlighter/mod.rs

@@ -5,6 +5,20 @@ use crate::StyledText;
 
 pub use example::ExampleHighlighter;
 pub use simple_match::SimpleMatchHighlighter;
+
+/// The context in which abbreviation expansion is being attempted
+///
+/// Passed to [`Highlighter::should_expand_abbr`] so implementations can apply
+/// different veto rules depending on which expansion triggered the check
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum AbbrExpandContext {
+    /// Fish-style word abbreviation
+    WordAbbreviation,
+    /// Bashism history expansion
+    #[cfg(feature = "bashisms")]
+    BangExpansion,
+}
+
 /// The syntax highlighting trait. Implementers of this trait will take in the current string and then
 /// return a `StyledText` object, which represents the contents of the original line as styled strings
 pub trait Highlighter: Send {
@@ -13,10 +27,15 @@ pub trait Highlighter: Send {
     /// Cursor position as byte offsets in the string
     fn highlight(&self, line: &str, cursor: usize) -> StyledText;
 
-    /// The action that will take the current buffer and return whether the cursor position (a byte
-    /// offset) is inside a string literal
-    fn is_inside_string_literal(&self, line: &str, cursor: usize) -> bool {
-        let _ = (line, cursor);
-        false // default for simple highlighters
+    /// Returns `true` if an abbreviation should be expanded at the given cursor position
+    /// (a byte offset into `line`), `false` if expansion should be suppressed
+    ///
+    /// `context` indicates which kind of expansion is being attempted so implementations
+    /// can apply different veto rules per site
+    ///
+    /// The default implementation always returns `true` (always expand)
+    fn should_expand_abbr(&self, line: &str, cursor: usize, context: AbbrExpandContext) -> bool {
+        let _ = (line, cursor, context);
+        true
     }
 }

src/lib.rs

@@ -277,7 +277,7 @@ pub use edit_mode::{
 };
 
 mod highlighter;
-pub use highlighter::{ExampleHighlighter, Highlighter, SimpleMatchHighlighter};
+pub use highlighter::{AbbrExpandContext, ExampleHighlighter, Highlighter, SimpleMatchHighlighter};
 
 mod completion;
 pub use completion::{Completer, DefaultCompleter, Span, Suggestion};