Incorrect span label when non-() else-less if is tail expression in -> () function

[estebank]

Code

fn main() {
    if true {
        ""
    }
}

Current output

error[E0308]: mismatched types
 --> src/main.rs:3:9
  |
1 | fn main() {
  |          - expected `()` because of default return type
2 |     if true {
3 |         ""
  |         ^^ expected `()`, found `&str`

Desired output

error[E0308]: mismatched types
 --> src/main.rs:3:9
  |
2 | /     if true {
3 | |         ""
  | |         ^^ expected `()`, found `&str`
4 | |     }
  | |_____- expected this to be `()`

Rationale and extra context

When the if is not the tail expression, we provide the right output. We might just have to change the order of operation when looking for additional context to print.

Other cases

if true { "" } else { "" } as tail expression has the same problem.

Rust Version

All checked, current version 1.78.

[chenyukang]

why we don't suggest add ; in this case:

fn main() {
    if true {
        "";  // add `;` at here.
    }
}

[estebank]

@chenyukang I believe it is because we check whether the tail expression is not a value. ""; is never useful and people would never want it for any reason. For cases where it makes sense, we do suggest adding the ;.

[cardigan1008]

Hi, I'm new to the rust development and want to start with this issue.

[jplatte]

@cardigan1008 you can make a comment saying @rustbot claim to be assigned to the issue :)

(so people filtering for unassigned issues to work on don't have it show up)

[cardigan1008]

@rustbot claim

[cardigan1008]

I tried to investigate on this issue and here is a case that has the correct output:

fn main() {
    if true {
        ""
    }
    1;
}

The difference between the two cases lies in:

rust/compiler/rustc_middle/src/hir/map/mod.rs

Line 547 in 37dc766

if let (Some((_, next_node)), false) = (iter.peek(), ignore_tail) {

The correct case is parsed into nodes that include a Block node where the expr field is None. In contrast, in the erroneous case, the expr field of the Block node is not None, which is the content of stmt filed in the Block of the correct case. More details are available in this gist: https://gist.github.com/cardigan1008/6d537043d86e13a5c1bad96bc28ada24. So the wrong case keeps iterating until node is the Node::Item(_), which leads to the return here:

rust/compiler/rustc_middle/src/hir/map/mod.rs

Line 566 in 37dc766

return Some(hir_id)

The correct case has a early return with None here:

rust/compiler/rustc_middle/src/hir/map/mod.rs

Line 549 in 37dc766

Node::Block(Block { expr: None, .. }) => return None,

Finally, in the function suggest_mismatched_types_on_tail, the wrong case enters into the if condition unlike the other one:

rust/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs

Line 81 in 37dc766

if let Some((fn_id, fn_decl, can_suggest)) = self.get_fn_decl(blk_id) {

I'm uncertain about where to start with a fix. Should I modify the structure of this node or introduce some limitations? I've drafted a potential fix here: #124917, but it's tailored specifically to this case. I'd appreciate any thoughts or feedback. Thanks!

[estebank]

@cardigan1008 I believe you can just modify get_return_block to additionally check whether the node is an else-less if (adding an arm to the match like Node::Block(Block { expr: Some(e), .. }) if matches!(e.kind, ExprKind::If(_, _, None)) => return None,) with no adverse effects, but you'll have to try it out to see if there's any covered use of get_return_block that expects that case to continue climbing the hir tree. Just run ./x.py test tests/ui --bless and that will modify the .stderr files to see the effects of your change.

The other case:

fn main() {
    if true {
        ""
    } else {
        ""
    }
}

error[E0308]: mismatched types
 --> src/main.rs:3:9
  |
1 | fn main() {
  |          - expected `()` because of default return type
2 |     if true {
3 |         ""
  |         ^^ expected `()`, found `&str`

error[E0308]: mismatched types
 --> src/main.rs:5:9
  |
1 | fn main() {
  |          - expected `()` because of default return type
...
5 |         ""
  |         ^^ expected `()`, found `&str`

I think is ok. It could do with a more targeted error that will be more involved (to emit a single error instead of multiple), but I wouldn't consider it in scope for this ticket.

[cardigan1008]

@estebank Thanks for you kind and clear instructions! I've ran ./x.py test tests/ui --bless and it turned out that this change has no effect on the other cases. Considering the other case you mentioned, in my opinion, I think it may be better to emit multiple errors. Here is a case:

fn main() {
    if true {
        ""
    } else if false {
        
    } else {
        ""
    }
}

error[E0308]: mismatched types
 --> src/main.rs:3:9
  |
1 | fn main() {
  |          - expected `()` because of default return type
2 |     if true {
3 |         ""
  |         ^^ expected `()`, found `&str`

error[E0308]: mismatched types
 --> src/main.rs:5:9
  |
1 | fn main() {
  |          - expected `()` because of default return type
...
5 |         ""
  |         ^^ expected `()`, found `&str`

error[E0308]: mismatched types
 --> src/main.rs:9:9
  |
1 | fn main() {
  |          - expected `()` because of default return type
...
9 |         ""
  |         ^^ expected `()`, found `&str`

For more information about this error, try `rustc --explain E0308`.
error: could not compile `playground` (bin "playground") due to 3 previous errors

In this case, would aggregating errors into a single message make the correction less clear? I think the multiple errors might be a good reminder for users to fix errors in several specific branches. Maybe I misunderstand your meaning about the single error. Sorry for that.

[estebank]

Yeah, you're right. The only case where emitting a single E0308 error for this is when the expected type of the whole expression is one and all of the arms are of the same type.