Merge pull request #432 from Enselic/dont-panic

Replace direct panics with Errors

.github/workflows/CI.yml

@@ -31,6 +31,8 @@ jobs:
           --allow clippy::unknown_clippy_lints \
           --allow clippy::unnecessary_cast \
           --allow clippy::block_in_if_condition_stmt
+        # Prevent regressions of https://github.com/trishume/syntect/issues/98
+        cargo clippy --all-features --lib -- --deny clippy::panic
     - name: Run cargo check
       run: |
         cargo check --all-features --all-targets

examples/synhtml-css-classes.rs

@@ -106,15 +106,15 @@ int main() {
     let css_dark_file = File::create(Path::new("theme-dark.css"))?;
     let mut css_dark_writer = BufWriter::new(&css_dark_file);
 
-    let css_dark = css_for_theme_with_class_style(dark_theme, ClassStyle::Spaced);
+    let css_dark = css_for_theme_with_class_style(dark_theme, ClassStyle::Spaced).unwrap();
     writeln!(css_dark_writer, "{}", css_dark)?;
 
     // create light color scheme css
     let light_theme = &ts.themes["Solarized (light)"];
     let css_light_file = File::create(Path::new("theme-light.css"))?;
     let mut css_light_writer = BufWriter::new(&css_light_file);
 
-    let css_light = css_for_theme_with_class_style(light_theme, ClassStyle::Spaced);
+    let css_light = css_for_theme_with_class_style(light_theme, ClassStyle::Spaced).unwrap();
     writeln!(css_light_writer, "{}", css_light)?;
 
     Ok(())

examples/synstats.rs

@@ -85,7 +85,7 @@ fn count_line(ops: &[(usize, ScopeStackOp)], line: &str, stack: &mut ScopeStack,
     let mut line_has_doc_comment = false;
     let mut line_has_code = false;
     for (s, op) in ScopeRegionIterator::new(ops, line) {
-        stack.apply(op);
+        stack.apply(op).unwrap();
         if s.is_empty() { // in this case we don't care about blank tokens
             continue;
         }

examples/syntest.rs

@@ -283,7 +283,7 @@ fn test_file(
             }
             let mut col: usize = 0;
             for (s, op) in ScopeRegionIterator::new(&ops, &line) {
-                stack.apply(op);
+                stack.apply(op).unwrap();
                 if s.is_empty() {
                     // in this case we don't care about blank tokens
                     continue;

src/easy.rs

@@ -298,7 +298,7 @@ mod tests {
         let mut stack = ScopeStack::new();
         let mut token_count = 0;
         for (s, op) in ScopeRegionIterator::new(&ops, line) {
-            stack.apply(op);
+            stack.apply(op).expect("#[cfg(test)]");
             if s.is_empty() { // in this case we don't care about blank tokens
                 continue;
             }
@@ -326,7 +326,7 @@ mod tests {
 
             let mut iterated_ops: Vec<&ScopeStackOp> = Vec::new();
             for (_, op) in ScopeRegionIterator::new(&ops, line) {
-                stack.apply(op);
+                stack.apply(op).expect("#[cfg(test)]");
                 iterated_ops.push(op);
                 println!("{:?}", op);
             }

src/highlighting/highlighter.rs

@@ -181,7 +181,7 @@ impl<'a, 'b> Iterator for RangedHighlightIterator<'a, 'b> {
                         m_caches.pop();
                     }
                 }
-            });
+            }).ok()?;
         }
         self.pos = end;
         self.index += 1;

src/html.rs

@@ -94,7 +94,7 @@ impl<'a> ClassedHTMLGenerator<'a> {
             parsed_line.as_slice(),
             self.style,
             &mut self.scope_stack,
-        );
+        )?;
         self.open_spans += delta;
         self.html.push_str(formatted_line.as_str());
 
@@ -128,11 +128,11 @@ impl<'a> ClassedHTMLGenerator<'a> {
 
 #[deprecated(since="4.2.0", note="Please use `css_for_theme_with_class_style` instead.")]
 pub fn css_for_theme(theme: &Theme) -> String {
-    css_for_theme_with_class_style(theme, ClassStyle::Spaced)
+    css_for_theme_with_class_style(theme, ClassStyle::Spaced).expect("Please use `css_for_theme_with_class_style` instead.")
 }
 
 /// Create a complete CSS for a given theme. Can be used inline, or written to a CSS file.
-pub fn css_for_theme_with_class_style(theme: &Theme, style: ClassStyle) -> String {
+pub fn css_for_theme_with_class_style(theme: &Theme, style: ClassStyle) -> Result<String, Error> {
     let mut css = String::new();
 
     css.push_str("/*\n");
@@ -201,7 +201,7 @@ pub fn css_for_theme_with_class_style(theme: &Theme, style: ClassStyle) -> Strin
         css.push_str("}\n");
     }
 
-    css
+    Ok(css)
 }
 
 #[derive(Debug, PartialEq, Eq, Clone, Copy)]
@@ -337,7 +337,7 @@ pub fn line_tokens_to_classed_spans(
     ops: &[(usize, ScopeStackOp)],
     style: ClassStyle,
     stack: &mut ScopeStack,
-) -> (String, isize) {
+) -> Result<(String, isize), Error> {
     let mut s = String::with_capacity(line.len() + ops.len() * 8); // a guess
     let mut cur_index = 0;
     let mut span_delta = 0;
@@ -349,7 +349,7 @@ pub fn line_tokens_to_classed_spans(
     for &(i, ref op) in ops {
         if i > cur_index {
             span_empty = false;
-            write!(s, "{}", Escape(&line[cur_index..i])).unwrap();
+            write!(s, "{}", Escape(&line[cur_index..i]))?;
             cur_index = i
         }
         stack.apply_with_hook(op, |basic_op, _| match basic_op {
@@ -370,10 +370,10 @@ pub fn line_tokens_to_classed_spans(
                 span_delta -= 1;
                 span_empty = false;
             }
-        });
+        })?;
     }
-    write!(s, "{}", Escape(&line[cur_index..line.len()])).unwrap();
-    (s, span_delta)
+    write!(s, "{}", Escape(&line[cur_index..line.len()]))?;
+    Ok((s, span_delta))
 }
 
 /// Preserved for compatibility, always use `line_tokens_to_classed_spans`
@@ -385,15 +385,15 @@ pub fn tokens_to_classed_spans(
     ops: &[(usize, ScopeStackOp)],
     style: ClassStyle,
 ) -> (String, isize) {
-    line_tokens_to_classed_spans(line, ops, style, &mut ScopeStack::new())
+    line_tokens_to_classed_spans(line, ops, style, &mut ScopeStack::new()).expect("Use `line_tokens_to_classed_spans` instead, this can panic and highlight incorrectly")
 }
 
 #[deprecated(since="3.1.0", note="Use `line_tokens_to_classed_spans` instead to avoid incorrect highlighting and panics")]
 pub fn tokens_to_classed_html(line: &str,
                               ops: &[(usize, ScopeStackOp)],
                               style: ClassStyle)
                               -> String {
-    line_tokens_to_classed_spans(line, ops, style, &mut ScopeStack::new()).0
+    line_tokens_to_classed_spans(line, ops, style, &mut ScopeStack::new()).expect("Use `line_tokens_to_classed_spans` instead to avoid incorrect highlighting and panics").0
 }
 
 /// Determines how background color attributes are generated
@@ -543,7 +543,7 @@ mod tests {
         // use util::debug_print_ops;
         // debug_print_ops(line, &ops);
 
-        let (html, _) = line_tokens_to_classed_spans(line, &ops[..], ClassStyle::Spaced, &mut stack);
+        let (html, _) = line_tokens_to_classed_spans(line, &ops[..], ClassStyle::Spaced, &mut stack).expect("#[cfg(test)]");
         println!("{}", html);
         assert_eq!(html, include_str!("../testdata/test2.html").trim_end());
 

src/lib.rs

@@ -58,6 +58,9 @@ pub enum Error {
     #[cfg(feature = "parsing")]
     #[error("Parsing error: {0}")]
     ParsingError(#[from] crate::parsing::ParsingError),
+    /// Scope error
+    #[error("Scope error: {0}")]
+    ScopeError(#[from] crate::parsing::ScopeError),
     /// Formatting error
     #[error("Formatting error: {0}")]
     Fmt(#[from] std::fmt::Error),

src/parsing/parser.rs

@@ -25,10 +25,16 @@ use crate::parsing::syntax_definition::ContextId;
 #[derive(Debug, thiserror::Error)]
 #[non_exhaustive]
 pub enum ParsingError {
+    #[error("Somehow main context was popped from the stack")]
+    MissingMainContext,
     /// A context is missing. Usually caused by a syntax referencing a another
     /// syntax that is not known to syntect. See e.g. <https://github.com/trishume/syntect/issues/421>
     #[error("Missing context with ID '{0:?}'")]
     MissingContext(ContextId),
+    #[error("Bad index to match_at: {0}")]
+    BadMatchIndex(usize),
+    #[error("Tried to use a ContextReference that has not bee resolved yet: {0:?}")]
+    UnresolvedContextReference(ContextReference),
 }
 
 /// Keeps the current parser state (the internal syntax interpreter stack) between lines of parsing.
@@ -210,8 +216,9 @@ impl ParseState {
         line: &str,
         syntax_set: &SyntaxSet,
     ) -> Result<Vec<(usize, ScopeStackOp)>, ParsingError> {
-        assert!(!self.stack.is_empty(),
-                "Somehow main context was popped from the stack");
+        if self.stack.is_empty() {
+            return Err(ParsingError::MissingMainContext)
+        }
         let mut match_start = 0;
         let mut res = Vec::new();
 
@@ -300,7 +307,7 @@ impl ParseState {
                 // check the next "pop" for loops. Otherwise leave the state,
                 // e.g. non-consuming "set" could also result in a loop.
                 let context = reg_match.context;
-                let match_pattern = context.match_at(reg_match.pat_index);
+                let match_pattern = context.match_at(reg_match.pat_index)?;
                 if let MatchOperation::Push(_) = match_pattern.operation {
                     *non_consuming_push_at = (match_end, self.stack.len() + 1);
                 }
@@ -362,7 +369,7 @@ impl ParseState {
 
         for (from_with_proto, ctx, captures) in context_chain {
             for (pat_context, pat_index) in context_iter(syntax_set, syntax_set.get_context(ctx)?) {
-                let match_pat = pat_context.match_at(pat_index);
+                let match_pat = pat_context.match_at(pat_index)?;
 
                 if let Some(match_region) = self.search(
                     line, start, match_pat, captures, search_cache, regions
@@ -475,10 +482,10 @@ impl ParseState {
     ) -> Result<bool, ParsingError> {
         let (match_start, match_end) = reg_match.regions.pos(0).unwrap();
         let context = reg_match.context;
-        let pat = context.match_at(reg_match.pat_index);
+        let pat = context.match_at(reg_match.pat_index)?;
         // println!("running pattern {:?} on '{}' at {}, operation {:?}", pat.regex_str, line, match_start, pat.operation);
 
-        self.push_meta_ops(true, match_start, level_context, &pat.operation, syntax_set, ops);
+        self.push_meta_ops(true, match_start, level_context, &pat.operation, syntax_set, ops)?;
         for s in &pat.scope {
             // println!("pushing {:?} at {}", s, match_start);
             ops.push((match_start, ScopeStackOp::Push(*s)));
@@ -511,7 +518,7 @@ impl ParseState {
             // println!("popping at {}", match_end);
             ops.push((match_end, ScopeStackOp::Pop(pat.scope.len())));
         }
-        self.push_meta_ops(false, match_end, &*level_context, &pat.operation, syntax_set, ops);
+        self.push_meta_ops(false, match_end, &*level_context, &pat.operation, syntax_set, ops)?;
 
         self.perform_op(line, &reg_match.regions, pat, syntax_set)
     }
@@ -524,7 +531,7 @@ impl ParseState {
         match_op: &MatchOperation,
         syntax_set: &'a SyntaxSet,
         ops: &mut Vec<(usize, ScopeStackOp)>,
-    ) {
+    ) -> Result<(), ParsingError>{
         // println!("metas ops for {:?}, initial: {}",
         //          match_op,
         //          initial);
@@ -542,7 +549,7 @@ impl ParseState {
 
                 // cleared scopes are restored after the scopes from match pattern that invoked the pop are applied
                 if !initial && cur_context.clear_scopes != None {
-                    ops.push((index, ScopeStackOp::Restore));
+                    ops.push((index, ScopeStackOp::Restore))
                 }
             },
             // for some reason the ST3 behaviour of set is convoluted and is inconsistent with the docs and other ops
@@ -560,7 +567,7 @@ impl ParseState {
                     }
                     // add each context's meta scope
                     for r in context_refs.iter() {
-                        let ctx = r.resolve(syntax_set);
+                        let ctx = r.resolve(syntax_set)?;
 
                         if !is_set {
                             if let Some(clear_amount) = ctx.clear_scopes {
@@ -574,14 +581,14 @@ impl ParseState {
                     }
                 } else {
                     let repush = (is_set && (!cur_context.meta_scope.is_empty() || !cur_context.meta_content_scope.is_empty())) || context_refs.iter().any(|r| {
-                        let ctx = r.resolve(syntax_set);
+                        let ctx = r.resolve(syntax_set).unwrap();
 
                         !ctx.meta_content_scope.is_empty() || (ctx.clear_scopes.is_some() && is_set)
                     });
                     if repush {
                         // remove previously pushed meta scopes, so that meta content scopes will be applied in the correct order
                         let mut num_to_pop : usize = context_refs.iter().map(|r| {
-                            let ctx = r.resolve(syntax_set);
+                            let ctx = r.resolve(syntax_set).unwrap();
                             ctx.meta_scope.len()
                         }).sum();
 
@@ -597,7 +604,7 @@ impl ParseState {
 
                         // now we push meta scope and meta context scope for each context pushed
                         for r in context_refs {
-                            let ctx = r.resolve(syntax_set);
+                            let ctx = r.resolve(syntax_set)?;
 
                             // for some reason, contrary to my reading of the docs, set does this after the token
                             if is_set {
@@ -618,6 +625,8 @@ impl ParseState {
             },
             MatchOperation::None => (),
         }
+
+        Ok(())
     }
 
     /// Returns true if the stack was changed
@@ -657,10 +666,10 @@ impl ParseState {
                 // referred to as the "target" of the push by sublimehq - see
                 // https://forum.sublimetext.com/t/dev-build-3111/19240/17 for more info
                 if let Some(ref p) = pat.with_prototype {
-                    proto_ids.push(p.id());
+                    proto_ids.push(p.id()?);
                 }
             }
-            let context_id = r.id();
+            let context_id = r.id()?;
             let context = syntax_set.get_context(&context_id)?;
             let captures = {
                 let mut uses_backrefs = context.uses_backrefs;
@@ -770,7 +779,7 @@ mod tests {
 
         let mut stack = ScopeStack::new();
         for &(_, ref op) in ops.iter() {
-            stack.apply(op);
+            stack.apply(op).expect("#[cfg(test)]");
         }
         assert_eq!(stack, test_stack);
     }
@@ -1827,7 +1836,7 @@ contexts:
         let mut states = Vec::new();
         let mut stack = ScopeStack::new();
         for &(_, ref op) in ops.iter() {
-            stack.apply(op);
+            stack.apply(op).expect("#[cfg(test)]");
             let scopes: Vec<String> = stack.as_slice().iter().map(|s| format!("{:?}", s)).collect();
             let stack_str = scopes.join(", ");
             states.push(stack_str);

src/parsing/scope.rs

@@ -11,6 +11,14 @@ use std::mem;
 use serde::{Deserialize, Deserializer, Serialize, Serializer};
 use serde::de::{Error, Visitor};
 
+/// Scope related errors
+#[derive(Debug, thiserror::Error)]
+#[non_exhaustive]
+pub enum ScopeError {
+    #[error("Tried to restore cleared scopes, but none were cleared")]
+    NoClearedScopesToRestore,
+}
+
 /// Multiplier on the power of 2 for MatchPower. This is only useful if you compute your own
 /// [`MatchPower`] scores
 ///
@@ -219,6 +227,7 @@ impl Scope {
     /// I can't think of any reason you'd find this useful. It is used internally for turning a
     /// scope back into a string.
     pub fn atom_at(self, index: usize) -> u16 {
+        #[allow(clippy::panic)] // The below panic is too much of an edge-case for it to be worth propagating
         let shifted = if index < 4 {
             self.a >> ((3 - index) * 16)
         } else if index < 8 {
@@ -398,7 +407,7 @@ impl ScopeStack {
     /// Modifies this stack according to the operation given
     ///
     /// Use this to create a stack from a `Vec` of changes given by the parser.
-    pub fn apply(&mut self, op: &ScopeStackOp) {
+    pub fn apply(&mut self, op: &ScopeStackOp) -> Result<(), ScopeError> {
         self.apply_with_hook(op, |_,_|{})
     }
 
@@ -410,7 +419,7 @@ impl ScopeStack {
     /// [`apply`]: #method.apply
     /// [`BasicScopeStackOp`]: enum.BasicScopeStackOp.html
     #[inline]
-    pub fn apply_with_hook<F>(&mut self, op: &ScopeStackOp, mut hook: F)
+    pub fn apply_with_hook<F>(&mut self, op: &ScopeStackOp, mut hook: F) -> Result<(), ScopeError>
         where F: FnMut(BasicScopeStackOp, &[Scope])
     {
         match *op {
@@ -451,11 +460,13 @@ impl ScopeStack {
                             hook(BasicScopeStackOp::Push(*s), self.as_slice());
                         }
                     }
-                    None => panic!("tried to restore cleared scopes, but none were cleared"),
+                    None => return Err(ScopeError::NoClearedScopesToRestore),
                 }
             }
             ScopeStackOp::Noop => (),
         }
+
+        Ok(())
     }
 
     /// Prints out each scope in the stack separated by spaces