Generate PHP function documentation in Diviner

Summary:
Ref T988. Various improvements:

  - Generate function documentation, mostly correctly.
  - Raise some warnings about bad documentation.
  - Allow `.book` files to exclude paths from generation.
  - Add a book for technical docs.
  - Exclude "ghosts" from common queries (atoms which used to exist, but no longer do, but which we want to keep the PHIDs around for in case they come back later).

This is a bit rough still, but puts us much closer to being able to get rid of the old Diviner.

Test Plan: See screenshots.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T988

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

src/__celerity_resource_map__.php

@@ -1147,6 +1147,15 @@ celerity_register_resource_map(array(
     ),
     'disk' => '/rsrc/css/application/diffusion/diffusion-source.css',
   ),
+  'diviner-shared-css' =>
+  array(
+    'uri' => '/res/f462d51d/rsrc/css/diviner/diviner-shared.css',
+    'type' => 'css',
+    'requires' =>
+    array(
+    ),
+    'disk' => '/rsrc/css/diviner/diviner-shared.css',
+  ),
   'global-drag-and-drop-css' =>
   array(
     'uri' => '/res/4e24cb65/rsrc/css/application/files/global-drag-and-drop.css',

src/__phutil_library_map__.php

@@ -534,10 +534,13 @@ phutil_register_library_map(array(
     'DivinerLiveSymbol' => 'applications/diviner/storage/DivinerLiveSymbol.php',
     'DivinerPHIDTypeAtom' => 'applications/diviner/phid/DivinerPHIDTypeAtom.php',
     'DivinerPHIDTypeBook' => 'applications/diviner/phid/DivinerPHIDTypeBook.php',
+    'DivinerPHPAtomizer' => 'applications/diviner/atomizer/DivinerPHPAtomizer.php',
+    'DivinerParameterTableView' => 'applications/diviner/view/DivinerParameterTableView.php',
     'DivinerPublishCache' => 'applications/diviner/cache/DivinerPublishCache.php',
     'DivinerPublisher' => 'applications/diviner/publisher/DivinerPublisher.php',
     'DivinerRemarkupRuleSymbol' => 'applications/diviner/markup/DivinerRemarkupRuleSymbol.php',
     'DivinerRenderer' => 'applications/diviner/renderer/DivinerRenderer.php',
+    'DivinerReturnTableView' => 'applications/diviner/view/DivinerReturnTableView.php',
     'DivinerStaticPublisher' => 'applications/diviner/publisher/DivinerStaticPublisher.php',
     'DivinerWorkflow' => 'applications/diviner/workflow/DivinerWorkflow.php',
     'DoorkeeperBridge' => 'applications/doorkeeper/bridge/DoorkeeperBridge.php',
@@ -2559,8 +2562,11 @@ phutil_register_library_map(array(
     ),
     'DivinerPHIDTypeAtom' => 'PhabricatorPHIDType',
     'DivinerPHIDTypeBook' => 'PhabricatorPHIDType',
+    'DivinerPHPAtomizer' => 'DivinerAtomizer',
+    'DivinerParameterTableView' => 'AphrontTagView',
     'DivinerPublishCache' => 'DivinerDiskCache',
     'DivinerRemarkupRuleSymbol' => 'PhutilRemarkupRule',
+    'DivinerReturnTableView' => 'AphrontTagView',
     'DivinerStaticPublisher' => 'DivinerPublisher',
     'DivinerWorkflow' => 'PhutilArgumentWorkflow',
     'DoorkeeperBridge' => 'Phobject',

src/applications/diviner/atom/DivinerAtom.php

@@ -23,6 +23,7 @@ final class DivinerAtom {
   private $extends = array();
   private $links = array();
   private $book;
+  private $properties = array();
 
   /**
    * Returns a sorting key which imposes an unambiguous, stable order on atoms.
@@ -59,7 +60,7 @@ final class DivinerAtom {
   }
 
   public static function getAtomSerializationVersion() {
-    return 1;
+    return 2;
   }
 
   public function addWarning($warning) {
@@ -194,6 +195,7 @@ final class DivinerAtom {
       $this->getLanguage(),
       $this->getContentRaw(),
       $this->getDocblockRaw(),
+      $this->getProperties(),
       mpull($this->extends, 'toHash'),
       mpull($this->links, 'toHash'),
     );
@@ -280,6 +282,7 @@ final class DivinerAtom {
       'extends'     => $this->getExtendsDictionaries(),
       'links'       => $this->getLinkDictionaries(),
       'ref'         => $this->getRef()->toDictionary(),
+      'properties'  => $this->getProperties(),
     );
   }
 
@@ -312,7 +315,8 @@ final class DivinerAtom {
       ->setContext(idx($dictionary, 'context'))
       ->setLanguage(idx($dictionary, 'language'))
       ->setParentHash(idx($dictionary, 'parentHash'))
-      ->setDocblockRaw(idx($dictionary, 'docblockRaw'));
+      ->setDocblockRaw(idx($dictionary, 'docblockRaw'))
+      ->setProperties(idx($dictionary, 'properties'));
 
     foreach (idx($dictionary, 'warnings', array()) as $warning) {
       $atom->addWarning($warning);
@@ -325,4 +329,37 @@ final class DivinerAtom {
     return $atom;
   }
 
+  public function getProperty($key, $default = null) {
+    return idx($this->properties, $key, $default);
+  }
+
+  public function setProperty($key, $value) {
+    $this->properties[$key] = $value;
+  }
+
+  public function getProperties() {
+    return $this->properties;
+  }
+
+  public function setProperties(array $properties) {
+    $this->properties = $properties;
+    return $this;
+  }
+
+  public static function getThisAtomIsNotDocumentedString($type) {
+    switch ($type) {
+      case 'function':
+        return pht('This function is not documented.');
+      case 'class':
+        return pht('This class is not documented.');
+      case 'article':
+        return pht('This article is not documented.');
+      case 'method':
+        return pht('This method is not documented.');
+      default:
+        phlog("Need translation for '{$type}'.");
+        return pht('This %s is not documented.', $type);
+    }
+  }
+
 }

src/applications/diviner/atomizer/DivinerPHPAtomizer.php

@@ -0,0 +1,229 @@
+<?php
+
+final class DivinerPHPAtomizer extends DivinerAtomizer {
+
+  protected function executeAtomize($file_name, $file_data) {
+    $future = xhpast_get_parser_future($file_data);
+    $tree = XHPASTTree::newFromDataAndResolvedExecFuture(
+      $file_data,
+      $future->resolve());
+
+    $atoms = array();
+
+    $root = $tree->getRootNode();
+
+    $func_decl = $root->selectDescendantsOfType('n_FUNCTION_DECLARATION');
+    foreach ($func_decl as $func) {
+
+      $name = $func->getChildByIndex(2);
+
+      $atom = id(new DivinerAtom())
+        ->setType('function')
+        ->setName($name->getConcreteString())
+        ->setLine($func->getLineNumber())
+        ->setFile($file_name);
+
+      $this->findAtomDocblock($atom, $func);
+
+      $this->parseParams($atom, $func);
+      $this->parseReturnType($atom, $func);
+
+      $atoms[] = $atom;
+    }
+
+    return $atoms;
+  }
+
+  private function parseParams(DivinerAtom $atom, AASTNode $func) {
+    $params = $func
+      ->getChildByIndex(3, 'n_DECLARATAION_PARAMETER_LIST')
+      ->selectDescendantsOfType('n_DECLARATION_PARAMETER');
+
+    $param_spec = array();
+
+    if ($atom->getDocblockRaw()) {
+      $metadata = $atom->getDocblockMeta();
+    } else {
+      $metadata = array();
+    }
+
+    $docs = idx($metadata, 'param');
+    if ($docs) {
+      $docs = explode("\n", $docs);
+      $docs = array_filter($docs);
+    } else {
+      $docs = array();
+    }
+
+    if (count($docs)) {
+      if (count($docs) < count($params)) {
+        $atom->addWarning(
+          pht(
+            'This call takes %d parameters, but only %d are documented.',
+            count($params),
+            count($docs)));
+      }
+    }
+
+    foreach ($params as $param) {
+      $name = $param->getChildByIndex(1)->getConcreteString();
+      $dict = array(
+        'type'    => $param->getChildByIndex(0)->getConcreteString(),
+        'default' => $param->getChildByIndex(2)->getConcreteString(),
+      );
+
+      if ($docs) {
+        $doc = array_shift($docs);
+        if ($doc) {
+          $dict += $this->parseParamDoc($atom, $doc, $name);
+        }
+      }
+
+      $param_spec[] = array(
+        'name' => $name,
+      ) + $dict;
+    }
+
+    if ($docs) {
+      foreach ($docs as $doc) {
+        if ($doc) {
+          $param_spec[] = $this->parseParamDoc($atom, $doc, null);
+        }
+      }
+    }
+
+    // TODO: Find `assert_instances_of()` calls in the function body and
+    // add their type information here. See T1089.
+
+    $atom->setProperty('parameters', $param_spec);
+  }
+
+
+  private function findAtomDocblock(DivinerAtom $atom, XHPASTNode $node) {
+    $token = $node->getDocblockToken();
+    if ($token) {
+      $atom->setDocblockRaw($token->getValue());
+      return true;
+    } else {
+      $tokens = $node->getTokens();
+      if ($tokens) {
+        $prev = head($tokens);
+        while ($prev = $prev->getPrevToken()) {
+          if ($prev->isAnyWhitespace()) {
+            continue;
+          }
+          break;
+        }
+
+        if ($prev && $prev->isComment()) {
+          $value = $prev->getValue();
+          $matches = null;
+          if (preg_match('/@(return|param|task|author)/', $value, $matches)) {
+            $atom->addWarning(
+              pht(
+                'Atom "%s" is preceded by a comment containing "@%s", but the '.
+                'comment is not a documentation comment. Documentation '.
+                'comments must begin with "/**", followed by a newline. Did '.
+                'you mean to use a documentation comment? (As the comment is '.
+                'not a documentation comment, it will be ignored.)',
+                $atom->getName(),
+                $matches[1]));
+          }
+        }
+      }
+
+      $atom->setDocblockRaw('');
+      return false;
+    }
+  }
+
+  protected function parseParamDoc(DivinerAtom $atom, $doc, $name) {
+    $dict = array();
+    $split = preg_split('/\s+/', trim($doc), $limit = 2);
+    if (!empty($split[0])) {
+      $dict['doctype'] = $split[0];
+    }
+
+    if (!empty($split[1])) {
+      $docs = $split[1];
+
+      // If the parameter is documented like "@param int $num Blah blah ..",
+      // get rid of the `$num` part (which Diviner considers optional). If it
+      // is present and different from the declared name, raise a warning.
+      $matches = null;
+      if (preg_match('/^(\\$\S+)\s+/', $docs, $matches)) {
+        if ($name !== null) {
+          if ($matches[1] !== $name) {
+            $atom->addWarning(
+              pht(
+                'Parameter "%s" is named "%s" in the documentation. The '.
+                'documentation may be out of date.',
+                $name,
+                $matches[1]));
+          }
+        }
+        $docs = substr($docs, strlen($matches[0]));
+      }
+
+      $dict['docs'] = $docs;
+    }
+
+    return $dict;
+  }
+
+  private function parseReturnType(DivinerAtom $atom, XHPASTNode $decl) {
+    $return_spec = array();
+
+    $metadata = $atom->getDocblockMeta();
+    $return = idx($metadata, 'return');
+
+    if (!$return) {
+      $return = idx($metadata, 'returns');
+      if ($return) {
+        $atom->addWarning(
+          pht('Documentation uses `@returns`, but should use `@return`.'));
+      }
+    }
+
+    if ($atom->getName() == '__construct' && $atom->getType() == 'method') {
+      $return_spec = array(
+        'doctype' => 'this',
+        'docs' => '//Implicit.//',
+      );
+
+      if ($return) {
+        $atom->addWarning(
+          'Method __construct() has explicitly documented @return. The '.
+          '__construct() method always returns $this. Diviner documents '.
+          'this implicitly.');
+      }
+    } else if ($return) {
+      $split = preg_split('/\s+/', trim($return), $limit = 2);
+      if (!empty($split[0])) {
+        $type = $split[0];
+      }
+
+      if ($decl->getChildByIndex(1)->getTypeName() == 'n_REFERENCE') {
+        $type = $type.' &';
+      }
+
+      $docs = null;
+      if (!empty($split[1])) {
+        $docs = $split[1];
+      }
+
+      $return_spec = array(
+        'doctype' => $type,
+        'docs'    => $docs,
+      );
+    } else {
+      $return_spec = array(
+        'type' => 'wild',
+      );
+    }
+
+    $atom->setProperty('return', $return_spec);
+  }
+
+}
+

src/applications/diviner/controller/DivinerAtomController.php

@@ -33,6 +33,11 @@ final class DivinerAtomController extends DivinerController {
       return new Aphront404Response();
     }
 
+    // TODO: This query won't load ghosts, because they'll fail `needAtoms()`.
+    // Instead, we might want to load ghosts and render a message like
+    // "this thing existed in an older version, but no longer does", especially
+    // if we add content like comments.
+
     $symbol = id(new DivinerAtomQuery())
       ->setViewer($viewer)
       ->withBookPHIDs(array($book->getPHID()))
@@ -85,6 +90,14 @@ final class DivinerAtomController extends DivinerController {
       pht('Defined'),
       $atom->getFile().':'.$atom->getLine());
 
+    $warnings = $atom->getWarnings();
+    if ($warnings) {
+      $warnings = id(new AphrontErrorView())
+        ->setErrors($warnings)
+        ->setTitle(pht('Documentation Warnings'))
+        ->setSeverity(AphrontErrorView::SEVERITY_WARNING);
+    }
+
     $field = 'default';
     $engine = id(new PhabricatorMarkupEngine())
       ->setViewer($viewer)
@@ -93,6 +106,23 @@ final class DivinerAtomController extends DivinerController {
 
     $content = $engine->getOutput($symbol, $field);
 
+    if (strlen(trim($symbol->getMarkupText($field)))) {
+      $content = phutil_tag(
+        'div',
+        array(
+          'class' => 'phabricator-remarkup',
+        ),
+        array(
+          $content,
+        ));
+    } else {
+      $undoc = DivinerAtom::getThisAtomIsNotDocumentedString($atom->getType());
+      $content = id(new AphrontErrorView())
+        ->appendChild($undoc)
+        ->setSeverity(AphrontErrorView::SEVERITY_NODATA);
+    }
+
+
     $toc = $engine->getEngineMetadata(
       $symbol,
       $field,
@@ -103,15 +133,29 @@ final class DivinerAtomController extends DivinerController {
       ->setBook($book->getTitle(), $group_name)
       ->setHeader($header)
       ->appendChild($properties)
-      ->appendChild(
-        phutil_tag(
-          'div',
-          array(
-            'class' => 'phabricator-remarkup',
-          ),
-          array(
-            $content,
-          )));
+      ->appendChild($warnings)
+      ->appendChild($content);
+
+    $parameters = $atom->getProperty('parameters');
+    if ($parameters !== null) {
+      $document->appendChild(
+        id(new PhabricatorHeaderView())
+          ->setHeader(pht('Parameters')));
+
+      $document->appendChild(
+        id(new DivinerParameterTableView())
+          ->setParameters($parameters));
+    }
+
+    $return = $atom->getProperty('return');
+    if ($return !== null) {
+      $document->appendChild(
+        id(new PhabricatorHeaderView())
+          ->setHeader(pht('Return')));
+      $document->appendChild(
+        id(new DivinerReturnTableView())
+          ->setReturn($return));
+    }
 
     if ($toc) {
       $side = new PHUIListView();

src/applications/diviner/markup/DivinerRemarkupRuleSymbol.php

@@ -114,6 +114,17 @@ final class DivinerRemarkupRuleSymbol extends PhutilRemarkupRule {
             ));
       }
 
+      // TODO: This probably is not the best place to do this. Move it somewhere
+      // better when it becomes more clear where it should actually go.
+      if ($ref) {
+        switch ($ref->getType()) {
+          case 'function':
+          case 'method':
+            $title = $title.'()';
+            break;
+        }
+      }
+
       if ($this->getEngine()->isTextMode()) {
         if ($href) {
           $link = $title.' <'.PhabricatorEnv::getProductionURI($href).'>';

src/applications/diviner/publisher/DivinerLivePublisher.php

@@ -34,6 +34,7 @@ final class DivinerLivePublisher extends DivinerPublisher {
       ->withContexts(array($atom->getContext()))
       ->withIndexes(array($this->getAtomSimilarIndex($atom)))
       ->withIncludeUndocumentable(true)
+      ->withIncludeGhosts(true)
       ->executeOne();
 
     if ($symbol) {

src/applications/diviner/query/DivinerAtomQuery.php

@@ -11,6 +11,7 @@ final class DivinerAtomQuery
   private $contexts;
   private $indexes;
   private $includeUndocumentable;
+  private $includeGhosts;
 
   private $needAtoms;
 
@@ -54,6 +55,29 @@ final class DivinerAtomQuery
     return $this;
   }
 
+  /**
+   * Include "ghosts", which are symbols which used to exist but do not exist
+   * currently (for example, a function which existed in an older version of
+   * the codebase but was deleted).
+   *
+   * These symbols had PHIDs assigned to them, and may have other sorts of
+   * metadata that we don't want to lose (like comments or flags), so we don't
+   * delete them outright. They might also come back in the future: the change
+   * which deleted the symbol might be reverted, or the documentation might
+   * have been generated incorrectly by accident. In these cases, we can
+   * restore the original data.
+   *
+   * However, most callers are not interested in these symbols, so they are
+   * excluded by default. You can use this method to include them in results.
+   *
+   * @param bool  True to include ghosts.
+   * @return this
+   */
+  public function withIncludeGhosts($include) {
+    $this->includeGhosts = $include;
+    return $this;
+  }
+
   public function withIncludeUndocumentable($include) {
     $this->includeUndocumentable = $include;
     return $this;
@@ -190,6 +214,12 @@ final class DivinerAtomQuery
         'isDocumentable = 1');
     }
 
+    if (!$this->includeGhosts) {
+      $where[] = qsprintf(
+        $conn_r,
+        'graphHash IS NOT NULL');
+    }
+
     $where[] = $this->buildPagingClause($conn_r);
 
     return $this->formatWhereClause($where);

src/applications/diviner/view/DivinerParameterTableView.php

@@ -0,0 +1,64 @@
+<?php
+
+final class DivinerParameterTableView extends AphrontTagView {
+
+  private $parameters;
+
+  public function setParameters(array $parameters) {
+    $this->parameters = $parameters;
+    return $this;
+  }
+
+  public function getTagName() {
+    return 'table';
+  }
+
+  public function getTagAttributes() {
+    return array(
+      'class' => 'diviner-parameter-table-view',
+    );
+  }
+
+  public function getTagContent() {
+    require_celerity_resource('diviner-shared-css');
+
+    $rows = array();
+    foreach ($this->parameters as $param) {
+      $cells = array();
+
+      $type = idx($param, 'doctype');
+      if (!$type) {
+        $type = idx($param, 'type');
+      }
+
+      $name = idx($param, 'name');
+      $docs = idx($param, 'docs');
+
+      $cells[] = phutil_tag(
+        'td',
+        array(
+          'class' => 'diviner-parameter-table-type diviner-monospace',
+        ),
+        $type);
+
+      $cells[] = phutil_tag(
+        'td',
+        array(
+          'class' => 'diviner-parameter-table-name diviner-monospace',
+        ),
+        $name);
+
+      $cells[] = phutil_tag(
+        'td',
+        array(
+          'class' => 'diviner-parameter-table-docs',
+        ),
+        $docs);
+
+      $rows[] = phutil_tag('tr', array(), $cells);
+    }
+
+    return $rows;
+  }
+
+}

src/applications/diviner/view/DivinerReturnTableView.php

@@ -0,0 +1,53 @@
+<?php
+
+final class DivinerReturnTableView extends AphrontTagView {
+
+  private $return;
+
+  public function setReturn(array $return) {
+    $this->return = $return;
+    return $this;
+  }
+
+  public function getTagName() {
+    return 'table';
+  }
+
+  public function getTagAttributes() {
+    return array(
+      'class' => 'diviner-return-table-view',
+    );
+  }
+
+  public function getTagContent() {
+    require_celerity_resource('diviner-shared-css');
+
+    $return = $this->return;
+
+    $type = idx($return, 'doctype');
+    if (!$type) {
+      $type = idx($return, 'type');
+    }
+
+    $docs = idx($return, 'docs');
+
+    $cells = array();
+
+    $cells[] = phutil_tag(
+      'td',
+      array(
+        'class' => 'diviner-return-table-type diviner-monospace',
+      ),
+      $type);
+
+    $cells[] = phutil_tag(
+      'td',
+      array(
+        'class' => 'diviner-return-table-docs',
+      ),
+      $docs);
+
+    return phutil_tag('tr', array(), $cells);
+  }
+
+}

src/applications/diviner/workflow/DivinerGenerateWorkflow.php

@@ -170,10 +170,17 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
 
   private function getAtomizersForFiles(array $files) {
     $rules = $this->getRules();
+    $exclude = $this->getExclude();
 
     $atomizers = array();
 
     foreach ($files as $file) {
+      foreach ($exclude as $pattern) {
+        if (preg_match($pattern, $file)) {
+          continue 2;
+        }
+      }
+
       foreach ($rules as $rule => $atomizer) {
         $ok = preg_match($rule, $file);
         if ($ok === false) {
@@ -191,9 +198,32 @@ final class DivinerGenerateWorkflow extends DivinerWorkflow {
   }
 
   private function getRules() {
-    return $this->getConfig('rules', array()) + array(
+    $rules = $this->getConfig('rules', array(
       '/\\.diviner$/' => 'DivinerArticleAtomizer',
-    );
+      '/\\.php$/' => 'DivinerPHPAtomizer',
+    ));
+
+    foreach ($rules as $rule => $atomizer) {
+      if (@preg_match($rule, '') === false) {
+        throw new Exception(
+          "Rule '{$rule}' is not a valid regular expression!");
+      }
+    }
+
+    return $rules;
+  }
+
+  private function getExclude() {
+    $exclude = $this->getConfig('exclude', array());
+
+    foreach ($exclude as $rule) {
+      if (@preg_match($rule, '') === false) {
+        throw new Exception(
+          "Exclude rule '{$rule}' is not a valid regular expression!");
+      }
+    }
+
+    return $exclude;
   }
 
 

src/docs/book/phabricator.book

@@ -0,0 +1,17 @@
+{
+  "name" : "phabdev",
+  "title" : "Phabricator Technical Documentation",
+  "short" : "Phabricator Tech Docs",
+  "root" : "../../../",
+  "rules" : {
+    "(\\.php$)" : "DivinerPHPAtomizer"
+  },
+  "exclude" : [
+    "(^externals/)",
+    "(^scripts/)",
+    "(^support/)",
+    "(^resources/)"
+  ],
+  "groups" : {
+  }
+}

src/docs/book/user.book

@@ -3,6 +3,14 @@
   "title" : "Phabricator User Documentation",
   "short" : "Phabricator User Docs",
   "root" : "../../../",
+  "rules" : {
+    "(\\.diviner$)" : "DivinerArticleAtomizer"
+  },
+  "exclude" : [
+    "(^externals/)",
+    "(^scripts/)",
+    "(^support/)"
+  ],
   "groups" : {
     "intro" : {
       "name" : "Introduction"

webroot/rsrc/css/diviner/diviner-shared.css

@@ -0,0 +1,38 @@
+/**
+ * @provides diviner-shared-css
+ */
+
+.diviner-monospace {
+  font-family: monospace;
+  font-size: 13px;
+}
+
+.diviner-return-table-view,
+.diviner-parameter-table-view {
+  width: 100%;
+  margin: 0 0 16px;
+  background: #f6f6f6;
+  border-bottom: 1px solid #c0c5d1;
+  width: 100%;
+}
+
+.diviner-return-table-type,
+.diviner-parameter-table-type {
+  padding: 4px 8px 4px 12px;
+  white-space: nowrap;
+  text-align: right;
+  color: #666666;
+  width: 20%;
+}
+
+.diviner-parameter-table-name {
+  padding: 4px 8px;
+  white-space: nowrap;
+  font-weight: bold;
+}
+
+.diviner-return-table-docs,
+.diviner-parameter-table-docs {
+  padding: 4px 12px 4px 8px;
+  width: 80%;
+}