From 761849edd19a057c94ed1b0f6f3b131b24e97c98 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Wed, 20 Feb 2019 11:38:39 -0800 Subject: [PATCH] Fix nightly cron ctypes enum failure (#7249) ### Problem Resolves #7247. `ToolchainVariant('gnu')` does not in fact `== 'gnu'`. ### Solution - Use `.resolve_for_enum_variant()` instead of comparing with equality in that one failing test (I missed this in #7226, I fixed the instance earlier in the file though). - Raise an error when trying to use `==` on an enum to avoid this from happening again. - Note that in Python 3 it appears that `__hash__` must be explicitly implemented whenever `__eq__` is overridden, and this appears undocumented. ### Result The nightly cron job should be fixed, and enums are now a little more difficult to screw up. # Open Questions It's a little unclear why this didn't fail in CI -- either the test was cached, or some but not all travis osx images are provisioned with the correct dylib, causing a nondeterministic error, or something else? --- src/python/pants/util/objects.py | 19 +++++++++++++++++- .../tasks/native/test_ctypes_integration.py | 5 ++++- tests/python/pants_test/util/test_objects.py | 20 +++++++++++++++++++ 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/src/python/pants/util/objects.py b/src/python/pants/util/objects.py index e223a5d22fc..f66ebc5d717 100644 --- a/src/python/pants/util/objects.py +++ b/src/python/pants/util/objects.py @@ -132,13 +132,16 @@ def __eq__(self, other): def __ne__(self, other): return not (self == other) + # NB: in Python 3, whenever __eq__ is overridden, __hash__() must also be + # explicitly implemented, otherwise Python will raise "unhashable type". See + # https://docs.python.org/3/reference/datamodel.html#object.__hash__. def __hash__(self): return super(DataType, self).__hash__() # NB: As datatype is not iterable, we need to override both __iter__ and all of the # namedtuple methods that expect self to be iterable. def __iter__(self): - raise TypeError("'{}' object is not iterable".format(type(self).__name__)) + raise self.make_type_error("datatype object is not iterable") def _super_iter(self): return super(DataType, self).__iter__() @@ -281,6 +284,20 @@ def __new__(cls, value): """ return cls.create(value) + # TODO: figure out if this will always trigger on primitives like strings, and what situations + # won't call this __eq__ (and therefore won't raise like we want). + def __eq__(self, other): + """Redefine equality to raise to nudge people to use static pattern matching.""" + raise self.make_type_error( + "enum equality is defined to be an error -- use .resolve_for_enum_variant() instead!") + # Redefine the canary so datatype __new__ doesn't raise. + __eq__._eq_override_canary = None + + # NB: as noted in datatype(), __hash__ must be explicitly implemented whenever __eq__ is + # overridden. See https://docs.python.org/3/reference/datamodel.html#object.__hash__. + def __hash__(self): + return super(ChoiceDatatype, self).__hash__() + @classmethod def create(cls, *args, **kwargs): """Create an instance of this enum, using the default value if specified. diff --git a/tests/python/pants_test/backend/python/tasks/native/test_ctypes_integration.py b/tests/python/pants_test/backend/python/tasks/native/test_ctypes_integration.py index 619ac036d31..2a7f1984545 100644 --- a/tests/python/pants_test/backend/python/tasks/native/test_ctypes_integration.py +++ b/tests/python/pants_test/backend/python/tasks/native/test_ctypes_integration.py @@ -183,7 +183,10 @@ def test_ctypes_third_party_integration(self, toolchain_variant): # TODO(#6848): this fails when run with gcc on osx as it requires gcc's libstdc++.so.6.dylib to # be available on the runtime library path. attempt_pants_run = Platform.create().resolve_for_enum_variant({ - 'darwin': toolchain_variant != 'gnu', + 'darwin': toolchain_variant.resolve_for_enum_variant({ + 'gnu': False, + 'llvm': True, + }), 'linux': True, }) if attempt_pants_run: diff --git a/tests/python/pants_test/util/test_objects.py b/tests/python/pants_test/util/test_objects.py index 4813e0cd5cf..8cb1423c772 100644 --- a/tests/python/pants_test/util/test_objects.py +++ b/tests/python/pants_test/util/test_objects.py @@ -741,6 +741,26 @@ def test_enum_instance_creation_errors(self): with self.assertRaisesRegexp(EnumVariantSelectionError, expected_rx_falsy_value): SomeEnum('') + def test_enum_comparison_fails(self): + enum_instance = SomeEnum(1) + rx_str = re.escape("enum equality is defined to be an error") + with self.assertRaisesRegexp(TypeCheckError, rx_str): + enum_instance == enum_instance + with self.assertRaisesRegexp(TypeCheckError, rx_str): + enum_instance != enum_instance + # Test that comparison also fails against another type. + with self.assertRaisesRegexp(TypeCheckError, rx_str): + enum_instance == 1 + with self.assertRaisesRegexp(TypeCheckError, rx_str): + 1 == enum_instance + + class StrEnum(enum(['a'])): pass + enum_instance = StrEnum('a') + with self.assertRaisesRegexp(TypeCheckError, rx_str): + enum_instance == 'a' + with self.assertRaisesRegexp(TypeCheckError, rx_str): + 'a' == enum_instance + def test_enum_resolve_variant(self): one_enum_instance = SomeEnum(1) two_enum_instance = SomeEnum(2)