Check for `Any` in other types for `ANN401` #5601

dhruvmanila · 2023-07-07T20:38:51Z

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

dhruvmanila · 2023-07-07T20:39:04Z

Current dependencies on/for this PR:

main
- PR Check for Any in other types for ANN401 #5601 👈
  - PR Consider single element subscript expr for implicit optional #5717

This comment was auto-generated by Graphite.

github-actions · 2023-07-07T21:13:08Z

PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.03      8.6±0.05ms     4.7 MB/sec    1.00      8.4±0.03ms     4.9 MB/sec
formatter/numpy/ctypeslib.py               1.03   1940.9±3.08µs     8.6 MB/sec    1.00   1893.5±4.20µs     8.8 MB/sec
formatter/numpy/globals.py                 1.01    206.8±1.06µs    14.3 MB/sec    1.00    204.3±0.33µs    14.4 MB/sec
formatter/pydantic/types.py                1.04      4.3±0.01ms     5.9 MB/sec    1.00      4.2±0.01ms     6.1 MB/sec
linter/all-rules/large/dataset.py          1.00     14.1±0.11ms     2.9 MB/sec    1.01     14.3±0.11ms     2.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.5±0.02ms     4.7 MB/sec    1.00      3.6±0.01ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.00    364.3±1.25µs     8.1 MB/sec    1.01    369.0±2.21µs     8.0 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.3±0.03ms     4.1 MB/sec    1.02      6.4±0.05ms     4.0 MB/sec
linter/default-rules/large/dataset.py      1.00      7.1±0.03ms     5.7 MB/sec    1.01      7.2±0.03ms     5.7 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1466.1±5.45µs    11.4 MB/sec    1.02   1500.1±5.44µs    11.1 MB/sec
linter/default-rules/numpy/globals.py      1.00    155.1±0.25µs    19.0 MB/sec    1.03    159.9±0.90µs    18.5 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.2±0.01ms     8.0 MB/sec    1.02      3.2±0.01ms     7.9 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.03     12.7±0.41ms     3.2 MB/sec    1.00     12.4±0.29ms     3.3 MB/sec
formatter/numpy/ctypeslib.py               1.05      2.9±0.34ms     5.7 MB/sec    1.00      2.8±0.07ms     6.0 MB/sec
formatter/numpy/globals.py                 1.00   320.1±12.87µs     9.2 MB/sec    1.00   320.1±15.93µs     9.2 MB/sec
formatter/pydantic/types.py                1.00      6.1±0.20ms     4.2 MB/sec    1.00      6.1±0.20ms     4.2 MB/sec
linter/all-rules/large/dataset.py          1.02     21.4±0.54ms  1949.2 KB/sec    1.00     21.0±0.57ms  1981.4 KB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      5.5±0.19ms     3.0 MB/sec    1.00      5.4±0.20ms     3.1 MB/sec
linter/all-rules/numpy/globals.py          1.00   652.1±23.18µs     4.5 MB/sec    1.04   679.3±53.73µs     4.3 MB/sec
linter/all-rules/pydantic/types.py         1.00      9.5±0.35ms     2.7 MB/sec    1.00      9.5±0.25ms     2.7 MB/sec
linter/default-rules/large/dataset.py      1.02     10.9±0.36ms     3.7 MB/sec    1.00     10.6±0.18ms     3.8 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.02      2.2±0.09ms     7.4 MB/sec    1.00      2.2±0.06ms     7.5 MB/sec
linter/default-rules/numpy/globals.py      1.00    271.3±7.94µs    10.9 MB/sec    1.00   270.0±11.35µs    10.9 MB/sec
linter/default-rules/pydantic/types.py     1.01      4.8±0.18ms     5.3 MB/sec    1.00      4.8±0.15ms     5.3 MB/sec

dhruvmanila · 2023-07-08T05:38:44Z

crates/ruff/src/rules/flake8_annotations/rules/definition.rs

    annotation: &Expr,
    func: F,
    diagnostics: &mut Vec<Diagnostic>,
-    is_overridden: bool,


I've moved the is_overriden check outside the function as it seems common. This avoids having many arguments to a function.

dhruvmanila · 2023-07-08T05:39:52Z

crates/ruff/src/rules/ruff/mod.rs

@@ -1,6 +1,7 @@
 //! Ruff-specific rules.

 pub(crate) mod rules;
+pub(crate) mod typing;


Is this the correct place to keep the common functionality? We could move this to ruff_python_ast crate which would add dependencies to other crates mainly ruff_python_semantic.

It's probably okay for now, but you may be able to make it pub(super).

dhruvmanila · 2023-07-08T05:44:07Z

crates/ruff/src/rules/ruff/typing.rs

+    /// Special type used to represent an unknown type (and not a typing target).
+    /// This could be a type alias. It's also used when parsing a forward
+    /// reference fails.
+    ///
+    /// Note that this is not the same as returning `None` in `try_from_expr` as
+    /// `None` means that it's a known type but we don't know which one.
+    Unknown,


Here's the difference between None and Some(TypingTarget::Unknown):

None Some(TypingTarget::Unknown)

Known type but not a TypingTarget Unknown type and not a TypingTarget

When ExprSubscript.slice is not a ExprTuple Parsing fails for forward references

Flag violation Do NOT flag violation

My thought is to just remove the None variant. This means in failure we would not flag violation (currently we would). Any thoughts? \cc @charliermarsh

Could we replace None with Known, and use Result to return an error when "Parsing fails for forward references" and "When ExprSubscript.slice is not a ExprTuple"? Even if we can't use Result, I think it makes sense to encode "Known type but not a TypingTarget" as its own variant. It's confusing that it's represented by None.

Even if we can't use Result, I think it makes sense to encode "Known type but not a TypingTarget" as its own variant. It's confusing that it's represented by None.

This makes sense. Thanks!

I'm going with Option<...> and not flagging any violation in case of None.

dhruvmanila · 2023-07-10T05:38:12Z

(forced push to check ecosystem result via links :))

crates/ruff/src/rules/ruff/typing.rs

dhruvmanila · 2023-07-12T17:43:24Z

Updates since last review

Explicit Known type for known types and not TypingTarget
Refactor to use match expressions wherever possible
Fix a bug where single element like Union[None] would give false positive for implicit optionals (done in the child PR)

The (3) was due to the fact that None represented the "When ExprSubscript.slice is not a ExprTuple" case but the slice could be other types as well such as ExprConstant or ExprName in Union[None] and Union[int] respectively.

dhruvmanila · 2023-07-12T18:20:23Z

crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF013_RUF013_0.py.snap

-29 | def f(arg: typing.List[str] = None):  # RUF013
-   |            ^^^^^^^^^^^^^^^^ RUF013
-30 |     pass


This is intentional in this PR and is fixed in the child PR.

charliermarsh · 2023-07-12T20:47:28Z

Still LGTM.

dhruvmanila · 2023-07-13T05:41:41Z

(Re-opening the PR to trigger CI, don't know why it went into "waiting...")

dhruvmanila · 2023-07-13T05:58:37Z

FYI, the PR stack is in reverse order which is the reason for pre-commit failure. Re-ordering involved resolving a bunch of merge conflicts and so I decided not to do it. I didn't realize the bug until quite later. Sorry for any failure notification 😅

## 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

dhruvmanila marked this pull request as ready for review July 8, 2023 05:37

dhruvmanila commented Jul 8, 2023

View reviewed changes

dhruvmanila force-pushed the dhruv/typing-target-contains-any branch from 9840a4d to 1f9910b Compare July 10, 2023 05:23

dhruvmanila requested a review from charliermarsh July 10, 2023 05:38

charliermarsh reviewed Jul 12, 2023

View reviewed changes

crates/ruff/src/rules/ruff/typing.rs Outdated Show resolved Hide resolved

charliermarsh approved these changes Jul 12, 2023

View reviewed changes

dhruvmanila force-pushed the dhruv/typing-target-contains-any branch from 452e65e to 2ef8500 Compare July 12, 2023 17:39

dhruvmanila mentioned this pull request Jul 12, 2023

Consider single element subscript expr for implicit optional #5717

Merged

dhruvmanila requested a review from charliermarsh July 12, 2023 17:43

dhruvmanila commented Jul 12, 2023

View reviewed changes

dhruvmanila removed the request for review from charliermarsh July 12, 2023 18:34

dhruvmanila closed this Jul 13, 2023

dhruvmanila reopened this Jul 13, 2023

dhruvmanila added the rule Implementing or modifying a lint rule label Jul 13, 2023

dhruvmanila enabled auto-merge (squash) July 13, 2023 05:42

dhruvmanila disabled auto-merge July 13, 2023 05:58

dhruvmanila added 5 commits July 13, 2023 11:56

Move TypingTarget related logic out of rule file

f8e5acc

Check if TypingTarget can be resolved to Any

1553d2a

Add Unknown type to keep Any separate

53f4a2a

Tests

713d500

Explicit Known type, avoid flagging in case of failures

182def1

dhruvmanila added 3 commits July 13, 2023 11:56

Wrap the only expr in the vector instead of panic

b082d10

Remove bugfix code (results in false negatives)

efbfc9c

Temporarily disable ANN401 in scripts

01866f9

dhruvmanila force-pushed the dhruv/typing-target-contains-any branch from 63894df to 01866f9 Compare July 13, 2023 06:42

dhruvmanila merged commit f44acc0 into main Jul 13, 2023

dhruvmanila deleted the dhruv/typing-target-contains-any branch July 13, 2023 12:49

chenrui333 mentioned this pull request Jul 21, 2023

ruff 0.0.279 Homebrew/homebrew-core#137262

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for `Any` in other types for `ANN401` #5601

Check for `Any` in other types for `ANN401` #5601

dhruvmanila commented Jul 7, 2023 •

edited

Loading

dhruvmanila commented Jul 7, 2023 •

edited

Loading

github-actions bot commented Jul 7, 2023 •

edited

Loading

dhruvmanila Jul 8, 2023

dhruvmanila Jul 8, 2023

charliermarsh Jul 12, 2023

dhruvmanila Jul 8, 2023

charliermarsh Jul 12, 2023

dhruvmanila Jul 12, 2023

dhruvmanila commented Jul 10, 2023

dhruvmanila commented Jul 12, 2023 •

edited

Loading

dhruvmanila Jul 12, 2023

charliermarsh commented Jul 12, 2023

dhruvmanila commented Jul 13, 2023

dhruvmanila commented Jul 13, 2023

`None`	`Some(TypingTarget::Unknown)`
Known type but not a `TypingTarget`	Unknown type and not a `TypingTarget`
When `ExprSubscript.slice` is not a `ExprTuple`	Parsing fails for forward references
Flag violation	Do NOT flag violation

Check for Any in other types for ANN401 #5601

Check for Any in other types for ANN401 #5601

Conversation

dhruvmanila commented Jul 7, 2023 • edited Loading

Summary

Test Plan

dhruvmanila commented Jul 7, 2023 • edited Loading

github-actions bot commented Jul 7, 2023 • edited Loading

PR Check Results

Benchmark

Linux

Windows

dhruvmanila Jul 8, 2023

Choose a reason for hiding this comment

dhruvmanila Jul 8, 2023

Choose a reason for hiding this comment

charliermarsh Jul 12, 2023

Choose a reason for hiding this comment

dhruvmanila Jul 8, 2023

Choose a reason for hiding this comment

charliermarsh Jul 12, 2023

Choose a reason for hiding this comment

dhruvmanila Jul 12, 2023

Choose a reason for hiding this comment

dhruvmanila commented Jul 10, 2023

dhruvmanila commented Jul 12, 2023 • edited Loading

Updates since last review

dhruvmanila Jul 12, 2023

Choose a reason for hiding this comment

charliermarsh commented Jul 12, 2023

dhruvmanila commented Jul 13, 2023

dhruvmanila commented Jul 13, 2023

Check for `Any` in other types for `ANN401` #5601

Check for `Any` in other types for `ANN401` #5601

dhruvmanila commented Jul 7, 2023 •

edited

Loading

dhruvmanila commented Jul 7, 2023 •

edited

Loading

github-actions bot commented Jul 7, 2023 •

edited

Loading

dhruvmanila commented Jul 12, 2023 •

edited

Loading