[engine] Better error messages for missing targets (pantsbuild#4509)

### Problem
There's two problems.
1. For some classes of missing targets, we always print a trace. V1 doesn't, it displays a short error message that's pretty clear.
2. For other error cases, we just say that the spec was missing without further information.

### Solution

This patch brings v2's behavior more in line with v1's for the above cases. It adds special handling for `ResolveError`s in those cases.

It does not attempt to improve trace generation generally, or to communicate where a non-existent address is referenced from if it has references to it. Those are follow one bits of work.

See pantsbuild#3912 for a number of related cases.

src/python/pants/engine/build_files.py

@@ -109,9 +109,10 @@ def __hash__(self):
 
 
 def _raise_did_you_mean(address_family, name):
-  possibilities = '\n  '.join(str(a) for a in address_family.addressables)
-  raise ResolveError('A Struct was not found in namespace {} for name "{}". '
-                     'Did you mean one of?:\n  {}'.format(address_family.namespace, name, possibilities))
+  possibilities = '\n  '.join(':{}'.format(a.target_name) for a in address_family.addressables)
+  raise ResolveError('"{}" was not found in namespace "{}". '
+                     'Did you mean one of:\n  {}'
+                     .format(name, address_family.namespace, possibilities))
 
 
 @rule(UnhydratedStruct,
@@ -237,11 +238,16 @@ def addresses_from_address_families(address_families, spec):
   if type(spec) in (DescendantAddresses, SiblingAddresses, AscendantAddresses):
     addresses = tuple(a for af in address_families for a in af.addressables.keys())
   elif type(spec) is SingleAddress:
+    # TODO Could assert len(address_families) == 1, as it should always be true in this case.
     addresses = tuple(a
                       for af in address_families
                       for a in af.addressables.keys() if a.target_name == spec.name)
+    if not addresses:
+      if len(address_families) == 1:
+        _raise_did_you_mean(address_families[0], spec.name)
   else:
     raise ValueError('Unrecognized Spec type: {}'.format(spec))
+
   return BuildFileAddresses(addresses)
 
 

src/python/pants/engine/legacy/BUILD

@@ -9,8 +9,9 @@ python_library(
     'src/python/pants/base:specs',
     'src/python/pants/build_graph',
     'src/python/pants/engine',
-    'src/python/pants/engine:fs',
     'src/python/pants/engine:build_files',
+    'src/python/pants/engine:fs',
+    'src/python/pants/engine:mapper',
     'src/python/pants/engine:selectors',
     'src/python/pants/util:dirutil',
   ],
@@ -57,6 +58,7 @@ python_library(
     'src/python/pants/base:parse_context',
     'src/python/pants/build_graph',
     'src/python/pants/engine:build_files',
+    'src/python/pants/engine:mapper',
     'src/python/pants/engine:parser',
     'src/python/pants/engine:selectors',
     'src/python/pants/source',

src/python/pants/engine/legacy/address_mapper.py

@@ -13,6 +13,8 @@
 from pants.build_graph.address_mapper import AddressMapper
 from pants.engine.addressable import BuildFileAddresses
 from pants.engine.build_files import BuildFilesCollection
+from pants.engine.mapper import ResolveError
+from pants.engine.nodes import Throw
 from pants.util.dirutil import fast_relpath
 
 
@@ -75,9 +77,13 @@ def _internal_scan_specs(self, specs, fail_fast=True, missing_is_fatal=True):
 
     addresses = set()
     for (spec, _), state in root_entries.items():
-      if missing_is_fatal and not state.value.dependencies:
-        raise self.BuildFileScanError(
-          'Spec `{}` does not match any targets.'.format(spec.to_spec_string()))
+      if missing_is_fatal:
+        if isinstance(state, Throw) and isinstance(state.exc, ResolveError):
+          raise self.BuildFileScanError(
+            'Spec `{}` does not match any targets.\n{}'.format(spec.to_spec_string(), str(state.exc)))
+        elif not state.value.dependencies:
+          raise self.BuildFileScanError(
+            'Spec `{}` does not match any targets.'.format(spec.to_spec_string()))
       addresses.update(state.value.dependencies)
     return addresses
 

src/python/pants/engine/legacy/graph.py

@@ -20,6 +20,7 @@
 from pants.engine.addressable import BuildFileAddresses, Collection
 from pants.engine.fs import PathGlobs, Snapshot
 from pants.engine.legacy.structs import BundleAdaptor, BundlesField, SourcesField, TargetAdaptor
+from pants.engine.mapper import ResolveError
 from pants.engine.nodes import Return
 from pants.engine.rules import TaskRule, rule
 from pants.engine.selectors import Select, SelectDependencies, SelectProjection, SelectTransitive
@@ -89,9 +90,15 @@ def _index(self, roots):
     # Index the ProductGraph.
     for node, state in roots.items():
       if type(state) is not Return:
-        trace = '\n'.join(self._scheduler.trace())
-        raise AddressLookupError(
-            'Build graph construction failed for {}:\n{}'.format(node, trace))
+        # NB: ResolveError means that a target was not found, which is a common user facing error.
+        # TODO Come up with a better error reporting mechanism so that we don't need this as a special case.
+        #      Possibly as part of https://github.com/pantsbuild/pants/issues/4446
+        if isinstance(state.exc, ResolveError):
+          raise AddressLookupError(str(state.exc))
+        else:
+          trace = '\n'.join(self._scheduler.trace())
+          raise AddressLookupError(
+              'Build graph construction failed for {}:\n{}'.format(node, trace))
       if type(state.value) is not HydratedTargets:
         raise TypeError('Expected roots to hold {}; got: {}'.format(
           HydratedTargets, type(state.value)))

tests/python/pants_test/engine/legacy/test_graph.py

@@ -14,6 +14,7 @@
 
 from pants.bin.engine_initializer import EngineInitializer, LegacySymbolTable
 from pants.build_graph.address import Address
+from pants.build_graph.address_lookup_error import AddressLookupError
 from pants.build_graph.build_file_aliases import BuildFileAliases, TargetMacro
 from pants.build_graph.target import Target
 from pants.init.target_roots import TargetRoots
@@ -43,7 +44,7 @@ def aliases(cls):
       )
 
 
-class GraphInvalidationTest(unittest.TestCase):
+class GraphTestBase(unittest.TestCase):
 
   _native = init_native()
 
@@ -65,6 +66,22 @@ def open_scheduler(self, specs, symbol_table_cls=None):
       addresses = tuple(graph.inject_specs_closure(target_roots.as_specs()))
       yield graph, addresses, graph_helper.scheduler
 
+
+class GraphTargetScanFailureTests(GraphTestBase):
+
+  def test_with_missing_target_in_existing_build_file(self):
+    with self.assertRaises(AddressLookupError) as cm:
+      with self.open_scheduler(['3rdparty/python:rutabaga']) as (_, _, scheduler):
+        self.fail('Expected an exception.')
+
+    self.assertIn('"rutabaga" was not found in namespace "3rdparty/python". Did you mean one of:\n'
+                  '  :psutil\n'
+                  '  :isort',
+                  str(cm.exception))
+
+
+class GraphInvalidationTest(GraphTestBase):
+
   def test_invalidate_fsnode(self):
     with self.open_scheduler(['3rdparty/python::']) as (_, _, scheduler):
       initial_node_count = scheduler.node_count()