From ae46190f9370f5c8a25143e7489e78779bd22ed5 Mon Sep 17 00:00:00 2001
From: Eric Arellano <ericarellano@me.com>
Date: Thu, 9 Jan 2020 12:56:36 -0700
Subject: [PATCH] Fix confusing error for an `@console_rule` not returning a
 `Goal`

---
 src/python/pants/engine/rules.py             | 20 +++++++++++++------
 tests/python/pants_test/engine/test_rules.py | 21 ++++++++++++++++++++
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/src/python/pants/engine/rules.py b/src/python/pants/engine/rules.py
index 2eef57ac79f..d3cc4201398 100644
--- a/src/python/pants/engine/rules.py
+++ b/src/python/pants/engine/rules.py
@@ -75,8 +75,11 @@ def _get_starting_indent(source):
 
 
 def _make_rule(
-  return_type: Type, parameter_types: typing.Iterable[Type], cacheable: bool = True,
-  name: Optional[bool] = None
+  return_type: Type,
+  parameter_types: typing.Iterable[Type],
+  *,
+  cacheable: bool = True,
+  name: Optional[str] = None
 ) -> Callable[[Callable], Callable]:
   """A @decorator that declares that a particular static function may be used as a TaskRule.
 
@@ -90,12 +93,17 @@ def _make_rule(
                     its inputs.
   """
 
-  is_goal_cls = issubclass(return_type, Goal)
-  if is_goal_cls == cacheable:
+  has_goal_return_type = issubclass(return_type, Goal)
+  if cacheable and has_goal_return_type:
     raise TypeError(
-      'An `@rule` that produces a `Goal` must be declared with @console_rule in order to signal '
+      'An `@rule` that returns a `Goal` must be declared with `@console_rule` in order to signal '
       'that it is not cacheable.'
     )
+  if not cacheable and not has_goal_return_type:
+    raise TypeError(
+      'An `@console_rule` must return a subclass of `engine.goal.Goal`.'
+    )
+  is_goal_cls = has_goal_return_type
 
   def wrapper(func):
     if not inspect.isfunction(func):
@@ -213,7 +221,7 @@ def rule_decorator(*args, **kwargs) -> Callable:
   func = args[0]
 
   cacheable: bool = kwargs['cacheable']
-  name = kwargs.get('name')
+  name: Optional[str] = kwargs.get('name')
 
   signature = inspect.signature(func)
   func_id = f'@rule {func.__module__}:{func.__name__}'
diff --git a/tests/python/pants_test/engine/test_rules.py b/tests/python/pants_test/engine/test_rules.py
index 9d65f20c849..1237a76251d 100644
--- a/tests/python/pants_test/engine/test_rules.py
+++ b/tests/python/pants_test/engine/test_rules.py
@@ -167,6 +167,27 @@ def dry(a: int, b: 42, c: float) -> bool:
         return False
 
 
+class ConsoleRuleValidation(TestBase):
+
+  def test_not_properly_marked_console_rule(self) -> None:
+    with self.assertRaisesWithMessage(
+      TypeError,
+      "An `@rule` that returns a `Goal` must be declared with `@console_rule` in order to signal "
+      "that it is not cacheable."
+    ):
+      @rule
+      def normal_rule_trying_to_return_a_goal() -> Example:
+        return Example(0)
+
+  def test_console_rule_not_returning_a_goal(self) -> None:
+    with self.assertRaisesWithMessage(
+      TypeError, "An `@console_rule` must return a subclass of `engine.goal.Goal`."
+    ):
+      @console_rule
+      def console_rule_returning_a_non_goal() -> int:
+        return 0
+
+
 class RuleGraphTest(TestBase):
   def test_ruleset_with_missing_product_type(self):
     @rule