mirror of
https://we.phorge.it/source/arcanist.git
synced 2024-11-24 15:52:40 +01:00
Fix some problems with the module linter that would cause various conflicting
warnings raised in __init__.php to overwrite eachother in uncomfortable ways.
This commit is contained in:
parent
2c235bdf38
commit
efb8219196
10 changed files with 104 additions and 34 deletions
|
@ -68,7 +68,9 @@ abstract class ArcanistLintEngine {
|
||||||
}
|
}
|
||||||
|
|
||||||
public function getFilePathOnDisk($path) {
|
public function getFilePathOnDisk($path) {
|
||||||
return $path;
|
return Filesystem::resolvePath(
|
||||||
|
$path,
|
||||||
|
$this->getWorkingCopy()->getProjectRoot());
|
||||||
}
|
}
|
||||||
|
|
||||||
public function setMinimumSeverity($severity) {
|
public function setMinimumSeverity($severity) {
|
||||||
|
@ -145,12 +147,12 @@ abstract class ArcanistLintEngine {
|
||||||
}
|
}
|
||||||
|
|
||||||
foreach ($this->results as $path => $result) {
|
foreach ($this->results as $path => $result) {
|
||||||
|
$result->setFilePathOnDisk($this->getFilePathOnDisk($path));
|
||||||
if (isset($this->fileData[$path])) {
|
if (isset($this->fileData[$path])) {
|
||||||
// Only set the data if any linter loaded it. The goal here is to
|
// Only set the data if any linter loaded it. The goal here is to
|
||||||
// avoid binaries when we don't actually care about their contents,
|
// avoid binaries when we don't actually care about their contents,
|
||||||
// for performance.
|
// for performance.
|
||||||
$result->setData($this->fileData[$path]);
|
$result->setData($this->fileData[$path]);
|
||||||
$result->setFilePathOnDisk($this->getFilePathOnDisk($path));
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -156,7 +156,12 @@ abstract class ArcanistLinter {
|
||||||
|
|
||||||
$path = $this->getActivePath();
|
$path = $this->getActivePath();
|
||||||
$engine = $this->getEngine();
|
$engine = $this->getEngine();
|
||||||
list($line, $char) = $engine->getLineAndCharFromOffset($path, $offset);
|
if ($offset === null) {
|
||||||
|
$line = null;
|
||||||
|
$char = null;
|
||||||
|
} else {
|
||||||
|
list($line, $char) = $engine->getLineAndCharFromOffset($path, $offset);
|
||||||
|
}
|
||||||
|
|
||||||
return $this->raiseLintAtLine(
|
return $this->raiseLintAtLine(
|
||||||
$line + 1,
|
$line + 1,
|
||||||
|
|
|
@ -136,12 +136,18 @@ class ArcanistPhutilModuleLinter extends ArcanistLinter {
|
||||||
$bin = dirname($arc_root).'/scripts/phutil_analyzer.php';
|
$bin = dirname($arc_root).'/scripts/phutil_analyzer.php';
|
||||||
|
|
||||||
$futures = array();
|
$futures = array();
|
||||||
foreach ($modules as $key) {
|
foreach ($modules as $mkey => $key) {
|
||||||
$disk_path = $this->getModulePathOnDisk($key);
|
$disk_path = $this->getModulePathOnDisk($key);
|
||||||
$futures[$key] = new ExecFuture(
|
if (Filesystem::pathExists($disk_path)) {
|
||||||
'%s %s',
|
$futures[$key] = new ExecFuture(
|
||||||
$bin,
|
'%s %s',
|
||||||
$disk_path);
|
$bin,
|
||||||
|
$disk_path);
|
||||||
|
} else {
|
||||||
|
// This can occur in git when you add a module in HEAD and then remove
|
||||||
|
// it in unstaged changes in the working copy. Just ignore it.
|
||||||
|
unset($modules[$mkey]);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
$requirements = array();
|
$requirements = array();
|
||||||
|
@ -340,19 +346,26 @@ class ArcanistPhutilModuleLinter extends ArcanistLinter {
|
||||||
$need_functions,
|
$need_functions,
|
||||||
$drop_modules);
|
$drop_modules);
|
||||||
$init_path = $this->getModulePathOnDisk($key).'/__init__.php';
|
$init_path = $this->getModulePathOnDisk($key).'/__init__.php';
|
||||||
$init_path = Filesystem::readablePath($init_path);
|
$try_path = Filesystem::readablePath($init_path);
|
||||||
if (file_exists($init_path)) {
|
if (Filesystem::pathExists($try_path)) {
|
||||||
|
$init_path = $try_path;
|
||||||
$old_file = Filesystem::readFile($init_path);
|
$old_file = Filesystem::readFile($init_path);
|
||||||
$this->willLintPath($init_path);
|
} else {
|
||||||
$message = $this->raiseLintAtOffset(
|
$old_file = '';
|
||||||
0,
|
|
||||||
self::LINT_INIT_REBUILD,
|
|
||||||
"This regenerated phutil '__init__.php' file is suggested to ".
|
|
||||||
"address lint problems with static dependencies in the module.",
|
|
||||||
$old_file,
|
|
||||||
$new_file);
|
|
||||||
$message->setDependentMessages($resolvable);
|
|
||||||
}
|
}
|
||||||
|
$this->willLintPath($init_path);
|
||||||
|
$message = $this->raiseLintAtOffset(
|
||||||
|
null,
|
||||||
|
self::LINT_INIT_REBUILD,
|
||||||
|
"This generated phutil '__init__.php' file is suggested to address ".
|
||||||
|
"lint problems with static dependencies in the module.",
|
||||||
|
$old_file,
|
||||||
|
$new_file);
|
||||||
|
$message->setDependentMessages($resolvable);
|
||||||
|
foreach ($resolvable as $message) {
|
||||||
|
$message->setObsolete(true);
|
||||||
|
}
|
||||||
|
$message->setGenerateFile(true);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -456,7 +469,9 @@ EOHEADER;
|
||||||
foreach ($places as $place) {
|
foreach ($places as $place) {
|
||||||
list($file, $offset) = explode(':', $place);
|
list($file, $offset) = explode(':', $place);
|
||||||
$this->willLintPath(
|
$this->willLintPath(
|
||||||
Filesystem::readablePath($this->getModulePathOnDisk($key).'/'.$file));
|
Filesystem::readablePath(
|
||||||
|
$this->getModulePathOnDisk($key).'/'.$file,
|
||||||
|
$this->getEngine()->getWorkingCopy()->getProjectRoot()));
|
||||||
return $this->raiseLintAtOffset(
|
return $this->raiseLintAtOffset(
|
||||||
$offset,
|
$offset,
|
||||||
$code,
|
$code,
|
||||||
|
|
|
@ -28,7 +28,9 @@ class ArcanistLintMessage {
|
||||||
protected $originalText;
|
protected $originalText;
|
||||||
protected $replacementText;
|
protected $replacementText;
|
||||||
protected $appliedToDisk;
|
protected $appliedToDisk;
|
||||||
|
protected $generateFile;
|
||||||
protected $dependentMessages = array();
|
protected $dependentMessages = array();
|
||||||
|
protected $obsolete;
|
||||||
|
|
||||||
public static function newFromDictionary(array $dict) {
|
public static function newFromDictionary(array $dict) {
|
||||||
$message = new ArcanistLintMessage();
|
$message = new ArcanistLintMessage();
|
||||||
|
@ -142,6 +144,24 @@ class ArcanistLintMessage {
|
||||||
return ($this->getLine() !== null);
|
return ($this->getLine() !== null);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
public function setGenerateFile($generate_file) {
|
||||||
|
$this->generateFile = $generate_file;
|
||||||
|
return $this;
|
||||||
|
}
|
||||||
|
|
||||||
|
public function getGenerateFile() {
|
||||||
|
return $this->generateFile;
|
||||||
|
}
|
||||||
|
|
||||||
|
public function setObsolete($obsolete) {
|
||||||
|
$this->obsolete = $obsolete;
|
||||||
|
return $this;
|
||||||
|
}
|
||||||
|
|
||||||
|
public function getObsolete() {
|
||||||
|
return $this->obsolete;
|
||||||
|
}
|
||||||
|
|
||||||
public function isPatchable() {
|
public function isPatchable() {
|
||||||
return ($this->getReplacementText() !== null);
|
return ($this->getReplacementText() !== null);
|
||||||
}
|
}
|
||||||
|
|
|
@ -53,7 +53,10 @@ final class ArcanistLintPatcher {
|
||||||
|
|
||||||
// Copy existing file to preserve permissions. 'chmod --reference' is not
|
// Copy existing file to preserve permissions. 'chmod --reference' is not
|
||||||
// supported under OSX.
|
// supported under OSX.
|
||||||
execx('cp -p %s %s', $path, $lint);
|
if (Filesystem::pathExists($path)) {
|
||||||
|
// This path may not exist if we're generating a new file.
|
||||||
|
execx('cp -p %s %s', $path, $lint);
|
||||||
|
}
|
||||||
Filesystem::writeFile($lint, $data);
|
Filesystem::writeFile($lint, $data);
|
||||||
|
|
||||||
list($err) = exec_manual("mv -f %s %s", $lint, $path);
|
list($err) = exec_manual("mv -f %s %s", $lint, $path);
|
||||||
|
|
|
@ -115,7 +115,8 @@ class ArcanistLintRenderer {
|
||||||
|
|
||||||
for (; $cursor < $line_num + $text_length; $cursor++) {
|
for (; $cursor < $line_num + $text_length; $cursor++) {
|
||||||
$chevron = ($cursor == $line_num);
|
$chevron = ($cursor == $line_num);
|
||||||
$data = $line_data[$cursor - 1];
|
// We may not have any data if, e.g., the old file does not exist.
|
||||||
|
$data = idx($line_data, $cursor - 1, null);
|
||||||
|
|
||||||
// Highlight the problem substring.
|
// Highlight the problem substring.
|
||||||
$text_line = $text_lines[$cursor - $line_num];
|
$text_line = $text_lines[$cursor - $line_num];
|
||||||
|
|
|
@ -9,6 +9,7 @@
|
||||||
phutil_require_module('arcanist', 'lint/severity');
|
phutil_require_module('arcanist', 'lint/severity');
|
||||||
|
|
||||||
phutil_require_module('phutil', 'console');
|
phutil_require_module('phutil', 'console');
|
||||||
|
phutil_require_module('phutil', 'utils');
|
||||||
|
|
||||||
|
|
||||||
phutil_require_source('ArcanistLintRenderer.php');
|
phutil_require_source('ArcanistLintRenderer.php');
|
||||||
|
|
|
@ -22,6 +22,7 @@ final class ArcanistLintResult {
|
||||||
protected $data;
|
protected $data;
|
||||||
protected $filePathOnDisk;
|
protected $filePathOnDisk;
|
||||||
protected $messages = array();
|
protected $messages = array();
|
||||||
|
protected $effectiveMessages = array();
|
||||||
private $needsSort;
|
private $needsSort;
|
||||||
|
|
||||||
public function setPath($path) {
|
public function setPath($path) {
|
||||||
|
@ -41,9 +42,9 @@ final class ArcanistLintResult {
|
||||||
|
|
||||||
public function getMessages() {
|
public function getMessages() {
|
||||||
if ($this->needsSort) {
|
if ($this->needsSort) {
|
||||||
$this->sortMessages();
|
$this->sortAndFilterMessages();
|
||||||
}
|
}
|
||||||
return $this->messages;
|
return $this->effectiveMessages;
|
||||||
}
|
}
|
||||||
|
|
||||||
public function setData($data) {
|
public function setData($data) {
|
||||||
|
@ -73,18 +74,32 @@ final class ArcanistLintResult {
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
private function sortMessages() {
|
private function sortAndFilterMessages() {
|
||||||
$messages = $this->messages;
|
$messages = $this->messages;
|
||||||
|
|
||||||
|
foreach ($messages as $key => $message) {
|
||||||
|
if ($message->getObsolete()) {
|
||||||
|
unset($messages[$key]);
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
if ($message->getGenerateFile()) {
|
||||||
|
$messages = array(
|
||||||
|
$key => $message,
|
||||||
|
);
|
||||||
|
break;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
$map = array();
|
$map = array();
|
||||||
foreach ($messages as $key => $message) {
|
foreach ($messages as $key => $message) {
|
||||||
$map[$key] = ($message->getLine() * (2 << 12)) + $message->getChar();
|
$map[$key] = ($message->getLine() * (2 << 12)) + $message->getChar();
|
||||||
}
|
}
|
||||||
asort($map);
|
asort($map);
|
||||||
$messages = array_select_keys($messages, array_keys($map));
|
$messages = array_select_keys($messages, array_keys($map));
|
||||||
$this->messages = $messages;
|
|
||||||
|
$this->effectiveMessages = $messages;
|
||||||
$this->needsSort = false;
|
$this->needsSort = false;
|
||||||
|
|
||||||
return $this;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|
|
@ -478,12 +478,17 @@ class ArcanistBaseWorkflow {
|
||||||
}
|
}
|
||||||
|
|
||||||
if (empty($this->changeCache[$path])) {
|
if (empty($this->changeCache[$path])) {
|
||||||
// TODO: This can legitimately occur under git if you make a change,
|
if ($repository_api instanceof ArcanistGitAPI) {
|
||||||
// "git commit" it, and then revert the change in the working copy and
|
// This can legitimately occur under git if you make a change, "git
|
||||||
// run "arc lint". We should probably just make a dummy, empty changeset
|
// commit" it, and then revert the change in the working copy and run
|
||||||
// in this case, at least under git.
|
// "arc lint".
|
||||||
throw new Exception(
|
$change = new ArcanistDiffChange();
|
||||||
"Trying to get change for unchanged path '{$path}'!");
|
$change->setCurrentPath($path);
|
||||||
|
return $change;
|
||||||
|
} else {
|
||||||
|
throw new Exception(
|
||||||
|
"Trying to get change for unchanged path '{$path}'!");
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
return $this->changeCache[$path];
|
return $this->changeCache[$path];
|
||||||
|
|
|
@ -168,6 +168,9 @@ EOTEXT
|
||||||
|
|
||||||
if ($prompt_patches) {
|
if ($prompt_patches) {
|
||||||
$old_file = $result->getFilePathOnDisk();
|
$old_file = $result->getFilePathOnDisk();
|
||||||
|
if (!Filesystem::pathExists($old_file)) {
|
||||||
|
$old_file = '/dev/null';
|
||||||
|
}
|
||||||
$new_file = new TempFile();
|
$new_file = new TempFile();
|
||||||
Filesystem::writeFile($new_file, $new);
|
Filesystem::writeFile($new_file, $new);
|
||||||
|
|
||||||
|
@ -193,7 +196,7 @@ EOTEXT
|
||||||
"Amend HEAD with lint patches?",
|
"Amend HEAD with lint patches?",
|
||||||
$default_no = false);
|
$default_no = false);
|
||||||
if (!$amend) {
|
if (!$amend) {
|
||||||
throw new ArcanistUsageException("Resolve lint changes and rediff.");
|
throw new ArcanistUsageException("Resolve lint changes and relint.");
|
||||||
}
|
}
|
||||||
execx(
|
execx(
|
||||||
'(cd %s; git commit -a --amend -C HEAD)',
|
'(cd %s; git commit -a --amend -C HEAD)',
|
||||||
|
|
Loading…
Reference in a new issue