Fix issues about block expression evaluation

[lrh2000]

I've tried my best to split #364. Its core part is here. All unrelated fixes should have been removed and I will make them separate PRs, but this PR still has to contain 2 commits and introduce limited support for the never type.

gccrs/gcc/testsuite/rust.test/compile/deadcode1.rs

Lines 7 to 15 in 935aff5

	fn test2(x: i32) -> i32 {
	if x > 1 {
	return 5;
	} else {
	return 0;
	}
	// { dg-warning "unreachable statement" "" { target *-*-* } .+1 }
	return 1;
	}

gccrs/gcc/testsuite/rust.test/compile/implicit_returns1.rs

Lines 44 to 50 in 935aff5

	fn test7(x: i32) -> i32 {
	if x > 1 {
	return 5;
	} else {
	return 0;
	}
	}

The reason is that the above two tests failed to pass simultaneously after the first commit. The first test requires the if/else expression resolves to () but the second test requires the if/else expression resolves to <integer>. They are conflicted.

I wonder whether or not this PR is OK, and anyway thanks for your review.

[philberty]

Can you make new test cases instead of amending existing ones. I would prefer to keep these test cases as they are.

[philberty]

and here too this is a new test case

[philberty]

I think this check stlll needs to be in the BlockExpr type resolution this is_unit_check_needed is exposed on HIR::Stmts and makes it more general for other cases.

[philberty]

Can you add test cases from #317 since this PR is meant to address this issues there too.

[philberty]

I think this PR is looking good my main issue is the test cases. I believe it is important that we keep each test case simple and very explicit as to what it is testing so it is easier to pin point what exactly it is testing especially since the compiler is early on.

[lrh2000]

Thanks! @philberty
I've made the corresponding changes according to your comments.

An exception is this code mentioned in #317 (comment):

fn function() -> i32 {
    return 15;
    return 1.2;
}

The second statement is removed at HIR lowering so no error messages can be issued by gccrs. There is a fix contained in #364 (7569e17) but I think I will file a separate PR for the separate issue. Thus the code segment is not contained in test cases.

[philberty]

Is this the right grammar? https://doc.rust-lang.org/stable/reference/statements.html

@SimplyTheOther what do you think?

[philberty]

I think it does look like what @yizhepku mentioned in #317 which seems right

[lrh2000]

"An expression that consists of only a block expression or control flow expression, if used in a context where a statement is permitted, can omit the trailing semicolon. [...] When the trailing semicolon is omitted, the result must be type ()." quoted from https://doc.rust-lang.org/stable/reference/statements.html. So I think it is right.

[philberty]

Thanks @lrh2000 this is shaping up well, do you have your copyright assignment stuff done?

[lrh2000]

Thanks @lrh2000 this is shaping up well, do you have your copyright assignment stuff done?

Actually I started to deal with my copyright assignment more than a week ago. I was asked to complete a questionnaire and email back to [email protected]. But [email protected] does not reply to me at all. I asked whether I did something wrong but I was told that FSF doesn't reply immediately and FSF is experiencing internal problems so the response may be delayed.

Now it seems that I can do nothing but wait. Do you have any suggestions for this?

[philberty]

What are your plans in moving forward with NeverType after this PR? Do you plan to see it as a kind of InferType with a fallback TyVar? Such that:

return 123, could be represented as:

NeverType [ integer ] and by default it would be NeverType [ () ]?

[lrh2000]

return 123, could be represented as:

NeverType [ integer ] and by default it would be NeverType [ () ]?

I am not clear about what problems this idea can solve. And I do not think there is a case where the never type generated by return 123 will fall back to an integer. Could you please give an example that shows the feature is useful? As far as I know, the never type can only fall back to ().

In stable Rust, the never type will fall back to () if it is not unified with any other types (just like <integer> will fall back to i32 and <float> will fall back to f64, see _impl.rs#L655):

fn main() {
    let a = return;
    let b = a + 1; // no implementation for `() + i32`
}

unless we specify #![feature(never_type_fallback)]:

#![feature(never_type_fallback)]
fn main() {
    let a = return;
    let b = a + 1; // no implementation for `! + i32`
}

It may be worth noting rustc can compile this:

fn main() {
    let a = return;
    let b = a + 1;
    a = 32;
}

In my opinion, it is better to follow stable Rust‘s approach, since it seems that it is too early for gccrs to introduce unstable Rust.

[philberty]

return 123, could be represented as:
NeverType [ integer ] and by default it would be NeverType [ () ]?

I am not clear about what problems this idea can solve. And I do not think there is a case where the never type generated by return 123 will fall back to an integer. Could you please give an example that shows the feature is useful? As far as I know, the never type can only fall back to ().

In stable Rust, the never type will fall back to () if it is not unified with any other types (just like <integer> will fall back to i32 and <float> will fall back to f64, see _impl.rs#L655):

fn main() {
    let a = return;
    let b = a + 1; // no implementation for `() + i32`
}

unless we specify #![feature(never_type_fallback)]:

#![feature(never_type_fallback)]
fn main() {
    let a = return;
    let b = a + 1; // no implementation for `! + i32`
}

It may be worth noting rustc can compile this:

fn main() {
    let a = return;
    let b = a + 1;
    a = 32;
}

In my opinion, it is better to follow stable Rust‘s approach, since it seems that it is too early for gccrs to introduce unstable Rust.

Thanks, for that final case how does that work in rustc? what type does a get is it ()? But then the final assignment should fail?

fn main() {
    let a = return;
    let b = a + 1;
    a = 32;
}

[yizhepku]

return 123, could be represented as:
NeverType [ integer ] and by default it would be NeverType [ () ]?

There's some confusion here. The Never type is a single type that can be coerced into any other type. return 123 has the never type, period.

I think the last example given by @lrh2000 works this way:

1. The compiler sees let a = return; and deduces that a has type !.
2. The compiler sees let b = a + 1; and coerces the type of a to an integer.
3. The compiler sees a = 32; and coerces the type of a to i32, by default.

[lrh2000]

Thanks, for that final case how does that work in rustc? what type does a get is it ()? But then the final assignment should fail?

fn main() {
    let a = return;
    let b = a + 1;
    a = 32;
}

My intention is to show that the never type will fall back to () if it does not coerce to another type.

2. The compiler sees let b = a + 1; and coerces the type of a to an integer.

I don't think it is correct. The coercion should occur at a = 32 instead of let b = a + 1. I think rustc use obligations to denote that a should implement the Add trait and check whether these obligations are satisfied later.

[philberty]

Thanks, for that final case how does that work in rustc? what type does a get is it ()? But then the final assignment should fail?

fn main() {
    let a = return;
    let b = a + 1;
    a = 32;
}

My intention is to show that the never type will fall back to () if it does not coerce to another type.

2. The compiler sees let b = a + 1; and coerces the type of a to an integer.

I don't think it is correct. The coercion should occur at a = 32 instead of let b = a + 1. I think rustc use obligations to denote that a should implement the Add trait and check whether these obligations are satisfied later.

Ah ok that makes more sense now, this is something that i was thinking about in Traits which i should be starting in aboiut 4 weeks or so.

To me it is kind of similar to the case of:

let a;
a = None;
a = Some(123);

Then a becomes Option<i32>.

Thanks for bearing with this @lrh2000 i know this has been a long process for this PR but its important changes for the compiler and has impact in how i will be aproaching things down the line.

[philberty]

Ok, let's go for this if we don't hear from other reviewers we will merge tomorrow or so.

See our zulip discussion over on: https://gcc-rust.zulipchat.com/#narrow/stream/266897-general/topic/Wierd.20cases

[philberty]

bors try

[bors]

try

Build succeeded:

• clang-format
• build-and-check

[lrh2000]

Ok, let's go for this if we don't hear from other reviewers we will merge tomorrow or so.

Thanks. I noticed that some fixes contained in #364 do not work without this PR. So if everything goes well, I will be able to open separate PRs for them as soon as possible.

[philberty]

Ok, let's go for this if we don't hear from other reviewers we will merge tomorrow or so.

Thanks. I noticed that some fixes contained in #364 do not work without this PR. So if everything goes well, I will be able to open separate PRs for them as soon as possible.

Sounds good. I think the discussion on zulip and here have helped.

Also, it's important to point out I will be starting work on traits by the end of next month and there is going to be overlap with that for type coercion and the edge cases pointed out here.

[philberty]

bors try

[bors]

try

Build failed:

• build-and-check

[lrh2000]

I interpret the purpose of this test as testing whether or not dg-ice works well. Without a trailing (), the unsafe block is a trailing expression, therefore there is a fatal error instead of an internal compiler error because of the following code segment:

gccrs/gcc/rust/hir/rust-ast-lower-expr.h

Lines 60 to 65 in dd3c58b

	if (resolver.translated == nullptr)
	{
	rust_fatal_error (expr->get_locus_slow (), "Failed to lower expr: [%s]",
	expr->as_string ().c_str ());
	return nullptr;
	}

[philberty]

bors r+

[bors]

Build succeeded:

• build-and-check
• clang-format