Merge pull request #159 from kaj/refactor-css-dest

Refactor css output.

Author: kaj

Diff for: CHANGELOG.md

@@ -23,6 +23,9 @@ project adheres to
 * Moved the `SourcePos` type into the `input` module and converted it from
   keeping a copy of the relevant line to keeping a range with a (reference
   counted) `SourceFile` (PR #158).
+* Changed css creation from just writing to a text buffer to building
+  a tree representation of the css (and serialize it to text as a
+  final step) (PR #159).
 * Changed `css::Item::AtRule` to wrap the new type `css::AtRule`.
 * Changed handling of `hue` arguments to color functions, to allow
   different angle units, matching updates in sass-spec.

Diff for: src/css/atrule.rs

@@ -1,4 +1,4 @@
-use super::{Comment, Import, Property, Rule};
+use super::{Comment, Import, Property, Rule, Value};
 use crate::output::CssBuf;
 use std::io::{self, Write};
 
@@ -9,28 +9,37 @@ use std::io::{self, Write};
 #[derive(Clone, Debug)]
 pub struct AtRule {
     name: String,
-    args: String, // TODO: More typed?  Value? Or separate types for @media etc?
-    body: Vec<AtRuleBodyItem>,
+    args: Value,
+    // Some<[]> outputs "{}", None outputs ";".
+    body: Option<Vec<AtRuleBodyItem>>,
 }
 
 impl AtRule {
     pub(crate) fn new(
         name: String,
-        args: String,
-        body: Vec<AtRuleBodyItem>,
+        args: Value,
+        body: Option<Vec<AtRuleBodyItem>>,
     ) -> Self {
         AtRule { name, args, body }
     }
+    pub(crate) fn no_body(&self) -> bool {
+        self.body.is_none()
+    }
     pub(crate) fn write(&self, buf: &mut CssBuf) -> io::Result<()> {
+        buf.do_indent_no_nl();
         write!(buf, "@{}", self.name)?;
-        if !self.args.is_empty() {
-            write!(buf, " {}", self.args)?;
+        if !self.args.is_null() {
+            write!(buf, " {}", self.args.format(buf.format()))?;
         }
-        buf.start_block();
-        for item in &self.body {
-            item.write(buf)?;
+        if let Some(body) = &self.body {
+            buf.start_block();
+            for item in body {
+                item.write(buf)?;
+            }
+            buf.end_block();
+        } else {
+            buf.add_one(";\n", ";");
         }
-        buf.end_block();
         Ok(())
     }
 }
@@ -46,6 +55,8 @@ pub enum AtRuleBodyItem {
     Rule(Rule),
     /// A raw property.
     Property(Property),
+    /// An `@` rule.
+    AtRule(AtRule),
 }
 
 impl AtRuleBodyItem {
@@ -55,6 +66,7 @@ impl AtRuleBodyItem {
             AtRuleBodyItem::Comment(comment) => comment.write(buf),
             AtRuleBodyItem::Rule(rule) => rule.write(buf)?,
             AtRuleBodyItem::Property(property) => property.write(buf),
+            AtRuleBodyItem::AtRule(rule) => rule.write(buf)?,
         }
         Ok(())
     }
@@ -79,3 +91,8 @@ impl From<Property> for AtRuleBodyItem {
         AtRuleBodyItem::Property(rule)
     }
 }
+impl From<AtRule> for AtRuleBodyItem {
+    fn from(rule: AtRule) -> Self {
+        AtRuleBodyItem::AtRule(rule)
+    }
+}

Diff for: src/css/item.rs

@@ -13,6 +13,8 @@ pub enum Item {
     Rule(Rule),
     /// An `@` rule, e.g. `@media ... { ... }`
     AtRule(AtRule),
+    /// An extra newline for grouping (unless compressed format).
+    Separator,
 }
 
 impl Item {
@@ -22,6 +24,7 @@ impl Item {
             Item::Import(import) => import.write(buf)?,
             Item::Rule(rule) => rule.write(buf)?,
             Item::AtRule(atrule) => atrule.write(buf)?,
+            Item::Separator => buf.opt_nl(),
         }
         Ok(())
     }

Diff for: src/css/rule.rs

@@ -1,4 +1,4 @@
-use super::{Comment, CssString, Import, Selectors, Value};
+use super::{AtRule, Comment, CssString, Import, Selectors, Value};
 use crate::output::CssBuf;
 use std::io::{self, Write};
 
@@ -55,6 +55,8 @@ pub enum BodyItem {
     CustomProperty(String, CssString),
     /// A comment
     Comment(Comment),
+    /// Empty at-rules are allowed in a rule body.
+    ARule(AtRule),
 }
 
 impl BodyItem {
@@ -80,6 +82,7 @@ impl BodyItem {
                 )?;
                 buf.add_one(";\n", ";");
             }
+            BodyItem::ARule(rule) => rule.write(buf)?,
         }
         Ok(())
     }
@@ -101,6 +104,18 @@ impl From<Property> for BodyItem {
     }
 }
 
+impl TryFrom<AtRule> for BodyItem {
+    type Error = AtRule;
+
+    fn try_from(value: AtRule) -> Result<Self, Self::Error> {
+        if value.no_body() {
+            Ok(BodyItem::ARule(value))
+        } else {
+            Err(value)
+        }
+    }
+}
+
 /// A css property; a name and [Value].
 #[derive(Clone, Debug)]
 pub struct Property {
@@ -113,15 +128,6 @@ impl Property {
     pub fn new(name: String, value: Value) -> Self {
         Property { name, value }
     }
-    /// Return this property but with a prefix.
-    ///
-    /// The prefix is separated from the old name with a dash.
-    pub fn prefix(self, prefix: &str) -> Self {
-        Property {
-            name: format!("{}-{}", prefix, self.name),
-            value: self.value,
-        }
-    }
     pub(crate) fn write(&self, buf: &mut CssBuf) {
         buf.do_indent_no_nl();
         buf.add_str(&self.name);

Diff for: src/css/value.rs

@@ -208,6 +208,10 @@ impl Value {
             format,
         }
     }
+    /// Format this value for error messages.
+    pub fn introspect(&self) -> Formatted<Value> {
+        self.format(Format::introspect())
+    }
 }
 
 /// Some Values are equal according to spec even with some

Diff for: src/error.rs

@@ -156,6 +156,14 @@ pub enum Invalid {
     DuplicateArgument,
     /// Positional arguments must come before keyword arguments.
     PositionalArgAfterNamed,
+    /// Declarations may only be used within style rules.
+    DeclarationOutsideRule,
+    /// Only properties are valid inside namespace rules.
+    InNsRule,
+    /// Global custom property not allowed.
+    GlobalCustomProperty,
+    /// Global namespaced property not allowed.
+    GlobalNsProperty,
     /// Some invalid scope operation.
     InScope(ScopeError),
     /// An `@error` reached.
@@ -194,8 +202,36 @@ impl fmt::Display for Invalid {
                 "Positional arguments must come before keyword arguments."
                     .fmt(out)
             }
+            Invalid::DeclarationOutsideRule => {
+                "Declarations may only be used within style rules.".fmt(out)
+            }
+            Invalid::InNsRule => {
+                "Only properties are valid inside namespace rules.".fmt(out)
+            }
+            Invalid::GlobalCustomProperty => {
+                "Global custom property not allowed.".fmt(out)
+            }
+            Invalid::GlobalNsProperty => {
+                "Global namespaced property not allowed.".fmt(out)
+            }
             Invalid::InScope(err) => err.fmt(out),
             Invalid::AtError(msg) => msg.fmt(out),
         }
     }
 }
+
+pub(crate) trait ResultPos<T> {
+    /// Someting bad happened at a given source position.
+    fn at(self, pos: &SourcePos) -> Result<T, Error>;
+    /// Something bad happened, but we don't know the position.
+    /// This is probably room for improvement in rsass error handling.
+    fn no_pos(self) -> Result<T, Error>;
+}
+impl<T> ResultPos<T> for Result<T, Invalid> {
+    fn at(self, pos: &SourcePos) -> Result<T, Error> {
+        self.map_err(|e| e.at(pos.clone()))
+    }
+    fn no_pos(self) -> Result<T, Error> {
+        self.map_err(|e| Error::error(e.to_string()))
+    }
+}

Diff for: src/input/context.rs

@@ -1,7 +1,7 @@
 use super::{
     CargoLoader, FsLoader, LoadError, Loader, SourceFile, SourceKind,
 };
-use crate::output::{handle_parsed, CssBuf, CssHead, Format};
+use crate::output::{handle_parsed, CssData, Format};
 use crate::{Error, ScopeRef};
 use std::{borrow::Cow, collections::BTreeMap, fmt, path::Path};
 use tracing::instrument;
@@ -146,18 +146,11 @@ impl<AnyLoader: Loader> Context<AnyLoader> {
             .clone()
             .unwrap_or_else(|| ScopeRef::new_global(Default::default()));
         self.lock_loading(&file, false)?;
-        let mut head = CssHead::new();
-        let mut body = CssBuf::new(scope.get_format());
-        handle_parsed(
-            file.parse()?,
-            &mut head,
-            None,
-            &mut body,
-            scope,
-            &mut self,
-        )?;
+        let mut css = CssData::new();
+        let format = scope.get_format();
+        handle_parsed(file.parse()?, &mut css, scope, &mut self)?;
         self.unlock_loading(&file);
-        Ok(head.combine_final(body))
+        css.into_buffer(format)
     }
 
     /// Set the output format for this context.