Skip to content

B018 false positive on attribute access with side effect #10536

@jaraco

Description

@jaraco

Using ruff 0.3.4 and attempting to adopt bugbear (B) checks in the keyring project, I'm encountering what appear to be false positives:

 keyring main @ ruff check --select B018 --fix --unsafe-fixes
keyring/backend.py:71:13: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/backends/SecretService.py:35:13: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/backends/Windows.py:14:9: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/backends/Windows.py:21:9: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/core.py:137:5: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/testing/backend.py:177:13: B018 Found useless expression. Either assign it to a variable or remove it.

(there was one other prior to jaraco/keyring@7881db6)

Looking at keyring.backend:71, it's apparent that the "useless expression" is cls.priority. The protocol for keyring, however, is for a backend to raise an exception in the .priority access if the backend is not viable at all.

Looking at keyring.backends.Windows:14, the purpose of the attribute access is explicitly documented (force demand import to import the module).

The advice is wrong. In particular, the expression isn't useless (the application would fail without the side effect of accessing the property), and the guidance is misleading (assigning to a variable would introduce more lint and removing it would break the the behavior).

I can imagine one possible workaround is to replace direct attribute access with a getattr call:

diff --git a/keyring/backend.py b/keyring/backend.py
index a21418f..e082fbe 100644
--- a/keyring/backend.py
+++ b/keyring/backend.py
@@ -68,7 +68,7 @@ class KeyringBackend(metaclass=KeyringBackendMeta):
     @properties.classproperty
     def viable(cls):
         with errors.ExceptionRaisedContext() as exc:
-            cls.priority
+            getattr(cls, 'priority')
         return not exc
 
     @classmethod
diff --git a/keyring/backends/Windows.py b/keyring/backends/Windows.py
index 7208ed1..5d057ec 100644
--- a/keyring/backends/Windows.py
+++ b/keyring/backends/Windows.py
@@ -11,7 +11,7 @@ with ExceptionRaisedContext() as missing_deps:
         from win32ctypes.pywin32 import pywintypes, win32cred
 
         # force demand import to raise ImportError
-        win32cred.__name__
+        getattr(win32cred, '__name__')
     except ImportError:
         # fallback to pywin32
         import pywintypes

However, this approach makes the code less readable and idiosyncratic (the canonical way to access an attribute is with dot notation), and making the change introduces B009 errors:

 keyring main @ ruff check --select B009
keyring/backend.py:71:13: B009 [*] Do not call `getattr` with a constant attribute value. It is not any safer than normal property access.
keyring/backends/Windows.py:14:9: B009 [*] Do not call `getattr` with a constant attribute value. It is not any safer than normal property access.

Another workaround could be to disable B018 checks for all projects (or possibly just this project; I'm not yet sure how much this issue is isolated to keyring). That would be somewhat undesirable. It would be nice to get an error for failing to call a method (e.g. self._init).

The workaround I'm leaning toward is to put noqa: B018 on each of these expressions, but that feels messy.

Since I'm not confident that ruff could readily distinguish between a property access that has a side-effect and a useless property access, I wonder if the documentation should at least capture this scenario and make a recommendation on what to do in that case.

Metadata

Metadata

Assignees

Labels

documentationImprovements or additions to documentation

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions