Type check error on non-unit expression statements

[alexvitkov]

Aim

Non-unit expression statements inside a Block fail to type check, if they're not the last statement in the block:

fn main() {
    5;  // error here
    10;
}

Note that this type checks properly:

fn main() { 5; }

This is particularly weird in the case of function calls, since if we call a function and ignore its return value we get a cryptic error:

fn foo() -> Field {
    100
}

fn main() {
    foo(); // error: Expected type (), found type Field
    foo();
}

Expected Behavior

No type errors on any of these examples

Bug

The reason is that we set the statement's type to the expression type which is non-unit:

// type_check/stmt.rs:41
            HirStatement::Expression(expr_id) => {
                return self.check_expression(&expr_id);
            }

... and later on we attempt to unify it to the unit type:

/// type_check/expr.rs:241:
                        self.unify(&expr_type, &Type::Unit, || TypeCheckError::TypeMismatch {
                            expected_typ: Type::Unit.to_string(),
                            expr_typ: expr_type.to_string(),
                            expr_span: span,
                        });

To Reproduce

No response

Installation Method

Compiled from source

Nargo Version

No response

Additional Context

No response

Would you like to submit a PR for this Issue?

Yes

Support Needs

No response

[ghost]

#1933 (comment)

[alexvitkov]

huh...
closing since it's apparently intended

[jfecher]

Right, this is intended as it is similar to the unused variable warning where the compiler warns if a variable is never used. Here, it is the expression itself that is never used. Like the unused variable warning though, this should likely be a warning rather than an error.

Note that this type checks properly:
fn main() { 5; }

Good catch, we should make sure this is consistent

[alexvitkov]

I'll submit a PR that handles the case where there's only one expression with semi.

You're right we should probably convert it to a warning since it's a breaking change (there are tests failing if we handle that case and keep it an error). We could also print a nicer message while we're at it (e.g. Unused Expression (of type Field) instead of Expected (), got Field