Hacking N363: `in (not_a_tuple)`

A few places in the code had conditionals including:

 if something in (element):

which was clearly intended to be

 if something in (element,):

(I.e. `in $tuple`, not `in element` with redundant parens) or just

 if something == element:

Fix those [1] and introduce hacking rule N363 to disallow this kind of
thing in the future.

[1] NOTE: These weren't actually latent bugs because

 'foo' in ('foo')

which is the same as

 'foo' in 'foo'

returns True. In order to be a bug, the left operand would have to be
able to be a substring of the right:

 'foo' in ('foobar')  # True
 'foo' in ('foobar',) # False

...which I don't think is possible in any of the scenarios found.

Change-Id: I950d07eb533e0d43466c58e36b314aaaf8560251

nova/db/sqlalchemy/migrate_repo/versions/216_havana.py

@@ -1507,7 +1507,7 @@ def upgrade(migrate_engine):
             ])
 
     for fkey_pair in fkeys:
-        if migrate_engine.name in ('mysql'):
+        if migrate_engine.name == 'mysql':
             # For MySQL we name our fkeys explicitly
             # so they match Havana
             fkey = ForeignKeyConstraint(columns=fkey_pair[0],

nova/hacking/checks.py

@@ -104,6 +104,16 @@ asse_regexpmatches = re.compile(
 privsep_file_re = re.compile('^nova/privsep[./]')
 privsep_import_re = re.compile(
     r"^(?:import|from).*\bprivsep\b")
+# Redundant parenthetical masquerading as a tuple, used with ``in``:
+# Space, "in", space, open paren
+# Optional single or double quote (so we match strings or symbols)
+# A sequence of the characters that can make up a symbol. (This is weak: a
+#   string can contain other characters; and a numeric symbol can start with a
+#   minus, and a method call has a param list, and... Not sure this gets better
+#   without a lexer.)
+# The same closing quote
+# Close paren
+disguised_as_tuple_re = re.compile(r''' in \((['"]?)[a-zA-Z0-9_.]+\1\)''')
 
 
 class BaseASTChecker(ast.NodeVisitor):
@@ -878,6 +888,16 @@ def privsep_imports_not_aliased(logical_line, filename):
                   "'from nova.privsep import path'.")
 
 
+def did_you_mean_tuple(logical_line):
+    """Disallow ``(not_a_tuple)`` because you meant ``(a_tuple_of_one,)``.
+
+    N363
+    """
+    if disguised_as_tuple_re.search(logical_line):
+        yield (0, "N363: You said ``in (not_a_tuple)`` when you almost "
+                  "certainly meant ``in (a_tuple_of_one,)``.")
+
+
 def factory(register):
     register(import_no_db_in_virt)
     register(no_db_session_in_public_api)
@@ -923,3 +943,4 @@ def factory(register):
     register(yield_followed_by_space)
     register(assert_regexpmatches)
     register(privsep_imports_not_aliased)
+    register(did_you_mean_tuple)

nova/tests/unit/test_hacking.py

@@ -894,3 +894,27 @@ class HackingTestCase(test.NoDBTestCase):
         for filename in (good_filenames + bad_filenames):
             self._assert_has_no_errors(
                 code, checks.privsep_imports_not_aliased, filename=filename)
+
+    def test_did_you_mean_tuple(self):
+        code = """
+                    if foo in (bar):
+                    if foo in ('bar'):
+                    if foo in (path.to.CONST_1234):
+                    if foo in (
+                        bar):
+               """
+        errors = [(x + 1, 0, 'N363') for x in range(4)]
+        self._assert_has_errors(
+            code, checks.did_you_mean_tuple, expected_errors=errors)
+        code = """
+                    def in(this_would_be_weird):
+                        # A match in (any) comment doesn't count
+                        if foo in (bar,)
+                            or foo in ('bar',)
+                            or foo in ("bar",)
+                            or foo in (set1 + set2)
+                            or foo in ("string continuations "
+                                "are probably okay")
+                            or foo in (method_call_should_this_work()):
+               """
+        self._assert_has_no_errors(code, checks.did_you_mean_tuple)

nova/tests/unit/virt/xenapi/stubs.py

@@ -63,7 +63,7 @@ class FakeSessionForVMTests(fake.SessionBase):
     def host_call_plugin(self, _1, _2, plugin, method, _5):
         plugin = plugin.rstrip('.py')
 
-        if plugin == 'glance' and method in ('download_vhd2'):
+        if plugin == 'glance' and method == 'download_vhd2':
             root_uuid = _make_fake_vdi()
             return pickle.dumps(dict(root=dict(uuid=root_uuid)))
         elif (plugin, method) == ('xenhost', 'iptables_config'):

nova/virt/libvirt/driver.py

@@ -2026,7 +2026,7 @@ class LibvirtDriver(driver.ComputeDriver):
         #               It is necessary in case this situation changes in the
         #               future.
         if (self._host.has_min_version(hv_type=host.HV_DRIVER_QEMU)
-                and source_type not in ('lvm')
+                and source_type != 'lvm'
                 and not CONF.ephemeral_storage_encryption.enabled
                 and not CONF.workarounds.disable_libvirt_livesnapshot
                 # NOTE(rmk): We cannot perform live snapshots when a
@@ -4832,7 +4832,7 @@ class LibvirtDriver(driver.ComputeDriver):
             # NOTE(ldbragst): PowerKVM doesn't support 'cirrus' be default
             # so use 'vga' instead when running on Power hardware.
             video.type = 'vga'
-        elif guestarch in (fields.Architecture.AARCH64):
+        elif guestarch == fields.Architecture.AARCH64:
             # NOTE(kevinz): Only virtio device type is supported by AARCH64
             # so use 'virtio' instead when running on AArch64 hardware.
             video.type = 'virtio'