Merge pull request #396 from stasm/throw-errors

Omitting errors in formatPattern() makes it throw

Author: stasm

fluent/src/bundle.js

@@ -1,6 +1,7 @@
 import {resolveComplexPattern} from "./resolver.js";
 import FluentResource from "./resource.js";
 import { FluentNone } from "./types.js";
+import Scope from "./scope.js";
 
 /**
  * Message bundles are single-language stores of translations.  They are
@@ -207,40 +208,32 @@ export default class FluentBundle {
    *         // [<ReferenceError: Unknown variable: name>]
    *     }
    *
+   * If `errors` is omitted, the first encountered error will be thrown.
+   *
    * @param   {string|Array} pattern
    * @param   {?Object} args
    * @param   {?Array} errors
    * @returns {string}
    */
   formatPattern(pattern, args, errors) {
-    // Resolve a simple pattern without creating a scope.
+    // Resolve a simple pattern without creating a scope. No error handling is
+    // required; by definition simple patterns don't have placeables.
     if (typeof pattern === "string") {
       return this._transform(pattern);
     }
 
     // Resolve a complex pattern.
-    if (Array.isArray(pattern)) {
-      let scope = this._createScope(args, errors);
-      try {
-        let value = resolveComplexPattern(scope, pattern);
-        return value.toString(scope);
-      } catch (err) {
+    let scope = new Scope(this, errors, args);
+    try {
+      let value = resolveComplexPattern(scope, pattern);
+      return value.toString(scope);
+    } catch (err) {
+      if (scope.errors) {
         scope.errors.push(err);
         return new FluentNone().toString(scope);
       }
+      throw err;
     }
-
-    throw new TypeError("Invalid Pattern type");
-  }
-
-  _createScope(args, errors = []) {
-    return {
-      args, errors,
-      bundle: this,
-      dirty: new WeakSet(),
-      // TermReferences are resolved in a new scope.
-      insideTermReference: false,
-    };
   }
 
   _memoizeIntlObject(ctor, opts) {

fluent/src/resolver.js

@@ -79,7 +79,7 @@ function getDefault(scope, variants, star) {
     return resolvePattern(scope, variants[star].value);
   }
 
-  scope.errors.push(new RangeError("No default"));
+  scope.reportError(new RangeError("No default"));
   return new FluentNone();
 }
 
@@ -127,7 +127,7 @@ function resolveExpression(scope, expr) {
 function VariableReference(scope, {name}) {
   if (!scope.args || !scope.args.hasOwnProperty(name)) {
     if (scope.insideTermReference === false) {
-      scope.errors.push(new ReferenceError(`Unknown variable: ${name}`));
+      scope.reportError(new ReferenceError(`Unknown variable: ${name}`));
     }
     return new FluentNone(`$${name}`);
   }
@@ -150,7 +150,7 @@ function VariableReference(scope, {name}) {
         return new FluentDateTime(arg);
       }
     default:
-      scope.errors.push(
+      scope.reportError(
         new TypeError(`Unsupported variable type: ${name}, ${typeof arg}`)
       );
       return new FluentNone(`$${name}`);
@@ -161,7 +161,7 @@ function VariableReference(scope, {name}) {
 function MessageReference(scope, {name, attr}) {
   const message = scope.bundle._messages.get(name);
   if (!message) {
-    scope.errors.push(new ReferenceError(`Unknown message: ${name}`));
+    scope.reportError(new ReferenceError(`Unknown message: ${name}`));
     return new FluentNone(name);
   }
 
@@ -170,15 +170,15 @@ function MessageReference(scope, {name, attr}) {
     if (attribute) {
       return resolvePattern(scope, attribute);
     }
-    scope.errors.push(new ReferenceError(`Unknown attribute: ${attr}`));
+    scope.reportError(new ReferenceError(`Unknown attribute: ${attr}`));
     return new FluentNone(`${name}.${attr}`);
   }
 
   if (message.value) {
     return resolvePattern(scope, message.value);
   }
 
-  scope.errors.push(new ReferenceError(`No value: ${name}`));
+  scope.reportError(new ReferenceError(`No value: ${name}`));
   return new FluentNone(name);
 }
 
@@ -187,21 +187,20 @@ function TermReference(scope, {name, attr, args}) {
   const id = `-${name}`;
   const term = scope.bundle._terms.get(id);
   if (!term) {
-    const err = new ReferenceError(`Unknown term: ${id}`);
-    scope.errors.push(err);
+    scope.reportError(new ReferenceError(`Unknown term: ${id}`));
     return new FluentNone(id);
   }
 
-  // Every TermReference has its own args.
-  const [, keyargs] = getArguments(scope, args);
-  const local = {...scope, args: keyargs, insideTermReference: true};
+  // Every TermReference has its own variables.
+  const [, params] = getArguments(scope, args);
+  const local = scope.cloneForTermReference(params);
 
   if (attr) {
     const attribute = term.attributes[attr];
     if (attribute) {
       return resolvePattern(local, attribute);
     }
-    scope.errors.push(new ReferenceError(`Unknown attribute: ${attr}`));
+    scope.reportError(new ReferenceError(`Unknown attribute: ${attr}`));
     return new FluentNone(`${id}.${attr}`);
   }
 
@@ -214,12 +213,12 @@ function FunctionReference(scope, {name, args}) {
   // the `FluentBundle` constructor.
   const func = scope.bundle._functions[name] || builtins[name];
   if (!func) {
-    scope.errors.push(new ReferenceError(`Unknown function: ${name}()`));
+    scope.reportError(new ReferenceError(`Unknown function: ${name}()`));
     return new FluentNone(`${name}()`);
   }
 
   if (typeof func !== "function") {
-    scope.errors.push(new TypeError(`Function ${name}() is not callable`));
+    scope.reportError(new TypeError(`Function ${name}() is not callable`));
     return new FluentNone(`${name}()`);
   }
 
@@ -252,7 +251,7 @@ function SelectExpression(scope, {selector, variants, star}) {
 // Resolve a pattern (a complex string with placeables).
 export function resolveComplexPattern(scope, ptn) {
   if (scope.dirty.has(ptn)) {
-    scope.errors.push(new RangeError("Cyclic reference"));
+    scope.reportError(new RangeError("Cyclic reference"));
     return new FluentNone();
   }
 

fluent/src/scope.js

@@ -0,0 +1,29 @@
+export default class Scope {
+  constructor(
+    bundle,
+    errors,
+    args,
+    insideTermReference = false,
+    dirty = new WeakSet()
+  ) {
+    this.bundle = bundle;
+    this.errors = errors;
+    this.args = args;
+
+    // Term references require different variable lookup logic.
+    this.insideTermReference = insideTermReference;
+    // Keeps track of visited Patterns. Used to detect cyclic references.
+    this.dirty = dirty;
+  }
+
+  cloneForTermReference(args) {
+    return new Scope(this.bundle, this.errors, args, true, this.dirty);
+  }
+
+  reportError(error) {
+    if (!this.errors) {
+      throw error;
+    }
+    this.errors.push(error);
+  }
+}

fluent/test/bomb_test.js

@@ -35,6 +35,16 @@ suite('Reference bombs', function() {
       const val = bundle.formatPattern(msg.value, args, errs);
       assert.strictEqual(val, '{???}');
       assert.strictEqual(errs.length, 1);
+      assert.ok(errs[0] instanceof RangeError);
+    });
+
+    test('throws when errors are undefined', function() {
+      const msg = bundle.getMessage('lolz');
+      assert.throws(
+        () => bundle.formatPattern(msg.value),
+        RangeError,
+        "Too many characters in placeable"
+      );
     });
   });
 });

fluent/test/errors_test.js

@@ -0,0 +1,45 @@
+"use strict";
+
+import assert from "assert";
+import ftl from "@fluent/dedent";
+
+import FluentBundle from "../src/bundle";
+
+suite("Errors", function() {
+  let bundle;
+
+  suiteSetup(function() {
+    bundle = new FluentBundle("en-US", { useIsolating: false });
+    bundle.addMessages(ftl`
+      foo = {$one} and {$two}
+      `);
+  });
+
+  test("Reporting into an array", function() {
+    let errors = [];
+    let message = bundle.getMessage("foo");
+
+    let val1 = bundle.formatPattern(message.value, {}, errors);
+    assert.strictEqual(val1, "{$one} and {$two}");
+    assert.strictEqual(errors.length, 2);
+    assert.ok(errors[0] instanceof ReferenceError);
+    assert.ok(errors[1] instanceof ReferenceError);
+
+    let val2 = bundle.formatPattern(message.value, {}, errors);
+    assert.strictEqual(val2, "{$one} and {$two}");
+    assert.strictEqual(errors.length, 4);
+    assert.ok(errors[0] instanceof ReferenceError);
+    assert.ok(errors[1] instanceof ReferenceError);
+    assert.ok(errors[2] instanceof ReferenceError);
+    assert.ok(errors[3] instanceof ReferenceError);
+  });
+
+  test("First error is thrown", function() {
+    let message = bundle.getMessage("foo");
+    assert.throws(
+      () => bundle.formatPattern(message.value, {}),
+      ReferenceError,
+      "Unknown variable: $one"
+    );
+  });
+});