Skip to content
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

Should unions including Any trigger ANN401 (or some other rule)? #5458

Closed
bersbersbers opened this issue Jul 1, 2023 · 10 comments · Fixed by #5601
Closed

Should unions including Any trigger ANN401 (or some other rule)? #5458

bersbersbers opened this issue Jul 1, 2023 · 10 comments · Fixed by #5601
Assignees
Labels
accepted Ready for implementation rule Implementing or modifying a lint rule

Comments

@bersbersbers
Copy link
Contributor

from typing import Any

def fun_ann401(abc: Any) -> None:
    print(abc)

def fun_no_error(abc: Any | None = None) -> None:
    print(abc)

gives

bug.py:4:21: ANN401 Dynamically typed expressions (typing.Any) are disallowed in abc

I wonder why that error is not also emitted for fun_no_error. Any | None is at least as undefined as Any is.

Another option would be to add a new rule that says

Any | Foo is redundant and should be replaced by Any.

@charliermarsh
Copy link
Member

I think it should be included, yeah.

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Jul 2, 2023
@charliermarsh
Copy link
Member

Maybe we can reuse the logic that @dhruvmanila added for implicit optionals.

@dhruvmanila
Copy link
Member

Maybe we can reuse the logic that @dhruvmanila added for implicit optionals.

If I understand correctly, do you mean to check if there's typing.Any instead of None in the TypingTarget? Or, do you just mean to use the | union iterator to check if there's Any in it?

@charliermarsh
Copy link
Member

The latter! Does that make sense?

@dhruvmanila
Copy link
Member

dhruvmanila commented Jul 4, 2023

It does but then Union[Any, None] (which is same as Any | None) won't be triggered.

@dhruvmanila
Copy link
Member

Actually, this then brings up the question if we should look for Any in other places as well (Optional[Any]).

I believe the main question would be "does this type resolve to Any"?

@charliermarsh
Copy link
Member

Err, sorry, I may have misunderstood your question. We're just looking to understand whether the type resolves to Any, I think.

@dhruvmanila
Copy link
Member

Right, then I believe the following cases should also be considered although the original plugin doesn't support them.

  • Any | ...
  • Union[Any, ...]
  • Optional[Any]
  • Annotated[<any of the above variants>, ...]
  • Forward references?

Does this make sense?

I don't think Any in container types (e.g., list[Any]) should be considered, right?

@charliermarsh
Copy link
Member

Yes, agreed.

@dhruvmanila dhruvmanila self-assigned this Jul 5, 2023
@dhruvmanila
Copy link
Member

It should be simple enough to extract out the logic from implicit-optional check and add another method to check for Any.

@charliermarsh charliermarsh added the accepted Ready for implementation label Jul 10, 2023
konstin added a commit that referenced this issue Jul 11, 2023
See #5458 for detailed information

I've tried to split this into commits for individual rules as much as
feasible and left a bunch of TODO where i'm not sure about the rules

---------

Co-authored-by: Micha Reiser <[email protected]>
konstin added a commit that referenced this issue Jul 11, 2023
See #5458 for detailed information

I've tried to split this into commits for individual rules as much as
feasible and left a bunch of TODO where i'm not sure about the rules

---------

Co-authored-by: Micha Reiser <[email protected]>
konstin added a commit that referenced this issue Jul 12, 2023
See #5458 for detailed information

I've tried to split this into commits for individual rules as much as
feasible and left a bunch of TODO where i'm not sure about the rules

---------

Co-authored-by: Micha Reiser <[email protected]>
dhruvmanila added a commit that referenced this issue Jul 13, 2023
## Summary

Check for `Any` in other types for `ANN401`. This reuses the logic from
`implicit-optional` rule to resolve the type to `Any`.

Following types are supported:
* `Union[Any, ...]`
* `Any | ...`
* `Optional[Any]`
* `Annotated[<any of the above variant>, ...]`
* Forward references i.e., `"Any | ..."`

## Test Plan

Added test cases for various combinations.

fixes: #5458
konstin added a commit that referenced this issue Jul 13, 2023
See #5458 for detailed information

I've tried to split this into commits for individual rules as much as
feasible and left a bunch of TODO where i'm not sure about the rules

---------

Co-authored-by: Micha Reiser <[email protected]>
konstin added a commit that referenced this issue Jul 16, 2023
See #5458 for detailed information

I've tried to split this into commits for individual rules as much as
feasible and left a bunch of TODO where i'm not sure about the rules

---------

Co-authored-by: Micha Reiser <[email protected]>
konstin added a commit that referenced this issue Jul 18, 2023
See #5458 for detailed information

I've tried to split this into commits for individual rules as much as
feasible and left a bunch of TODO where i'm not sure about the rules

---------

Co-authored-by: Micha Reiser <[email protected]>
konstin pushed a commit that referenced this issue Jul 19, 2023
## Summary

Check for `Any` in other types for `ANN401`. This reuses the logic from
`implicit-optional` rule to resolve the type to `Any`.

Following types are supported:
* `Union[Any, ...]`
* `Any | ...`
* `Optional[Any]`
* `Annotated[<any of the above variant>, ...]`
* Forward references i.e., `"Any | ..."`

## Test Plan

Added test cases for various combinations.

fixes: #5458
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants