Refactor getValue to take allowVariantList

Author: stasm

fluent-syntax/src/errors.js

@@ -62,8 +62,6 @@ function getErrorMessage(code, args) {
       return "Positional arguments must not follow named arguments";
     case "E0022":
       return "Named arguments must be unique";
-    case "E0023":
-      return "VariantLists are only allowed inside of other VariantLists.";
     case "E0024":
       return "Cannot access variants of a message.";
     case "E0025": {

fluent-syntax/src/parser.js

@@ -43,7 +43,7 @@ export default class FluentParser {
     // Poor man's decorators.
     const methodNames = [
       "getComment", "getMessage", "getTerm", "getAttribute", "getIdentifier",
-      "getVariant", "getNumber", "getValue", "getPattern", "getVariantList",
+      "getVariant", "getNumber", "getPattern", "getVariantList",
       "getTextElement", "getPlaceable", "getExpression",
       "getSelectorExpression", "getCallArg", "getString", "getLiteral"
     ];
@@ -231,7 +231,7 @@ export default class FluentParser {
     ps.skipBlankInline();
     ps.expectChar("=");
 
-    const value = this.getInlineOrBlock(ps, this.getPattern);
+    const value = this.getValue(ps, {allowVariantList: false});
     const attrs = this.getAttributes(ps);
 
     if (value === null && attrs.length === 0) {
@@ -248,7 +248,11 @@ export default class FluentParser {
     ps.skipBlankInline();
     ps.expectChar("=");
 
-    const value = this.getInlineOrBlock(ps, this.getValue);
+    // XXX Once https://github.com/projectfluent/fluent/pull/220 lands,
+    // getTerm will be the only place where VariantLists are still legal. Move
+    // the code currently present in getValueType up to here then, and remove
+    // the allowVariantList switch.
+    const value = this.getValue(ps, {allowVariantList: true});
     if (value === null) {
       throw new ParseError("E0006", id.name);
     }
@@ -257,22 +261,6 @@ export default class FluentParser {
     return new AST.Term(id, value, attrs);
   }
 
-  getInlineOrBlock(ps, method) {
-    ps.peekBlankInline();
-    if (ps.isValueStart()) {
-      ps.skipToPeek();
-      return method.call(this, ps, {isBlock: false});
-    }
-
-    ps.peekBlankBlock();
-    if (ps.isValueContinuation()) {
-      ps.skipToPeek();
-      return method.call(this, ps, {isBlock: true});
-    }
-
-    return null;
-  }
-
   getAttribute(ps) {
     ps.expectChar(".");
 
@@ -281,7 +269,7 @@ export default class FluentParser {
     ps.skipBlankInline();
     ps.expectChar("=");
 
-    const value = this.getInlineOrBlock(ps, this.getPattern);
+    const value = this.getValue(ps, {allowVariantList: false});
     if (value === null) {
       throw new ParseError("E0012");
     }
@@ -328,7 +316,7 @@ export default class FluentParser {
     return this.getIdentifier(ps);
   }
 
-  getVariant(ps, hasDefault) {
+  getVariant(ps, {hasDefault, allowVariantList}) {
     let defaultIndex = false;
 
     if (ps.currentChar === "*") {
@@ -349,21 +337,23 @@ export default class FluentParser {
     ps.skipBlank();
     ps.expectChar("]");
 
-    const value = this.getInlineOrBlock(ps, this.getValue);
+    // XXX We need to pass allowVariantList all the way down to here because
+    // nested VariantLists in Terms are legal for now.
+    const value = this.getValue(ps, {allowVariantList});
     if (value === null) {
       throw new ParseError("E0012");
     }
 
     return new AST.Variant(key, value, defaultIndex);
   }
 
-  getVariants(ps) {
+  getVariants(ps, {allowVariantList}) {
     const variants = [];
     let hasDefault = false;
 
     ps.skipBlank();
     while (ps.isVariantStart()) {
-      const variant = this.getVariant(ps, hasDefault);
+      const variant = this.getVariant(ps, {allowVariantList, hasDefault});
 
       if (variant.default) {
         hasDefault = true;
@@ -419,29 +409,54 @@ export default class FluentParser {
     return new AST.NumberLiteral(num);
   }
 
-  getValue(ps, {isBlock}) {
+  // getValue distinguished between patterns which start on the same line as the
+  // identifier (a.k.a. inline signleline patterns and inline multiline
+  // patterns) and patterns which start on a new line (a.k.a. block multiline
+  // patterns). The distinction is important for the dedentation logic: the
+  // indent of the first line of a block pattern must be taken into account when
+  // calculating the maximum common indent.
+  getValue(ps, {allowVariantList}) {
     ps.peekBlankInline();
-    const peekOffset = ps.peekOffset;
-    if (ps.currentPeek === "{") {
+    if (ps.isValueStart()) {
+      ps.skipToPeek();
+      return this.getValueType(
+        ps, {isBlock: false, allowVariantList});
+    }
+
+    ps.peekBlankBlock();
+    if (ps.isValueContinuation()) {
+      ps.skipToPeek();
+      return this.getValueType(ps, {isBlock: true, allowVariantList});
+    }
+
+    return null;
+  }
+
+  // Parse a VariantList (if allowed) or a Pattern.
+  getValueType(ps, {isBlock, allowVariantList}) {
+    ps.peekBlankInline();
+    if (allowVariantList && ps.currentPeek === "{") {
+      const start = ps.peekOffset;
       ps.peek();
-      ps.peekBlank();
-      if (ps.isVariantStart()) {
-        ps.resetPeek(peekOffset);
-        ps.skipToPeek();
-        return this.getVariantList(ps);
+      ps.peekBlankInline();
+      if (ps.currentPeek === EOL) {
+        ps.peekBlank();
+        if (ps.isVariantStart()) {
+          ps.resetPeek(start);
+          ps.skipToPeek();
+          return this.getVariantList(ps, {allowVariantList});
+        }
       }
     }
 
     ps.resetPeek();
-    return this.getPattern(ps, {isBlock});
+    const pattern = this.getPattern(ps, {isBlock});
+    return pattern;
   }
 
   getVariantList(ps) {
     ps.expectChar("{");
-    ps.skipBlankInline();
-    ps.expectLineEnd();
-    const variants = this.getVariants(ps);
-    ps.skipBlank();
+    var variants = this.getVariants(ps, {allowVariantList: true});
     ps.expectChar("}");
     return new AST.VariantList(variants);
   }
@@ -490,6 +505,9 @@ export default class FluentParser {
     return new AST.Pattern(this.dedent(elements, commonIndentLength));
   }
 
+  // Create a token representing an indent. It's not part of the AST and it will
+  // be trimmed and merged into adjacent TextElements, or turned into a new
+  // TextElement, if it's surrounded by two Placeables.
   getIndent(ps, value, start) {
     return {
       type: "Indent",
@@ -498,6 +516,8 @@ export default class FluentParser {
     };
   }
 
+  // Dedent a list of elements by removing the maximum common indent from the
+  // beginning of text lines. The common indent is calculated in getPattern.
   dedent(elements, commonIndent) {
     const trimmed = [];
 
@@ -528,6 +548,8 @@ export default class FluentParser {
       }
 
       if (element.type === "Indent") {
+        // Is the indent hasn't been merged into a preceding TextElement,
+        // convert it into a new TextElement.
         const textElement = new AST.TextElement(element.value);
         if (this.withSpans) {
           textElement.addSpan(element.span.start, element.span.end);
@@ -538,7 +560,7 @@ export default class FluentParser {
       trimmed.push(element);
     }
 
-    // Trim trailing whitespace.
+    // Trim trailing whitespace from the Pattern.
     const lastElement = trimmed[trimmed.length - 1];
     if (lastElement.type === "TextElement") {
       lastElement.value = lastElement.value.replace(trailingWSRe, "");
@@ -600,16 +622,14 @@ export default class FluentParser {
 
   getPlaceable(ps) {
     ps.expectChar("{");
+    ps.skipBlank();
     const expression = this.getExpression(ps);
     ps.expectChar("}");
     return new AST.Placeable(expression);
   }
 
   getExpression(ps) {
-    ps.skipBlank();
-
     const selector = this.getSelectorExpression(ps);
-
     ps.skipBlank();
 
     if (ps.currentChar === "-") {
@@ -638,21 +658,13 @@ export default class FluentParser {
       ps.skipBlankInline();
       ps.expectLineEnd();
 
-      const variants = this.getVariants(ps);
-
-      // VariantLists are only allowed in other VariantLists.
-      if (variants.some(v => v.value.type === "VariantList")) {
-        throw new ParseError("E0023");
-      }
-
+      const variants = this.getVariants(ps, {allowVariantList: false});
       return new AST.SelectExpression(selector, variants);
     } else if (selector.type === "AttributeExpression" &&
                selector.ref.type === "TermReference") {
       throw new ParseError("E0019");
     }
 
-    ps.skipBlank();
-
     return selector;
   }
 

fluent-syntax/test/fixtures_behavior/variant_lists.ftl

@@ -4,7 +4,7 @@ message1 =
         *[one] One
     }
 
-# ~ERROR E0023, pos 123
+# ~ERROR E0014, pos 97
 message2 =
     { $sel ->
         *[one] {
@@ -24,7 +24,7 @@ message2 =
          }
     }
 
-# ~ERROR E0023, pos 318
+# ~ERROR E0014, pos 292
 -term3 =
     { $sel ->
         *[one] {

fluent-syntax/test/fixtures_structure/leading_dots.json

@@ -836,7 +836,7 @@
               ],
               "span": {
                 "type": "Span",
-                "start": 614,
+                "start": 615,
                 "end": 684
               }
             },

fluent-syntax/test/fixtures_structure/sparse-messages.json

@@ -322,7 +322,7 @@
               ],
               "span": {
                 "type": "Span",
-                "start": 154,
+                "start": 155,
                 "end": 208
               }
             },

fluent-syntax/test/fixtures_structure/term.json

@@ -481,7 +481,7 @@
               ],
               "span": {
                 "type": "Span",
-                "start": 198,
+                "start": 199,
                 "end": 436
               }
             },

fluent-syntax/test/fixtures_structure/variant_with_empty_pattern.json

@@ -77,7 +77,7 @@
               ],
               "span": {
                 "type": "Span",
-                "start": 12,
+                "start": 13,
                 "end": 41
               }
             },