diff --git a/integration_tests/test_with_threading_lock.py b/integration_tests/test_with_threading_lock.py new file mode 100644 index 00000000..72add390 --- /dev/null +++ b/integration_tests/test_with_threading_lock.py @@ -0,0 +1,21 @@ +from core_codemods.with_threading_lock import WithThreadingLock +from integration_tests.base_test import ( + BaseIntegrationTest, + original_and_expected_from_code_path, +) + + +class TestWithThreadingLock(BaseIntegrationTest): + codemod = WithThreadingLock + code_path = "tests/samples/with_threading_lock.py" + original_code, expected_new_code = original_and_expected_from_code_path( + code_path, + [ + (1, "lock = threading.Lock()\n"), + (2, "with lock:\n"), + (4, ' print("Hello")\n'), + ], + ) + expected_diff = '--- \n+++ \n@@ -1,3 +1,4 @@\n import threading\n-with threading.Lock():\n+lock = threading.Lock()\n+with lock:\n print("Hello")\n' + expected_line_change = "2" + change_description = WithThreadingLock.CHANGE_DESCRIPTION diff --git a/src/core_codemods/__init__.py b/src/core_codemods/__init__.py index eb1cff26..041db883 100644 --- a/src/core_codemods/__init__.py +++ b/src/core_codemods/__init__.py @@ -21,6 +21,7 @@ from .upgrade_sslcontext_tls import UpgradeSSLContextTLS from .url_sandbox import UrlSandbox from .use_walrus_if import UseWalrusIf +from .with_threading_lock import WithThreadingLock registry = CodemodCollection( origin="pixee", @@ -48,5 +49,6 @@ UpgradeSSLContextTLS, UrlSandbox, UseWalrusIf, + WithThreadingLock, ], ) diff --git a/src/core_codemods/docs/pixee_python_bad-lock-with-statement.md b/src/core_codemods/docs/pixee_python_bad-lock-with-statement.md new file mode 100644 index 00000000..55530232 --- /dev/null +++ b/src/core_codemods/docs/pixee_python_bad-lock-with-statement.md @@ -0,0 +1,15 @@ +This codemod separates creating a threading lock instance from calling it as a context manager. +Calling `with threading.Lock()` does not have the effect you would expect. The lock is not acquired. +Instead, to correctly acquire a lock, create the instance separately, before calling it as a context manager. + +The change will apply to any of these `threading` classes: `Lock`, `RLock`, `Condition`, `Semaphore`, and `BoundedSemaphore`. + +The change looks like this: + +```diff + import threading +- with threading.Lock(): ++ lock = threading.Lock() ++ with lock: + ... +``` diff --git a/src/core_codemods/with_threading_lock.py b/src/core_codemods/with_threading_lock.py index 022106f0..349bb48f 100644 --- a/src/core_codemods/with_threading_lock.py +++ b/src/core_codemods/with_threading_lock.py @@ -5,7 +5,7 @@ class WithThreadingLock(SemgrepCodemod): NAME = "bad-lock-with-statement" - SUMMARY = "Replace deprecated usage of threading.Lock context manager" + SUMMARY = "Replace deprecated usage of threading lock classes as context managers" REVIEW_GUIDANCE = ReviewGuidance.MERGE_AFTER_CURSORY_REVIEW DESCRIPTION = "Separates threading lock instantiation and call with `with` statement into two steps." @@ -20,7 +20,12 @@ def rule(cls): - metavariable-pattern: metavariable: $BODY patterns: - - pattern: threading.Lock() + - pattern-either: + - pattern: threading.Lock() + - pattern: threading.RLock() + - pattern: threading.Condition() + - pattern: threading.Semaphore() + - pattern: threading.BoundedSemaphore() - pattern-inside: | import threading ... diff --git a/tests/codemods/test_with_threading_lock.py b/tests/codemods/test_with_threading_lock.py index 2e5d65fb..483cf0a8 100644 --- a/tests/codemods/test_with_threading_lock.py +++ b/tests/codemods/test_with_threading_lock.py @@ -1,6 +1,18 @@ +import pytest from core_codemods.with_threading_lock import WithThreadingLock from tests.codemods.base_codemod_test import BaseSemgrepCodemodTest +each_class = pytest.mark.parametrize( + "klass", + [ + "Lock", + "RLock", + "Condition", + "Semaphore", + "BoundedSemaphore", + ], +) + class TestWithThreadingLock(BaseSemgrepCodemodTest): codemod = WithThreadingLock @@ -8,52 +20,56 @@ class TestWithThreadingLock(BaseSemgrepCodemodTest): def test_rule_ids(self): assert self.codemod.name() == "bad-lock-with-statement" - def test_import(self, tmpdir): - input_code = """import threading -with threading.Lock(): + @each_class + def test_import(self, tmpdir, klass): + input_code = f"""import threading +with threading.{klass}(): ... """ - expected = """import threading -lock = threading.Lock() + expected = f"""import threading +lock = threading.{klass}() with lock: ... """ self.run_and_assert(tmpdir, input_code, expected) - def test_from_import(self, tmpdir): - input_code = """from threading import Lock -with Lock(): + @each_class + def test_from_import(self, tmpdir, klass): + input_code = f"""from threading import {klass} +with {klass}(): ... """ - expected = """from threading import Lock -lock = Lock() + expected = f"""from threading import {klass} +lock = {klass}() with lock: ... """ self.run_and_assert(tmpdir, input_code, expected) - def test_simple_replacement_with_as(self, tmpdir): - input_code = """import threading -with threading.Lock() as foo: + @each_class + def test_simple_replacement_with_as(self, tmpdir, klass): + input_code = f"""import threading +with threading.{klass}() as foo: ... """ - expected = """import threading -lock = threading.Lock() + expected = f"""import threading +lock = threading.{klass}() with lock as foo: ... """ self.run_and_assert(tmpdir, input_code, expected) - def test_no_effect_sanity_check(self, tmpdir): - input_code = """import threading -with threading.Lock(): + @each_class + def test_no_effect_sanity_check(self, tmpdir, klass): + input_code = f"""import threading +with threading.{klass}(): ... with threading_lock(): ... """ - expected = """import threading -lock = threading.Lock() + expected = f"""import threading +lock = threading.{klass}() with lock: ... @@ -62,10 +78,11 @@ def test_no_effect_sanity_check(self, tmpdir): """ self.run_and_assert(tmpdir, input_code, expected) - def test_no_effect_multiple_with_clauses(self, tmpdir): + @each_class + def test_no_effect_multiple_with_clauses(self, tmpdir, klass): """This is currently an unsupported case""" - input_code = """import threading -with threading.Lock(), foo(): + input_code = f"""import threading +with threading.{klass}(), foo(): ... """ self.run_and_assert(tmpdir, input_code, input_code) diff --git a/tests/samples/with_threading_lock.py b/tests/samples/with_threading_lock.py new file mode 100644 index 00000000..375a3906 --- /dev/null +++ b/tests/samples/with_threading_lock.py @@ -0,0 +1,3 @@ +import threading +with threading.Lock(): + print("Hello")