From 8899f8b5b5dfc84e8e0481a5587d0285c6835ec5 Mon Sep 17 00:00:00 2001 From: Eric Fried Date: Fri, 7 Jun 2019 14:17:31 -0500 Subject: [PATCH] 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 --- .../migrate_repo/versions/216_havana.py | 2 +- nova/hacking/checks.py | 21 ++++++++++++++++ nova/tests/unit/test_hacking.py | 24 +++++++++++++++++++ nova/tests/unit/virt/xenapi/stubs.py | 2 +- nova/virt/libvirt/driver.py | 4 ++-- 5 files changed, 49 insertions(+), 4 deletions(-) diff --git a/nova/db/sqlalchemy/migrate_repo/versions/216_havana.py b/nova/db/sqlalchemy/migrate_repo/versions/216_havana.py index efb29a40a178..65e652e429f5 100644 --- a/nova/db/sqlalchemy/migrate_repo/versions/216_havana.py +++ b/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], diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py index 0f3cf06572f2..5ac954fa54eb 100644 --- a/nova/hacking/checks.py +++ b/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) diff --git a/nova/tests/unit/test_hacking.py b/nova/tests/unit/test_hacking.py index aa519e91d668..00631e07b561 100644 --- a/nova/tests/unit/test_hacking.py +++ b/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) diff --git a/nova/tests/unit/virt/xenapi/stubs.py b/nova/tests/unit/virt/xenapi/stubs.py index 4864675eacd2..ace30720c0f1 100644 --- a/nova/tests/unit/virt/xenapi/stubs.py +++ b/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'): diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index a4572d322ec2..d633bdfd8fda 100644 --- a/nova/virt/libvirt/driver.py +++ b/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'