add further checking to Get() arguments to overcome ambiguity that led to test failure

cosmicexplorer · cosmicexplorer · commit 78e5c317e856 · 2019-01-20T22:18:23.000-08:00
diff --git a/src/python/pants/engine/rules.py b/src/python/pants/engine/rules.py
@@ -392,9 +392,12 @@ def __new__(cls,
       )
 
   def __str__(self):
-    return '({}, {!r}, {})'.format(type_or_constraint_repr(self.output_constraint),
-                                   self.input_selectors,
-                                   self.func.__name__)
+    return ('({}, {!r}, {}, gets={}, opts={})'
+            .format(type_or_constraint_repr(self.output_constraint),
+                    self.input_selectors,
+                    self.func.__name__,
+                    self.input_gets,
+                    self.dependency_optionables))
 
 
 class SingletonRule(datatype(['output_constraint', 'value']), Rule):
diff --git a/src/python/pants/engine/scheduler.py b/src/python/pants/engine/scheduler.py
@@ -510,7 +510,6 @@ def run_console_rule(self, product, subject):
     """
     :param product: product type for the request.
     :param subject: subject for the request.
-    :param v2_ui: whether to render the v2 engine UI
     """
     request = self.execution_request([product], [subject])
     returns, throws = self.execute(request)
diff --git a/src/python/pants/engine/selectors.py b/src/python/pants/engine/selectors.py
@@ -6,7 +6,7 @@
 
 import ast
 
-from pants.util.objects import Exactly, datatype
+from pants.util.objects import Exactly, TypeConstraint, datatype
 
 
 def type_or_constraint_repr(constraint):
@@ -79,16 +79,29 @@ def create_statically_for_rule_graph(cls, product_type, subject_type):
     return cls(product_type, subject_type, None)
 
   def __new__(cls, *args):
-    # TODO(#7114): typecheck the args (as in, ensure they are types or constraints)! After #7114, we
-    # can just check that they are types.
+    # TODO(#7114): Use datatype type checking for these fields! We can wait until after #7114, when
+    # we can just check that they are types.
     if len(args) == 2:
       product, subject = args
+
+      if isinstance(subject, (type, TypeConstraint)):
+        raise TypeError("""\
+The two-argument form of Get does not accept a type as its second argument.
+
+args were: Get({args!r})
+
+Get.create_statically_for_rule_graph() should be used to generate a Get() for
+the `input_gets` field of a rule. If you are using a `yield Get(...)` in a rule
+and a type was intended, use the 3-argument version:
+Get({product!r}, {subject_type!r}, {subject!r})
+""".format(args=args, product=product, subject_type=type(subject), subject=subject))
+
       subject_declared_type = type(subject)
     elif len(args) == 3:
       product, subject_declared_type, subject = args
     else:
-      raise Exception('Expected either two or three arguments to {}; got {}.'.format(
-        Get.__name__, args))
+      raise ValueError('Expected either two or three arguments to {}; got {}.'
+                       .format(Get.__name__, args))
     return super(Get, cls).__new__(cls, product, subject_declared_type, subject)
 
 
diff --git a/src/python/pants/option/optionable.py b/src/python/pants/option/optionable.py
@@ -49,7 +49,7 @@ def signature(cls):
         output_type=cls.optionable_cls,
         input_selectors=tuple(),
         func=partial_construct_optionable,
-        input_gets=(Get(ScopedOptions, Scope),),
+        input_gets=(Get.create_statically_for_rule_graph(ScopedOptions, Scope),),
         dependency_optionables=(cls.optionable_cls,),
       )
 
diff --git a/tests/python/pants_test/engine/test_selectors.py b/tests/python/pants_test/engine/test_selectors.py
@@ -16,7 +16,9 @@ class AClass(object):
 
 
 class BClass(object):
-  pass
+
+  def __eq__(self, other):
+    return type(self) == type(other)
 
 
 class SubBClass(BClass):
@@ -33,6 +35,29 @@ def assert_repr(self, expected, selector):
 
 
 class GetTest(unittest.TestCase):
+  def test_create(self):
+    # Test the equivalence of the 2-arg and 3-arg versions.
+    self.assertEqual(Get(AClass, BClass()),
+                     Get(AClass, BClass, BClass()))
+
+    with self.assertRaises(TypeError) as cm:
+      Get(AClass, BClass)
+    self.assertEqual("""\
+The two-argument form of Get does not accept a type as its second argument.
+
+args were: Get(({a!r}, {b!r}))
+
+Get.create_statically_for_rule_graph() should be used to generate a Get() for
+the `input_gets` field of a rule. If you are using a `yield Get(...)` in a rule
+and a type was intended, use the 3-argument version:
+Get({a!r}, {t!r}, {b!r})
+""".format(a=AClass, t=type(BClass), b=BClass), str(cm.exception))
+
+    with self.assertRaises(ValueError) as cm:
+      Get(1)
+    self.assertEqual("Expected either two or three arguments to Get; got (1,).",
+                     str(cm.exception))
+
   def _get_call_node(self, input_string):
     return ast.parse(input_string).body[0].value
 

Original file line number	Diff line number	Diff line change
`@@ -49,7 +49,7 @@ def signature(cls):`
`49`	`49`	`output_type=cls.optionable_cls,`
`50`	`50`	`input_selectors=tuple(),`
`51`	`51`	`func=partial_construct_optionable,`
`52`		`- input_gets=(Get(ScopedOptions, Scope),),`
	`52`	`+ input_gets=(Get.create_statically_for_rule_graph(ScopedOptions, Scope),),`
`53`	`53`	`dependency_optionables=(cls.optionable_cls,),`
`54`	`54`	`)`
`55`	`55`