Bug 1375231 - Make OptionValue.__eq__ more type aware; r?glandium
OptionValue and its derived classes are exposed to moz.configure
files. As the previous bug fix showed, it is really easy to
accidentally assume the type is a simple string value and do a
string compare against it.
To prevent this class of bugs, this commit adds additional type
awareness to OptionValue.__eq__.
We first check that the argument is a tuple (including any OptionValue
types). If not, we raise a TypeError because the comparison is
invalid. This is arguably a violation of __eq__. But since OptionValue
is exposed to the moz.configure sandbox and typing '==' will invoke
__eq__, we have to do something to prevent this foot gun.
The change also changes the comparison logic.
If we compare against a non-derived tuple instance, we do a tuple
compare. Otherwise, we fall back to the previous logic of
requiring an identical type then doing a tuple compare.
MozReview-Commit-ID: 7IVSL2Asg9j
--- a/python/mozbuild/mozbuild/configure/options.py
+++ b/python/mozbuild/mozbuild/configure/options.py
@@ -49,19 +49,33 @@ class OptionValue(tuple):
if len(self):
return '%s=%s' % (option, ','.join(self))
return option
elif self and not len(self):
return '%s=1' % option
return '%s=%s' % (option, ','.join(self))
def __eq__(self, other):
- if type(other) != type(self):
+ # This is to catch naive comparisons against strings and other
+ # types in moz.configure files, as it is really easy to write
+ # value == 'foo'.
+ if not isinstance(other, tuple):
+ raise TypeError('cannot compare a %s against an %s; OptionValue '
+ 'instances are tuples - did you mean to compare '
+ 'against member elements using [x]?' % (
+ type(other).__name__, type(self).__name__))
+
+ # Allow explicit tuples to be compared.
+ if type(other) == tuple:
+ return tuple.__eq__(self, other)
+ # Else we're likely an OptionValue class.
+ elif type(other) != type(self):
return False
- return super(OptionValue, self).__eq__(other)
+ else:
+ return super(OptionValue, self).__eq__(other)
def __ne__(self, other):
return not self.__eq__(other)
def __repr__(self):
return '%s%s' % (self.__class__.__name__,
super(OptionValue, self).__repr__())
--- a/python/mozbuild/mozbuild/test/configure/test_options.py
+++ b/python/mozbuild/mozbuild/test/configure/test_options.py
@@ -9,16 +9,17 @@ import unittest
from mozunit import main
from mozbuild.configure.options import (
CommandLineHelper,
ConflictingOptionError,
InvalidOptionError,
NegativeOptionValue,
Option,
+ OptionValue,
PositiveOptionValue,
)
class Option(Option):
def __init__(self, *args, **kwargs):
kwargs['help'] = 'Dummy help'
super(Option, self).__init__(*args, **kwargs)
@@ -271,16 +272,36 @@ class TestOption(unittest.TestCase):
self.assertEquals(e.exception.message,
"'e' is not one of 'a', 'b', 'c', 'd'")
with self.assertRaises(InvalidOptionError) as e:
option.get_value('--with-option=e')
self.assertEquals(e.exception.message,
"'e' is not one of 'a', 'b', 'c', 'd'")
+ def test_option_value_compare(self):
+ # OptionValue are tuple and equivalence should compare as tuples.
+ val = PositiveOptionValue(('foo',))
+
+ self.assertEqual(val[0], 'foo')
+ self.assertEqual(val, PositiveOptionValue(('foo',)))
+ self.assertNotEqual(val, PositiveOptionValue(('foo', 'bar')))
+
+ # Can compare a tuple to an OptionValue.
+ self.assertEqual(val, ('foo',))
+ self.assertNotEqual(val, ('foo', 'bar'))
+
+ # Different OptionValue types are never equal.
+ self.assertNotEqual(val, OptionValue(('foo',)))
+
+ # For usability reasons, we raise TypeError when attempting to compare
+ # against a non-tuple.
+ with self.assertRaisesRegexp(TypeError, 'cannot compare a'):
+ val == 'foo'
+
def test_option_value_format(self):
val = PositiveOptionValue()
self.assertEquals('--with-value', val.format('--with-value'))
self.assertEquals('--with-value', val.format('--without-value'))
self.assertEquals('--enable-value', val.format('--enable-value'))
self.assertEquals('--enable-value', val.format('--disable-value'))
self.assertEquals('--value', val.format('--value'))
self.assertEquals('VALUE=1', val.format('VALUE'))