[flake8-pyi] Ignore 'unused' private type dicts in class scopes#9952
[flake8-pyi] Ignore 'unused' private type dicts in class scopes#9952charliermarsh merged 2 commits intomainfrom
flake8-pyi] Ignore 'unused' private type dicts in class scopes#9952Conversation
flake8-pyi] Ignore 'unused' private type dicts in class scopes
|
AlexWaygood
left a comment
There was a problem hiding this comment.
I think maybe this rule should work differently for .py and .pyi files.
For a .py file, it makes sense not to flag something like this since, as you say, it's an attribute and maybe it's used elsewhere:
class Foo:
class _Unused(TypeDict):
passBut in a .pyi file, I'd definitely still want that flagged. (I'm not sure why you'd do something like that in a .pyi file... but I still feel like the principle is important 😄)
In a .pyi file, it's still highly likely that the TypedDict class there doesn't exist at runtime, even though it's inside the class scope. One of the heuristics these rules rely on (which in general is nearly always accurate) is the idea that TypedDicts, protocols, type aliases and protocols in stub files are nearly always "stub-only constructs" that don't exist at runtime, and are used purely for improving the annotations that we're able to supply in the stubs. I think that heuristic applies just as well to private definitions inside class scopes as it does to private definitions in the global scope.
|
Ok, I'll gate it by type. Makes sense. |
a8b43e9 to
ca822be
Compare
|
Fixed. |
AlexWaygood
left a comment
There was a problem hiding this comment.
LGTM.
I think there would still be a false positive if you did something like this in a .pyi file:
class Foo:
class _VeryPrivate(TypedDict):
x: str
bar: _VeryPrivateBut it seems pretty unlikely to actually come up. And I think this PR is probably an improvement as-is since it gets rid of the .py-file false positive (and I think it's probably correct to not worry about any of these rules inside class scopes in .py files). So I'm happy with this to go in as-is.
| # Error | ||
| class _CustomClass3: | ||
| class _UnusedTypeDict4(TypedDict): | ||
| pass | ||
|
|
||
| def f(self) -> None: | ||
| print(_CustomClass3._UnusedTypeDict4()) |
There was a problem hiding this comment.
Nit: maybe get rid of the print() call in the .pyi fixture here; I'm not sure it adds anything
| # Error | |
| class _CustomClass3: | |
| class _UnusedTypeDict4(TypedDict): | |
| pass | |
| def f(self) -> None: | |
| print(_CustomClass3._UnusedTypeDict4()) | |
| # Error | |
| # (in .pyi files, we flag unused definitions in class scopes | |
| # as well as the global scope, unlike in .py files) | |
| class _CustomClass3: | |
| class _UnusedTypeDict4(TypedDict): | |
| pass |
Hmm, no, it looks like that isn't flagged as being unused -- sorry, I should have checked. I think I might have slightly misunderstood the root cause of this issue here, in that case |
|
Ah, is the cause that the visitor recurses into class scopes when looking for references that might count as "usages", but does not recurse into method definitions inside class scopes? In that case, we should definitely be fine, since the bodies of methods in |
|
The root cause is that given: class A:
class _B(TypedDict):
pass
def f(self) -> None:
print(A._B())Our semantic model doesn't currently detect that But given: class Foo:
class _VeryPrivate(TypedDict):
x: str
bar: _VeryPrivateWe can resolve |
ca822be to
b19130e
Compare
|
Makes sense. I think this should be fine, then, since accessing private attributes in general is very rare in Sorry for the slightly confused review! |
…tral-sh#9952) ## Summary If these are defined within class scopes, they're actually attributes of the class, and can be accessed through the class itself. (We preserve our existing behavior for `.pyi` files.) Closes astral-sh#9948.
Summary
If these are defined within class scopes, they're actually attributes of the class, and can be accessed through the class itself.
(We preserve our existing behavior for
.pyifiles.)Closes #9948.