From a1581eca87325299e9acb8cf02e1b4e358eb1cc7 Mon Sep 17 00:00:00 2001 From: Igor Canadi Date: Mon, 10 Aug 2015 13:58:55 -0700 Subject: [PATCH] Modernize RocksDB linters Summary: This was motivated by t7518166. checkCpp crashes on db_test.cc because the file is too big :( Couple of changes: * Added clang-format linter. Now we can catch all code that is not formatted correctly. * Added Howtoeven in our list of linters * Replaced cpplint with flint * Removed checkCpp lint. Nobody ownes it and it doesn't work on db_test.cc Test Plan: Made a random lint error and `arc lint`. Saw an error. Reviewers: yhchiang, kradhakrishnan, anthony, sdong Reviewed By: sdong Subscribers: dhruba, leveldb Differential Revision: https://reviews.facebook.net/D41949 --- arcanist_util/__phutil_library_map__.php | 10 +- .../config/FacebookArcanistConfiguration.php | 3 + .../BaseDirectoryScopedFormatLinter.php | 74 ++++++ .../cpp_linter/FacebookHowtoevenLinter.php | 223 ++++++++++++++++++ .../cpp_linter/FbcodeClangFormatLinter.php | 52 ++++ arcanist_util/cpp_linter/FbcodeCppLinter.php | 94 +++++--- arcanist_util/cpp_linter/PfffCppLinter.php | 68 ------ .../lint_engine/FacebookFbcodeLintEngine.php | 17 +- .../FacebookHowtoevenLintEngine.php | 27 +++ .../FacebookFbcodeUnitTestEngine.php | 3 + 10 files changed, 458 insertions(+), 113 deletions(-) create mode 100644 arcanist_util/cpp_linter/BaseDirectoryScopedFormatLinter.php create mode 100644 arcanist_util/cpp_linter/FacebookHowtoevenLinter.php create mode 100644 arcanist_util/cpp_linter/FbcodeClangFormatLinter.php delete mode 100644 arcanist_util/cpp_linter/PfffCppLinter.php create mode 100644 arcanist_util/lint_engine/FacebookHowtoevenLintEngine.php diff --git a/arcanist_util/__phutil_library_map__.php b/arcanist_util/__phutil_library_map__.php index 99b2b71e9..274ad16e3 100644 --- a/arcanist_util/__phutil_library_map__.php +++ b/arcanist_util/__phutil_library_map__.php @@ -11,11 +11,14 @@ phutil_register_library_map(array( 'class' => array( 'ArcanistCpplintLinter' => 'cpp_linter/ArcanistCpplintLinter.php', + 'BaseDirectoryScopedFormatLinter' => 'cpp_linter/BaseDirectoryScopedFormatLinter.php', 'FacebookArcanistConfiguration' => 'config/FacebookArcanistConfiguration.php', 'FacebookFbcodeLintEngine' => 'lint_engine/FacebookFbcodeLintEngine.php', 'FacebookFbcodeUnitTestEngine' => 'unit_engine/FacebookFbcodeUnitTestEngine.php', + 'FacebookHowtoevenLintEngine' => 'lint_engine/FacebookHowtoevenLintEngine.php', + 'FacebookHowtoevenLinter' => 'cpp_linter/FacebookHowtoevenLinter.php', + 'FbcodeClangFormatLinter' => 'cpp_linter/FbcodeClangFormatLinter.php', 'FbcodeCppLinter' => 'cpp_linter/FbcodeCppLinter.php', - 'PfffCppLinter' => 'cpp_linter/PfffCppLinter.php', ), 'function' => array( @@ -23,10 +26,13 @@ phutil_register_library_map(array( 'xmap' => array( 'ArcanistCpplintLinter' => 'ArcanistLinter', + 'BaseDirectoryScopedFormatLinter' => 'ArcanistLinter', 'FacebookArcanistConfiguration' => 'ArcanistConfiguration', 'FacebookFbcodeLintEngine' => 'ArcanistLintEngine', 'FacebookFbcodeUnitTestEngine' => 'ArcanistBaseUnitTestEngine', + 'FacebookHowtoevenLintEngine' => 'ArcanistLintEngine', + 'FacebookHowtoevenLinter' => 'ArcanistLinter', + 'FbcodeClangFormatLinter' => 'BaseDirectoryScopedFormatLinter', 'FbcodeCppLinter' => 'ArcanistLinter', - 'PfffCppLinter' => 'ArcanistLinter', ), )); diff --git a/arcanist_util/config/FacebookArcanistConfiguration.php b/arcanist_util/config/FacebookArcanistConfiguration.php index 10c5767fa..c3454903b 100644 --- a/arcanist_util/config/FacebookArcanistConfiguration.php +++ b/arcanist_util/config/FacebookArcanistConfiguration.php @@ -1,5 +1,8 @@ getPathsToLint() as $p) { + // check if $path starts with $p + if (strncmp($path, $p, strlen($p)) === 0) { + return true; + } + } + return false; + } + + // API to tell this linter which lines were changed + final public function setPathChangedLines($path, $changed) { + $this->changedLines[$path] = $changed; + } + + final public function willLintPaths(array $paths) { + $futures = array(); + foreach ($paths as $path) { + if (!$this->shouldLintPath($path)) { + continue; + } + + $changed = $this->changedLines[$path]; + if (!isset($changed)) { + // do not run linter if there are no changes + continue; + } + + $futures[$path] = $this->getFormatFuture($path, $changed); + } + + foreach (Futures($futures)->limit(8) as $p => $f) { + $this->rawLintOutput[$p] = $f->resolvex(); + } + } + + abstract protected function getFormatFuture($path, array $changed); + abstract protected function getLintMessage($diff); + + final public function lintPath($path) { + if (!isset($this->rawLintOutput[$path])) { + return; + } + + list($new_content) = $this->rawLintOutput[$path]; + $old_content = $this->getData($path); + + if ($new_content != $old_content) { + $diff = ArcanistDiffUtils::renderDifferences($old_content, $new_content); + $this->raiseLintAtOffset( + 0, + self::LINT_FORMATTING, + $this->getLintMessage($diff), + $old_content, + $new_content); + } + } + +} diff --git a/arcanist_util/cpp_linter/FacebookHowtoevenLinter.php b/arcanist_util/cpp_linter/FacebookHowtoevenLinter.php new file mode 100644 index 000000000..6edb114b6 --- /dev/null +++ b/arcanist_util/cpp_linter/FacebookHowtoevenLinter.php @@ -0,0 +1,223 @@ +localExecx("rm -rf _build/_lint"); + + // Build compilation database. + $lintable_paths = $this->getLintablePaths($paths); + $interesting_paths = $this->getInterestingPaths($lintable_paths); + + if (!$lintable_paths) { + return; + } + + // Run lint. + try { + $this->localExecx( + "%C %C -p _build/dev/ %Ls", + $this->getBinaryPath(), + $this->getFilteredIssues(), + $lintable_paths); + } catch (CommandException $exception) { + PhutilConsole::getConsole()->writeErr($exception->getMessage()); + } + + // Load results. + $result = id( + new SQLite3( + $this->getProjectRoot().'/_build/_lint/lint.db', + SQLITE3_OPEN_READONLY)) + ->query("SELECT * FROM raised_issues"); + + while ($issue = $result->fetchArray(SQLITE3_ASSOC)) { + // Skip issues not part of the linted file. + if (in_array($issue['file'], $interesting_paths)) { + $this->addLintMessage(id(new ArcanistLintMessage()) + ->setPath($issue['file']) + ->setLine($issue['line']) + ->setChar($issue['column']) + ->setCode('Howtoeven') + ->setSeverity($this->getSeverity($issue['severity'])) + ->setName('Hte-'.$issue['name']) + ->setDescription( + sprintf( + "%s\n\n%s", + ($issue['message']) ? $issue['message'] : $issue['description'], + $issue['explanation'])) + ->setOriginalText(idx($issue, 'original', '')) + ->setReplacementText(idx($issue, 'replacement', ''))); + } + } + } + + public function lintPath($path) { + } + + /** + * Get the paths that we know how to lint. + * + * The strategy is to first look whether there's an existing compilation + * database and use that if it's exhaustive. We generate our own only if + * necessary. + */ + private function getLintablePaths($paths) { + // Replace headers with existing sources. + for ($i = 0; $i < count($paths); $i++) { + if (preg_match("/\.h$/", $paths[$i])) { + $header = preg_replace("/\.h$/", ".cpp", $paths[$i]); + if (file_exists($header)) { + $paths[$i] = $header; + } + } + } + + // Check if database exists and is exhaustive. + $available_paths = $this->getAvailablePaths(); + $lintable_paths = array_intersect($paths, $available_paths); + if ($paths === $lintable_paths) { + return $lintable_paths; + } + + // Generate our own database. + $targets = $this->getTargetsFor($paths); + if (!$targets) { + PhutilConsole::getConsole()->writeErr( + "No build targets found for %s\n", + implode(', ', $paths)); + return array(); + } + + $this->localExecx("./tools/build/bin/fbconfig.par -r %Ls", $targets); + $this->localExecx("./tools/build/bin/fbmake.par gen_cdb"); + + $available_paths = $this->getAvailablePaths(); + $lintable_paths = array_intersect($paths, $available_paths); + if ($paths != $lintable_paths) { + PhutilConsole::getConsole()->writeErr( + "Can't lint %s\n", + implode(', ', array_diff($paths, $available_paths))); + } + + // Return what we know how to lint. + return $lintable_paths; + } + + /** + * Get the available paths in the current compilation database. + */ + private function getAvailablePaths() { + $database_path = $this->getProjectRoot() + .'/_build/dev/compile_commands.json'; + if (!file_exists($database_path)) { + return array(); + } + + $entries = json_decode(file_get_contents($database_path), true); + $paths = array(); + foreach ($entries as $entry) { + $paths[] = $entry['file']; + } + return $paths; + } + + /** + * Search for the targets directories for the given files. + */ + private static function getTargetsFor($paths) { + $targets = array(); + foreach ($paths as $path) { + while (($path = dirname($path)) !== '.') { + if (in_array('TARGETS', scandir($path))) { + $contents = file_get_contents($path.'/TARGETS'); + if (strpos($contents, 'cpp_binary') !== false) { + $targets[] = $path; + break; + } + } + } + } + return array_unique($targets); + } + + /** + * The paths that we actually want to report on. + */ + private function getInterestingPaths($paths) { + $headers = array(); + foreach ($paths as $path) { + $headers[] = preg_replace("/\.cpp$/", ".h", $path); + } + return array_merge($paths, $headers); + } + + /** + * The path where the binary is located. Will return the current dewey binary + * unless the `HOWTOEVEN_BUILD` environment variable is set. + */ + private function getBinaryPath() { + $path = sprintf( + "/mnt/dewey/fbcode/.commits/%s/builds/howtoeven/client", + self::VERSION); + + $build = getenv('HOWTOEVEN_BUILD'); + if ($build) { + $path = sprintf( + "./_build/%s/tools/howtoeven/client", + $build); + if (!file_exists($path)) { + PhutilConsole::getConsole()->writeErr(">> %s does not exist\n", $path); + exit(1); + } + } + + return $path; + } + + /** + * Execute the command in the root directory. + */ + private function localExecx($command /* , ... */) { + $arguments = func_get_args(); + return newv('ExecFuture', $arguments) + ->setCWD($this->getProjectRoot()) + ->resolvex(); + } + + /** + * The root of the project. + */ + private function getProjectRoot() { + return $this->getEngine()->getWorkingCopy()->getProjectRoot(); + } + + private function getFilteredIssues() { + $issues = getenv('HOWTOEVEN_ISSUES'); + return ($issues) ? csprintf('-issues %s', $issues) : ''; + } + +} diff --git a/arcanist_util/cpp_linter/FbcodeClangFormatLinter.php b/arcanist_util/cpp_linter/FbcodeClangFormatLinter.php new file mode 100644 index 000000000..8b5d86475 --- /dev/null +++ b/arcanist_util/cpp_linter/FbcodeClangFormatLinter.php @@ -0,0 +1,52 @@ + ArcanistLintSeverity::SEVERITY_ADVICE, + ); + } + + public function getLintNameMap() { + return array( + self::LINT_FORMATTING => pht('Changes are not clang-formatted'), + ); + } + + protected function getFormatFuture($path, array $changed) { + $args = ""; + foreach ($changed as $key => $value) { + $args .= " --lines=$key:$key"; + } + + return new ExecFuture( + "%s %s $args", + self::CLANG_FORMAT_BINARY, + $this->getEngine()->getFilePathOnDisk($path)); + } + + protected function getLintMessage($diff) { + $link_to_clang_format = + "[[ http://fburl.com/clang-format | clang-format ]]"; + return <<getEngine()->getFilePathOnDisk($p); - $lpath_file = file($lpath); - if (preg_match('/\.(c)$/', $lpath) || - preg_match('/-\*-.*Mode: C[; ].*-\*-/', $lpath_file[0]) || - preg_match('/vim(:.*)*:\s*(set\s+)?filetype=c\s*:/', $lpath_file[0]) - ) { - $futures[$p] = new ExecFuture("%s %s %s 2>&1", - $CPP_LINT, self::C_FLAG, - $this->getEngine()->getFilePathOnDisk($p)); - } else { - $futures[$p] = new ExecFuture("%s %s 2>&1", - $CPP_LINT, $this->getEngine()->getFilePathOnDisk($p)); - } + foreach ($paths as $p) { + $lpath = $this->getEngine()->getFilePathOnDisk($p); + $lpath_file = file($lpath); + if (preg_match('/\.(c)$/', $lpath) || + preg_match('/-\*-.*Mode: C[; ].*-\*-/', $lpath_file[0]) || + preg_match('/vim(:.*)*:\s*(set\s+)?filetype=c\s*:/', $lpath_file[0]) + ) { + $futures[$p] = new ExecFuture("%s %s %s 2>&1", + self::FLINT, self::C_FLAG, + $this->getEngine()->getFilePathOnDisk($p)); + } else { + $futures[$p] = new ExecFuture("%s %s 2>&1", + self::FLINT, $this->getEngine()->getFilePathOnDisk($p)); } + } - foreach (Futures($futures)->limit(8) as $p => $f) { - $this->rawLintOutput[$p] = $f->resolvex(); - } + foreach (Futures($futures)->limit(8) as $p => $f) { + $this->rawLintOutput[$p] = $f->resolvex(); } + return; } @@ -47,44 +40,70 @@ class FbcodeCppLinter extends ArcanistLinter { } public function lintPath($path) { + $this->runCppLint($path); + } + + private function runCppLint($path) { $msgs = $this->getCppLintOutput($path); foreach ($msgs as $m) { $this->raiseLintAtLine($m['line'], 0, $m['severity'], $m['msg']); } } + private function adviseOnEachPattern( + $path, + $regex, + $message, + $lint_type = self::LINT_ADVICE, + $match_idx = 0) { + $file_data = $this->getData($path); + $matches = array(); + if (!preg_match_all($regex, $file_data, $matches, PREG_OFFSET_CAPTURE)) { + return; + } + + foreach ($matches[$match_idx] as $match) { + list($match_str, $offset) = $match; + $this->raiseLintAtOffset($offset, $lint_type, $message, $match_str); + } + } + public function getLintSeverityMap() { return array( self::LINT_WARNING => ArcanistLintSeverity::SEVERITY_WARNING, + self::LINT_ADVICE => ArcanistLintSeverity::SEVERITY_ADVICE, self::LINT_ERROR => ArcanistLintSeverity::SEVERITY_ERROR ); } public function getLintNameMap() { return array( - self::LINT_WARNING => "CppLint Warning", - self::LINT_ERROR => "CppLint Error" + self::LINT_ADVICE => "CppLint Advice", + self::LINT_WARNING => "CppLint Warning", + self::LINT_ERROR => "CppLint Error" ); } private function getCppLintOutput($path) { - if (!array_key_exists($path, $this->rawLintOutput)) { - return array(); - } list($output) = $this->rawLintOutput[$path]; $msgs = array(); $current = null; + $matches = array(); foreach (explode("\n", $output) as $line) { - if (preg_match('/[^:]*\((\d+)\):(.*)$/', $line, $matches)) { + if (preg_match('/.*?:(\d+):(.*)/', $line, $matches)) { if ($current) { $msgs[] = $current; } $line = $matches[1]; $text = $matches[2]; - $sev = preg_match('/.*Warning.*/', $text) - ? self::LINT_WARNING - : self::LINT_ERROR; + if (preg_match('/.*Warning.*/', $text)) { + $sev = self::LINT_WARNING; + } else if (preg_match('/.*Advice.*/', $text)) { + $sev = self::LINT_ADVICE; + } else { + $sev = self::LINT_ERROR; + } $current = array('line' => $line, 'msg' => $text, 'severity' => $sev); @@ -99,4 +118,3 @@ class FbcodeCppLinter extends ArcanistLinter { return $msgs; } } - diff --git a/arcanist_util/cpp_linter/PfffCppLinter.php b/arcanist_util/cpp_linter/PfffCppLinter.php deleted file mode 100644 index 67366143c..000000000 --- a/arcanist_util/cpp_linter/PfffCppLinter.php +++ /dev/null @@ -1,68 +0,0 @@ -&1", - $program, $this->getEngine()->getFilePathOnDisk($p)); - } - foreach (Futures($futures)->limit(8) as $p => $f) { - - list($stdout, $stderr) = $f->resolvex(); - $raw = json_decode($stdout, true); - if (!is_array($raw)) { - throw new Exception( - "checkCpp returned invalid JSON!". - "Stdout: {$stdout} Stderr: {$stderr}" - ); - } - foreach($raw as $err) { - $this->addLintMessage( - ArcanistLintMessage::newFromDictionary( - array( - 'path' => $err['file'], - 'line' => $err['line'], - 'char' => 0, - 'name' => $err['name'], - 'description' => $err['info'], - 'code' => $this->getLinterName(), - 'severity' => ArcanistLintSeverity::SEVERITY_WARNING, - ) - ) - ); - } - } - } - return; - } - - public function lintPath($path) { - return; - } -} diff --git a/arcanist_util/lint_engine/FacebookFbcodeLintEngine.php b/arcanist_util/lint_engine/FacebookFbcodeLintEngine.php index 6b10b8c06..7b12cccdd 100644 --- a/arcanist_util/lint_engine/FacebookFbcodeLintEngine.php +++ b/arcanist_util/lint_engine/FacebookFbcodeLintEngine.php @@ -1,5 +1,8 @@ getCommitHookMode()) { $cpp_linters = array(); - $google_linter = new ArcanistCpplintLinter(); - $cpp_linters[] = $linters[] = $google_linter; + $cpp_linters[] = $linters[] = new ArcanistCpplintLinter(); $cpp_linters[] = $linters[] = new FbcodeCppLinter(); - $cpp_linters[] = $linters[] = new PfffCppLinter(); + + $clang_format_linter = new FbcodeClangFormatLinter(); + $linters[] = $clang_format_linter; } $spelling_linter = new ArcanistSpellingLinter(); @@ -93,6 +95,11 @@ class FacebookFbcodeLintEngine extends ArcanistLintEngine { $linter->addPath($path); $linter->addData($path, $this->loadData($path)); } + + $clang_format_linter->addPath($path); + $clang_format_linter->addData($path, $this->loadData($path)); + $clang_format_linter->setPathChangedLines( + $path, $this->getPathChangedLines($path)); } // Match *.py and contbuild config files diff --git a/arcanist_util/lint_engine/FacebookHowtoevenLintEngine.php b/arcanist_util/lint_engine/FacebookHowtoevenLintEngine.php new file mode 100644 index 000000000..2e0148141 --- /dev/null +++ b/arcanist_util/lint_engine/FacebookHowtoevenLintEngine.php @@ -0,0 +1,27 @@ +getPaths() as $path) { + // Don't try to lint deleted files or changed directories. + if (!Filesystem::pathExists($path) || is_dir($path)) { + continue; + } + + if (preg_match('/\.(cpp|c|cc|cxx|h|hh|hpp|hxx|tcc)$/', $path)) { + $paths[] = $path; + } + } + + $howtoeven = new FacebookHowtoevenLinter(); + $howtoeven->setPaths($paths); + return array($howtoeven); + } +} diff --git a/arcanist_util/unit_engine/FacebookFbcodeUnitTestEngine.php b/arcanist_util/unit_engine/FacebookFbcodeUnitTestEngine.php index 3639f11fc..f9a9e70e5 100644 --- a/arcanist_util/unit_engine/FacebookFbcodeUnitTestEngine.php +++ b/arcanist_util/unit_engine/FacebookFbcodeUnitTestEngine.php @@ -1,5 +1,8 @@