Bug 1375231 - Make OptionValue.__eq__ more type aware; r?glandium draft
authorGregory Szorc <gps@mozilla.com>
Thu, 06 Jul 2017 17:53:45 -0700
changeset 605069 54e61fb4018734208c8e48869b7fcebee7fd5f5a
parent 605068 9eafc29dad14f6ee3bcbd71f5e30f7a36b1df491
child 636386 505da61d2f92b25c3f9af50f2160e2f7e4d6d3b9
push id67290
push usergszorc@mozilla.com
push dateFri, 07 Jul 2017 00:53:56 +0000
reviewersglandium
bugs1375231
milestone56.0a1
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
python/mozbuild/mozbuild/configure/options.py
python/mozbuild/mozbuild/test/configure/test_options.py
--- 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'))