From 70a6004e2661fca6dd24805a8861f0d94d4bdc88 Mon Sep 17 00:00:00 2001
From: Stu Hood <stuhood@gmail.com>
Date: Wed, 15 Jul 2020 21:38:39 -0700
Subject: [PATCH] Always include types in the engine's definition of equality.
 (#10377)

### Problem

I spent a few hours debugging why a boolean param was turning into an integer in some cases in the simple test in #10230, and determined that it was due to the behavior that `1 == True` and `0 == False` in Python. This impacts the engine in strange ways. For example: before the fix, the test in this change that attempts to use both a `bool` and an `int` fails with:
```
Exception: Values used as `Params` must have distinct types, but the following values had the same type (`int`):
  1
  1
```

### Solution

Always include types in the engine's definition of equality. Although this affects more than just memoization/interning, my expectation is that in any possible position where the engine will use `equals` the default behavior would be undesirable.
---
 .../pants/engine/internals/scheduler_test.py      | 15 +++++++++++++++
 src/rust/engine/src/externs/mod.rs                |  8 ++++++++
 2 files changed, 23 insertions(+)

diff --git a/src/python/pants/engine/internals/scheduler_test.py b/src/python/pants/engine/internals/scheduler_test.py
index 52075825d34..2840adb08d6 100644
--- a/src/python/pants/engine/internals/scheduler_test.py
+++ b/src/python/pants/engine/internals/scheduler_test.py
@@ -147,6 +147,11 @@ async def c_unhashable(_: CollectionType) -> C:
     return C()
 
 
+@rule
+def boolean_and_int(i: int, b: bool) -> A:
+    return A()
+
+
 @contextmanager
 def assert_execution_error(test_case, expected_msg):
     with test_case.assertRaises(ExecutionError) as cm:
@@ -176,6 +181,9 @@ def rules(cls):
             RootRule(UnionB),
             select_union_b,
             a_union_test,
+            boolean_and_int,
+            RootRule(int),
+            RootRule(bool),
         ]
 
     def test_use_params(self):
@@ -215,6 +223,13 @@ def test_consumed_types(self):
             self.scheduler.scheduler.rule_graph_consumed_types([A, C], str)
         )
 
+    def test_strict_equals(self):
+        # With the default implementation of `__eq__` for boolean and int, `1 == True`. But in the
+        # engine that behavior would be surprising, and would cause both of these Params to intern
+        # to the same value, triggering an error. Instead, the engine additionally includes the
+        # type of a value in equality.
+        assert A() == self.request_single_product(A, Params(1, True))
+
     @contextmanager
     def _assert_execution_error(self, expected_msg):
         with assert_execution_error(self, expected_msg):
diff --git a/src/rust/engine/src/externs/mod.rs b/src/rust/engine/src/externs/mod.rs
index d7426ba1b97..811068984a6 100644
--- a/src/rust/engine/src/externs/mod.rs
+++ b/src/rust/engine/src/externs/mod.rs
@@ -57,6 +57,14 @@ pub fn is_union(ty: TypeId) -> bool {
 
 pub fn equals(h1: &PyObject, h2: &PyObject) -> bool {
   let gil = Python::acquire_gil();
+  let py = gil.python();
+  // NB: Although it does not precisely align with Python's definition of equality, we ban matches
+  // between non-equal types to avoid legacy behavior like `assert True == 1`, which is very
+  // surprising in interning, and would likely be surprising anywhere else in the engine where we
+  // compare things.
+  if h1.get_type(py) != h2.get_type(py) {
+    return false;
+  }
   h1.rich_compare(gil.python(), h2, CompareOp::Eq)
     .unwrap()
     .cast_as::<PyBool>(gil.python())