Skip to content

Commit

Permalink
Fix nightly cron ctypes enum failure (pantsbuild#7249)
Browse files Browse the repository at this point in the history
### Problem

Resolves pantsbuild#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 pantsbuild#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?
  • Loading branch information
cosmicexplorer committed Feb 20, 2019
1 parent 0e6a144 commit 761849e
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 2 deletions.
19 changes: 18 additions & 1 deletion src/python/pants/util/objects.py
Expand Up @@ -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__()
Expand Down Expand Up @@ -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.
Expand Down
Expand Up @@ -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:
Expand Down
20 changes: 20 additions & 0 deletions tests/python/pants_test/util/test_objects.py
Expand Up @@ -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)
Expand Down

0 comments on commit 761849e

Please sign in to comment.