Rollup merge of #101813 - GuillaumeGomez:check-css-variables, r=notriddle

Extend CSS check to CSS variables

This PR is a bit big because the first commit is a rewrite of the CSS parser to something a bit simpler which still allows to get easily access to CSS properties name.

The other two are about adding tests and adding the CSS variables check.

This check was missing because we are relying more and more on CSS variables rather than CSS selectors in themes.

r? `@notriddle`

src/librustdoc/config.rs

@@ -412,7 +412,13 @@ impl Options {
 
         let to_check = matches.opt_strs("check-theme");
         if !to_check.is_empty() {
-            let paths = theme::load_css_paths(static_files::themes::LIGHT.as_bytes());
+            let paths = match theme::load_css_paths(static_files::themes::LIGHT) {
+                Ok(p) => p,
+                Err(e) => {
+                    diag.struct_err(&e.to_string()).emit();
+                    return Err(1);
+                }
+            };
             let mut errors = 0;
 
             println!("rustdoc: [check-theme] Starting tests! (Ignoring all other arguments)");
@@ -547,7 +553,13 @@ impl Options {
 
         let mut themes = Vec::new();
         if matches.opt_present("theme") {
-            let paths = theme::load_css_paths(static_files::themes::LIGHT.as_bytes());
+            let paths = match theme::load_css_paths(static_files::themes::LIGHT) {
+                Ok(p) => p,
+                Err(e) => {
+                    diag.struct_err(&e.to_string()).emit();
+                    return Err(1);
+                }
+            };
 
             for (theme_file, theme_s) in
                 matches.opt_strs("theme").iter().map(|s| (PathBuf::from(&s), s.to_owned()))

src/librustdoc/theme.rs

@@ -1,271 +1,252 @@
-use rustc_data_structures::fx::FxHashSet;
+use rustc_data_structures::fx::FxHashMap;
+use std::collections::hash_map::Entry;
 use std::fs;
-use std::hash::{Hash, Hasher};
+use std::iter::Peekable;
 use std::path::Path;
+use std::str::Chars;
 
 use rustc_errors::Handler;
 
 #[cfg(test)]
 mod tests;
 
-#[derive(Debug, Clone, Eq)]
+#[derive(Debug)]
 pub(crate) struct CssPath {
-    pub(crate) name: String,
+    pub(crate) rules: FxHashMap<String, String>,
-    pub(crate) children: FxHashSet<CssPath>,
+    pub(crate) children: FxHashMap<String, CssPath>,
 }
 
-// This PartialEq implementation IS NOT COMMUTATIVE!!!
+/// When encountering a `"` or a `'`, returns the whole string, including the quote characters.
-//
+fn get_string(iter: &mut Peekable<Chars<'_>>, string_start: char, buffer: &mut String) {
-// The order is very important: the second object must have all first's rules.
+    buffer.push(string_start);
-// However, the first is not required to have all of the second's rules.
+    while let Some(c) = iter.next() {
-impl PartialEq for CssPath {
+        buffer.push(c);
-    fn eq(&self, other: &CssPath) -> bool {
+        if c == '\\' {
-        if self.name != other.name {
+            iter.next();
-            false
+        } else if c == string_start {
-        } else {
+            break;
-            for child in &self.children {
-                if !other.children.iter().any(|c| child == c) {
-                    return false;
-                }
-            }
-            true
         }
     }
 }
 
-impl Hash for CssPath {
+fn get_inside_paren(
-    fn hash<H: Hasher>(&self, state: &mut H) {
+    iter: &mut Peekable<Chars<'_>>,
-        self.name.hash(state);
+    paren_start: char,
-        for x in &self.children {
+    paren_end: char,
-            x.hash(state);
+    buffer: &mut String,
+) {
+    buffer.push(paren_start);
+    while let Some(c) = iter.next() {
+        handle_common_chars(c, buffer, iter);
+        if c == paren_end {
+            break;
         }
     }
 }
 
-impl CssPath {
+/// Skips a `/*` comment.
-    fn new(name: String) -> CssPath {
+fn skip_comment(iter: &mut Peekable<Chars<'_>>) {
-        CssPath { name, children: FxHashSet::default() }
+    while let Some(c) = iter.next() {
+        if c == '*' && iter.next() == Some('/') {
+            break;
+        }
     }
 }
 
-/// All variants contain the position they occur.
+/// Skips a line comment (`//`).
-#[derive(Debug, Clone, Copy)]
+fn skip_line_comment(iter: &mut Peekable<Chars<'_>>) {
-enum Events {
+    while let Some(c) = iter.next() {
-    StartLineComment(usize),
+        if c == '\n' {
-    StartComment(usize),
+            break;
-    EndComment(usize),
-    InBlock(usize),
-    OutBlock(usize),
         }
-
-impl Events {
-    fn get_pos(&self) -> usize {
-        match *self {
-            Events::StartLineComment(p)
-            | Events::StartComment(p)
-            | Events::EndComment(p)
-            | Events::InBlock(p)
-            | Events::OutBlock(p) => p,
     }
 }
 
-    fn is_comment(&self) -> bool {
+fn handle_common_chars(c: char, buffer: &mut String, iter: &mut Peekable<Chars<'_>>) {
-        matches!(
+    match c {
-            self,
+        '"' | '\'' => get_string(iter, c, buffer),
-            Events::StartLineComment(_) | Events::StartComment(_) | Events::EndComment(_)
+        '/' if iter.peek() == Some(&'*') => skip_comment(iter),
-        )
+        '/' if iter.peek() == Some(&'/') => skip_line_comment(iter),
+        '(' => get_inside_paren(iter, c, ')', buffer),
+        '[' => get_inside_paren(iter, c, ']', buffer),
+        _ => buffer.push(c),
     }
 }
 
-fn previous_is_line_comment(events: &[Events]) -> bool {
+/// Returns a CSS property name. Ends when encountering a `:` character.
-    matches!(events.last(), Some(&Events::StartLineComment(_)))
+///
+/// If the `:` character isn't found, returns `None`.
+///
+/// If a `{` character is encountered, returns an error.
+fn parse_property_name(iter: &mut Peekable<Chars<'_>>) -> Result<Option<String>, String> {
+    let mut content = String::new();
+
+    while let Some(c) = iter.next() {
+        match c {
+            ':' => return Ok(Some(content.trim().to_owned())),
+            '{' => return Err("Unexpected `{` in a `{}` block".to_owned()),
+            '}' => break,
+            _ => handle_common_chars(c, &mut content, iter),
+        }
+    }
+    Ok(None)
 }
 
-fn is_line_comment(pos: usize, v: &[u8], events: &[Events]) -> bool {
+/// Try to get the value of a CSS property (the `#fff` in `color: #fff`). It'll stop when it
-    if let Some(&Events::StartComment(_)) = events.last() {
+/// encounters a `{` or a `;` character.
-        return false;
+///
+/// It returns the value string and a boolean set to `true` if the value is ended with a `}` because
+/// it means that the parent block is done and that we should notify the parent caller.
+fn parse_property_value(iter: &mut Peekable<Chars<'_>>) -> (String, bool) {
+    let mut value = String::new();
+    let mut out_block = false;
+
+    while let Some(c) = iter.next() {
+        match c {
+            ';' => break,
+            '}' => {
+                out_block = true;
+                break;
             }
-    v[pos + 1] == b'/'
+            _ => handle_common_chars(c, &mut value, iter),
+        }
+    }
+    (value.trim().to_owned(), out_block)
 }
 
-fn load_css_events(v: &[u8]) -> Vec<Events> {
+/// This is used to parse inside a CSS `{}` block. If we encounter a new `{` inside it, we consider
-    let mut pos = 0;
+/// it as a new block and therefore recurse into `parse_rules`.
-    let mut events = Vec::with_capacity(100);
+fn parse_rules(
+    content: &str,
+    selector: String,
+    iter: &mut Peekable<Chars<'_>>,
+    paths: &mut FxHashMap<String, CssPath>,
+) -> Result<(), String> {
+    let mut rules = FxHashMap::default();
+    let mut children = FxHashMap::default();
 
-    while pos + 1 < v.len() {
-        match v[pos] {
-            b'/' if v[pos + 1] == b'*' => {
-                events.push(Events::StartComment(pos));
-                pos += 1;
-            }
-            b'/' if is_line_comment(pos, v, &events) => {
-                events.push(Events::StartLineComment(pos));
-                pos += 1;
-            }
-            b'\n' if previous_is_line_comment(&events) => {
-                events.push(Events::EndComment(pos));
-            }
-            b'*' if v[pos + 1] == b'/' => {
-                events.push(Events::EndComment(pos + 2));
-                pos += 1;
-            }
-            b'{' if !previous_is_line_comment(&events) => {
-                if let Some(&Events::StartComment(_)) = events.last() {
-                    pos += 1;
-                    continue;
-                }
-                events.push(Events::InBlock(pos + 1));
-            }
-            b'}' if !previous_is_line_comment(&events) => {
-                if let Some(&Events::StartComment(_)) = events.last() {
-                    pos += 1;
-                    continue;
-                }
-                events.push(Events::OutBlock(pos + 1));
-            }
-            _ => {}
-        }
-        pos += 1;
-    }
-    events
-}
-
-fn get_useful_next(events: &[Events], pos: &mut usize) -> Option<Events> {
-    while *pos < events.len() {
-        if !events[*pos].is_comment() {
-            return Some(events[*pos]);
-        }
-        *pos += 1;
-    }
-    None
-}
-
-fn get_previous_positions(events: &[Events], mut pos: usize) -> Vec<usize> {
-    let mut ret = Vec::with_capacity(3);
-
-    ret.push(events[pos].get_pos());
-    if pos > 0 {
-        pos -= 1;
-    }
     loop {
-        if pos < 1 || !events[pos].is_comment() {
+        // If the parent isn't a "normal" CSS selector, we only expect sub-selectors and not CSS
-            let x = events[pos].get_pos();
+        // properties.
-            if *ret.last().unwrap() != x {
+        if selector.starts_with('@') {
-                ret.push(x);
+            parse_selectors(content, iter, &mut children)?;
-            } else {
-                ret.push(0);
-            }
             break;
         }
-        ret.push(events[pos].get_pos());
+        let rule = match parse_property_name(iter)? {
-        pos -= 1;
+            Some(r) => {
+                if r.is_empty() {
+                    return Err(format!("Found empty rule in selector `{selector}`"));
                 }
-    if ret.len() & 1 != 0 && events[pos].is_comment() {
+                r
-        ret.push(0);
             }
-    ret.iter().rev().cloned().collect()
+            None => break,
+        };
+        let (value, out_block) = parse_property_value(iter);
+        if value.is_empty() {
+            return Err(format!("Found empty value for rule `{rule}` in selector `{selector}`"));
         }
-
+        match rules.entry(rule) {
-fn build_rule(v: &[u8], positions: &[usize]) -> String {
+            Entry::Occupied(mut o) => {
-    minifier::css::minify(
+                *o.get_mut() = value;
-        &positions
-            .chunks(2)
-            .map(|x| ::std::str::from_utf8(&v[x[0]..x[1]]).unwrap_or(""))
-            .collect::<String>()
-            .trim()
-            .chars()
-            .filter_map(|c| match c {
-                '\n' | '\t' => Some(' '),
-                '/' | '{' | '}' => None,
-                c => Some(c),
-            })
-            .collect::<String>()
-            .split(' ')
-            .filter(|s| !s.is_empty())
-            .intersperse(" ")
-            .collect::<String>(),
-    )
-    .map(|css| css.to_string())
-    .unwrap_or_else(|_| String::new())
             }
-
+            Entry::Vacant(v) => {
-fn inner(v: &[u8], events: &[Events], pos: &mut usize) -> FxHashSet<CssPath> {
+                v.insert(value);
-    let mut paths = Vec::with_capacity(50);
-
-    while *pos < events.len() {
-        if let Some(Events::OutBlock(_)) = get_useful_next(events, pos) {
-            *pos += 1;
-            break;
-        }
-        if let Some(Events::InBlock(_)) = get_useful_next(events, pos) {
-            paths.push(CssPath::new(build_rule(v, &get_previous_positions(events, *pos))));
-            *pos += 1;
-        }
-        while let Some(Events::InBlock(_)) = get_useful_next(events, pos) {
-            if let Some(ref mut path) = paths.last_mut() {
-                for entry in inner(v, events, pos).iter() {
-                    path.children.insert(entry.clone());
             }
         }
-        }
+        if out_block {
-        if let Some(Events::OutBlock(_)) = get_useful_next(events, pos) {
-            *pos += 1;
-        }
-    }
-    paths.iter().cloned().collect()
-}
-
-pub(crate) fn load_css_paths(v: &[u8]) -> CssPath {
-    let events = load_css_events(v);
-    let mut pos = 0;
-
-    let mut parent = CssPath::new("parent".to_owned());
-    parent.children = inner(v, &events, &mut pos);
-    parent
-}
-
-pub(crate) fn get_differences(against: &CssPath, other: &CssPath, v: &mut Vec<String>) {
-    if against.name == other.name {
-        for child in &against.children {
-            let mut found = false;
-            let mut found_working = false;
-            let mut tmp = Vec::new();
-
-            for other_child in &other.children {
-                if child.name == other_child.name {
-                    if child != other_child {
-                        get_differences(child, other_child, &mut tmp);
-                    } else {
-                        found_working = true;
-                    }
-                    found = true;
             break;
         }
     }
-            if !found {
+
-                v.push(format!("  Missing \"{}\" rule", child.name));
+    match paths.entry(selector) {
-            } else if !found_working {
+        Entry::Occupied(mut o) => {
-                v.extend(tmp.iter().cloned());
+            let v = o.get_mut();
+            for (key, value) in rules.into_iter() {
+                v.rules.insert(key, value);
             }
+            for (sel, child) in children.into_iter() {
+                v.children.insert(sel, child);
+            }
+        }
+        Entry::Vacant(v) => {
+            v.insert(CssPath { rules, children });
+        }
+    }
+    Ok(())
+}
+
+pub(crate) fn parse_selectors(
+    content: &str,
+    iter: &mut Peekable<Chars<'_>>,
+    paths: &mut FxHashMap<String, CssPath>,
+) -> Result<(), String> {
+    let mut selector = String::new();
+
+    while let Some(c) = iter.next() {
+        match c {
+            '{' => {
+                let s = minifier::css::minify(selector.trim()).map(|s| s.to_string())?;
+                parse_rules(content, s, iter, paths)?;
+                selector.clear();
+            }
+            '}' => break,
+            ';' => selector.clear(), // We don't handle inline selectors like `@import`.
+            _ => handle_common_chars(c, &mut selector, iter),
+        }
+    }
+    Ok(())
+}
+
+/// The entry point to parse the CSS rules. Every time we encounter a `{`, we then parse the rules
+/// inside it.
+pub(crate) fn load_css_paths(content: &str) -> Result<FxHashMap<String, CssPath>, String> {
+    let mut iter = content.chars().peekable();
+    let mut paths = FxHashMap::default();
+
+    parse_selectors(content, &mut iter, &mut paths)?;
+    Ok(paths)
+}
+
+pub(crate) fn get_differences(
+    origin: &FxHashMap<String, CssPath>,
+    against: &FxHashMap<String, CssPath>,
+    v: &mut Vec<String>,
+) {
+    for (selector, entry) in origin.iter() {
+        match against.get(selector) {
+            Some(a) => {
+                get_differences(&entry.children, &a.children, v);
+                if selector == ":root" {
+                    // We need to check that all variables have been set.
+                    for rule in entry.rules.keys() {
+                        if !a.rules.contains_key(rule) {
+                            v.push(format!("  Missing CSS variable `{rule}` in `:root`"));
+                        }
+                    }
+                }
+            }
+            None => v.push(format!("  Missing rule `{selector}`")),
         }
     }
 }
 
 pub(crate) fn test_theme_against<P: AsRef<Path>>(
     f: &P,
-    against: &CssPath,
+    origin: &FxHashMap<String, CssPath>,
     diag: &Handler,
 ) -> (bool, Vec<String>) {
-    let data = match fs::read(f) {
+    let against = match fs::read_to_string(f)
+        .map_err(|e| e.to_string())
+        .and_then(|data| load_css_paths(&data))
+    {
         Ok(c) => c,
         Err(e) => {
-            diag.struct_err(&e.to_string()).emit();
+            diag.struct_err(&e).emit();
             return (false, vec![]);
         }
     };
 
-    let paths = load_css_paths(&data);
     let mut ret = vec![];
-    get_differences(against, &paths, &mut ret);
+    get_differences(origin, &against, &mut ret);
     (true, ret)
 }

src/librustdoc/theme/tests.rs

@@ -44,11 +44,7 @@ rule j end {}
 "#;
 
     let mut ret = Vec::new();
-    get_differences(
+    get_differences(&load_css_paths(against).unwrap(), &load_css_paths(text).unwrap(), &mut ret);
-        &load_css_paths(against.as_bytes()),
-        &load_css_paths(text.as_bytes()),
-        &mut ret,
-    );
     assert!(ret.is_empty());
 }
 
@@ -61,46 +57,45 @@ a
 c // sdf
 d {}
 "#;
-    let paths = load_css_paths(text.as_bytes());
+    let paths = load_css_paths(text).unwrap();
-    assert!(paths.children.contains(&CssPath::new("a b c d".to_owned())));
+    assert!(paths.contains_key(&"a b c d".to_owned()));
 }
 
 #[test]
 fn test_comparison() {
-    let x = r#"
+    let origin = r#"
-a {
+@a {
-    b {
+    b {}
     c {}
 }
-}
 "#;
 
-    let y = r#"
+    let against = r#"
-a {
+@a {
     b {}
 }
 "#;
 
-    let against = load_css_paths(y.as_bytes());
+    let origin = load_css_paths(origin).unwrap();
-    let other = load_css_paths(x.as_bytes());
+    let against = load_css_paths(against).unwrap();
 
     let mut ret = Vec::new();
-    get_differences(&against, &other, &mut ret);
+    get_differences(&against, &origin, &mut ret);
     assert!(ret.is_empty());
-    get_differences(&other, &against, &mut ret);
+    get_differences(&origin, &against, &mut ret);
-    assert_eq!(ret, vec!["  Missing \"c\" rule".to_owned()]);
+    assert_eq!(ret, vec!["  Missing rule `c`".to_owned()]);
 }
 
 #[test]
 fn check_empty_css() {
-    let events = load_css_events(&[]);
+    let paths = load_css_paths("").unwrap();
-    assert_eq!(events.len(), 0);
+    assert_eq!(paths.len(), 0);
 }
 
 #[test]
 fn check_invalid_css() {
-    let events = load_css_events(b"*");
+    let paths = load_css_paths("*").unwrap();
-    assert_eq!(events.len(), 0);
+    assert_eq!(paths.len(), 0);
 }
 
 #[test]
@@ -108,10 +103,85 @@ fn test_with_minification() {
     let text = include_str!("../html/static/css/themes/dark.css");
     let minified = minifier::css::minify(&text).expect("CSS minification failed").to_string();
 
-    let against = load_css_paths(text.as_bytes());
+    let against = load_css_paths(text).unwrap();
-    let other = load_css_paths(minified.as_bytes());
+    let other = load_css_paths(&minified).unwrap();
 
     let mut ret = Vec::new();
     get_differences(&against, &other, &mut ret);
     assert!(ret.is_empty());
 }
+
+#[test]
+fn test_media() {
+    let text = r#"
+@media (min-width: 701px) {
+    a:hover {
+        color: #fff;
+    }
+
+    b {
+        x: y;
+    }
+}
+
+@media (max-width: 1001px) {
+    b {
+        x: y;
+    }
+}
+"#;
+
+    let paths = load_css_paths(text).unwrap();
+    let p = paths.get("@media (min-width:701px)");
+    assert!(p.is_some());
+    let p = p.unwrap();
+    assert!(p.children.get("a:hover").is_some());
+    assert!(p.children.get("b").is_some());
+
+    let p = paths.get("@media (max-width:1001px)");
+    assert!(p.is_some());
+    let p = p.unwrap();
+    assert!(p.children.get("b").is_some());
+}
+
+#[test]
+fn test_css_variables() {
+    let x = r#"
+:root {
+    --a: #fff;
+}
+"#;
+
+    let y = r#"
+:root {
+    --a: #fff;
+    --b: #fff;
+}
+"#;
+
+    let against = load_css_paths(x).unwrap();
+    let other = load_css_paths(y).unwrap();
+
+    let mut ret = Vec::new();
+    get_differences(&against, &other, &mut ret);
+    assert!(ret.is_empty());
+    get_differences(&other, &against, &mut ret);
+    assert_eq!(ret, vec!["  Missing CSS variable `--b` in `:root`".to_owned()]);
+}
+
+#[test]
+fn test_weird_rule_value() {
+    let x = r#"
+a[text=("a")] {
+    b: url({;}.png);
+    c: #fff
+}
+"#;
+
+    let paths = load_css_paths(&x).unwrap();
+    let p = paths.get("a[text=(\"a\")]");
+    assert!(p.is_some());
+    let p = p.unwrap();
+    assert_eq!(p.rules.get("b"), Some(&"url({;}.png)".to_owned()));
+    assert_eq!(p.rules.get("c"), Some(&"#fff".to_owned()));
+}