revi-archive/phorge-arcanist
1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2025-01-01 10:20:58 +01:00
Code Issues Releases Wiki Activity

(stable) Promote 2017 Week 35

Browse source
This commit is contained in:
epriestley 2017-09-01 13:58:22 -07:00
commit 17515c94b2
24 changed files with 629 additions and 138 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
src/__phutil_library_map__.php
View file

@ -87,6 +87,7 @@ phutil_register_library_map(array(
'ArcanistConfigurationDrivenUnitTestEngine' => 'unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php',
'ArcanistConfigurationManager' => 'configuration/ArcanistConfigurationManager.php',
'ArcanistConsoleLintRenderer' => 'lint/renderer/ArcanistConsoleLintRenderer.php',
'ArcanistConsoleLintRendererTestCase' => 'lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php',
'ArcanistConstructorParenthesesXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistConstructorParenthesesXHPASTLinterRule.php',
'ArcanistConstructorParenthesesXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistConstructorParenthesesXHPASTLinterRuleTestCase.php',
'ArcanistControlStatementSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistControlStatementSpacingXHPASTLinterRule.php',
@ -221,6 +222,7 @@ phutil_register_library_map(array(
'ArcanistLibraryTestCase' => '__tests__/ArcanistLibraryTestCase.php',
'ArcanistLintEngine' => 'lint/engine/ArcanistLintEngine.php',
'ArcanistLintMessage' => 'lint/ArcanistLintMessage.php',
'ArcanistLintMessageTestCase' => 'lint/__tests__/ArcanistLintMessageTestCase.php',
'ArcanistLintPatcher' => 'lint/ArcanistLintPatcher.php',
'ArcanistLintRenderer' => 'lint/renderer/ArcanistLintRenderer.php',
'ArcanistLintResult' => 'lint/ArcanistLintResult.php',
@ -502,6 +504,7 @@ phutil_register_library_map(array(
'ArcanistConfigurationDrivenUnitTestEngine' => 'ArcanistUnitTestEngine',
'ArcanistConfigurationManager' => 'Phobject',
'ArcanistConsoleLintRenderer' => 'ArcanistLintRenderer',
'ArcanistConsoleLintRendererTestCase' => 'PhutilTestCase',
'ArcanistConstructorParenthesesXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
'ArcanistConstructorParenthesesXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
'ArcanistControlStatementSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
@ -636,6 +639,7 @@ phutil_register_library_map(array(
'ArcanistLibraryTestCase' => 'PhutilLibraryTestCase',
'ArcanistLintEngine' => 'Phobject',
'ArcanistLintMessage' => 'Phobject',
'ArcanistLintMessageTestCase' => 'PhutilTestCase',
'ArcanistLintPatcher' => 'Phobject',
'ArcanistLintRenderer' => 'Phobject',
'ArcanistLintResult' => 'Phobject',

71
src/lint/ArcanistLintMessage.php
View file

@ -274,4 +274,75 @@ final class ArcanistLintMessage extends Phobject {
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.
$suffix_length = 0;
for ($ii = 1; $ii <= $minimum_length; $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);
// 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);
}
}

80
src/lint/__tests__/ArcanistLintMessageTestCase.php Normal file
View file

@ -0,0 +1,80 @@
<?php
final class ArcanistLintMessageTestCase
extends PhutilTestCase {
public function testMessageTrimming() {
$map = array(
'simple' => 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,
),
);
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));
}
}
}

4
src/lint/linter/__tests__/xml/attr2.lint-test
View file

File diff suppressed because one or more lines are too long

334
src/lint/renderer/ArcanistConsoleLintRenderer.php
View file

@ -6,20 +6,30 @@
final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
private $showAutofixPatches = false;
private $testableMode;
public function setShowAutofixPatches($show_autofix_patches) {
$this->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();
$lines = explode("\n", $result->getData());
$line_map = $this->newOffsetMap($data);
$text = array();
foreach ($messages as $message) {
if (!$this->showAutofixPatches && $message->isAutofix()) {
continue;
@ -57,7 +67,7 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
phutil_console_wrap($description, 4));
if ($message->hasFileContext()) {
$text[] = $this->renderContext($message, $lines);
$text[] = $this->renderContext($message, $data, $line_map);
}
}
@ -75,153 +85,185 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
protected function renderContext(
ArcanistLintMessage $message,
array $line_data) {
$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));
}
} 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;
}
$lines_of_context = 3;
$out = array();
$num_lines = count($line_data);
// make line numbers line up with array indexes
array_unshift($line_data, '');
$line_num = min($message->getLine(), $num_lines);
$line_num = max(1, $line_num);
// Print out preceding context before the impacted region.
$cursor = max(1, $line_num - $lines_of_context);
for (; $cursor < $line_num; $cursor++) {
$out[] = $this->renderLine($cursor, $line_data[$cursor]);
$head = max(1, $start - $context);
for ($ii = $head; $ii < $start; $ii++) {
$out[] = array(
'text' => $old_lines[$ii - 1],
'number' => $ii,
);
}
$text = $message->getOriginalText();
$start = $message->getChar() - 1;
$patch = '';
// Refine original and replacement text to eliminate start and end in common
if ($message->isPatchable()) {
$patch = $message->getReplacementText();
$text_strlen = strlen($text);
$patch_strlen = strlen($patch);
$min_length = min($text_strlen, $patch_strlen);
$same_at_front = 0;
for ($ii = 0; $ii < $min_length; $ii++) {
if ($text[$ii] !== $patch[$ii]) {
break;
}
$same_at_front++;
$start++;
if ($text[$ii] == "\n") {
$out[] = $this->renderLine($cursor, $line_data[$cursor]);
$cursor++;
$start = 0;
$line_num++;
}
}
// deal with shorter string ' ' longer string ' a '
$min_length -= $same_at_front;
// And check the end of the string
$same_at_end = 0;
for ($ii = 1; $ii <= $min_length; $ii++) {
if ($text[$text_strlen - $ii] !== $patch[$patch_strlen - $ii]) {
break;
}
$same_at_end++;
}
$text = substr(
$text,
$same_at_front,
$text_strlen - $same_at_end - $same_at_front);
$patch = substr(
$patch,
$same_at_front,
$patch_strlen - $same_at_end - $same_at_front);
}
// Print out the impacted region itself.
$diff = $message->isPatchable() ? '-' : null;
$text_lines = explode("\n", $text);
$text_length = count($text_lines);
$intraline = ($text != '' || $start || !preg_match('/\n$/', $patch));
if ($intraline) {
for (; $cursor < $line_num + $text_length; $cursor++) {
$chevron = ($cursor == $line_num);
// We may not have any data if, e.g., the old file does not exist.
$data = idx($line_data, $cursor, null);
// Highlight the problem substring.
$text_line = $text_lines[$cursor - $line_num];
if (strlen($text_line)) {
$data = substr_replace(
$data,
phutil_console_format('##%s##', $text_line),
($cursor == $line_num ? ($start > 0 ? $start : null) : 0),
strlen($text_line));
}
$out[] = $this->renderLine($cursor, $data, $chevron, $diff);
}
for ($ii = $start; $ii < $start + $old_impact; $ii++) {
$out[] = array(
'text' => $old_lines[$ii - 1],
'number' => $ii,
'type' => '-',
'chevron' => ($ii == $start),
);
}
// Print out replacement text.
if ($message->isPatchable()) {
// Strip trailing newlines, since "explode" will create an extra patch
// line for these.
if (strlen($patch) && ($patch[strlen($patch) - 1] === "\n")) {
$patch = substr($patch, 0, -1);
}
$patch_lines = explode("\n", $patch);
$patch_length = count($patch_lines);
$patch_line = $patch_lines[0];
$len = isset($text_lines[0]) ? strlen($text_lines[0]) : 0;
$patched = phutil_console_format('##%s##', $patch_line);
if ($intraline) {
$patched = substr_replace(
$line_data[$line_num],
$patched,
$start,
$len);
}
$out[] = $this->renderLine(null, $patched, false, '+');
foreach (array_slice($patch_lines, 1) as $patch_line) {
$out[] = $this->renderLine(
null,
phutil_console_format('##%s##', $patch_line), false, '+');
}
for ($ii = $start; $ii < $start + $new_impact; $ii++) {
$out[] = array(
'text' => $new_lines[$ii - 1],
'type' => '+',
'chevron' => ($ii == $start),
);
}
$end = min($num_lines, $cursor + $lines_of_context);
for (; $cursor < $end; $cursor++) {
// If there is no original text, we didn't print out a chevron or any
// highlighted text above, so print it out here. This allows messages
// which don't have any original/replacement information to still
// render with indicator chevrons.
if ($text || $message->isPatchable()) {
$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 = ($cursor == $line_num);
$chevron = !empty($spec['chevron']);
if ($chevron) {
$seen_chevron = true;
}
}
$out[] = $this->renderLine($cursor, $line_data[$cursor], $chevron);
// With original text, we'll render the text highlighted above. If the
// lint message only has a line/char offset there's nothing to
// highlight, so print out a caret on the next line instead.
if ($chevron && $message->getChar()) {
$out[] = $this->renderCaret($message->getChar());
$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";
}
}
}
$out[] = null;
return implode("\n", $out);
return implode('', $result);
}
private function renderCaret($pos) {
@ -245,4 +287,28 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
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);
}
}
}

145
src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php Normal file
View file

@ -0,0 +1,145 @@
<?php
final class ArcanistConsoleLintRendererTestCase
extends PhutilTestCase {
public function testRendering() {
$map = array(
'simple' => 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',
),
);
$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);
}
}

12
src/lint/renderer/__tests__/data/addline.expect Normal file
View file

@ -0,0 +1,12 @@
>>> Lint for path/to/example.c:
Warning (WARN123) Lint Warning
Consider this.
1 apple
2 banana
>>> + cherry
3 date
4 eclaire
5 fig

5
src/lint/renderer/__tests__/data/addline.txt Normal file
View file

@ -0,0 +1,5 @@
apple
banana
date
eclaire
fig

12
src/lint/renderer/__tests__/data/addlinesuffix.expect Normal file
View file

@ -0,0 +1,12 @@
>>> Lint for path/to/example.c:
Warning (WARN123) Lint Warning
Consider this.
1 apple
2 banana
>>> + cherry
3 date
4 eclaire
5 fig

5
src/lint/renderer/__tests__/data/addlinesuffix.txt Normal file
View file

@ -0,0 +1,5 @@
apple
banana
date
eclaire
fig

11
src/lint/renderer/__tests__/data/caret.expect Normal file
View file

@ -0,0 +1,11 @@
>>> Lint for path/to/example.c:
Warning (WARN123) Fruit Misinformation
Arguably untrue.
1 Apples are round.
>>> 2 Bananas are round.
^
3 Cherries are round.
4 Dates are round.

4
src/lint/renderer/__tests__/data/caret.txt Normal file
View file

@ -0,0 +1,4 @@
Apples are round.
Bananas are round.
Cherries are round.
Dates are round.

8
src/lint/renderer/__tests__/data/inline.expect Normal file
View file

@ -0,0 +1,8 @@
>>> Lint for path/to/example.c:
Warning (WARN123) Lint Warning
Consider this.
>>> - 1 adjudi>cat<ed
+ adjudi>dog<ed

1
src/lint/renderer/__tests__/data/inline.txt Normal file
View file

@ -0,0 +1 @@
adjudicated

14
src/lint/renderer/__tests__/data/newline.expect Normal file
View file

@ -0,0 +1,14 @@
>>> Lint for path/to/example.c:
Warning (WARN123) Lint Warning
Consider this.
3 ccc
4 ddd
5 eee
>>> - 6 ~
7 fff
8 ggg
9 hhh
10 iii

11
src/lint/renderer/__tests__/data/newline.txt Normal file
View file

@ -0,0 +1,11 @@
aaa
bbb
ccc
ddd
eee
fff
ggg
hhh
iii
jjj

7
src/lint/renderer/__tests__/data/original.expect Normal file
View file

@ -0,0 +1,7 @@
>>> Lint for path/to/example.c:
Warning (WARN123) Lint Warning
Consider this.
>>> 1 He >should of< known.

1
src/lint/renderer/__tests__/data/original.txt Normal file
View file

@ -0,0 +1 @@
He should of known.

8
src/lint/renderer/__tests__/data/overlap.expect Normal file
View file

@ -0,0 +1,8 @@
>>> Lint for path/to/example.c:
Warning (WARN123) Lint Warning
Consider this.
>>> - 1 tanta>w<ount
+ tanta>m<ount

1
src/lint/renderer/__tests__/data/overlap.txt Normal file
View file

@ -0,0 +1 @@
tantawount

10
src/lint/renderer/__tests__/data/simple.expect Normal file
View file

@ -0,0 +1,10 @@
>>> Lint for path/to/example.c:
Warning (WARN123) Lint Warning
Consider this.
>>> - 1 >a<
+ >z<
2 b
3 c

3
src/lint/renderer/__tests__/data/simple.txt Normal file
View file

@ -0,0 +1,3 @@
a
b
c

12
src/lint/renderer/__tests__/data/xml.expect Normal file
View file

@ -0,0 +1,12 @@
>>> Lint for path/to/example.c:
Warning (WARN123) Lint Warning
Consider this.
1 <
2 wow
>>> - 3 xml>
+ xml
+ >
4 <xml />

4
src/lint/renderer/__tests__/data/xml.txt Normal file
View file

@ -0,0 +1,4 @@
<
wow
xml>
<xml />