Auto merge of #180 - upsuper:at-rule-parser, r=SimonSapin

Have at-rule without block handled in two stages as well

This change makes `AtRuleParser::parse_prelude` always return a prelude if parsed successfully, so that at-rules which do not accept block can be handled in two phases just like those with block.

This is important because at-rules without block usually have side effects. If `parse_prelude` executes the side effect, but `parse_at_rule` later determined that the rule is actually invalid (e.g. because it is followed by a block), there is no way for impl of `AtRuleParser` to undo the side effect.

It provides the necessary parser side change for fixing [Gecko bug 1388911](https://bugzilla.mozilla.org/show_bug.cgi?id=1388911).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-cssparser/180)
<!-- Reviewable:end -->

src/css-parsing-tests/one_rule.json

@@ -7,13 +7,17 @@
 "@foo", ["at-rule", "foo", [], null],
 
 "@foo bar; \t/* comment */", ["at-rule", "foo", [" ", ["ident", "bar"]], null],
-" /**/ @foo bar{[(4", ["at-rule", "foo",
+" /**/ @foo-with-block bar{[(4", ["at-rule", "foo-with-block",
     [" ", ["ident", "bar"]],
     [["[]", ["()", ["number", "4", 4, "integer"]]]]
 ],
 
-"@foo { bar", ["at-rule", "foo", [" "], [" ", ["ident", "bar"]]],
-"@foo [ bar", ["at-rule", "foo", [" ", ["[]", " ", ["ident", "bar"]]], null],
+"@foo-with-block { bar", ["at-rule", "foo-with-block", [" "], [
+    " ", ["ident", "bar"]]
+],
+"@foo [ bar", ["at-rule", "foo",
+    [" ", ["[]", " ", ["ident", "bar"]]], null
+],
 
 " /**/ div > p { color: #aaa;  } /**/ ", ["qualified rule",
     [["ident", "div"], " ", ">", " ", ["ident", "p"], " "],

src/css-parsing-tests/rule_list.json

@@ -7,18 +7,20 @@
 "@foo", [["at-rule", "foo", [], null]],
 
 "@charset; @foo", [
-    ["at-rule", "charset", [], null],
+    ["error", "invalid"],
     ["at-rule", "foo", [], null]
 ],
 
 "@foo bar; \t/* comment */", [["at-rule", "foo", [" ", ["ident", "bar"]], null]],
 
-" /**/ @foo bar{[(4", [["at-rule", "foo",
+" /**/ @foo-with-block bar{[(4", [["at-rule", "foo-with-block",
     [" ", ["ident", "bar"]],
     [["[]", ["()", ["number", "4", 4, "integer"]]]]
 ]],
 
-"@foo { bar", [["at-rule", "foo", [" "], [" ", ["ident", "bar"]]]],
+"@foo-with-block { bar", [["at-rule", "foo-with-block", [" "], [
+    " ", ["ident", "bar"]]]
+],
 "@foo [ bar", [["at-rule", "foo", [" ", ["[]", " ", ["ident", "bar"]]], null]],
 
 " /**/ div > p { color: #aaa;  } /**/ ", [["qualified rule",

src/css-parsing-tests/stylesheet.json

@@ -10,17 +10,19 @@
 
 "@foo; @charset 4 {}", [
     ["at-rule", "foo", [], null],
-    ["at-rule", "charset", [" ", ["number", "4", 4, "integer"], " "], []]
+    ["error", "invalid"]
 ],
 
 "@foo bar; \t/* comment */", [["at-rule", "foo", [" ", ["ident", "bar"]], null]],
 
-" /**/ @foo bar{[(4", [["at-rule", "foo",
+" /**/ @foo-with-block bar{[(4", [["at-rule", "foo-with-block",
 	[" ", ["ident", "bar"]],
 	[["[]", ["()", ["number", "4", 4, "integer"]]]]
 ]],
 
-"@foo { bar", [["at-rule", "foo", [" "], [" ", ["ident", "bar"]]]],
+"@foo-with-block { bar", [["at-rule", "foo-with-block", [" "], [
+    " ", ["ident", "bar"]]]
+],
 "@foo [ bar", [["at-rule", "foo", [" ", ["[]", " ", ["ident", "bar"]]], null]],
 
 " /**/ div > p { color: #aaa;  } /**/ ", [["qualified rule",

src/rules_and_declarations.rs

@@ -23,24 +23,17 @@ pub fn parse_important<'i, 't>(input: &mut Parser<'i, 't>) -> Result<(), BasicPa
 /// The return value for `AtRuleParser::parse_prelude`.
 /// Indicates whether the at-rule is expected to have a `{ /* ... */ }` block
 /// or end with a `;` semicolon.
-pub enum AtRuleType<P, R> {
+pub enum AtRuleType<P, PB> {
     /// The at-rule is expected to end with a `;` semicolon. Example: `@import`.
     ///
-    /// The value is the finished representation of an at-rule
-    /// as returned by `RuleListParser::next` or `DeclarationListParser::next`.
-    WithoutBlock(R),
+    /// The value is the representation of all data of the rule which would be
+    /// handled in rule_without_block.
+    WithoutBlock(P),
 
     /// The at-rule is expected to have a a `{ /* ... */ }` block. Example: `@media`
     ///
     /// The value is the representation of the "prelude" part of the rule.
-    WithBlock(P),
-
-    /// The at-rule may either have a block or end with a semicolon.
-    ///
-    /// This is mostly for testing. As of this writing no real CSS at-rule behaves like this.
-    ///
-    /// The value is the representation of the "prelude" part of the rule.
-    OptionalBlock(P),
+    WithBlock(PB),
 }
 
 /// A trait to provide various parsing of declaration values.
@@ -85,8 +78,11 @@ pub trait DeclarationParser<'i> {
 /// so that `impl AtRuleParser<(), ()> for ... {}` can be used
 /// for using `DeclarationListParser` to parse a declartions list with only qualified rules.
 pub trait AtRuleParser<'i> {
-    /// The intermediate representation of an at-rule prelude.
-    type Prelude;
+    /// The intermediate representation of prelude of an at-rule without block;
+    type PreludeNoBlock;
+
+    /// The intermediate representation of prelude of an at-rule with block;
+    type PreludeBlock;
 
     /// The finished representation of an at-rule.
     type AtRule;
@@ -112,36 +108,39 @@ pub trait AtRuleParser<'i> {
     /// that ends wherever the prelude should end.
     /// (Before the next semicolon, the next `{`, or the end of the current block.)
     fn parse_prelude<'t>(&mut self, name: CowRcStr<'i>, input: &mut Parser<'i, 't>)
-                     -> Result<AtRuleType<Self::Prelude, Self::AtRule>, ParseError<'i, Self::Error>> {
+                     -> Result<AtRuleType<Self::PreludeNoBlock, Self::PreludeBlock>,
+                               ParseError<'i, Self::Error>> {
         let _ = name;
         let _ = input;
         Err(ParseError::Basic(BasicParseError::AtRuleInvalid(name)))
     }
 
+    /// End an at-rule which doesn't have block. Return the finished
+    /// representation of the at-rule.
+    ///
+    /// This is only called when `parse_prelude` returned `WithoutBlock`, and
+    /// either the `;` semicolon indeed follows the prelude, or parser is at
+    /// the end of the input.
+    fn rule_without_block(&mut self, prelude: Self::PreludeNoBlock) -> Self::AtRule {
+        let _ = prelude;
+        panic!("The `AtRuleParser::rule_without_block` method must be overriden \
+                if `AtRuleParser::parse_prelude` ever returns `AtRuleType::WithoutBlock`.")
+    }
+
     /// Parse the content of a `{ /* ... */ }` block for the body of the at-rule.
     ///
     /// Return the finished representation of the at-rule
     /// as returned by `RuleListParser::next` or `DeclarationListParser::next`,
     /// or `Err(())` to ignore the entire at-rule as invalid.
     ///
-    /// This is only called when `parse_prelude` returned `WithBlock` or `OptionalBlock`,
-    /// and a block was indeed found following the prelude.
-    fn parse_block<'t>(&mut self, prelude: Self::Prelude, input: &mut Parser<'i, 't>)
+    /// This is only called when `parse_prelude` returned `WithBlock`, and a block
+    /// was indeed found following the prelude.
+    fn parse_block<'t>(&mut self, prelude: Self::PreludeBlock, input: &mut Parser<'i, 't>)
                        -> Result<Self::AtRule, ParseError<'i, Self::Error>> {
         let _ = prelude;
         let _ = input;
         Err(ParseError::Basic(BasicParseError::AtRuleBodyInvalid))
     }
-
-    /// An `OptionalBlock` prelude was followed by `;`.
-    ///
-    /// Convert the prelude into the finished representation of the at-rule
-    /// as returned by `RuleListParser::next` or `DeclarationListParser::next`.
-    fn rule_without_block(&mut self, prelude: Self::Prelude) -> Self::AtRule {
-        let _ = prelude;
-        panic!("The `AtRuleParser::rule_without_block` method must be overriden \
-                if `AtRuleParser::parse_prelude` ever returns `AtRuleType::OptionalBlock`.")
-    }
 }
 
 /// A trait to provide various parsing of qualified rules.
@@ -460,9 +459,9 @@ fn parse_at_rule<'i: 't, 't, P, E>(start: &ParserState, name: CowRcStr<'i>,
         parser.parse_prelude(name, input)
     });
     match result {
-        Ok(AtRuleType::WithoutBlock(rule)) => {
+        Ok(AtRuleType::WithoutBlock(prelude)) => {
             match input.next() {
-                Ok(&Token::Semicolon) | Err(_) => Ok(rule),
+                Ok(&Token::Semicolon) | Err(_) => Ok(parser.rule_without_block(prelude)),
                 Ok(&Token::CurlyBracketBlock) => Err(PreciseParseError {
                     error: ParseError::Basic(BasicParseError::UnexpectedToken(Token::CurlyBracketBlock)),
                     slice: input.slice_from(start.position()),
@@ -495,21 +494,6 @@ fn parse_at_rule<'i: 't, 't, P, E>(start: &ParserState, name: CowRcStr<'i>,
                 Ok(_) => unreachable!()
             }
         }
-        Ok(AtRuleType::OptionalBlock(prelude)) => {
-            match input.next() {
-                Ok(&Token::Semicolon) | Err(_) => Ok(parser.rule_without_block(prelude)),
-                Ok(&Token::CurlyBracketBlock) => {
-                    // FIXME: https://github.com/rust-lang/rust/issues/42508
-                    parse_nested_block::<'i, 't, _, _, _>(input, move |input| parser.parse_block(prelude, input))
-                        .map_err(|e| PreciseParseError {
-                            error: e,
-                            slice: input.slice_from(start.position()),
-                            location: start.source_location(),
-                        })
-                }
-                _ => unreachable!()
-            }
-        }
         Err(error) => {
             let end_position = input.position();
             match input.next() {

src/tests.rs

@@ -745,29 +745,35 @@ impl<'i> DeclarationParser<'i> for JsonParser {
 }
 
 impl<'i> AtRuleParser<'i> for JsonParser {
-    type Prelude = Vec<Json>;
+    type PreludeNoBlock = Vec<Json>;
+    type PreludeBlock = Vec<Json>;
     type AtRule = Json;
     type Error = ();
 
     fn parse_prelude<'t>(&mut self, name: CowRcStr<'i>, input: &mut Parser<'i, 't>)
-                         -> Result<AtRuleType<Vec<Json>, Json>, ParseError<'i, ()>> {
-        Ok(AtRuleType::OptionalBlock(vec![
+                         -> Result<AtRuleType<Vec<Json>, Vec<Json>>, ParseError<'i, ()>> {
+        let prelude = vec![
             "at-rule".to_json(),
             name.to_json(),
             Json::Array(component_values_to_json(input)),
-        ]))
+        ];
+        match_ignore_ascii_case! { &*name,
+            "media" | "foo-with-block" => Ok(AtRuleType::WithBlock(prelude)),
+            "charset" => Err(BasicParseError::AtRuleInvalid(name.clone()).into()),
+            _ => Ok(AtRuleType::WithoutBlock(prelude)),
+        }
+    }
+
+    fn rule_without_block(&mut self, mut prelude: Vec<Json>) -> Json {
+        prelude.push(Json::Null);
+        Json::Array(prelude)
     }
 
     fn parse_block<'t>(&mut self, mut prelude: Vec<Json>, input: &mut Parser<'i, 't>)
                        -> Result<Json, ParseError<'i, ()>> {
         prelude.push(Json::Array(component_values_to_json(input)));
         Ok(Json::Array(prelude))
     }
-
-    fn rule_without_block(&mut self, mut prelude: Vec<Json>) -> Json {
-        prelude.push(Json::Null);
-        Json::Array(prelude)
-    }
 }
 
 impl<'i> QualifiedRuleParser<'i> for JsonParser {