return statements are sometimes required, even when they shouldn't be

[ahicks92]

My repro is unfortunately private code, so sorry...if it's not obvious what's going on I'll work up something else for you. But if you do something like this:

#[auto_enum(Iterator)]
fn foo() -> Impl Iterator<Item=whatever> {
    if a {
        return std::iter::empty();
    }
    // Some more of those

    // And then return without an explicit return statement...
    std::iter::from_fn(|| stuff)
}

Then it only picks up the variants that explicitly had a return statement, and ignores the final expression in the function.

[taiki-e]

I believe this is because we do not currently handle last expressions that are not considered branches.

auto_enums/src/auto_enum/expr.rs

Lines 21 to 37 in 5c3ffbe

	match expr {
	Expr::Block(ExprBlock { block, .. }) | Expr::Unsafe(ExprUnsafe { block, .. }) => {
	if let Some(Stmt::Expr(expr, None)) = block.stmts.last_mut() {
	child_expr(cx, expr);
	}
	}
	Expr::Match(expr) => visit_last_expr_match(cx, expr),
	Expr::If(expr) => visit_last_expr_if(cx, expr),
	Expr::Loop(expr) => visit_last_expr_loop(cx, expr),
	// Search recursively
	Expr::MethodCall(ExprMethodCall { receiver: expr, .. })
	| Expr::Paren(ExprParen { expr, .. }) => child_expr(cx, expr),
	_ => {}
	}

[ahicks92]

My guess is that you will have to special case the last expression in a function because afaik that's the only place this edge case appears.

It's also probably worth noting that returning () is something a function might want to do even if it doesn't make sense in most cases, so there might be a holistic solution.

I can look at a PR if you don't have the time. I ended up not using this at work for other reasons unrelated to the quality of the crate. Yay code simplification, heh.

[taiki-e]

My guess is that you will have to special case the last expression in a function because afaik that's the only place this edge case appears.

Yeah, the code I linked to is a function (child_expr) that handles that case, but even if the last expression itself is not considered to have a branch, it would indeed need to handle the last expression as one of the branches if a return is found in another expression.

auto_enums/src/auto_enum/mod.rs

Lines 206 to 207 in 5c3ffbe

	match item.block.stmts.last_mut() {
	Some(Stmt::Expr(expr, None)) => child_expr(cx, expr),

I can look at a PR if you don't have the time.

I don't have the bandwidth to work on the fix this right now, so if you could take a look that would be great!

[ahicks92]

I'll try to at least poke at it this weekend.

[ahicks92]

I couldn't untangle the many nested visitors and such to figure out how to funnel the data around generically, which is problematic because the same bug applies to closures. That said, here is a diff that shows some broken test cases:

diff --git a/tests/auto_enum.rs b/tests/auto_enum.rs
index 3c1f583..514997b 100644
--- a/tests/auto_enum.rs
+++ b/tests/auto_enum.rs
@@ -340,6 +340,22 @@ fn stable() {
     }
     assert_eq!(try_operator3(10).unwrap().sum::<i32>(), 54);

+    #[auto_enum(Iterator)]
+    fn trailing_expr_as_return(x: usize) -> impl Iterator<Item = i32> {
+        if x == 0 {
+            return 1..8;
+        }
+
+        if x > 3 {
+            return 2..=10;
+        }
+
+        (0..2).map(|x| x + 1)
+    }
+    for (i, x) in ANS.iter().enumerate() {
+        assert_eq!(trailing_expr_as_return(i).sum::<i32>(), *x);
+    }
+
     #[auto_enum]
     fn closure() {
         #[auto_enum(Iterator)]
@@ -360,6 +376,25 @@ fn stable() {
             assert_eq!(f(i).sum::<i32>(), *x);
         }

+        // Same as above, but the final else is a trailing return expression.
+        #[auto_enum(Iterator)]
+        let f = |x| {
+            if x > 10 {
+                return (0..x as _).map(|x| x - 1);
+            }
+            if x == 0 {
+                return 1..8;
+            } else if x > 3 {
+                return 2..=10;
+            }
+
+            (0..2).map(|x| x + 1)
+        };

Hopefully that helps you or whoever else comes after figure this out quickly. Unfortunately without familiarity, this is taking much longer than I can justify spending on it at the moment. I will circle back if this ever becomes a blocker, but since I'm not currently using the crate this bug is low priority for me at the moment.