diff --git a/src/lint/ArcanistLintMessage.php b/src/lint/ArcanistLintMessage.php index eb575f58..7be1d52d 100644 --- a/src/lint/ArcanistLintMessage.php +++ b/src/lint/ArcanistLintMessage.php @@ -1,348 +1,356 @@ setPath($dict['path']); $message->setLine($dict['line']); $message->setChar($dict['char']); $message->setCode($dict['code']); $message->setSeverity($dict['severity']); $message->setName($dict['name']); $message->setDescription($dict['description']); if (isset($dict['original'])) { $message->setOriginalText($dict['original']); } if (isset($dict['replacement'])) { $message->setReplacementText($dict['replacement']); } $message->setGranularity(idx($dict, 'granularity')); $message->setOtherLocations(idx($dict, 'locations', array())); if (isset($dict['bypassChangedLineFiltering'])) { $message->setBypassChangedLineFiltering( $dict['bypassChangedLineFiltering']); } return $message; } public function toDictionary() { return array( 'path' => $this->getPath(), 'line' => $this->getLine(), 'char' => $this->getChar(), 'code' => $this->getCode(), 'severity' => $this->getSeverity(), 'name' => $this->getName(), 'description' => $this->getDescription(), 'original' => $this->getOriginalText(), 'replacement' => $this->getReplacementText(), 'granularity' => $this->getGranularity(), 'locations' => $this->getOtherLocations(), 'bypassChangedLineFiltering' => $this->shouldBypassChangedLineFiltering(), ); } public function setPath($path) { $this->path = $path; return $this; } public function getPath() { return $this->path; } public function setLine($line) { $this->line = $this->validateInteger($line, 'setLine'); return $this; } public function getLine() { return $this->line; } public function setChar($char) { $this->char = $this->validateInteger($char, 'setChar'); return $this; } public function getChar() { return $this->char; } public function setCode($code) { $code = (string)$code; $maximum_bytes = 128; $actual_bytes = strlen($code); if ($actual_bytes > $maximum_bytes) { throw new Exception( pht( 'Parameter ("%s") passed to "%s" when constructing a lint message '. 'must be a scalar with a maximum string length of %s bytes, but is '. '%s bytes in length.', $code, 'setCode()', new PhutilNumber($maximum_bytes), new PhutilNumber($actual_bytes))); } $this->code = $code; return $this; } public function getCode() { return $this->code; } public function setSeverity($severity) { $this->severity = $severity; return $this; } public function getSeverity() { return $this->severity; } public function setName($name) { $this->name = $name; return $this; } public function getName() { return $this->name; } public function setDescription($description) { $this->description = $description; return $this; } public function getDescription() { return $this->description; } public function setOriginalText($original) { $this->originalText = $original; return $this; } public function getOriginalText() { return $this->originalText; } public function setReplacementText($replacement) { $this->replacementText = $replacement; return $this; } public function getReplacementText() { return $this->replacementText; } /** * @param dict Keys 'path', 'line', 'char', 'original'. */ public function setOtherLocations(array $locations) { assert_instances_of($locations, 'array'); $this->otherLocations = $locations; return $this; } public function getOtherLocations() { return $this->otherLocations; } public function isError() { return $this->getSeverity() == ArcanistLintSeverity::SEVERITY_ERROR; } public function isWarning() { return $this->getSeverity() == ArcanistLintSeverity::SEVERITY_WARNING; } public function isAutofix() { return $this->getSeverity() == ArcanistLintSeverity::SEVERITY_AUTOFIX; } public function hasFileContext() { return ($this->getLine() !== null); } public function setObsolete($obsolete) { $this->obsolete = $obsolete; return $this; } public function getObsolete() { return $this->obsolete; } public function isPatchable() { return ($this->getReplacementText() !== null) && ($this->getReplacementText() !== $this->getOriginalText()); } public function didApplyPatch() { if ($this->appliedToDisk) { return $this; } $this->appliedToDisk = true; foreach ($this->dependentMessages as $message) { $message->didApplyPatch(); } return $this; } public function isPatchApplied() { return $this->appliedToDisk; } public function setGranularity($granularity) { $this->granularity = $granularity; return $this; } public function getGranularity() { return $this->granularity; } public function setDependentMessages(array $messages) { assert_instances_of($messages, __CLASS__); $this->dependentMessages = $messages; return $this; } public function setBypassChangedLineFiltering($bypass_changed_lines) { $this->bypassChangedLineFiltering = $bypass_changed_lines; return $this; } public function shouldBypassChangedLineFiltering() { return $this->bypassChangedLineFiltering; } /** * Validate an integer-like value, returning a strict integer. * * Further on, the pipeline is strict about types. We want to be a little * less strict in linters themselves, since they often parse command line * output or XML and will end up with string representations of numbers. * * @param mixed Integer or digit string. * @return int Integer. */ private function validateInteger($value, $caller) { if ($value === null) { // This just means that we don't have any information. return null; } // Strings like "234" are fine, coerce them to integers. if (is_string($value) && preg_match('/^\d+\z/', $value)) { $value = (int)$value; } if (!is_int($value)) { throw new Exception( pht( 'Parameter passed to "%s" must be an integer.', $caller.'()')); } return $value; } public function newTrimmedMessage() { if (!$this->isPatchable()) { return clone $this; } // If the original and replacement text have a similar prefix or suffix, // we trim it to reduce the size of the diff we show to the user. $replacement = $this->getReplacementText(); $original = $this->getOriginalText(); $replacement_length = strlen($replacement); $original_length = strlen($original); $minimum_length = min($original_length, $replacement_length); $prefix_length = 0; for ($ii = 0; $ii < $minimum_length; $ii++) { if ($original[$ii] !== $replacement[$ii]) { break; } $prefix_length++; } // NOTE: The two strings can't be the same because the message won't be // "patchable" if they are, so we don't need a special check for the case // where the entire string is a shared prefix. + // However, if the two strings are in the form "ABC" and "ABBC", we may + // find a prefix and a suffix with a combined length greater than the + // total size of the smaller string if we don't limit the search. + $max_suffix = ($minimum_length - $prefix_length); + $suffix_length = 0; - for ($ii = 1; $ii <= $minimum_length; $ii++) { + for ($ii = 1; $ii <= $max_suffix; $ii++) { $original_char = $original[$original_length - $ii]; $replacement_char = $replacement[$replacement_length - $ii]; if ($original_char !== $replacement_char) { break; } $suffix_length++; } if ($suffix_length) { $original = substr($original, 0, -$suffix_length); $replacement = substr($replacement, 0, -$suffix_length); } $line = $this->getLine(); $char = $this->getChar(); if ($prefix_length) { $prefix = substr($original, 0, $prefix_length); - $original = substr($original, $prefix_length); - $replacement = substr($replacement, $prefix_length); + // NOTE: Prior to PHP7, `substr("a", 1)` returned false instead of + // the empty string. Cast these to force the PHP7-ish behavior we + // expect. + $original = (string)substr($original, $prefix_length); + $replacement = (string)substr($replacement, $prefix_length); // If we've removed a prefix, we need to push the character and line // number for the warning forward to account for the characters we threw // away. for ($ii = 0; $ii < $prefix_length; $ii++) { $char++; if ($prefix[$ii] == "\n") { $line++; $char = 1; } } } return id(clone $this) ->setOriginalText($original) ->setReplacementText($replacement) ->setLine($line) ->setChar($char); } } diff --git a/src/lint/__tests__/ArcanistLintMessageTestCase.php b/src/lint/__tests__/ArcanistLintMessageTestCase.php index ae02b2b6..c27acd1b 100644 --- a/src/lint/__tests__/ArcanistLintMessageTestCase.php +++ b/src/lint/__tests__/ArcanistLintMessageTestCase.php @@ -1,80 +1,88 @@ array( 'old' => 'a', 'new' => 'b', 'old.expect' => 'a', 'new.expect' => 'b', 'line' => 1, 'char' => 1, ), 'prefix' => array( 'old' => 'ever after', 'new' => 'evermore', 'old.expect' => ' after', 'new.expect' => 'more', 'line' => 1, 'char' => 5, ), 'suffix' => array( 'old' => 'arcane archaeology', 'new' => 'mythic archaeology', 'old.expect' => 'arcane', 'new.expect' => 'mythic', 'line' => 1, 'char' => 1, ), 'both' => array( 'old' => 'large red apple', 'new' => 'large blue apple', 'old.expect' => 'red', 'new.expect' => 'blue', 'line' => 1, 'char' => 7, ), 'prefix-newline' => array( 'old' => "four score\nand five years ago", 'new' => "four score\nand seven years ago", 'old.expect' => 'five', 'new.expect' => 'seven', 'line' => 2, 'char' => 5, ), + 'mid-newline' => array( + 'old' => 'ABA', + 'new' => 'ABBA', + 'old.expect' => '', + 'new.expect' => 'B', + 'line' => 1, + 'char' => 3, + ), ); foreach ($map as $key => $test_case) { $message = id(new ArcanistLintMessage()) ->setOriginalText($test_case['old']) ->setReplacementText($test_case['new']) ->setLine(1) ->setChar(1); $actual = $message->newTrimmedMessage(); $this->assertEqual( $test_case['old.expect'], $actual->getOriginalText(), pht('Original text for "%s".', $key)); $this->assertEqual( $test_case['new.expect'], $actual->getReplacementText(), pht('Replacement text for "%s".', $key)); $this->assertEqual( $test_case['line'], $actual->getLine(), pht('Line for "%s".', $key)); $this->assertEqual( $test_case['char'], $actual->getChar(), pht('Char for "%s".', $key)); } } } diff --git a/src/lint/renderer/ArcanistConsoleLintRenderer.php b/src/lint/renderer/ArcanistConsoleLintRenderer.php index 8880fbe8..12ef4e69 100644 --- a/src/lint/renderer/ArcanistConsoleLintRenderer.php +++ b/src/lint/renderer/ArcanistConsoleLintRenderer.php @@ -1,314 +1,311 @@ showAutofixPatches = $show_autofix_patches; return $this; } public function setTestableMode($testable_mode) { $this->testableMode = $testable_mode; return $this; } public function getTestableMode() { return $this->testableMode; } public function renderLintResult(ArcanistLintResult $result) { $messages = $result->getMessages(); $path = $result->getPath(); $data = $result->getData(); $line_map = $this->newOffsetMap($data); $text = array(); foreach ($messages as $message) { if (!$this->showAutofixPatches && $message->isAutofix()) { continue; } if ($message->isError()) { $color = 'red'; } else { $color = 'yellow'; } $severity = ArcanistLintSeverity::getStringForSeverity( $message->getSeverity()); $code = $message->getCode(); $name = $message->getName(); $description = $message->getDescription(); if ($message->getOtherLocations()) { $locations = array(); foreach ($message->getOtherLocations() as $location) { $locations[] = idx($location, 'path', $path). (!empty($location['line']) ? ":{$location['line']}" : ''); } $description .= "\n".pht( 'Other locations: %s', implode(', ', $locations)); } $text[] = phutil_console_format( " ** %s ** (%s) __%s__\n%s\n", $severity, $code, $name, phutil_console_wrap($description, 4)); if ($message->hasFileContext()) { $text[] = $this->renderContext($message, $data, $line_map); } } if ($text) { $prefix = phutil_console_format( "**>>>** %s\n\n\n", pht( 'Lint for %s:', phutil_console_format('__%s__', $path))); return $prefix.implode("\n", $text); } else { return null; } } protected function renderContext( ArcanistLintMessage $message, $data, array $line_map) { $context = 3; $message = $message->newTrimmedMessage(); $original = $message->getOriginalText(); $replacement = $message->getReplacementText(); $line = $message->getLine(); $char = $message->getChar(); $old = $data; $old_lines = phutil_split_lines($old); $old_impact = substr_count($original, "\n") + 1; $start = $line; if ($message->isPatchable()) { $patch_offset = $line_map[$line] + ($char - 1); $new = substr_replace( $old, $replacement, $patch_offset, strlen($original)); $new_lines = phutil_split_lines($new); // Figure out how many "-" and "+" lines we have by counting the newlines // for the relevant patches. This may overestimate things if we are adding // or removing entire lines, but we'll adjust things below. $new_impact = substr_count($replacement, "\n") + 1; // If this is a change on a single line, we'll try to highlight the // changed character range to make it easier to pick out. if ($old_impact === 1 && $new_impact === 1) { $old_lines[$start - 1] = substr_replace( $old_lines[$start - 1], $this->highlightText($original), $char - 1, strlen($original)); $new_lines[$start - 1] = substr_replace( $new_lines[$start - 1], $this->highlightText($replacement), $char - 1, strlen($replacement)); } // If lines at the beginning of the changed line range are actually the // same, shrink the range. This happens when a patch just adds a line. do { if ($old_lines[$start - 1] != $new_lines[$start - 1]) { break; } $start++; $old_impact--; $new_impact--; if ($old_impact < 0 || $new_impact < 0) { throw new Exception( pht( 'Modified prefix line range has become negative '. '(old = %d, new = %d).', $old_impact, $new_impact)); } } while (true); // If the lines at the end of the changed line range are actually the // same, shrink the range. This happens when a patch just removes a // line. do { $old_suffix = $old_lines[$start + $old_impact - 2]; $new_suffix = $new_lines[$start + $new_impact - 2]; if ($old_suffix != $new_suffix) { break; } $old_impact--; $new_impact--; - if ($old_impact < 0 || $new_impact < 0) { - throw new Exception( - pht( - 'Modified suffix line range has become negative '. - '(old = %d, new = %d).', - $old_impact, - $new_impact)); + // We can end up here if a patch removes a line which occurs after + // another identical line. + if ($old_impact <= 0 || $new_impact <= 0) { + break; } } while (true); } else { // If we have "original" text and it is contained on a single line, // highlight the affected area. If we don't have any text, we'll mark // the character with a caret (below, in rendering) instead. if ($old_impact == 1 && strlen($original)) { $old_lines[$start - 1] = substr_replace( $old_lines[$start - 1], $this->highlightText($original), $char - 1, strlen($original)); } $old_impact = 0; $new_impact = 0; } $out = array(); $head = max(1, $start - $context); for ($ii = $head; $ii < $start; $ii++) { $out[] = array( 'text' => $old_lines[$ii - 1], 'number' => $ii, ); } for ($ii = $start; $ii < $start + $old_impact; $ii++) { $out[] = array( 'text' => $old_lines[$ii - 1], 'number' => $ii, 'type' => '-', 'chevron' => ($ii == $start), ); } for ($ii = $start; $ii < $start + $new_impact; $ii++) { $out[] = array( 'text' => $new_lines[$ii - 1], 'type' => '+', 'chevron' => ($ii == $start), ); } $cursor = $start + $old_impact; $foot = min(count($old_lines), $cursor + $context); for ($ii = $cursor; $ii <= $foot; $ii++) { $out[] = array( 'text' => $old_lines[$ii - 1], 'number' => $ii, 'chevron' => ($ii == $cursor), ); } $result = array(); $seen_chevron = false; foreach ($out as $spec) { if ($seen_chevron) { $chevron = false; } else { $chevron = !empty($spec['chevron']); if ($chevron) { $seen_chevron = true; } } $result[] = $this->renderLine( idx($spec, 'number'), $spec['text'], $chevron, idx($spec, 'type')); // If this is just a message and does not have a patch, put a little // caret underneath the line to point out where the issue is. if ($chevron) { if (!$message->isPatchable() && !strlen($original)) { $result[] = $this->renderCaret($char)."\n"; } } } return implode('', $result); } private function renderCaret($pos) { return str_repeat(' ', 16 + $pos).'^'; } protected function renderLine($line, $data, $chevron = false, $diff = null) { $chevron = $chevron ? '>>>' : ''; return sprintf( ' %3s %1s %6s %s', $chevron, $diff, $line, $data); } public function renderOkayResult() { return phutil_console_format( "** %s ** %s\n", pht('OKAY'), pht('No lint warnings.')); } private function newOffsetMap($data) { $lines = phutil_split_lines($data); $line_map = array(); $number = 1; $offset = 0; foreach ($lines as $line) { $line_map[$number] = $offset; $number++; $offset += strlen($line); } return $line_map; } private function highlightText($text) { if ($this->getTestableMode()) { return '>'.$text.'<'; } else { return (string)tsprintf('##%s##', $text); } } } diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php index 68a35420..f36b30e7 100644 --- a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php +++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php @@ -1,145 +1,191 @@ array( 'line' => 1, 'char' => 1, 'original' => 'a', 'replacement' => 'z', ), 'inline' => array( 'line' => 1, 'char' => 7, 'original' => 'cat', 'replacement' => 'dog', ), // In this test, the original and replacement texts have a large // amount of overlap. 'overlap' => array( 'line' => 1, 'char' => 1, 'original' => 'tantawount', 'replacement' => 'tantamount', ), 'newline' => array( 'line' => 6, 'char' => 1, 'original' => "\n", 'replacement' => '', ), 'addline' => array( 'line' => 3, 'char' => 1, 'original' => '', 'replacement' => "cherry\n", ), 'addlinesuffix' => array( 'line' => 2, 'char' => 7, 'original' => '', 'replacement' => "\ncherry", ), 'xml' => array( 'line' => 3, 'char' => 6, 'original' => '', 'replacement' => "\n", ), 'caret' => array( 'line' => 2, 'char' => 13, 'name' => 'Fruit Misinformation', 'description' => 'Arguably untrue.', ), 'original' => array( 'line' => 1, 'char' => 4, 'original' => 'should of', ), + + 'midline' => array( + 'line' => 1, + 'char' => 1, + 'original' => $midline_original, + 'replacement' => $midline_replacement, + ), + + 'remline' => array( + 'line' => 1, + 'char' => 1, + 'original' => $remline_original, + 'replacement' => $remline_replacement, + ), ); $defaults = array( 'severity' => ArcanistLintSeverity::SEVERITY_WARNING, 'name' => 'Lint Warning', 'path' => 'path/to/example.c', 'description' => 'Consider this.', 'code' => 'WARN123', ); foreach ($map as $key => $test_case) { $data = $this->readTestData("{$key}.txt"); $expect = $this->readTestData("{$key}.expect"); $test_case = $test_case + $defaults; $path = $test_case['path']; $severity = $test_case['severity']; $name = $test_case['name']; $description = $test_case['description']; $code = $test_case['code']; $line = $test_case['line']; $char = $test_case['char']; $original = idx($test_case, 'original'); $replacement = idx($test_case, 'replacement'); $message = id(new ArcanistLintMessage()) ->setPath($path) ->setSeverity($severity) ->setName($name) ->setDescription($description) ->setCode($code) ->setLine($line) ->setChar($char) ->setOriginalText($original) ->setReplacementText($replacement); $result = id(new ArcanistLintResult()) ->setPath($path) ->setData($data) ->addMessage($message); $renderer = id(new ArcanistConsoleLintRenderer()) ->setTestableMode(true); try { PhutilConsoleFormatter::disableANSI(true); $actual = $renderer->renderLintResult($result); PhutilConsoleFormatter::disableANSI(false); } catch (Exception $ex) { PhutilConsoleFormatter::disableANSI(false); throw $ex; } // Trim "~" off the ends of lines. This allows the "expect" file to test // for trailing whitespace without actually containing trailing // whitespace. $expect = preg_replace('/~+$/m', '', $expect); $this->assertEqual( $expect, $actual, pht( 'Lint rendering for "%s".', $key)); } } private function readTestData($filename) { $path = dirname(__FILE__).'/data/'.$filename; return Filesystem::readFile($path); } } diff --git a/src/lint/renderer/__tests__/data/midline.expect b/src/lint/renderer/__tests__/data/midline.expect new file mode 100644 index 00000000..d5f513ea --- /dev/null +++ b/src/lint/renderer/__tests__/data/midline.expect @@ -0,0 +1,11 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + 1 import apple; + 2 import banana; + >>> + ~ + 3 import cat; + 4 import dog; diff --git a/src/lint/renderer/__tests__/data/midline.txt b/src/lint/renderer/__tests__/data/midline.txt new file mode 100644 index 00000000..8639daf5 --- /dev/null +++ b/src/lint/renderer/__tests__/data/midline.txt @@ -0,0 +1,4 @@ +import apple; +import banana; +import cat; +import dog; diff --git a/src/lint/renderer/__tests__/data/remline.expect b/src/lint/renderer/__tests__/data/remline.expect new file mode 100644 index 00000000..a4d65c95 --- /dev/null +++ b/src/lint/renderer/__tests__/data/remline.expect @@ -0,0 +1,12 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + 1 import apple; + 2 import banana; + 3 ~ + >>> - 4 ~ + 5 import cat; + 6 import dog; diff --git a/src/lint/renderer/__tests__/data/remline.txt b/src/lint/renderer/__tests__/data/remline.txt new file mode 100644 index 00000000..4e526abe --- /dev/null +++ b/src/lint/renderer/__tests__/data/remline.txt @@ -0,0 +1,6 @@ +import apple; +import banana; + + +import cat; +import dog; diff --git a/src/workflow/ArcanistVersionWorkflow.php b/src/workflow/ArcanistVersionWorkflow.php index 807c5732..9c470fc6 100644 --- a/src/workflow/ArcanistVersionWorkflow.php +++ b/src/workflow/ArcanistVersionWorkflow.php @@ -1,65 +1,71 @@ dirname(phutil_get_library_root('arcanist')), 'libphutil' => dirname(phutil_get_library_root('phutil')), ); foreach ($roots as $lib => $root) { $working_copy = ArcanistWorkingCopyIdentity::newFromPath($root); $configuration_manager = clone $this->getConfigurationManager(); $configuration_manager->setWorkingCopyIdentity($working_copy); $repository = ArcanistRepositoryAPI::newAPIFromConfigurationManager( $configuration_manager); if (!Filesystem::pathExists($repository->getMetadataPath())) { throw new ArcanistUsageException( pht('%s is not a git working copy.', $lib)); } - list($stdout) = $repository->execxLocal('log -1 --format=%s', '%H %ct'); - list($commit, $timestamp) = explode(' ', $stdout); + // NOTE: Carefully execute these commands in a way that works on Windows + // until T8298 is properly fixed. See PHI52. + + list($commit) = $repository->execxLocal('log -1 --format=%%H'); + $commit = trim($commit); + + list($timestamp) = $repository->execxLocal('log -1 --format=%%ct'); + $timestamp = trim($timestamp); $console->writeOut( "%s %s (%s)\n", $lib, $commit, date('j M Y', (int)$timestamp)); } } }