2
\$\begingroup\$

I've written a text editor in Rust.

make_highlights is responsible for syntax highlighting. It takes a Row, which is just a wrapper around a String, and determines the appropriate syntax highlighting for each character. Currently, the function is very long - 84 lines - and thus hard to understand all at once. I'd like to break it up and make it more understandable.

impl Row {
    fn make_highlights(
        &self,
        syntax: &Option<EditorSyntax>,
        prev_open_quote: Option<char>,
    ) -> (Vec<EditorHighlight>, Option<char>) {
        if let Some(syntax) = syntax.as_ref() {
            let text: Vec<char> = self.text.chars().collect();
            let mut new_hls = vec![];
            let mut open_quote = if text.is_empty() {
                prev_open_quote
            } else {
                None
            };
            while new_hls.len() < text.len() {
                let prev_is_sep = new_hls.is_empty() || is_separator(text[new_hls.len() - 1]);
                if new_hls.is_empty() && prev_open_quote.is_some() ||
                    syntax.quotes.contains(text[new_hls.len()])
                {
                    let active_quote = if new_hls.is_empty() && prev_open_quote.is_some() {
                        prev_open_quote.expect("Just checked_it")
                    } else {
                        let start_quote = text[new_hls.len()];
                        new_hls.push(EditorHighlight::String);
                        start_quote
                    };
                    open_quote = Some(active_quote);
                    while new_hls.len() < text.len() {
                        if text[new_hls.len()] == active_quote {
                            new_hls.push(EditorHighlight::String);
                            open_quote = None;
                            break;
                        }
                        if text[new_hls.len()] == '\\' && new_hls.len() + 1 < text.len() {
                            new_hls.push(EditorHighlight::String);
                        }
                        new_hls.push(EditorHighlight::String);
                    }
                } else if syntax.has_digits && text[new_hls.len()].is_digit(10) && prev_is_sep {
                    while new_hls.len() < text.len() && text[new_hls.len()].is_digit(10) {
                        new_hls.push(EditorHighlight::Number);
                    }
                } else if self.text[new_hls.len()..].starts_with(syntax.singleline_comment) {
                    let chars_remaining = text.len() - new_hls.len();
                    new_hls.extend(repeat(EditorHighlight::Comment).take(chars_remaining));
                } else if prev_is_sep {
                    let following_string: String = self.text[new_hls.len()..].to_string();
                    let key_and_highlight: Vec<(usize, EditorHighlight)> = syntax
                        .keywords
                        .iter()
                        .enumerate()
                        .flat_map(|(highlight_index, keywords)| {
                            keywords
                                .iter()
                                .filter(|&keyword| {
                                    following_string.starts_with(keyword) &&
                                        (keyword.len() + new_hls.len() == text.len() ||
                                             is_separator(text[keyword.len() + new_hls.len()]))
                                })
                                .map(move |keyword| {
                                    (keyword.len(), EditorHighlight::KEYWORDS[highlight_index])
                                })
                        })
                        .collect();
                    assert!(key_and_highlight.len() <= 1);
                    if let Some(&(keyword_len, highlight)) = key_and_highlight.first() {
                        new_hls.extend(repeat(highlight).take(keyword_len));
                    } else {
                        new_hls.push(EditorHighlight::Normal);
                    }
                } else {
                    new_hls.push(EditorHighlight::Normal);
                }
            }
            (new_hls, open_quote)
        } else {
            (
                repeat(EditorHighlight::Normal)
                    .take(self.text.len())
                    .collect(),
                None,
            )
        }
    }
}

The relevant struct definitions and uses are:

use std::iter::repeat;

#[derive(Copy, Clone, Debug, PartialEq)]
enum EditorHighlight {
    Normal,
    Number,
    Match,
    String,
    Comment,
    Keyword1,
    Keyword2,
    Keyword3,
    Keyword4,
}

#[derive(Clone)]
struct Row {
    text: String,
}

struct EditorSyntax<'a> {
    filetype: &'a str,
    extensions: Vec<&'a str>,
    has_digits: bool,
    quotes: &'a str,
    singleline_comment: &'a str,
    keywords: [Vec<&'a str>; 4],
}

The code also requires a nightly feature:

#![feature(associated_consts)]

impl EditorHighlight {
    const KEYWORDS: [Self; 4] = [
        EditorHighlight::Keyword1,
        EditorHighlight::Keyword2,
        EditorHighlight::Keyword3,
        EditorHighlight::Keyword4,
    ];
}

Other code:

fn is_separator(c: char) -> bool {
    c.is_whitespace() || "&{}'\",.()+-/*=~%<>[];:!".contains(c)
}

I am aware that this code is not UTF-8 compatible, that's on my to-do list to fix. I'd like advice on how to split it up, as well as how to generally improve it.

\$\endgroup\$
1
  • \$\begingroup\$ error[E0425]: cannot find function `is_separator` in this scope: could you provide is_separator (or at the very least its contract)? \$\endgroup\$
    – CAD97
    Aug 3 '17 at 4:08
2
\$\begingroup\$

associated_consts is stable since 1.20 (currently in beta). You don't need the attribute anymore. Other than that, you pass Clippy with no warnings.

You should almost certainly #[derive(Debug)] on Row, and depending on usage, it may also make sense to also derive the other derivable traits that String impls.

The biggest thing here is rightward drift.

The first thing to notice is that you've practically wrapped the entire method in an if let block. I'd extract the part that uses the EditorSyntax into its own fn.

fn make_highlights(
    &self,
    syntax: &Option<EditorSyntax>,
    prev_open_quote: Option<char>,
) -> (Vec<EditorHighlight>, Option<char>) {
    if let Some(syntax) = syntax.as_ref() {
        self.make_syntax_highlights(syntax, prev_open_quote);
    } else {
        (
            repeat(EditorHighlight::Normal)
                .take(self.text.chars().count())
                .collect(),
            None,
        )
    }
}

It might also make sense to just drop the form that doesn't take a EditorSyntax reference and just make a EditorSyntax that represents plain text.

const EMPTY_SYNTAX: EditorSyntax<'static> {
    filetype: "text",
    extensions: vec!["txt"],
    has_digits: false,
    quotes: "",
    // for lack of a better choice
    // what do languages without this kind of comment do?
    singleline_comment: "",
    keywords: [vec![],vec![],vec![],vec![]],
}

I would rename new_hls to something more obviously meaningful like highlights. You can also initialize it by using Vec::with_capacity to set the required capacity and avoid having to reallocate the backing array.

fn make_syntax_highlights(
    &self,
    syntax: &EditorSyntax,
    prev_open_quote: Option<char>,
) -> (Vec<EditorHighlight>, Option<char>) {
    let text: Vec<char> = self.text.chars().collect();
    let mut highlights = Vec::with_capacity(text.len());

You can just initialize open_quote to prev_open_quote because you set it in your loop before using it.

    let mut open_quote = prev_open_quote;

dnt_abbr_ndlsly (don't abbreviate needlessly). prev_is_sep could be more clearly expressed as starts_on_word_boundary.

    while highlights.len() < text.len() {
        let starts_on_word_boundary = highlights.is_empty() ||
            is_separator(text[highlights.len() - 1]);

There is no reason to assign active_quote. Just assign into open_quote directly; this will also remove the need to unwrap prev_open_quote.

        if highlights.is_empty() && prev_open_quote.is_some() ||
            syntax.quotes.contains(text[highlights.len()])
        {
            open_quote = if highlights.is_empty() {
                prev_open_quote
            } else {
                highlights.push(EditorHighlight::String);
                Some(text[highlights.len()])
            };

I would check open_quote.is_some() in the loop condition here, allowing you to drop the break, and be 100% sure that you still have an open quote. I'd also move the unconditional push to the front of the loop.

            while open_quote.is_some() && highlights.len() < text.len() {
                highlights.push(EditorHighlight::String);
                if Some(text[highlights.len()]) == open_quote {
                    open_quote = None;
                } else if text[highlights.len()] == '\\' && highlights.len() < text.len() {
                    highlights.push(EditorHighlight::String);
                }
            }

Everything through the starts_on_word_boundary looks fine.

        } else if syntax.has_digits && text[highlights.len()].is_digit(10) &&
                   starts_on_word_boundary
        {
            while highlights.len() < text.len() && text[highlights.len()].is_digit(10) {
                highlights.push(EditorHighlight::Number);
            }
        } else if self.text[highlights.len()..].starts_with(syntax.singleline_comment) {
            let chars_remaining = text.len() - highlights.len();
            highlights.extend(repeat(EditorHighlight::Comment).take(chars_remaining));
        } else if starts_on_word_boundary {
            let following_string: String = self.text[highlights.len()..].to_string();

key_and_highlight looks fine, though a perfect target for an extract method refactoring. If I may self-advertise a little here, I would use the Single crate here.

            let key_and_highlight: Vec<_> = syntax
                .keywords
                .iter()
                .enumerate()
                .flat_map(|(highlight_index, keywords)| {
                    keywords
                        .iter()
                        .filter(|&keyword| {
                            following_string.starts_with(keyword) &&
                                (keyword.len() + highlights.len() == text.len() ||
                                     is_separator(text[keyword.len() + highlights.len()]))
                        })
                        .map(move |keyword| {
                            (keyword.len(), EditorHighlight::KEYWORDS[highlight_index])
                        })
                })
                .single();
            let (keyword_len, highlight) =
                key_and_highlight.unwrap_or((1, EditorHighlight::Normal));
            highlights.extend(repeat(highlight).take(keyword_len));
        } else {
            highlights.push(EditorHighlight::Normal);
        }
    }
    (highlights, open_quote)
}

To avoid rightward draft, I'd extract any of the sections in the if chain that are more than two lines of code (excluding lines with just a }). Additionally consider extracting helper fn for the conditions long enough that rustfmt wraps them. And again, the key_and_highlight chain is a perfect target for adding as a helper fn on EditorHighlight or Syntax.

Also of note is that this syntax struct is very C-like language focused. If you try to add support for something like ><>, I suspect you will need to expand your idea of what a syntax highlighter configuration needs to hold.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks - this sort of thing is really helpful. I'll let you know if I have any questions as I go through things in detail. \$\endgroup\$
    – isaacg
    Aug 3 '17 at 8:28
  • 1
    \$\begingroup\$ Almost everything you said was correct and helpful. However, open_quote = if highlights.is_empty() { is an incorrect refactor - it should be open_quote = if highlights.is_empty() && prev_open_quote.is_some() {. Fortunately, my test cases caught it, so no harm done. \$\endgroup\$
    – isaacg
    Aug 3 '17 at 9:09
  • \$\begingroup\$ Also Some(text[highlights.len()]) should be Some(text[highlights.len() - 1]), since it was moved past a push. \$\endgroup\$
    – isaacg
    Aug 3 '17 at 9:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.