-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ANN401 doesn't work in overloads #5803
Comments
Hey, thanks for using Ruff and opening this issue.
You've mentioned here that flake8 --extend-select=ANN401 test.py It's also mentioned in the plugin README that this rule isn't supported for nested dynamic types (https://github.com/sco1/flake8-annotations#dynamic-typing-caveats):
Ruff identifies redefinition
Yes, this is true as is the case for
This seems like a bug as it should be raising violations similar to the plugin. Thanks for noticing this! Overall, I think we should improve the documentation to be clear on where this rule will be checked and enable it on |
(Omitting Any in a concrete overload might be desirable, is that not a common pattern?) |
I'm not sure if it's a common pattern as the concrete annotation would probably be a union of all possible types. Although, |
Oh, you're actually right, it seems that That's pretty interesting, so in that case, I'd like to make this a feature request, as I think there is value in having a linter rule capable of finding use of I'm not sure if the As for the overload, after thinking on it for a bit, yeah, it should probably not behave like this, even though to the outside user of that function, the |
If possible can you open a new issue for this (better for tracking)? Otherwise I can open it later as well :)
I agree. Any thoughts here? /cc @charliermarsh @zanieb |
Regarding generic arguments, from typing import reveal_type
def foo(**kwargs):
reveal_type(kwargs) # dict[str, Any] It seems better to use a generic that's partially complete than not at all. It would probably be reasonable to allow people to opt-in to enforcing narrower types though. I see a less controversial diagnostic for cases like Variable annotations seem totally reasonable to address. I agree best practice for the overloaded function is a union of all overload types. I think we should raise a violation here and consider implementing an autofix. Type checkers will not detect issues within the function body otherwise. |
Alright, I've opened a separate issue for the feature request, leaving this one just as a bug report. By the way, I've also noticed another behavior that Ruff doesn't pick up on, this one might be harder to address though, and perhaps something that should be left for a type checker to identify like with mypy's dynamic typing disallowing options , and I would understand if it was not something you'll want to fix: from typing import TypeAlias, Any
MyAny: TypeAlias = Any
def foo(x: MyAny) -> None:
... In this code, Ruff doesn't report any annotation issues, even though |
Thanks!
Yes, type aliases are a known issue across multiple rules as they're difficult to detect reliably. |
That's totally fair, though it should probably be mentioned in the rule's docs, to make this clear. |
Note that some of the points I made here were moved to be addressed from another issue: #5871, as requested. These points were not actually bugs, and this behavior was intentionally not a part of ANN401, as these weren't a part of this rule in the original
flake8-annotations
extension. The new issue is a feature request, asking for these to be added, leaving this issue purely as a bug report from one of the points I listed, which was indeed a valid bug. I've marked the points that were moved as strikethrough and you can ignore those when reading the issue description. If you're interested in those, check out the linked issue instead.It seems that Ruff's implementation of ANN401 is very permissive, and probably unintentionally incomplete.
Unlike theflake8-annotation
extension, ruff doesn't seem to considerAny
being used as a generic argument an issue.Regular variable annotations (including class variables) don't trigger ANN401Any
(might be intentional though)I ran ruff with
ruff check --isolated --fix --show-source --show-fixes --select ANN401 test.py
, on version 0.0.278.Any in generic argumentsThis should be considered as an issue, but isn't. In case you'd feel like this would be too strict, there should at least be a new rule capable of enforcing this. Howeverflake8-annotations
would consider this as an issue.Any in variable annotationsNone of these are currently marked as violations, even though it's a clear use of the Any type as an annotation. It seems that ANN401 only triggers when used in a function annotation, variables are not checked.Any in concrete overload
I assume that this is intentional, but I didn't find any docs to support that (I looked here).
If it is indeed intentional, it should maybe be mentioned somewhere? As someone moving from
flake8-annotations
, suddenly seeing ruff report an unnecessary noqa: ANN401 there is surprising, and even though I think it makes sense here, it could be worth documenting that this is a special case, exempt from ANN401.Edit note: This behavior actually does NOT make sense, and is indeed a bug see #5803 (comment):
The text was updated successfully, but these errors were encountered: