This allows ArcanistBaseXHPASTLinter::getXHPASTTreeForPath to return the parse tree for a file that wasn't explicitly linted. In my particular use case, I am writing a linter rule to determine if it is necessary to import a PHP file with require_once (i.e. the file consists only of class/interface definitions which are autoloadable).
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Commits
- rARC26c4f12a2117: Allow getXHPASTTreeForPath to return the tree for an arbitrary path
Tested externally using a custom linter.
Diff Detail
Diff Detail
- Repository
- rARC Arcanist
- Branch
- master
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 6173 Build 6194: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Comment Actions
I don't love this because it's slower than parallelizing, the cost is hidden and implicit (similar looking code may have dramatically different costs), and it makes fixing the memory behavior in a solid way (e.g., T5097) much more difficult.
I'd accept an explicit method to add a tree (if such a method does not already exist).
In an ideal world, I'd guess the structure you probably want is something like:
- a cache on disk of <hash(content) -> is purely autoloadable>, which is tiny/fast/not meaningfully memory constrained;
- a getAllCacheEntriesForTheseFiles($list_of_paths) which builds missing cache entries in parallel, then throws away the trees, so it runs in roughly constant memory;
- a linter which looks like:
- get all includes in a file;
- batch lookup in the cache;
- ok good.
This is trickier if you also want to evaluate includes for includes (e.g., if an included file is autoloadable except for includes, but those are also autoloadable, mark the file as autoloadable), but in theory you don't really need to do that since you can just keep running the linter and removing unnecessary includes over and over again to end up in the same place.
Comment Actions
OK, how about this... if you call getXHPASTTreeForPath for a path that we don't have an ExecFuture for then we return null. Then, in my calling code I can do this:
$tree = $this->getXHPASTTreeForPath($path); if (!$tree) { $this->buildSharedFutures(array($path)); $tree = $this->getXHPASTTreeForPath($path); }
Which can be simplified to:
$this->buildSharedFutures(array($path)); $tree = $this->getXHPASTTreeForPath($path);
Which means that I don't actually require this diff (although return null seems nice anyway)
| src/lint/linter/ArcanistBaseXHPASTLinter.php | ||
|---|---|---|
| 171–172 | We should probably do the second check before this line. Otherwise, calling this method prevents a future from being built later, I think. | |
Comment Actions
And possibly this should be some kind of, like, throw new YouDidntAskForThatPathToBeBuilt thing.