tree 188387b9756d28b6b9512428ab609c44b0545fa9
parent fcefd1e4b1e5db3d8e10e2836c5a0fa2a79aadc0
author janakr <janakr@google.com> 1605558233 -0800
committer Copybara-Service <copybara-worker@google.com> 1605558449 -0800

Delete SkyframeLabelVisitor after its last neutering by unknown commit. None of the code was reachable or necessary in production, especially since it's only used in query.

Argued as follows (note that all production callers passed errorOnCycle=false):

- Because errors was reassigned with all cycles filtered out, no cycle reporting code below line 76 could be hit.
- "direct errors from top-level labels" is vestigial from the days when SkyframeLabelVisitor was used for builds, and we might not resolve some labels before passing them in here. It is now the case that, via target-pattern parsing, the top-level labels passed in here must correspond to actual targets, and so that code is dead.

What remains is the "transitive root cause" reporting. First, for --nokeep_going, there won't be any of those, because exceptions are not recoverable then. For keep_going, we could report some of them, but if the query was keep_going, then errors will be empty (because TransitiveTargetFunction swallows ~all errors), so we return early at line 78.

In short, the argument is:
- The labels passed in have already been found to correspond to existing Targets
- In --keep_going mode, TransitiveTargetFunction for an existing Target never throws
  - Therefore, errors will be empty, modulo cycles
- In --nokeep_going mode, there will be no values, only errors. However, all of those errors will already have been reported, and are not reported here.

Note that the above argument applies to the code @head, *not* the code before unknown commit. In particular, there was test code trying to assert that there were additional warnings/errors emitted, but because unknown commit did not change the tests to match production, the tests were no longer testing production code.

I verified that QueryPreloadingTest#testNewBuildFileConflict was already "failing" at head by replicating the tests on the commandline and seeing that `blaze query` at head would fail the test, this was a change in behavior from unknown commit.

QueryPreloadingTest#testRootCauseOnInconsistentFilesystem was testing for a property that held end-to-end, but no longer held for the transitive package loading in isolation. I moved the test to QueryIntegrationTest and added another test case for an inconsistent filesystem exception in a dep.

Also enabled a tiny bit of test coverage in IOExceptionsTest and deleted a bit of defunct test code elsewhere. I didn't do too much archaeology on how it died, since SkyframeLabelVisitor has changed so radically over its lifetime.

The stanza I really wanted to delete came from unknown commit (pre-Bazel, so I suspect it will not be translated to a github hash!). As best I can tell, the relevant test (testMissingCompilerDependency) added in that CL is still here and still passes.

PiperOrigin-RevId: 342693957