Include async functions in the len_without_is_empty #10359

mladedav · 2023-02-16T15:00:15Z

Changes done to the functionality:

Allowing different error types for the functions was disallowed. So the following was linted before but is not after this change

impl Foo {
    pub len(&self) -> Result<usize, Error1> { todo!(); }
    pub is_empty(&self) -> Result<bool, Error2> { todo!(); }
}

changelog: Enhancement: [len_without_is_empty]: Now also detects async functions
#10359

rustbot · 2023-02-16T15:00:22Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

Please see the contribution instructions for more information.

mladedav · 2023-02-16T15:01:48Z

r? @llogiq

llogiq · 2023-02-16T17:57:08Z

fmt and dogfood are still failing; those should be easy to fix.

@rustbot author

mladedav · 2023-02-16T21:30:47Z

The tests are fixed now, hope everything looks fine

@rustbot llogiq

mladedav · 2023-02-16T21:33:08Z

@rustbot label: +S-waiting-on-review, -S-waiting-on-author

samueltardieu · 2023-02-19T12:37:38Z

clippy_lints/src/len_zero.rs

-fn parse_len_output<'tcx>(cx: &LateContext<'_>, sig: FnSig<'tcx>) -> Option<LenOutput<'tcx>> {
+
+fn extract_future_output<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<&'tcx PathSegment<'tcx>> {
+    if let ty::Alias(_, alias_ty) = ty.kind() {


You chould use the if_chain! macro here for shorter and clearer code.

Nowadays we can even use let chains.

samueltardieu · 2023-02-19T12:40:39Z

clippy_lints/src/len_zero.rs

+                    return Some(LenOutput::Option(def_id));
+                }
+
+                return Some(LenOutput::Option(def_id));


What is the use of the if is_first_generic_integral(segment) above if it returns the same thing anyway?

Sorry, this is a bug. is_first_generic_integral checks that len() returns Option<T> where T is an integral type. This should ignore things like pub fn len() -> Option<String> because that's what happens for the sync version.

I've added a test for this.

samueltardieu · 2023-02-19T16:02:46Z

clippy_lints/src/len_zero.rs

+        let item = hir_map.get_if_local(def_id);
+        let item = item.unwrap();
+
+        if let Node::Item(item) = item {


or could use a unique if let … here since you are really matching deeper and deeper with one possible pattern.

I rewrote this with an if let without nesting. However, it can't be a unique if let since at times I want to check some bounds before indexing into an array.

I have collapsed the chains as much as possible though. I personally don't like that much (e.g. I'd rather split

let Some(Node::Item(Item { kind: ItemKind::OpaqueTy(opaque), .. })) = cx.tcx.hir().get_if_local(alias_ty.def_id) && ...

into

let Some(Node::Item(item)) = cx.tcx.hir().get_if_local(alias_ty.def_id) && let Item { kind: ItemKind::OpaqueTy(opaque), .. } = item && ...

but if you prefer it like this I have no issue with that.

edit, I had to split the example I provided to shorten the line under 120 characters anyway, but I would probably still cut some of the remaining ones.

mladedav · 2023-02-27T09:26:27Z

@llogiq

Hi, do you think you will have some time for this PR? I'd like to finish this soI don't forget about it.

bors · 2023-03-05T15:41:14Z

☔ The latest upstream changes (presumably #10438) made this pull request unmergeable. Please resolve the merge conflicts.

llogiq · 2023-03-07T15:21:54Z

clippy_lints/src/len_zero.rs

+                (Self::Option(_), Res::Def(_, def_id)) if cx.tcx.is_diagnostic_item(sym::Option, def_id) => true,
+                (Self::Result(_), Res::Def(_, def_id)) if cx.tcx.is_diagnostic_item(sym::Result, def_id) => true,


Suggested change

(Self::Option(_), Res::Def(_, def_id)) if cx.tcx.is_diagnostic_item(sym::Option, def_id) => true,

(Self::Result(_), Res::Def(_, def_id)) if cx.tcx.is_diagnostic_item(sym::Result, def_id) => true,

(Self::Option(_), Res::Def(_, def_id)) => cx.tcx.is_diagnostic_item(sym::Option, def_id),

(Self::Result(_), Res::Def(_, def_id)) => cx.tcx.is_diagnostic_item(sym::Result, def_id),

I think we don't need guards here.

llogiq

r=me after rebase

Co-authored-by: Akshay <[email protected]>

mladedav · 2023-03-07T21:16:19Z

r? @llogiq

rustbot · 2023-03-07T21:16:36Z

Could not assign reviewer from: llogiq.
User(s) llogiq are either the PR author or are already assigned, and there are no other candidates.
Use r? to specify someone else to assign.

llogiq · 2023-03-09T17:02:50Z

Thank you!

@bors r+

bors · 2023-03-09T17:02:52Z

📌 Commit 5369052 has been approved by llogiq

It is now in the queue for this repository.

bors · 2023-03-09T17:15:26Z

⌛ Testing commit 5369052 with merge 9074da0...

bors · 2023-03-09T17:29:04Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 9074da0 to master...

rustbot assigned Manishearth Feb 16, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 16, 2023

rustbot assigned llogiq and unassigned Manishearth Feb 16, 2023

mladedav force-pushed the dm/private/is-empty branch from 6f96437 to b83b963 Compare February 16, 2023 15:02

mladedav marked this pull request as draft February 16, 2023 15:16

mladedav force-pushed the dm/private/is-empty branch from b83b963 to 9d69b0b Compare February 16, 2023 17:11

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Feb 16, 2023

mladedav force-pushed the dm/private/is-empty branch from 9d69b0b to 31b3bd1 Compare February 16, 2023 20:58

mladedav marked this pull request as ready for review February 16, 2023 21:30

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Feb 16, 2023

samueltardieu reviewed Feb 19, 2023

View reviewed changes

mladedav force-pushed the dm/private/is-empty branch from 31b3bd1 to c68b745 Compare February 19, 2023 19:06

mladedav requested a review from samueltardieu February 23, 2023 13:21

llogiq reviewed Mar 7, 2023

View reviewed changes

llogiq approved these changes Mar 7, 2023

View reviewed changes

mladedav force-pushed the dm/private/is-empty branch from c68b745 to 1335978 Compare March 7, 2023 20:41

Include async functions in the len_without_is_empty

5369052

Co-authored-by: Akshay <[email protected]>

mladedav force-pushed the dm/private/is-empty branch from 1335978 to 5369052 Compare March 7, 2023 21:04

bors merged commit 9074da0 into rust-lang:master Mar 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include async functions in the len_without_is_empty #10359

Include async functions in the len_without_is_empty #10359

mladedav commented Feb 16, 2023 •

edited by xFrednet

Loading

rustbot commented Feb 16, 2023

mladedav commented Feb 16, 2023

llogiq commented Feb 16, 2023

mladedav commented Feb 16, 2023 •

edited

Loading

mladedav commented Feb 16, 2023

samueltardieu Feb 19, 2023

llogiq Mar 7, 2023

samueltardieu Feb 19, 2023

mladedav Feb 19, 2023

samueltardieu Feb 19, 2023

mladedav Feb 19, 2023 •

edited

Loading

mladedav commented Feb 27, 2023

bors commented Mar 5, 2023

llogiq Mar 7, 2023

llogiq left a comment

mladedav commented Mar 7, 2023 •

edited

Loading

rustbot commented Mar 7, 2023

llogiq commented Mar 9, 2023

bors commented Mar 9, 2023

bors commented Mar 9, 2023

bors commented Mar 9, 2023

		(Self::Option(_), Res::Def(_, def_id)) if cx.tcx.is_diagnostic_item(sym::Option, def_id) => true,
		(Self::Result(_), Res::Def(_, def_id)) if cx.tcx.is_diagnostic_item(sym::Result, def_id) => true,

Include async functions in the len_without_is_empty #10359

Include async functions in the len_without_is_empty #10359

Conversation

mladedav commented Feb 16, 2023 • edited by xFrednet Loading

rustbot commented Feb 16, 2023

mladedav commented Feb 16, 2023

llogiq commented Feb 16, 2023

mladedav commented Feb 16, 2023 • edited Loading

mladedav commented Feb 16, 2023

samueltardieu Feb 19, 2023

Choose a reason for hiding this comment

llogiq Mar 7, 2023

Choose a reason for hiding this comment

samueltardieu Feb 19, 2023

Choose a reason for hiding this comment

mladedav Feb 19, 2023

Choose a reason for hiding this comment

samueltardieu Feb 19, 2023

Choose a reason for hiding this comment

mladedav Feb 19, 2023 • edited Loading

Choose a reason for hiding this comment

mladedav commented Feb 27, 2023

bors commented Mar 5, 2023

llogiq Mar 7, 2023

Choose a reason for hiding this comment

llogiq left a comment

Choose a reason for hiding this comment

mladedav commented Mar 7, 2023 • edited Loading

rustbot commented Mar 7, 2023

llogiq commented Mar 9, 2023

bors commented Mar 9, 2023

bors commented Mar 9, 2023

bors commented Mar 9, 2023

mladedav commented Feb 16, 2023 •

edited by xFrednet

Loading

mladedav commented Feb 16, 2023 •

edited

Loading

mladedav Feb 19, 2023 •

edited

Loading

mladedav commented Mar 7, 2023 •

edited

Loading