Fix issues about block expression evaluation #364

lrh2000 · 2021-04-11T11:19:07Z

This PR introduces the ! type (named "never", see https://doc.rust-lang.org/std/primitive.never.html), so that the following code works.

fn foo(c: i32) -> i32 {
    let b = if c > 0 {
        return 1;
    } else {
        c
    };
    b
}

Note the code also works even without the PR, however, it is an accident because return 1 resolves to <integer> before this PR, which is not reasonable.

This PR is roughly complete:

The trailing expression is not necessarily without a block
Add termination check for GENERIC compiling
Reinforce termination check for HIR lowering
Introduce the ! type (named "never")
Perform more code testing
Fix existing tests and add new tests
Document the code

TODOs (I'll open separate issues or PRs for them):

Integration of the ! type (example code: fn foo() -> ! { /* ...*/})
Detection of infinite loops (which should resolve to !)
Lazy evaluation for lazy boolean operators (buggy code: func() || return -1; and func() && { bar(); false };)

philberty · 2021-04-11T15:45:56Z

Nice work, I would suggest that you run the 3 xfail tests manually. They might be failing only because an error message might be updated/changed as part of this PR so you might find it is all working as you expect. I would also suggest that you add any test cases you can from the relevant issue: #317

philberty · 2021-04-11T15:47:59Z

Also I would like to hear any opinions you have on the code you had to work with on this change.

gcc/rust/typecheck/rust-tyty-rules.h

lrh2000 · 2021-04-12T01:31:33Z

@philberty Thanks for your review.

Nice work, I would suggest that you run the 3 xfail tests manually. They might be failing only because an error message might be updated/changed as part of this PR so you might find it is all working as you expect. I would also suggest that you add any test cases you can from the relevant issue: #317

Yes, the failed tests did fail as expected. After I perform more code testing and (probably) discover and fix some bugs, I'll fix the existing tests and add new tests. (But I have not noticed the relevant issue before and I will take a closer look at it soon.)

Also I would like to hear any opinions you have on the code you had to work with on this change.

I've explained the NeverType in the above comments. Here I'll explain the termination check.

Since NeverType is not a real type (it contains no data and just means that it is not reachable), the termination check at GENERIC compiling is used to make sure that it won't mess up the generated GENERIC trees. It will exit as soon as it process one of the return, break and continue expression.

The termination check at HIR lowering is reinforced in this PR, but it is still not wonderful. It tries its best to remove all unreachable statements, instead of expressions. It matters because something like a = return -1; is always legal regardless of a's concrete type. So extra termination check at GENERIC compiling is needed.

lrh2000 · 2021-04-13T15:27:38Z

Also I would like to hear any opinions you have on the code you had to work with on this change.

There was one more thing I forgot to mention yesterday. Removing unreachable statements at HIR lowering will make gccrs and rustc behaves differently. But I'm not sure whether or not it is a problem that we need to address.

For example, this code fails to compile with rustc.

fn foo() -> i32 {
    return 1;
    let tmp: f32 = 0.0;
    tmp // expected `i32`, found `f32`
}

But at HIR lowering, gccrs will remove the let tmp: f32 statement. So gccrs cannot generate the same error message as rustc.

philberty

Thanks for adding more test cases here it helps with the review.

philberty · 2021-04-14T11:01:05Z

Also I would like to hear any opinions you have on the code you had to work with on this change.

There was one more thing I forgot to mention yesterday. Removing unreachable statements at HIR lowering will make gccrs and rustc behaves differently. But I'm not sure whether or not it is a problem that we need to address.

For example, this code fails to compile with rustc.
fn foo() -> i32 {
    return 1;
    let tmp: f32 = 0.0;
    tmp // expected `i32`, found `f32`
}
But at HIR lowering, gccrs will remove the let tmp: f32 statement. So gccrs cannot generate the same error message as rustc.

Let's make that a new issue I did this thinking it would make it easier for the dead code caes but we can change that. In a seperate PR.

flip1995

Note: I don't know anything about the GCC(rs) compiler internals, so take my suggestions about the code with a grain of salt.

flip1995 · 2021-04-14T11:21:35Z

gcc/rust/typecheck/rust-tyty-rules.h

+
+  virtual void visit (ADTType &type) override { resolved = type.clone (); }
+
+  virtual void visit (InferType &type) override { resolved = type.clone (); }


This case isn't specified in (stable) Rust yet. If InferType can't be inferred to anything, it will fall back to the () (unit) type when unified with the NeverType. This is unstable in Rust and may change to be inferred to the ! type in the future.

I don't think you have to change anything here yet, but I would add a FIXME here, since this will probably cause problems down the road. That is, because if you don't unify it with anything here, it will error after type inference iff no type could be inferred otherwise.[¹]

Since this is unstable Rust, I don't think you have to address this yet, but I would add a note. Some more explanation about this: https://hackmd.io/@FohtAO04T92wF-8_TVATYQ/SJ0vcjyWL?type=view

[¹]: So either you will have to deal with this here in some way (which I don't think you can do?) or mark it here somehow, that it might be the ! type and implement a fallback after type inference (or however this is down with integer types).

Thanks for your review first. I agree with you and I won't deal with this (at least in this PR). Actually when implementing NeverType, I tested what you said with rustc and its behavior really confuses me. I will add a FIXME later.

I think the confusion comes mainly from ! getting inferred to () currently. But yeah, the never type is just confusing by nature I guess.

Exact. I wonder why ! becomes () sometimes. Thanks for the article you provided, which answers the question.

gcc/testsuite/rust.test/compile/unused.rs

flip1995 · 2021-04-14T11:26:22Z

gcc/testsuite/rust.test/xfail_compile/continue1.rs

@@ -5,6 +5,6 @@ fn main() {
    let _fib = {
        continue; // { dg-error "cannot `continue` outside of a loop" }
        // { dg-error "failed to type resolve expression" "" { target { *-*-* } } .-1 }
-        123
+        123 // { dg-warning "unreachable expression" }


This warning appearing here is wrong-ish. It is true in the sense that it can't be reached, because the program doesn't compile. From a UX perspective this produces unnecessary clutter though. This shouldn't block the PR from going through.

Yeah, you are right. This code cannot compile with rustc.

But I won't deal with this (at least in this PR). I've explained the reason below, see #364 (comment). (Note removing unreachable statements/expressions happens eariler than type check.)

flip1995 · 2021-04-14T11:33:17Z

gcc/testsuite/rust.test/xfail_compile/implicit_returns_err2.rs

-    return 1;
-    // { dg-warning "unreachable expression" "" { target *-*-* } .+1 }
-    true
+    return true; // { dg-error "expected .i32. got .bool." }


I would keep the previous test and add this modification as a new test case. This tests a different thing. Before it checked that true is unreachable (warning, but compile-pass) AND has the wrong type (hard error, compile fail), where the first warning should not prevent detecting the second error.

With this change it removes the dependency of those errors.

Before: "This test fails even though the expression is unreachable"
Now: "This test fails because it returns the wrong type and emits a warning"

I agree with you. But I will justify my changes. Before this PR, all unreachable statements are removed but unreachable trailing expressions are kept. So here is a type-mismatched error. But it is not reasonable at all since the unreachable expression may refer removed variables:

fn foo() -> i32 { return 1; let tmp: f32 = 0.0; // removed because it is unreachable tmp // gccrs cannot find tmp since it is removed }

So in this PR, unreachable statements AND trailing expressions are removed. Thus the type-mismatched error disappears and the test is modified accordingly.

I'm not saying that your changes are wrong. I'm just saying that you should keep the old test and that it should still fail the compilation. It doesn't really matter what/how many errors/warnings are emitted as long as it still fails.

Generally removing regression tests isn't something you should do. This practically removes one test and replaces it with another. I would just add the new test and keep the old test (with the dg comments adjusted).

OK, I understand now. But maybe the key problem is that the original test won't fail anymore, because the trailing expression is removed.

In my opinion, the type check against unreachable statements and expressions is totally broken and unreasonable before this PR. It skips all unreachable statements and checks the unreachable expression. The original test is just an accident so we have got the right error message. (You can see the bad case in the example above.)

As far as I'm concerned, I would rather make the type check skip all unreachable statements AND the trailing expressions temporarily (and then the original test does compile). How do you think about it?

the type check against unreachable statements and expressions is totally broken and unreasonable before this PR

AFAICT it complies with the Rust reference and rustc. You can't skip unreachable code in the semantic analysis as long as the unreachable analysis only produces a warning and not a hard error.

As far as I'm concerned, I would rather make the type check skip all unreachable statements AND the trailing expressions temporarily (and then the original test does compile). How do you think about it?

This is against the Rust reference though, so it breaks the compiler. This test must not compile.

Indeed, that would be really awkward as some unreachable code would build fine until one day it's not unreachable anymore and causes type errors without having touched it...

The original implementation is also against the Rust reference. After all it skips all unreachable statements. So maybe I should either do not change it at all, or make it completely right?

Indeed, that would be really awkward as some unreachable code would build fine until one day it's not unreachable anymore and causes type errors without having touched it...

Ah yes, the python way 🐍

So maybe I should either do not change it at all, or make it completely right?

No, but you shouldn't break existing regression tests, aka introducing new bugs (that were fixed before hence the regression test).

OK. I'll revert the change.

This issue should be fixed completely. It is actually very easy to fix (sorry for this since I didn't think it carefully before and I thought it should be a separate issue).

flip1995 · 2021-04-14T11:50:36Z

gcc/rust/typecheck/rust-tyty-rules.h

+  virtual void visit (NeverType &type) override
+  {
+    resolved = get_base ()->clone ();
+  }


And what's more, assigning <integer> to ! is illegal and assigning ! to <integer> is legal. At present, it seems that we do not distinguish between A->unify(B) and B->unify(A).

Wouldn't emitting an error here solve this?

If A=! and B=?, then this is the direction B->unify(A) where you have a type B and try to make it work with the ! type, aka assigning B to A (?). This is only allowed if B==A==!, which is covered by the NeverRules.

On the other hand, the A->unify(B) is handled in the NeverRules, which means that you try to make A work with B, aka assigning A to B, which is always allowed.

I don't think it helps. My opinion is that we must distinguish unify and assign explicitly. For example, we do need unify ! with <integer> as well as unifying <integer> with ! type. Consider the following two code segments.

let a = if x > y { return -1; // ! type } else { 1 // <integer> type }; // `!`->unify(`<integer>`);

let a = if x > y { 1 // <integer> type } else { return -1; // ! type }; // `<integer>`->unify(`!`);

Ah yeah, I tried to think about a situation where the other direction also works. Should have thought about it longer 😄 Nvm then. This has to be addressed, but I would say this is too big for this PR. I suggest to add a FIXME here as well. ~~Maybe also add a XPASS test that does let a: ! = 123; (not sure if GCC has xpass tests though).~~

EDIT:

Yes, but actually this problem does not exist now. It is because ! has not been integrated and code like let a: ! or fn foo() -> ! { /* ... */ } simply does not compile.

If it doesn't compile anyway, just add a xfail test for it and write a comment in the test that it should actually fail for a different reason.

Yeah, I totally agree with this.

philberty · 2021-04-14T13:59:02Z

gcc/rust/typecheck/rust-hir-type-check.cc

+  else if (expr.is_tail_reachable ())
+    infered = new TyTy::TupleType (expr.get_mappings ().get_hirid ());
+  else
+    infered = new TyTy::NeverType (expr.get_mappings ().get_hirid ());


are we saying the end of every block is a NeverType now?

It says if the block's tail is unreachable (for example, there is a return/continue/break), its type is !, otherwise its type is (). I don't think there is a problem.

This is abusing the ! type while assuming that the type analysis works in a specific way. This is too much technical debt IMO.

For example, rustc can compile this. I don't think it is abusing the ! type?

fn foo() -> ! { loop {}; let mut a = 1; a = a + a; }

Yes, but only because the types unify successfully in the unreachable code. If the unreachable code produces an error the compilation will fail.

I think there is nothing to do with this PR. After all, the problem already exists before this PR.

philberty

Can we move forward by splitting this PR up?

The first commit is about tracking the semi-colon like rustc in the AST. That is a single simpler PR to merge.

That work could be built upon to remove the unused code termination stuff.

Then that work will fix #317

Then that leaves the NeverType open

lrh2000 · 2021-04-14T15:55:42Z

Can we move forward by splitting this PR up?

I'll consider this. At least I didn't realize there would be so many problems about the type system when introducing the NeverType, especially when the goal of the PR was not fully integrating the NeverType. (let a: ! or fn foo() -> ! {} is not supported by this PR. Does closing #367 requires this?)

flip1995 · 2021-04-14T16:05:25Z

At least I didn't realize there would be so many problems about the type system when introducing the NeverType, especially when the goal of the PR was not fully integrating the NeverType.

The ! type is really complicated. Not even rustc has fully figured out how to get it right. Integrating it in a known-to-be broken state complicates the type checking system even more and will most likely cause headaches down the road. I don't think we can justify integrating the never type like this without making sure we've done it right, as-in complying with rustc.

This should be done in a separate PR and not in conjunction with a bug fix of an unrelated issue. If the bug fix should depend on the ! type being implemented fully (and correctly), then this should be done right and not only in a good-enough-for-now state.

lrh2000 · 2021-04-14T16:15:50Z

At least I didn't realize there would be so many problems about the type system when introducing the NeverType, especially when the goal of the PR was not fully integrating the NeverType.

The ! type is really complicated. Not even rustc has fully figured out how to get it right. Integrating it in a known-to-be broken state complicates the type checking system even more and will most likely cause headaches down the road. I don't think we can justify integrating the never type like this without making sure we've done it right, as-in complying with rustc.

This should be done in a separate PR and not in conjunction with a bug fix of an unrelated issue. If the bug fix should depend on the ! type being implemented fully (and correctly), then this should be done right and not only in a good-enough-for-now state.

But the original purpose of the PR is simply making the following code work.

fn foo(c: i32) -> i32 {
    let b = if c > 0 {
        return 1;
    } else {
        c
    };
    b
}

It should be a basic support and shouldn't require us to figure out the complicated ! type (since it is even still unstable in rustc).

flip1995 · 2021-04-14T16:30:54Z

It should be a basic support and shouldn't require us to figure out the complicated ! type (since it is even still unstable in rustc).

Yes exactly. So this PR should not implement the ! type at all.

lrh2000 · 2021-04-15T02:05:36Z

It should be a basic support and shouldn't require us to figure out the complicated ! type (since it is even still unstable in rustc).

Yes exactly. So this PR should not implement the ! type at all.

OK, I got it. Then there is a question, which type should return/break/continue expressions resolve to?

Indeed when I started to write the code, I just thought that it was necessary to add a new type that could represent the tail of block is unreachable. And therefore we can make our code cleaner (otherwise we have to check whether or not the tail of each block is reachable when checking the type of if/else statement, which is a little awkward, in my opinion). After a while I found out there is already such a type in Rust named "never", then I introduce such a type. Maybe the decision is wrong and I'd better think it more deeply before.

How about keeping the first three commits in this PR (thus adding a few changes, we can make the above example work properly) and making everything related to NeverType a separate PR? The termination check is still necessary in this PR. Do you think it is OK?

EDIT: But it seems to leave the unreachable statements' type checking issue open. I am not clear whether or not it is acceptable. If it is, maybe I can deal with it in a new PR. If it is not, I'll split only the first commit into another PR and keep the last three commits here.

lrh2000 · 2021-04-15T07:05:46Z

I checked how rustc deals with NeverType. It is roughly like this:

In the rustc's function coerce, ! is allowed to coerce to other type but NOT vice versa (coercion.rs#L157).

So there is no problems in assignment expressions. As for if/else/match expressions, rustc use CoerceMany, which will try coercion in two directions (in our context, both A->unify(B) and B->unify(A) will be tried), see coercion.rs#L1015 and coercion.rs#L1034.

Is rustc's way better and should we do similar things?

lrh2000 · 2021-04-16T02:40:42Z

Can we move forward by splitting this PR up?

The first commit is about tracking the semi-colon like rustc in the AST. That is a single simpler PR to merge.

That work could be built upon to remove the unused code termination stuff.

Then that work will fix #317

Then that leaves the NeverType open

It is not trivial. Consider the following code:

fn foo(x: i32) -> i32 {
    if x >= 0 {
        return x;
    } else {
        return -x;
    }
}

Here the if/else expression should be the trailing expression. And its type must be unified with i32. So which type should it resolve to?

Then I think there are only two available options:

Mark the type by whether or not it is reachable. Then we'll essentially introduce the NeverType.
Pass the expected type from top to bottom during the type check. So if the tail is not reachable, we will get the expected type.

Now it looks like we have to make much more changes to the code if we choose the second option.

I wonder if we can adopt the NeverType but treat it as an internal type (at least for now). In the last commit, I revert support for NeverType partially. Now only fairly limited support of NeverType is introduced. How do you think about it?

@philberty @flip1995 And thanks for your review again!

philberty · 2021-04-16T09:44:28Z

Can we move forward by splitting this PR up?
The first commit is about tracking the semi-colon like rustc in the AST. That is a single simpler PR to merge.
That work could be built upon to remove the unused code termination stuff.
Then that work will fix #317
Then that leaves the NeverType open

It is not trivial. Consider the following code:
fn foo(x: i32) -> i32 {
    if x >= 0 {
        return x;
    } else {
        return -x;
    }
}
Here the if/else expression should be the trailing expression. And its type must be unified with i32. So which type should it resolve to?

Then I think there are only two available options:
* Mark the type by whether or not it is reachable. Then we'll essentially introduce the NeverType.

* Pass the expected type from top to bottom during the type check. So if the tail is not reachable, we will get the expected type.
Now it looks like we have to make much more changes to the code if we choose the second option.

I wonder if we can adopt the NeverType but treat it as an internal type (at least for now). In the last commit, I revert support for NeverType partially. Now only fairly limited support of NeverType is introduced. How do you think about it?

@philberty @flip1995 And thanks for your review again!

That test case currently works in master and there are plenty of test cases for it. The way it should work is is that the return expressions are of type and these unify with each block when its this type of if expression. This means the entire IfExpressions if of type integer.

The awkward part is when we have an expected failure test case such as:

fn foo(x: i32) -> i32 {
    if x >= 0 {
        x
    } else {
        -x
    }
    123
}

GCCRs and rustc both identify that the if expression result in an i32 and should be of type (). The awkward part of this is Rustc allows for:

fn foo(x: i32) -> i32 {
    if x >= 0 {
        return x;
    } else {
        return -x;
    }
    123
}

GCCRS works the same as rustc but only because the final expression is marked as unreachable. This is where the internal never type comes in to fix it. As the PR has been updated you have fixed the unify rules and can_eq etc which looks good. I havent finished this 2nd review but something I was thinking though was the the notion of the fallback type similar as we have in Inference variables.

So when we encounter a ReturnExpr without any expression we create a NeverType(hir_id) but when we do get an expression we can add in a HIRID TyVar reference for that such that we end up with:

NeverType::NeverType(Hirid ref, HirId ty_ref, ... HirId fallback_hir_id);

BaseType* get_fallback() const { return TyVar(fallback_hir_id).get_tyty(); )

private:
fallback_hir_id

That means we get everything we need.

philberty · 2021-04-16T09:49:12Z

gcc/rust/typecheck/rust-hir-type-check.cc

-	block_tyty = resolved;
-      }
-    else if (!resolved->is_unit ())
-      {


We still need this check for unit type for the case of:

fn test() -> i32 { if (...) { 123} else { 456 } 789 }

Now it is checked in

gccrs/gcc/rust/typecheck/rust-hir-type-check-stmt.h

Lines 46 to 50 in 3c251b9

if (stmt.is_unit_check_needed () && infered->get_kind () != TyTy::NEVER)

{

auto unit = new TyTy::TupleType (stmt.get_mappings ().get_hirid ());

infered = unit->unify (infered);

}

since the following code is correct.

fn test() { 123; }

philberty · 2021-04-16T09:56:02Z

gcc/rust/backend/rust-compile-expr.h

@@ -45,7 +47,11 @@ class CompileExpr : public HIRCompileBase
    HIR::Expr *tuple_expr = expr.get_tuple_expr ().get ();
    TupleIndex index = expr.get_tuple_index ();

-    Bexpression *receiver_ref = CompileExpr::Compile (tuple_expr, ctx);
+    Bexpression *receiver_ref
+      = CompileExpr::Compile (tuple_expr, ctx, &terminated);


Please remove the unreachable code termination in GENERIC generation, GCC should be able to optimize this. It is worth while doing but it's not the right time to be adding this to the GENERIC backend yet.

The reason why I add it is that I'm afraid that the following code will mess up the GENERIC tree.

fn main() { let a: (i32, i32, i32) = if true { return; // Here, we'll assign `()` to `(i32, i32, i32)`. } else { (1, 2, 3) }; }

Are you sure there is no problems at all? If there are no problems, I'll remove the unreachable code termination in GENERIC generation. (Maybe I'll have a try later.)

It shouldn't cause issues you can do it in C/C++ FE and they directly create GENERIC tress. Even if it did cause issues it shouldn't be in this PR since this is an optimization outside of the scope of what this PR is about and it is something we should discuss/design first on zulip or a github iussue.

Yeah, it is OK now since only fairly limited part of NeverType is introduced. There is a different case when I was writing this code.

philberty · 2021-04-16T10:05:25Z

gcc/rust/typecheck/rust-hir-type-check-item.h

@@ -82,7 +82,8 @@ class TypeCheckItem : public TypeCheckBase

    context->pop_return_type ();

-    expected_ret_tyty->unify (block_expr_ty);
+    if (block_expr_ty->get_kind () != TyTy::NEVER)
+      expected_ret_tyty->unify (block_expr_ty);


We can't afford to lose those checks against the expected return type. This is where i think the NeverType should use a fallback mechanism.

never_type->get_fallback()->unify(block_expr_ty)

But it has been checked, in

gccrs/gcc/rust/typecheck/rust-hir-type-check-expr.h

Line 165 in 3c251b9

infered = fn_return_tyty->unify (expr_ty);

Hasn't it?

Oh, I noticed if the expression is empty, it won't be checked.

gccrs/gcc/rust/typecheck/rust-hir-type-check-expr.h

Lines 148 to 152 in 3c251b9

if (!expr.has_return_expr ())

{

infered = new TyTy::NeverType (expr.get_mappings ().get_hirid ());

return;

}

But we can still fix it in that place?

Thats only if its a return expression. What about the case where there is an empty block such as:

fn test() -> i32 { }

Here the block will resolve to NeverType in this PR and not be checked against i32 which means we lose the existing check that will sa expected i32 got ().

Oh, I noticed if the expression is empty, it won't be checked.

gccrs/gcc/rust/typecheck/rust-hir-type-check-expr.h

Lines 148 to 152 in 3c251b9

if (!expr.has_return_expr ())

{

infered = new TyTy::NeverType (expr.get_mappings ().get_hirid ());

return;

}

But we can still fix it in that place?

I don't think we can generally assume that if the expression resolves to Never it must be checked against the return type.
What about nested if-expressions where there is a missing empty block, since nested if statement need to be dependant on the context of their parent block not nessecarily the return type of a function.

Here the block will resolve to NeverType in this PR and not be checked against i32 which means we lose the existing check that will sa expected i32 got ().

It will resolve to (), since the tail is reachable.

What about nested if-expressions where there is a missing empty block, since nested if statement need to be dependant on the context of their parent block not nessecarily the return type of a function.

If there is a missing empty block, it means the tail is reachable so we won't get a NeverType.

lrh2000 · 2021-04-16T10:40:56Z

That test case currently works in master and there are plenty of test cases for it. The way it should work is is that the return expressions are of type and these unify with each block when its this type of if expression. This means the entire IfExpressions if of type integer.

It shouldn't. Besides what you have said, it also means the following code won't work.

fn foo() -> f32 {
    let a = if true {
        return 3.0; // `<float>`
    } else {
        3 // `<integer>`
    };
    0.0
}

And what's more, the semicolon after the return expression means it is a statement. I don't think it should affect the block expression's type in such a way as you described. In the following code, let a = 1 and let b = 1 are not removed during the type check. What's the type of the if/else expression?

fn foo(x: i32) -> i32 {
    if x >= 0 {
        return x;
        let a = 1;
    } else {
        return -x;
        let b = -1;
    }
}

philberty · 2021-04-16T10:48:19Z

That test case currently works in master and there are plenty of test cases for it. The way it should work is is that the return expressions are of type and these unify with each block when its this type of if expression. This means the entire IfExpressions if of type integer.

It shouldn't. Besides what you have said, it also means the following code won't work.
fn foo() -> f32 {
    let a = if true {
        return 3.0; // `<float>`
    } else {
        3 // `<integer>`
    };
    0.0
}
And what's more, the semicolon after the return expression means it is a statement. I don't think it should affect the block expression's type in such a way as you described. In the following code, let a = 1 and let b = 1 are not removed during the type check. What's the type of the if/else expression?
fn foo(x: i32) -> i32 {
    if x >= 0 {
        return x;
        let a = 1;
    } else {
        return -x;
        let b = -1;
    }
}

Yes, Let a = block_expr is a different case as to what i mentioned.

This PR removes the unit type check for block-expr type resolution:

fn foo(x: i32) -> i32 {
    if x >= 0 {
        x
    } else {
        -x
    }
    123
}

The if the expression is resolving to integer but it should be ().

lrh2000 · 2021-04-16T10:54:34Z

This PR removes the unit type check for block-expr type resolution:
fn foo(x: i32) -> i32 {
    if x >= 0 {
        x
    } else {
        -x
    }
    123
}
The if the expression is resolving to integer but it should be ().

No, I've explained in #364 (comment).

philberty · 2021-04-16T11:00:17Z

Can we close this PR. The current state of the commit history is not mergeable as one big squash. This PR introduces many distinct changes which make it too difficult to review and agree on the behaviour:

Changes to the Parser Behaviour for block-expr
Changes to HIR lowering
Introduces unreachable code optimizations to GENERIC backend
Introduces never type on return, break and continue expressions
Changes how we type resolve block expressions in general
Indirectly means we start implementing unstable rust features although this is a case we probably need

There are already alot of comments mentioning that we wish this PR to be split up several times.

lrh2000 · 2021-04-16T11:42:41Z

Can we close this PR. The current state of the commit history is not mergeable as one big squash. This PR introduces many distinct changes which make it too difficult to review and agree on the behaviour:

Changes to the Parser Behaviour for block-expr

Changes to HIR lowering

Introduces unreachable code optimizations to GENERIC backend

Introduces never type on return, break and continue expressions

Changes how we type resolve block expressions in general

Indirectly means we start implementing unstable rust features although this is a case we probably need

There are already alot of comments mentioning that we wish this PR to be split up several times.

OK, I'll try to split this PR into several ones. Sorry for that.

380: Fix issues about block expression evaluation r=philberty a=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. https://github.com/Rust-GCC/gccrs/blob/935aff52a66a70e1e96f0a6aa7e0ad37eda3f93c/gcc/testsuite/rust.test/compile/deadcode1.rs#L7-L15 https://github.com/Rust-GCC/gccrs/blob/935aff52a66a70e1e96f0a6aa7e0ad37eda3f93c/gcc/testsuite/rust.test/compile/implicit_returns1.rs#L44-L50 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. Co-authored-by: lrh2000 <[email protected]>

387: Don't remove unreachable statements at HIR lowering r=philberty a=lrh2000 This is a legal code segment: ```rust fn foo() -> i32 { return 1; let a = 1; a } ``` And this is illegal: ```rust fn foo() -> i32 { return 1; let a = 1.5; a } ``` So we shouldn't remove unreachable statements at HIR lowering. We can issue warnings for them, but they need to be kept so that their types can be checked. *(Original in #364 Co-authored-by: lrh2000 <[email protected]>

390: Clean up the compilation of block expressions r=philberty a=lrh2000 The original implementation of compiling block expressions and function bodies is incorrect. Here is an example. ```rust fn foo() -> i32 { 1; 2; 0 } ``` The above Rust code will compile into the below GIMPLE code. ```c i32 foo () { i32 D.199; D.199 = 1; return D.199; D.199 = 2; return D.199; D.199 = 0; return D.199; } ``` It is obviously incorrect. *(Original in #364 Co-authored-by: lrh2000 <[email protected]>

lrh2000 added 4 commits April 10, 2021 22:53

The trailing expression is not necessarily without a block

889b5b8

Add termination check for GENERIC compiling

d7ef8ee

Reinforce termination check for HIR lowering

8ed81aa

Introduce the ! type (named "never")

ba5aaa8

lrh2000 force-pushed the fix-tail branch from fbc94cd to ba5aaa8 Compare April 11, 2021 13:44

philberty reviewed Apr 11, 2021

View reviewed changes

gcc/rust/typecheck/rust-tyty-rules.h Outdated Show resolved Hide resolved

philberty reviewed Apr 11, 2021

View reviewed changes

gcc/rust/typecheck/rust-tyty-rules.h Outdated Show resolved Hide resolved

philberty mentioned this pull request Apr 12, 2021

Never Type #367

Open

philberty linked an issue Apr 12, 2021 that may be closed by this pull request

Never Type #367

Open

philberty approved these changes Apr 12, 2021

View reviewed changes

philberty added bug cleanup labels Apr 12, 2021

philberty linked an issue Apr 12, 2021 that may be closed by this pull request

Invalid expression discarding when using semicolons #317

Closed

lrh2000 added 2 commits April 13, 2021 15:17

Merge branch 'master' into fix-tail

1060ed3

Fix existing tests and add new tests

098da6c

lrh2000 force-pushed the fix-tail branch from 0d3dba8 to 098da6c Compare April 13, 2021 08:13

Merge branch 'master' into fix-tail

61ce12f

lrh2000 marked this pull request as ready for review April 13, 2021 16:03

lrh2000 force-pushed the fix-tail branch from 11cfc09 to 61ce12f Compare April 14, 2021 00:33

lrh2000 requested a review from philberty April 14, 2021 01:00

philberty requested review from dkm, SimplyTheOther and flip1995 April 14, 2021 10:55

philberty approved these changes Apr 14, 2021

View reviewed changes

flip1995 suggested changes Apr 14, 2021

View reviewed changes

philberty reviewed Apr 14, 2021

View reviewed changes

philberty requested changes Apr 14, 2021

View reviewed changes

philberty removed a link to an issue Apr 14, 2021

Invalid expression discarding when using semicolons #317

Closed

This comment has been minimized.

Sign in to view

lrh2000 added 2 commits April 16, 2021 07:54

Don't remove unreachable statements at HIR lowering

7569e17

Revert support for the never type partially

3c251b9

lrh2000 force-pushed the fix-tail branch from 054c577 to 3c251b9 Compare April 16, 2021 02:48

philberty reviewed Apr 16, 2021

View reviewed changes

lrh2000 closed this Apr 16, 2021

lrh2000 mentioned this pull request Apr 17, 2021

Fix issues about block expression evaluation #380

Merged

lrh2000 mentioned this pull request Apr 24, 2021

Don't remove unreachable statements at HIR lowering #387

Merged

lrh2000 mentioned this pull request Apr 25, 2021

Clean up the compilation of block expressions #390

Merged


		virtual void visit (ADTType &type) override { resolved = type.clone (); }

		virtual void visit (InferType &type) override { resolved = type.clone (); }

	if (stmt.is_unit_check_needed () && infered->get_kind () != TyTy::NEVER)
	{
	auto unit = new TyTy::TupleType (stmt.get_mappings ().get_hirid ());
	infered = unit->unify (infered);
	}

	if (!expr.has_return_expr ())
	{
	infered = new TyTy::NeverType (expr.get_mappings ().get_hirid ());
	return;
	}

Fix issues about block expression evaluation #364

Fix issues about block expression evaluation #364

Conversation

lrh2000 commented Apr 11, 2021 • edited Loading

philberty commented Apr 11, 2021

philberty commented Apr 11, 2021

lrh2000 commented Apr 12, 2021

lrh2000 commented Apr 13, 2021 • edited Loading

philberty left a comment

Choose a reason for hiding this comment

philberty commented Apr 14, 2021

flip1995 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrh2000 Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrh2000 Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

flip1995 Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrh2000 Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrh2000 Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

flip1995 Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philberty left a comment

Choose a reason for hiding this comment

lrh2000 commented Apr 14, 2021 • edited Loading

flip1995 commented Apr 14, 2021

lrh2000 commented Apr 14, 2021 • edited Loading

flip1995 commented Apr 14, 2021

lrh2000 commented Apr 15, 2021 • edited Loading

This comment has been minimized.

lrh2000 commented Apr 15, 2021

lrh2000 commented Apr 16, 2021

philberty commented Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

lrh2000 Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philberty Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrh2000 Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrh2000 Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrh2000 commented Apr 16, 2021

philberty commented Apr 16, 2021

lrh2000 commented Apr 16, 2021

philberty commented Apr 16, 2021 • edited Loading

lrh2000 commented Apr 16, 2021

lrh2000 commented Apr 11, 2021 •

edited

Loading

lrh2000 commented Apr 13, 2021 •

edited

Loading

lrh2000 Apr 14, 2021 •

edited

Loading

lrh2000 Apr 14, 2021 •

edited

Loading

flip1995 Apr 14, 2021 •

edited

Loading

lrh2000 Apr 14, 2021 •

edited

Loading

lrh2000 Apr 14, 2021 •

edited

Loading

flip1995 Apr 14, 2021 •

edited

Loading

lrh2000 commented Apr 14, 2021 •

edited

Loading

lrh2000 commented Apr 14, 2021 •

edited

Loading

lrh2000 commented Apr 15, 2021 •

edited

Loading

philberty commented Apr 16, 2021 •

edited

Loading

lrh2000 Apr 16, 2021 •

edited

Loading

philberty Apr 16, 2021 •

edited

Loading

lrh2000 Apr 16, 2021 •

edited

Loading

lrh2000 Apr 16, 2021 •

edited

Loading

philberty commented Apr 16, 2021 •

edited

Loading