From 4d550e932b546055e49914b5d27cdc67f0e04bca Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Fri, 15 Feb 2019 11:20:51 -0800 Subject: [PATCH 1/5] disallow enum equality checking --- src/python/pants/util/objects.py | 9 +++++++++ .../python/tasks/native/test_ctypes_integration.py | 5 ++++- tests/python/pants_test/util/test_objects.py | 12 ++++++++++++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/python/pants/util/objects.py b/src/python/pants/util/objects.py index e223a5d22fc..91e48c45b53 100644 --- a/src/python/pants/util/objects.py +++ b/src/python/pants/util/objects.py @@ -281,6 +281,15 @@ def __new__(cls, value): """ return cls.create(value) + # TODO: figure out if there's a way to make the opposite direction of `==` raise + # (e.g. 1 == SomeEnum(1), which currently just returns False). + 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 + @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..7159f8b54da 100644 --- a/tests/python/pants_test/util/test_objects.py +++ b/tests/python/pants_test/util/test_objects.py @@ -741,6 +741,18 @@ 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) + with self.assertRaisesRegexp(TypeCheckError, + re.escape("enum equality is defined to be an error")): + enum_instance == enum_instance + # Test that comparison also fails against another type. + with self.assertRaisesRegexp(TypeCheckError, + re.escape("enum equality is defined to be an error")): + enum_instance == 1 + # TODO: figure out if there's a way to make this direction raise as well! + self.assertNotEqual(1, enum_instance) + def test_enum_resolve_variant(self): one_enum_instance = SomeEnum(1) two_enum_instance = SomeEnum(2) From 344d1369d72b8c70ee1a5830b1e37fc3f7822557 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Fri, 15 Feb 2019 11:48:53 -0800 Subject: [PATCH 2/5] actually, __eq__ overrides go both ways --- src/python/pants/util/objects.py | 4 ++-- tests/python/pants_test/util/test_objects.py | 18 ++++++++++++------ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/python/pants/util/objects.py b/src/python/pants/util/objects.py index 91e48c45b53..e4e0c956929 100644 --- a/src/python/pants/util/objects.py +++ b/src/python/pants/util/objects.py @@ -281,8 +281,8 @@ def __new__(cls, value): """ return cls.create(value) - # TODO: figure out if there's a way to make the opposite direction of `==` raise - # (e.g. 1 == SomeEnum(1), which currently just returns False). + # 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( diff --git a/tests/python/pants_test/util/test_objects.py b/tests/python/pants_test/util/test_objects.py index 7159f8b54da..05985796841 100644 --- a/tests/python/pants_test/util/test_objects.py +++ b/tests/python/pants_test/util/test_objects.py @@ -743,15 +743,21 @@ def test_enum_instance_creation_errors(self): def test_enum_comparison_fails(self): enum_instance = SomeEnum(1) - with self.assertRaisesRegexp(TypeCheckError, - re.escape("enum equality is defined to be an error")): + rx_str = re.escape("enum equality is defined to be an error") + with self.assertRaisesRegexp(TypeCheckError, rx_str): enum_instance == enum_instance # Test that comparison also fails against another type. - with self.assertRaisesRegexp(TypeCheckError, - re.escape("enum equality is defined to be an error")): + with self.assertRaisesRegexp(TypeCheckError, rx_str): enum_instance == 1 - # TODO: figure out if there's a way to make this direction raise as well! - self.assertNotEqual(1, enum_instance) + 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) From 1c0a09a5e189ba8358eeb4e139f54055e5d94d6d Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Fri, 15 Feb 2019 13:04:36 -0800 Subject: [PATCH 3/5] remove __hash__ noop override on datatype --- src/python/pants/util/objects.py | 8 ++++---- tests/python/pants_test/util/test_objects.py | 4 ++++ 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/python/pants/util/objects.py b/src/python/pants/util/objects.py index e4e0c956929..b6f4ab9e01f 100644 --- a/src/python/pants/util/objects.py +++ b/src/python/pants/util/objects.py @@ -132,13 +132,10 @@ def __eq__(self, other): def __ne__(self, other): return not (self == other) - 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__() @@ -285,6 +282,9 @@ def __new__(cls, value): # 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.""" + # TODO: determine why this is necessary for hashability. + # if type(self) == type(other): + # return super(ChoiceDatatype, self).__eq__(other) 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. diff --git a/tests/python/pants_test/util/test_objects.py b/tests/python/pants_test/util/test_objects.py index 05985796841..e25d7f1b08d 100644 --- a/tests/python/pants_test/util/test_objects.py +++ b/tests/python/pants_test/util/test_objects.py @@ -744,6 +744,10 @@ def test_enum_instance_creation_errors(self): def test_enum_comparison_fails(self): enum_instance = SomeEnum(1) rx_str = re.escape("enum equality is defined to be an error") + # # TODO: Figure out a way to raise on any use of `==` without breaking hashability for use by the + # # engine. + # self.assertEqual(enum_instance, SomeEnum(1)) + # self.assertNotEqual(enum_instance, SomeEnum(2)) with self.assertRaisesRegexp(TypeCheckError, rx_str): enum_instance == enum_instance # Test that comparison also fails against another type. From 58e204015b84e34fadb4c7262dbab732bff70424 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Fri, 15 Feb 2019 13:37:05 -0800 Subject: [PATCH 4/5] document how __hash__ needs to be explicitly overridden --- src/python/pants/util/objects.py | 14 +++++++++++--- tests/python/pants_test/util/test_objects.py | 6 ++---- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/python/pants/util/objects.py b/src/python/pants/util/objects.py index b6f4ab9e01f..85bf39271c3 100644 --- a/src/python/pants/util/objects.py +++ b/src/python/pants/util/objects.py @@ -132,6 +132,12 @@ def __eq__(self, other): def __ne__(self, other): return not (self == other) + # NB: it appears that in Python 3, whenever __eq__ is overridden, __hash__() must also be + # explicitly implemented, otherwise Python will raise "unhashable type". It's not clear whether + # or where this behavior is documented. + 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): @@ -282,14 +288,16 @@ def __new__(cls, value): # 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.""" - # TODO: determine why this is necessary for hashability. - # if type(self) == type(other): - # return super(ChoiceDatatype, self).__eq__(other) 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 in Python 3. This behavior appears to be undocumented. + 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/util/test_objects.py b/tests/python/pants_test/util/test_objects.py index e25d7f1b08d..8cb1423c772 100644 --- a/tests/python/pants_test/util/test_objects.py +++ b/tests/python/pants_test/util/test_objects.py @@ -744,12 +744,10 @@ def test_enum_instance_creation_errors(self): def test_enum_comparison_fails(self): enum_instance = SomeEnum(1) rx_str = re.escape("enum equality is defined to be an error") - # # TODO: Figure out a way to raise on any use of `==` without breaking hashability for use by the - # # engine. - # self.assertEqual(enum_instance, SomeEnum(1)) - # self.assertNotEqual(enum_instance, SomeEnum(2)) 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 From e57a102275e59f271099c45b5959cfe497de666c Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Fri, 15 Feb 2019 13:49:15 -0800 Subject: [PATCH 5/5] update comments to point to documented __hash__ py3 behavior --- src/python/pants/util/objects.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/python/pants/util/objects.py b/src/python/pants/util/objects.py index 85bf39271c3..f66ebc5d717 100644 --- a/src/python/pants/util/objects.py +++ b/src/python/pants/util/objects.py @@ -132,9 +132,9 @@ def __eq__(self, other): def __ne__(self, other): return not (self == other) - # NB: it appears that in Python 3, whenever __eq__ is overridden, __hash__() must also be - # explicitly implemented, otherwise Python will raise "unhashable type". It's not clear whether - # or where this behavior is documented. + # 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__() @@ -294,7 +294,7 @@ def __eq__(self, other): __eq__._eq_override_canary = None # NB: as noted in datatype(), __hash__ must be explicitly implemented whenever __eq__ is - # overridden in Python 3. This behavior appears to be undocumented. + # overridden. See https://docs.python.org/3/reference/datamodel.html#object.__hash__. def __hash__(self): return super(ChoiceDatatype, self).__hash__()