Make chart function argument parsing modular/flexible with 900 pages of error messages

Summary:
Depends on D20444. Ref T13279. Instead of ad-hoc parsing and messages, formalize chart function arguments.

Also, add a whole lot of extra type checking.

Test Plan: Built and charted various functions with various valid and invalid argument lists, got sensible-seeming errors and results.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20445

src/__phutil_library_map__.php

@@ -2652,6 +2652,8 @@ phutil_register_library_map(array(
     'PhabricatorChartAxis' => 'applications/fact/chart/PhabricatorChartAxis.php',
     'PhabricatorChartDataQuery' => 'applications/fact/chart/PhabricatorChartDataQuery.php',
     'PhabricatorChartFunction' => 'applications/fact/chart/PhabricatorChartFunction.php',
+    'PhabricatorChartFunctionArgument' => 'applications/fact/chart/PhabricatorChartFunctionArgument.php',
+    'PhabricatorChartFunctionArgumentParser' => 'applications/fact/chart/PhabricatorChartFunctionArgumentParser.php',
     'PhabricatorChatLogApplication' => 'applications/chatlog/application/PhabricatorChatLogApplication.php',
     'PhabricatorChatLogChannel' => 'applications/chatlog/storage/PhabricatorChatLogChannel.php',
     'PhabricatorChatLogChannelListController' => 'applications/chatlog/controller/PhabricatorChatLogChannelListController.php',
@@ -8629,6 +8631,8 @@ phutil_register_library_map(array(
     'PhabricatorChartAxis' => 'Phobject',
     'PhabricatorChartDataQuery' => 'Phobject',
     'PhabricatorChartFunction' => 'Phobject',
+    'PhabricatorChartFunctionArgument' => 'Phobject',
+    'PhabricatorChartFunctionArgumentParser' => 'Phobject',
     'PhabricatorChatLogApplication' => 'PhabricatorApplication',
     'PhabricatorChatLogChannel' => array(
       'PhabricatorChatLogDAO',

src/applications/fact/chart/PhabricatorChartFunction.php

@@ -7,6 +7,8 @@ abstract class PhabricatorChartFunction
   private $yAxis;
   private $limit;
 
+  private $argumentParser;
+
   final public function getFunctionKey() {
     return $this->getPhobjectClassConstant('FUNCTIONKEY', 32);
   }
@@ -19,11 +21,51 @@ abstract class PhabricatorChartFunction
   }
 
   final public function setArguments(array $arguments) {
-    $this->newArguments($arguments);
+    $parser = $this->getArgumentParser();
+    $parser->setRawArguments($arguments);
+
+    $specs = $this->newArguments();
+
+    if (!is_array($specs)) {
+      throw new Exception(
+        pht(
+          'Expected "newArguments()" in class "%s" to return a list of '.
+          'argument specifications, got %s.',
+          get_class($this),
+          phutil_describe_type($specs)));
+    }
+
+    assert_instances_of($specs, 'PhabricatorChartFunctionArgument');
+
+    foreach ($specs as $spec) {
+      $parser->addArgument($spec);
+    }
+
+    $parser->setHaveAllArguments(true);
+    $parser->parseArguments();
+
     return $this;
   }
 
-  abstract protected function newArguments(array $arguments);
+  abstract protected function newArguments();
+
+  final protected function newArgument() {
+    return new PhabricatorChartFunctionArgument();
+  }
+
+  final protected function getArgument($key) {
+    return $this->getArgumentParser()->getArgumentValue($key);
+  }
+
+  final protected function getArgumentParser() {
+    if (!$this->argumentParser) {
+      $parser = id(new PhabricatorChartFunctionArgumentParser())
+        ->setFunction($this);
+
+      $this->argumentParser = $parser;
+    }
+    return $this->argumentParser;
+  }
 
   public function loadData() {
     return;

src/applications/fact/chart/PhabricatorChartFunctionArgument.php

@@ -0,0 +1,129 @@
+<?php
+
+final class PhabricatorChartFunctionArgument
+  extends Phobject {
+
+  private $name;
+  private $type;
+
+  public function setName($name) {
+    $this->name = $name;
+    return $this;
+  }
+
+  public function getName() {
+    return $this->name;
+  }
+
+  public function setType($type) {
+    $types = array(
+      'fact-key' => true,
+      'function' => true,
+      'number' => true,
+    );
+
+    if (!isset($types[$type])) {
+      throw new Exception(
+        pht(
+          'Chart function argument type "%s" is unknown. Valid types '.
+          'are: %s.',
+          $type,
+          implode(', ', array_keys($types))));
+    }
+
+    $this->type = $type;
+    return $this;
+  }
+
+  public function getType() {
+    return $this->type;
+  }
+
+  public function newValue($value) {
+    switch ($this->getType()) {
+      case 'fact-key':
+        if (!is_string($value)) {
+          throw new Exception(
+            pht(
+              'Value for "fact-key" argument must be a string, got %s.',
+              phutil_describe_type($value)));
+        }
+
+        $facts = PhabricatorFact::getAllFacts();
+        $fact = idx($facts, $value);
+        if (!$fact) {
+          throw new Exception(
+            pht(
+              'Fact key "%s" is not a known fact key.',
+              $value));
+        }
+
+        return $fact;
+      case 'function':
+        // If this is already a function object, just return it.
+        if ($value instanceof PhabricatorChartFunction) {
+          return $value;
+        }
+
+        if (!is_array($value)) {
+          throw new Exception(
+            pht(
+              'Value for "function" argument must be a function definition, '.
+              'formatted as a list, like: [fn, arg1, arg, ...]. Actual value '.
+              'is %s.',
+              phutil_describe_type($value)));
+        }
+
+        if (!phutil_is_natural_list($value)) {
+          throw new Exception(
+            pht(
+              'Value for "function" argument must be a natural list, not '.
+              'a dictionary. Actual value is "%s".',
+              phutil_describe_type($value)));
+        }
+
+        if (!$value) {
+          throw new Exception(
+            pht(
+              'Value for "function" argument must be a list with a function '.
+              'name; got an empty list.'));
+        }
+
+        $function_name = array_shift($value);
+
+        if (!is_string($function_name)) {
+          throw new Exception(
+            pht(
+              'Value for "function" argument must be a natural list '.
+              'beginning with a function name as a string. The first list '.
+              'item has the wrong type, %s.',
+              phutil_describe_type($function_name)));
+        }
+
+        $functions = PhabricatorChartFunction::getAllFunctions();
+        if (!isset($functions[$function_name])) {
+          throw new Exception(
+            pht(
+              'Function "%s" is unknown. Valid functions are: %s',
+              $function_name,
+              implode(', ', array_keys($functions))));
+        }
+
+        return id(clone $functions[$function_name])
+          ->setArguments($value);
+      case 'number':
+        if (!is_float($value) && !is_int($value)) {
+          throw new Exception(
+            pht(
+              'Value for "number" argument must be an integer or double, '.
+              'got %s.',
+              phutil_describe_type($value)));
+        }
+
+        return $value;
+    }
+
+    throw new PhutilMethodNotImplementedException();
+  }
+
+}

src/applications/fact/chart/PhabricatorChartFunctionArgumentParser.php

@@ -0,0 +1,154 @@
+<?php
+
+final class PhabricatorChartFunctionArgumentParser
+  extends Phobject {
+
+  private $function;
+  private $rawArguments;
+  private $unconsumedArguments;
+  private $haveAllArguments = false;
+  private $unparsedArguments;
+  private $argumentMap = array();
+  private $argumentPosition = 0;
+  private $argumentValues = array();
+
+  public function setFunction(PhabricatorChartFunction $function) {
+    $this->function = $function;
+    return $this;
+  }
+
+  public function getFunction() {
+    return $this->function;
+  }
+
+  public function setRawArguments(array $arguments) {
+    $this->rawArguments = $arguments;
+    $this->unconsumedArguments = $arguments;
+  }
+
+  public function addArgument(PhabricatorChartFunctionArgument $spec) {
+    $name = $spec->getName();
+    if (!strlen($name)) {
+      throw new Exception(
+        pht(
+          'Chart function "%s" emitted an argument specification with no '.
+          'argument name. Argument specifications must have unique names.',
+          $this->getFunctionArgumentSignature()));
+    }
+
+    $type = $spec->getType();
+    if (!strlen($type)) {
+      throw new Exception(
+        pht(
+          'Chart function "%s" emitted an argument specification ("%s") with '.
+          'no type. Each argument specification must have a valid type.',
+          $name));
+    }
+
+    if (isset($this->argumentMap[$name])) {
+      throw new Exception(
+        pht(
+          'Chart function "%s" emitted multiple argument specifications '.
+          'with the same name ("%s"). Each argument specification must have '.
+          'a unique name.',
+          $this->getFunctionArgumentSignature(),
+          $name));
+    }
+
+    $this->argumentMap[$name] = $spec;
+    $this->unparsedArguments[] = $spec;
+
+    return $this;
+  }
+
+  public function parseArgument(
+    PhabricatorChartFunctionArgument $spec) {
+    $this->addArgument($spec);
+    return $this->parseArguments();
+  }
+
+  public function setHaveAllArguments($have_all) {
+    $this->haveAllArguments = $have_all;
+    return $this;
+  }
+
+  public function parseArguments() {
+    $have_count = count($this->rawArguments);
+    $want_count = count($this->argumentMap);
+
+    if ($this->haveAllArguments) {
+      if ($want_count !== $have_count) {
+        throw new Exception(
+          pht(
+            'Function "%s" expects %s argument(s), but %s argument(s) were '.
+            'provided.',
+            $this->getFunctionArgumentSignature(),
+            $want_count,
+            $have_count));
+      }
+    }
+
+    while ($this->unparsedArguments) {
+      $argument = array_shift($this->unparsedArguments);
+      $name = $argument->getName();
+
+      if (!$this->unconsumedArguments) {
+        throw new Exception(
+          pht(
+            'Function "%s" expects at least %s argument(s), but only %s '.
+            'argument(s) were provided.',
+            $this->getFunctionArgumentSignature(),
+            $want_count,
+            $have_count));
+      }
+
+      $raw_argument = array_shift($this->unconsumedArguments);
+      $this->argumentPosition++;
+
+      try {
+        $value = $argument->newValue($raw_argument);
+      } catch (Exception $ex) {
+        throw new Exception(
+          pht(
+            'Argument "%s" (in position "%s") to function "%s" is '.
+            'invalid: %s',
+            $name,
+            $this->argumentPosition,
+            $this->getFunctionArgumentSignature(),
+            $ex->getMessage()));
+      }
+
+      $this->argumentValues[$name] = $value;
+    }
+  }
+
+  public function getArgumentValue($key) {
+    if (!array_key_exists($key, $this->argumentValues)) {
+      throw new Exception(
+        pht(
+          'Function "%s" is requesting an argument ("%s") that it did '.
+          'not define.',
+          $this->getFunctionArgumentSignature(),
+          $key));
+    }
+
+    return $this->argumentValues[$key];
+  }
+
+  private function getFunctionArgumentSignature() {
+    $argument_list = array();
+    foreach ($this->argumentMap as $key => $spec) {
+      $argument_list[] = $key;
+    }
+
+    if (!$this->haveAllArguments) {
+      $argument_list[] = '...';
+    }
+
+    return sprintf(
+      '%s(%s)',
+      $this->getFunction()->getFunctionKey(),
+      implode(', ', $argument_list));
+  }
+
+}

src/applications/fact/chart/PhabricatorConstantChartFunction.php

@@ -7,36 +7,26 @@ final class PhabricatorConstantChartFunction
 
   private $value;
 
-  protected function newArguments(array $arguments) {
+  protected function newArguments() {
-    if (count($arguments) !== 1) {
+    return array(
-      throw new Exception(
+      $this->newArgument()
-        pht(
+        ->setName('n')
-          'Chart function "constant(...)" expects one argument, got %s. '.
+        ->setType('number'),
-          'Pass a constant.',
+    );
-          count($arguments)));
-    }
-
-    if (!is_int($arguments[0])) {
-      throw new Exception(
-        pht(
-          'First argument for "fact(...)" is invalid: expected int, '.
-          'got %s.',
-          phutil_describe_type($arguments[0])));
-    }
-
-    $this->value = $arguments[0];
   }
 
   public function getDatapoints(PhabricatorChartDataQuery $query) {
     $x_min = $query->getMinimumValue();
     $x_max = $query->getMaximumValue();
 
+    $value = $this->getArgument('n');
+
     $points = array();
     $steps = $this->newLinearSteps($x_min, $x_max, 2);
     foreach ($steps as $step) {
       $points[] = array(
         'x' => $step,
-        'y' => $this->value,
+        'y' => $value,
       );
     }
 

src/applications/fact/chart/PhabricatorFactChartFunction.php

@@ -5,40 +5,21 @@ final class PhabricatorFactChartFunction
 
   const FUNCTIONKEY = 'fact';
 
-  private $factKey;
   private $fact;
   private $datapoints;
 
-  protected function newArguments(array $arguments) {
+  protected function newArguments() {
-    if (count($arguments) !== 1) {
+    $key_argument = $this->newArgument()
-      throw new Exception(
+      ->setName('fact-key')
-        pht(
+      ->setType('fact-key');
-          'Chart function "fact(...)" expects one argument, got %s. '.
-          'Pass the key for a fact.',
-          count($arguments)));
-    }
 
-    if (!is_string($arguments[0])) {
+    $parser = $this->getArgumentParser();
-      throw new Exception(
+    $parser->parseArgument($key_argument);
-        pht(
-          'First argument for "fact(...)" is invalid: expected string, '.
-          'got %s.',
-          phutil_describe_type($arguments[0])));
-    }
 
-    $facts = PhabricatorFact::getAllFacts();
+    $fact = $this->getArgument('fact-key');
-    $fact = idx($facts, $arguments[0]);
-
-    if (!$fact) {
-      throw new Exception(
-        pht(
-          'Argument to "fact(...)" is invalid: "%s" is not a known fact '.
-          'key.',
-          $arguments[0]));
-    }
-
-    $this->factKey = $arguments[0];
     $this->fact = $fact;
+
+    return $fact->getFunctionArguments();
   }
 
   public function loadData() {

src/applications/fact/chart/PhabricatorSinChartFunction.php

@@ -5,30 +5,20 @@ final class PhabricatorSinChartFunction
 
   const FUNCTIONKEY = 'sin';
 
-  private $argument;
+  protected function newArguments() {
-
+    return array(
-  protected function newArguments(array $arguments) {
+      $this->newArgument()
-    if (count($arguments) !== 1) {
+        ->setName('x')
-      throw new Exception(
+        ->setType('function'),
-        pht(
+    );
-          'Chart function "sin(..)" expects one argument, got %s.',
-          count($arguments)));
   }
 
-    $argument = $arguments[0];
+  protected function assignArguments(array $arguments) {
-
+    $this->argument = $arguments[0];
-    if (!($argument instanceof PhabricatorChartFunction)) {
-      throw new Exception(
-        pht(
-          'Argument to chart function should be a function, got %s.',
-          phutil_describe_type($argument)));
-    }
-
-    $this->argument = $argument;
   }
 
   public function getDatapoints(PhabricatorChartDataQuery $query) {
-    $points = $this->argument->getDatapoints($query);
+    $points = $this->getArgument('x')->getDatapoints($query);
 
     foreach ($points as $key => $point) {
       $points[$key]['y'] = sin(deg2rad($points[$key]['y']));

src/applications/fact/chart/PhabricatorXChartFunction.php

@@ -5,13 +5,8 @@ final class PhabricatorXChartFunction
 
   const FUNCTIONKEY = 'x';
 
-  protected function newArguments(array $arguments) {
+  protected function newArguments() {
-    if (count($arguments) !== 0) {
+    return array();
-      throw new Exception(
-        pht(
-          'Chart function "x()" expects zero arguments, got %s.',
-          count($arguments)));
-    }
   }
 
   public function getDatapoints(PhabricatorChartDataQuery $query) {

src/applications/fact/controller/PhabricatorFactChartController.php

@@ -23,6 +23,9 @@ final class PhabricatorFactChartController extends PhabricatorFactController {
     $x_function = id(new PhabricatorXChartFunction())
       ->setArguments(array());
 
+    $functions[] = id(new PhabricatorConstantChartFunction())
+      ->setArguments(array(360));
+
     $functions[] = id(new PhabricatorSinChartFunction())
       ->setArguments(array($x_function));
 

src/applications/fact/fact/PhabricatorFact.php

@@ -37,4 +37,8 @@ abstract class PhabricatorFact extends Phobject {
 
   abstract protected function newTemplateDatapoint();
 
+  final public function getFunctionArguments() {
+    return array();
+  }
+
 }