lvalue: Inference rules for field expressions

[madmann91]

In the lvalue branch, the type inference rules are broken for field expressions:

const Type* FieldExpr::check(InferSema& sema) const {
    auto ltype = sema.check(lhs());
    if (is_ptr(ltype)) {
        PrefixExpr::create_deref(lhs_.get());
        ltype = sema.check(lhs());
    }

    auto ref = split_ref_type(ltype);

    if (auto struct_type = ltype->isa<StructType>()) {
        if (auto field_decl = struct_type->struct_decl()->field_decl(symbol())) {
            if (ref)
                Ref2ValueExpr::create(lhs())->type();
            return sema.wrap_ref(ref, struct_type->op(field_decl->index()));
        }
    }

    return sema.wrap_ref(ref, ltype->is_known() ? sema.type_error() : sema.find_type(this));
}

Assuming that the structure type is known, but the field type is not, we will return a type such as reference to ?23 in the first type inference iteration. In the next iteration, we will have inserted a Ref2Value node, which means that lhs will now have a non-reference type. Hence, this iteration will return a non-reference type as well, which cannot be unified with the type coming from the previous iteration.

This is an example that triggers the bug:

fn iterate_rays(rays: &Ray) -> () {
    rays.org;
}

struct Ray {
    org: Vec4
}

struct Vec4 {
    w: f32
}

My suggestion to fix this issue: Do not create Ref2Value nodes here, as it makes little sense from a type checking perspective (you need a reference to a structure to get a reference to a structure field). This Ref2Value should be located at the usage site, when necessary.

[madmann91]

Actually, couldn't we type this through subtyping (i.e &T > T), and add a intermediate pass between type inference and type checking that adds these explicit casts between references and rvalues ?

[leissa]

Does this fix the problem?

[madmann91]

No, the issue still prevails with more complex examples, like this one:

fn iterate_rays(rays: &Ray) -> () {
    rays.org.x;
}
struct Ray {
    org: Vec4
}
struct Vec4 {
    x: f32
}

[leissa]

yes, you're right

[madmann91]

One option is to allow FieldExprs to work with references. That simplifies a bit the code, and we just need to fix TypeSema to split the reference before checking:

void FieldExpr::check(TypeSema& sema) const {
    auto type = sema.check(lhs());

    // split reference
    split_ref_type(type);

    // ...
}

[madmann91]

Shall I apply that patch?

[madmann91]

See pull request #50

[leissa]

yes, this is also what I thought: having these Ref2Value nodes in-between doesn't really make sense. I'll keep this bug report opened. I want to double-check a few things. I think we have the same problem with MapExpr.

[madmann91]

We do indeed. I was about to create a new bug report for that...

[madmann91]

For MapExpr, the issue is slightly different. The semantics we have are incorrect. We should not automatically insert the * operator. The map operator, when applied on an array of references, should offset the pointer (some sort of GEP). Right now, when we have:

let a : &[i32] = /* ... */ ;
a(5)

this translates to:

let a : &[i32] = /* ... */ ;
(*a)(5)

Which is not correct. You cannot create a reference to an element of an array if you have already dereferenced the base pointer.

[leissa]

This is actually correct, because the type of *a in this example is not [i32] but reference of [i32] which still has this GEP/LEA kind of thing going.

[leissa]

I think this should do the trick, or do you have an example where this goes wrong?

[madmann91]

The following example still does not work on my machine:

fn test(array: &[i32]) -> () {
    array(0);
}

[madmann91]

You are correct though, I did not see that * actually creates a reference to the value. A simple fix for the bug is then simply to split the reference before a call to MapExpr::remit.

const Def* MapExpr::remit(CodeGen& cg, State state, Location eval_loc) const {
    auto ltype = lhs()->type();
    split_ref_type(ltype);

    // ...
}

[leissa]

yes, just a slight issue in the codegen. I'm currently inspecting that.

[leissa]

yes, that's the fix. Let me add some test case and then we can close this issue.