astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
32.66k stars 1.09k forks source link

Inconsistent implementation of F811 with respect to submodule imports #13490

Open AlexWaygood opened 1 month ago

AlexWaygood commented 1 month ago

Ruff emits no F811 diagnostics for this snippet:

import ctypes
import ctypes.wintypes

However, it emits an F811 diagnostic for this snippet:

import importlib

ctypes = importlib.import_module("ctypes")
import ctypes.wintypes

I think Ruff should either emit a diagnostic for both of these, or neither of these, since they're equivalent in terms of what happens with respect to symbol definitions:

  1. A ctypes symbol is defined. For the first snippet this is defined via an import statement, and for the second it is defined via a function call, but this difference shouldn't really be relevant.
  2. The import ctypes.wintypes statement causes:
    1. The ctypes variable to be reassigned in the global namespace (but to the same object, which is just looked up in the sys.modules cache)
    2. The ctypes.wintypes submodule to be loaded and stored as a wintypes attribute on the ctypes module object

I would lean towards not emitting a diagnostic on either of these, because although the submodule import of ctypes.wintypes is strictly-speaking a redefinition of the ctypes variable in both cases, many people consider it unidiomatic to access top-level members from ctypes in a module that only has import ctypes.wintypes in it. (It's sort-of an "implicit import" of the top-level ctypes module.)

To be clear: I'm not suggesting special-casing the importlib.import_module. I'm suggesting that a submodule foo.bar import should be considered a redefinition of a prior foo.bar import, and should never be considered a redefinition of a previously defined foo symbol.

This came up in https://github.com/python/cpython/pull/124384.

Terms I searched for before filing the issue:

zanieb commented 1 month ago

I agree we should probably not emit in the second case.