Consider single element subscript expr for implicit optional

[dhruvmanila]

Summary

Consider single element subscript expr for implicit optional.

On main, the cases where there is only a single element in the subscript
list was giving false positives such as for the following:

typing.Union[None]
typing.Literal[None]

Test Plan

cargo test

[dhruvmanila]

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]

PR Check Results

Ecosystem

ℹ️ ecosystem check detected changes. (+56, -0, 0 error(s))

airflow (+28, -0)

+ airflow/decorators/base.py:333:16: PLR1714 Consider merging multiple comparisons: `ttype in (dict, Dict)`. Use a `set` if the elements are hashable.
+ airflow/decorators/task_group.py:150:13: PLR1714 Consider merging multiple comparisons: `v.kind in (inspect.Parameter.VAR_POSITIONAL, inspect.Parameter.VAR_KEYWORD)`. Use a `set` if the elements are hashable.
+ airflow/models/dagrun.py:704:12: PLR1714 Consider merging multiple comparisons: `self._state in (DagRunState.FAILED, DagRunState.SUCCESS)`. Use a `set` if the elements are hashable.
+ airflow/providers/amazon/aws/hooks/redshift_data.py:115:18: PLR1714 Consider merging multiple comparisons: `status in ("FAILED", "ABORTED")`. Use a `set` if the elements are hashable.
+ airflow/providers/google/cloud/hooks/kubernetes_engine.py:127:16: PLR1714 Consider merging multiple comparisons: `operation.status in (Operation.Status.RUNNING, Operation.Status.PENDING)`. Use a `set` if the elements are hashable.
+ airflow/providers/google/cloud/operators/bigquery_dts.py:373:20: PLR1714 Consider merging multiple comparisons: `state in (TransferState.FAILED, TransferState.CANCELLED)`. Use a `set` if the elements are hashable.
+ airflow/providers/google/cloud/operators/bigquery_dts.py:395:12: PLR1714 Consider merging multiple comparisons: `event["status"] in ("failed", "cancelled")`. Use a `set` if the elements are hashable.
+ airflow/providers/google/cloud/operators/dataflow.py:708:12: PLR1714 Consider merging multiple comparisons: `event["status"] in ("error", "stopped")`. Use a `set` if the elements are hashable.
+ airflow/providers/google/cloud/operators/dataflow.py:893:12: PLR1714 Consider merging multiple comparisons: `event["status"] in ("error", "stopped")`. Use a `set` if the elements are hashable.
+ airflow/providers/google/cloud/operators/dataproc.py:1795:12: PLR1714 Consider merging multiple comparisons: `event["status"] in ("failed", "error")`. Use a `set` if the elements are hashable.
+ airflow/providers/google/cloud/operators/dataproc.py:1920:12: PLR1714 Consider merging multiple comparisons: `event["status"] in ("failed", "error")`. Use a `set` if the elements are hashable.
+ airflow/providers/google/cloud/operators/kubernetes_engine.py:166:12: PLR1714 Consider merging multiple comparisons: `status in ("failed", "error")`. Use a `set` if the elements are hashable.
+ airflow/providers/google/cloud/operators/kubernetes_engine.py:373:12: PLR1714 Consider merging multiple comparisons: `status in ("failed", "error")`. Use a `set` if the elements are hashable.
+ airflow/providers/google/cloud/triggers/kubernetes_engine.py:202:22: PLR1714 Consider merging multiple comparisons: `status in (Operation.Status.RUNNING, Operation.Status.PENDING)`. Use a `set` if the elements are hashable.
+ airflow/providers/google/common/hooks/base_google.py:101:16: PLR1714 Consider merging multiple comparisons: `exception.resp.status in (429, 409)`. Use a `set` if the elements are hashable.
+ airflow/www/fab_security/manager.py:616:12: PLR1714 Consider merging multiple comparisons: `provider in ("github", "githublocal")`. Use a `set` if the elements are hashable.
+ airflow/www/views.py:3999:24: PLR1714 Consider merging multiple comparisons: `dep.dependency_type in ("dag", "dataset")`. Use a `set` if the elements are hashable.
+ dev/breeze/src/airflow_breeze/utils/host_info_utils.py:38:8: PLR1714 Consider merging multiple comparisons: `os in ("linux", "darwin")`. Use a `set` if the elements are hashable.
+ dev/breeze/src/airflow_breeze/utils/host_info_utils.py:48:8: PLR1714 Consider merging multiple comparisons: `os in ("linux", "darwin")`. Use a `set` if the elements are hashable.
+ dev/provider_packages/prepare_provider_packages.py:742:12: PLR1714 Consider merging multiple comparisons: `ex.returncode in (128, 2)`. Use a `set` if the elements are hashable.
+ docker_tests/test_docker_compose_quick_start.py:90:16: PLR1714 Consider merging multiple comparisons: `health_status in ("healthy", "no-check")`. Use a `set` if the elements are hashable.
+ helm_tests/airflow_aux/test_basic_helm_chart.py:42:12: PLR1714 Consider merging multiple comparisons: `version in ("2.3.2", "default")`. Use a `set` if the elements are hashable.
+ scripts/ci/pre_commit/pre_commit_check_lazy_logging.py:44:17: PLR1714 Consider merging multiple comparisons: `self.cur_node.func.value.id in ("logger", "logging", "log")`. Use a `set` if the elements are hashable.
+ scripts/ci/pre_commit/pre_commit_json_schema.py:145:8: PLR1714 Consider merging multiple comparisons: `default not in (instance, "See values.yaml")`. Use a `set` if the elements are hashable.
+ tests/jobs/test_backfill_job.py:1286:16: PLR1714 Consider merging multiple comparisons: `ti.task_id in ("leave1", "leave2")`. Use a `set` if the elements are hashable.
+ tests/jobs/test_triggerer_job.py:203:16: PLR1714 Consider merging multiple comparisons: `job_runner.capacity in (input_str, 1000)`. Use a `set` if the elements are hashable.
+ tests/providers/elasticsearch/log/elasticmock/fake_elasticsearch.py:372:16: PLR1714 Consider merging multiple comparisons: `target in ("_all", "")`. Use a `set` if the elements are hashable.
+ tests/providers/google/cloud/operators/test_bigquery.py:1829:16: PLR1714 Consider merging multiple comparisons: `missing_param.value.args[0] in (expected, expected1)`. Use a `set` if the elements are hashable.

bokeh (+4, -0)

+ src/bokeh/server/tornado.py:619:12: PLR1714 Consider merging multiple comparisons: `mode in ("server", "server-dev")`. Use a `set` if the elements are hashable.
+ tests/unit/bokeh/embed/test_server__embed.py:215:16: PLR1714 Consider merging multiple comparisons: `r in ("&foo=10&bar=baz", "&bar=baz&foo=10")`. Use a `set` if the elements are hashable.
+ tests/unit/bokeh/embed/test_server__embed.py:222:16: PLR1714 Consider merging multiple comparisons: `r in ("&foo=10&bar=baz", "&bar=baz&foo=10")`. Use a `set` if the elements are hashable.
+ tests/unit/bokeh/test_objects.py:575:16: PLR1714 Consider merging multiple comparisons: `i in (('a', 1), ('b', 2), ('c', 3))`. Use a `set` if the elements are hashable.

typeshed (+2, -0)

+ stdlib/builtins.pyi:867:27: PYI036 The first argument in `__exit__` should be annotated with `object` or `type[BaseException] | None`
+ stdlib/typing.pyi:758:23: PYI036 The first argument in `__exit__` should be annotated with `object` or `type[BaseException] | None`

zulip (+22, -0)

+ analytics/views/user_activity.py:63:12: PLR1714 Consider merging multiple comparisons: `k in ("name", "user_profile_id")`. Use a `set` if the elements are hashable.
+ tools/lib/sanity_check.py:17:12: PLR1714 Consider merging multiple comparisons: `user_name not in ("vagrant", "zulipdev")`. Use a `set` if the elements are hashable.
+ zerver/actions/create_realm.py:102:9: PLR1714 Consider merging multiple comparisons: `realm.org_type in (Realm.ORG_TYPES["education_nonprofit"]["id"], Realm.ORG_TYPES["education"]["id"])`. Use a `set` if the elements are hashable.
+ zerver/actions/create_realm.py:220:13: PLR1714 Consider merging multiple comparisons: `realm.org_type in (Realm.ORG_TYPES["education_nonprofit"]["id"], Realm.ORG_TYPES["education"]["id"])`. Use a `set` if the elements are hashable.
+ zerver/actions/custom_profile_fields.py:113:9: PLR1714 Consider merging multiple comparisons: `field.field_type in (CustomProfileField.SELECT, CustomProfileField.EXTERNAL_ACCOUNT)`. Use a `set` if the elements are hashable.
+ zerver/actions/custom_profile_fields.py:65:9: PLR1714 Consider merging multiple comparisons: `custom_profile_field.field_type in (CustomProfileField.SELECT, CustomProfileField.EXTERNAL_ACCOUNT)`. Use a `set` if the elements are hashable.
+ zerver/lib/email_notifications.py:528:24: PLR1714 Consider merging multiple comparisons: `m["trigger"] in (NotificationTriggers.MENTION, NotificationTriggers.STREAM_WILDCARD_MENTION, NotificationTriggers.STREAM_WILDCARD_MENTION_IN_FOLLOWED_TOPIC)`. Use a `set` if the elements are hashable.
+ zerver/lib/logging_util.py:155:12: PLR1714 Consider merging multiple comparisons: `module_name in (logger_name, record.name)`. Use a `set` if the elements are hashable.
+ zerver/lib/onboarding.py:46:9: PLR1714 Consider merging multiple comparisons: `user.realm.org_type in (Realm.ORG_TYPES["education_nonprofit"]["id"], Realm.ORG_TYPES["education"]["id"])`. Use a `set` if the elements are hashable.
+ zerver/models.py:2039:13: PLR1714 Consider merging multiple comparisons: `self.role in (UserProfile.ROLE_REALM_ADMINISTRATOR, UserProfile.ROLE_REALM_OWNER)`. Use a `set` if the elements are hashable.
+ zerver/views/custom_profile_fields.py:83:8: PLR1714 Consider merging multiple comparisons: `field_type in (CustomProfileField.LONG_TEXT, CustomProfileField.USER)`. Use a `set` if the elements are hashable.
+ zerver/webhooks/clubhouse/view.py:403:12: PLR1714 Consider merging multiple comparisons: `entity in ("pull-request", "pull-request-comment")`. Use a `set` if the elements are hashable.
+ zerver/webhooks/github/view.py:80:8: PLR1714 Consider merging multiple comparisons: `action in ("opened", "merged")`. Use a `set` if the elements are hashable.
+ zerver/webhooks/gitlab/view.py:453:10: PLR1714 Consider merging multiple comparisons: `event in ("Job Hook", "Build Hook")`. Use a `set` if the elements are hashable.
+ zerver/webhooks/gitlab/view.py:478:10: PLR1714 Consider merging multiple comparisons: `event in ("Note Hook Issue", "Confidential Note Hook Issue")`. Use a `set` if the elements are hashable.
+ zerver/webhooks/pagerduty/view.py:217:9: PLR1714 Consider merging multiple comparisons: `message_type in ("incident.resolve", "incident.resolved")`. Use a `set` if the elements are hashable.
+ zerver/webhooks/pagerduty/view.py:221:9: PLR1714 Consider merging multiple comparisons: `message_type in ("incident.resolve", "incident.resolved")`. Use a `set` if the elements are hashable.
+ zerver/webhooks/pagerduty/view.py:224:10: PLR1714 Consider merging multiple comparisons: `message_type in ("incident.assign", "incident.reassigned")`. Use a `set` if the elements are hashable.
+ zerver/webhooks/raygun/view.py:230:8: PLR1714 Consider merging multiple comparisons: `event_type in ("NewErrorOccurred", "ErrorReoccurred")`. Use a `set` if the elements are hashable.
+ zerver/webhooks/raygun/view.py:289:9: PLR1714 Consider merging multiple comparisons: `event_type in ("StatusChanged", "AssignedToUser", "CommentAdded")`. Use a `set` if the elements are hashable.
+ zerver/webhooks/sonarr/view.py:59:8: PLR1714 Consider merging multiple comparisons: `event_type not in ("Test", "Health")`. Use a `set` if the elements are hashable.
+ zerver/webhooks/wordpress/view.py:46:8: PLR1714 Consider merging multiple comparisons: `hook in ("publish_post", "publish_page")`. Use a `set` if the elements are hashable.

Rules changed: 2

Rule	Changes	Additions	Removals
PLR1714	54	54	0
PYI036	2	2	0

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      8.4±0.01ms     4.9 MB/sec    1.01      8.4±0.01ms     4.8 MB/sec
formatter/numpy/ctypeslib.py               1.00   1901.0±4.02µs     8.8 MB/sec    1.01  1918.2±10.77µs     8.7 MB/sec
formatter/numpy/globals.py                 1.00    203.7±0.36µs    14.5 MB/sec    1.01    206.0±0.53µs    14.3 MB/sec
formatter/pydantic/types.py                1.00      4.2±0.01ms     6.0 MB/sec    1.01      4.3±0.01ms     6.0 MB/sec
linter/all-rules/large/dataset.py          1.00     14.1±0.04ms     2.9 MB/sec    1.00     14.1±0.02ms     2.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.5±0.00ms     4.7 MB/sec    1.00      3.5±0.01ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.00    365.8±1.20µs     8.1 MB/sec    1.00    364.4±1.00µs     8.1 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.3±0.02ms     4.1 MB/sec    1.00      6.3±0.04ms     4.1 MB/sec
linter/default-rules/large/dataset.py      1.00      7.2±0.01ms     5.7 MB/sec    1.00      7.2±0.02ms     5.6 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1473.3±5.73µs    11.3 MB/sec    1.00   1476.4±2.90µs    11.3 MB/sec
linter/default-rules/numpy/globals.py      1.00    156.7±0.22µs    18.8 MB/sec    1.00    157.4±0.61µs    18.7 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.2±0.01ms     8.0 MB/sec    1.00      3.2±0.01ms     8.0 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      9.4±0.06ms     4.3 MB/sec    1.00      9.4±0.08ms     4.3 MB/sec
formatter/numpy/ctypeslib.py               1.00      2.1±0.03ms     7.8 MB/sec    1.02      2.2±0.03ms     7.7 MB/sec
formatter/numpy/globals.py                 1.00    239.3±4.40µs    12.3 MB/sec    1.02    243.5±7.86µs    12.1 MB/sec
formatter/pydantic/types.py                1.00      4.6±0.05ms     5.5 MB/sec    1.02      4.7±0.05ms     5.4 MB/sec
linter/all-rules/large/dataset.py          1.01     15.8±0.14ms     2.6 MB/sec    1.00     15.6±0.13ms     2.6 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.1±0.07ms     4.0 MB/sec    1.00      4.1±0.05ms     4.0 MB/sec
linter/all-rules/numpy/globals.py          1.00    498.2±6.05µs     5.9 MB/sec    1.01    502.3±7.88µs     5.9 MB/sec
linter/all-rules/pydantic/types.py         1.00      7.1±0.12ms     3.6 MB/sec    1.00      7.1±0.05ms     3.6 MB/sec
linter/default-rules/large/dataset.py      1.01      8.0±0.05ms     5.1 MB/sec    1.00      8.0±0.05ms     5.1 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1698.3±16.61µs     9.8 MB/sec    1.00  1703.5±49.55µs     9.8 MB/sec
linter/default-rules/numpy/globals.py      1.00    199.2±3.42µs    14.8 MB/sec    1.01    200.4±3.34µs    14.7 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.6±0.03ms     7.1 MB/sec    1.00      3.6±0.04ms     7.1 MB/sec

[charliermarsh]

I think you can avoid these allocations by just storing the slice Expr on the enums directly, and then doing this match later, when you actually need to iterate over the members.

[charliermarsh]

Here's a patch of what I'm imagining:

diff --git a/crates/ruff/src/rules/ruff/typing.rs b/crates/ruff/src/rules/ruff/typing.rs
index f96ea22d8..3ad7d9c9f 100644
--- a/crates/ruff/src/rules/ruff/typing.rs
+++ b/crates/ruff/src/rules/ruff/typing.rs
@@ -1,3 +1,4 @@
+use itertools::Either::{Left, Right};
 use rustpython_parser::ast::{self, Constant, Expr, Operator};
 
 use ruff_python_ast::call_path::CallPath;
@@ -54,10 +55,10 @@ fn is_known_type(call_path: &CallPath, minor_version: u32) -> bool {
 
 /// Returns a vector of all the expressions in a slice. If the slice is not a
 /// tuple, it will be wrapped in a vector.
-fn resolve_slice_value(slice: &Expr) -> Vec<&Expr> {
+fn resolve_slice_value(slice: &Expr) -> impl Iterator<Item = &Expr> {
     match slice {
-        Expr::Tuple(ast::ExprTuple { elts: elements, .. }) => elements.iter().collect(),
-        _ => vec![slice],
+        Expr::Tuple(ast::ExprTuple { elts: elements, .. }) => Left(elements.iter()),
+        _ => Right(std::iter::once(slice)),
     }
 }
 
@@ -77,10 +78,14 @@ enum TypingTarget<'a> {
 
     /// A `typing.Union` type or `|` separated types e.g., `Union[int, str]`
     /// or `int | str`.
-    Union(Vec<&'a Expr>),
+    Union(&'a Expr),
+
+    /// A `typing.Union` type or `|` separated types e.g., `Union[int, str]`
+    /// or `int | str`.
+    Pep604Union(&'a Expr),
 
     /// A `typing.Literal` type e.g., `Literal[1, 2, 3]`.
-    Literal(Vec<&'a Expr>),
+    Literal(&'a Expr),
 
     /// A `typing.Optional` type e.g., `Optional[int]`.
     Optional(&'a Expr),
@@ -110,13 +115,13 @@ impl<'a> TypingTarget<'a> {
                 if semantic.match_typing_expr(value, "Optional") {
                     Some(TypingTarget::Optional(slice.as_ref()))
                 } else if semantic.match_typing_expr(value, "Literal") {
-                    Some(TypingTarget::Literal(resolve_slice_value(slice)))
+                    Some(TypingTarget::Literal(slice))
                 } else if semantic.match_typing_expr(value, "Union") {
-                    Some(TypingTarget::Union(resolve_slice_value(slice)))
+                    Some(TypingTarget::Union(slice))
                 } else if semantic.match_typing_expr(value, "Annotated") {
                     resolve_slice_value(slice.as_ref())
-                        .first()
-                        .map(|&expr| TypingTarget::Annotated(expr))
+                        .next()
+                        .map(|expr| TypingTarget::Annotated(expr))
                 } else {
                     semantic.resolve_call_path(value).map_or(
                         // If we can't resolve the call path, it must be defined
@@ -132,9 +137,7 @@ impl<'a> TypingTarget<'a> {
                     )
                 }
             }
-            Expr::BinOp(..) => Some(TypingTarget::Union(
-                PEP604UnionIterator::new(expr).collect(),
-            )),
+            Expr::BinOp(..) => Some(TypingTarget::Pep604Union(expr)),
             Expr::Constant(ast::ExprConstant {
                 value: Constant::None,
                 ..
@@ -179,7 +182,7 @@ impl<'a> TypingTarget<'a> {
             | TypingTarget::Object
             | TypingTarget::Unknown => true,
             TypingTarget::Known => false,
-            TypingTarget::Literal(elements) => elements.iter().any(|element| {
+            TypingTarget::Literal(element) => resolve_slice_value(element).any(|element| {
                 // Literal can only contain `None`, a literal value, other `Literal`
                 // or an enum value.
                 match TypingTarget::try_from_expr(element, semantic, locator, minor_version) {
@@ -190,12 +193,20 @@ impl<'a> TypingTarget<'a> {
                     _ => false,
                 }
             }),
-            TypingTarget::Union(elements) => elements.iter().any(|element| {
+            TypingTarget::Union(element) => resolve_slice_value(element).any(|element| {
                 TypingTarget::try_from_expr(element, semantic, locator, minor_version)
                     .map_or(true, |new_target| {
                         new_target.contains_none(semantic, locator, minor_version)
                     })
             }),
+            TypingTarget::Pep604Union(element) => {
+                PEP604UnionIterator::new(element).any(|element| {
+                    TypingTarget::try_from_expr(element, semantic, locator, minor_version)
+                        .map_or(true, |new_target| {
+                            new_target.contains_none(semantic, locator, minor_version)
+                        })
+                })
+            }
             TypingTarget::Annotated(element) => {
                 TypingTarget::try_from_expr(element, semantic, locator, minor_version)
                     .map_or(true, |new_target| {
@@ -226,12 +237,20 @@ impl<'a> TypingTarget<'a> {
             | TypingTarget::Object
             | TypingTarget::Known
             | TypingTarget::Unknown => false,
-            TypingTarget::Union(elements) => elements.iter().any(|element| {
+            TypingTarget::Union(element) => resolve_slice_value(element).any(|element| {
                 TypingTarget::try_from_expr(element, semantic, locator, minor_version)
                     .map_or(true, |new_target| {
                         new_target.contains_any(semantic, locator, minor_version)
                     })
             }),
+            TypingTarget::Pep604Union(element) => {
+                PEP604UnionIterator::new(element).any(|element| {
+                    TypingTarget::try_from_expr(element, semantic, locator, minor_version)
+                        .map_or(true, |new_target| {
+                            new_target.contains_any(semantic, locator, minor_version)
+                        })
+                })
+            }
             TypingTarget::Annotated(element) | TypingTarget::Optional(element) => {
                 TypingTarget::try_from_expr(element, semantic, locator, minor_version)
                     .map_or(true, |new_target| {

[charliermarsh]

This lets you avoid any allocations 😍

[dhruvmanila]

Oh wow! That's neat :)

We could even go further and merge Union and | separated unions together in the slice resolver but then that would need to an enum as itertools::Either is only for 2 sum types. It would be kinda similar to this implementation except that the duplication of PEP604UnionIterator call will be avoided at the expense of having an additional type.

I think this separation is ok for now.

[dhruvmanila]

Refactoring the implementation as described here introduced a subtle bug which this PR is solving but the bug exists in the parent PR. Ideally, the stack should've been in the reverse order such that this PR is based on the main branch but I didn't realize this until quite late. Then, the rebase required a lot of effort which didn't seem worthwhile.

So, the simple solution is to disable the check on the parent PR and enable it back again in this PR. Sorry for the confusion!