unification unifies different but identical types

[canndrew]

• module0.sw:

library module0;
pub struct Thing {}

impl Thing {
    pub fn new() -> Self {
        Thing {}
    }
}

• module1.sw:

library module1;
pub struct Thing {}

impl Thing {
    pub fn new() -> Self {
        Thing {}
    }
}

• main.sw:

script;
dep module0;
dep module1;

fn main() {
    let mut x = module0::~Thing::new();
    let y = module1::~Thing::new();
    x = y;
}

To compile this you need to relax the rules in hll.pest to allow parsing zero-field structs. This code will then compile despite the fact that module0::Thing and module1::Thing are two different types.

[canndrew]

@othro / @sezna: What causes this is that if two different types have the same name and the same fields then their TypeInfos are identical. One way to half-fix this would be to change the name field in TypeInfo::Enum/TypeInfo::Struct to be a full path (eg. library::module::type_name rather than just type_name). However we'll still end up running into the same problem if there are multiple versions of a dependency being built (eg. we depend on dep-a-v0.1 and dep-b, but dep-b depends on dep-a-v0.2). Instead I think TypeInfo::Enum/Struct need to be replaced with a variant that contains a declaration id that uniquely identifies the type.

However that unique declaration id seems like it might be the intended purpose of the TypeId type. Which brings me to another concern: Right now we are very greedily allocating TypeIds. For instance every expression of type bool gets a unique TypeId which maps to TypeInfo::Bool (or a TypeInfo::Ref to bool, or a reference-to-reference-to-bool, etc.). This is pretty inefficient and it isn't necessary. The way I see it, TypeId could be replaced with a type Type that looks like:

enum Type {
    Bool,
    /* other primitive types, contracts, etc. */

    Nominal(DeclId),
    Unknown(UnknownTypeId),
}

Where we then have a HashMap/Vec that maps UnknownTypeIds to their inferred type, only once they are inferred. That is Type::Unknown replaces TypeInfo::Ref, TypeInfo::Unknown is removed, and we use Type everywhere where we currently use TypeId.

Thoughts?

[otrho]

TypeId is meant to be a Copy tag for the type, a cheap way to declare on the spot what type some expression is if known -- e.g., literals, function arguments or expressions with type ascriptions -- or that it is Unknown. Then the unification process resolves all the types which are the same and catches type errors when they're not. This is essentially the inference process -- declare types we know from the source, call everything else Unknown, resolve things in the end.

So TypeId is designed to be allocated like crazy. The alternative is to look up the TypeId for the type we want all the time which only saves us some space in a hash map and takes more time.

I don't think changing the type inference algorithm is the solution here. I think just adding the full path to the declaration is good enough now, without having to worry about dependency hell yet.

[sezna]

It looks to me like Type::Unknown(UnknownTypeId) is isomorphic to TypeInfo::Ref(id), in that it says it is a reference to some other ID that may or may not be known at this particular time.

Since we don't even support dependency versions yet (no registry) the deps issue is an issue we can punt into the future. I think the answer here is to use a CallPath for the type name to prevent false unification of similar but identical structs.

[emilyaherbert]

Since we don't even support dependency versions yet (no registry) the deps issue is an issue we can punt into the future. I think the answer here is to use a CallPath for the type name to prevent false unification of similar but identical structs.

This is what I will be implementing.

[sezna]

@emilyaherbert @canndrew was this closed by #472? Is this still an issue?

[emilyaherbert]

This is still an issue, TypeInfo just uses Idents and an not CallPaths. Probably an oversight of the #472 PR as I expect there is undefined behavior in the type system right now, if there are multiple similar definitions.

sway/sway-core/src/type_engine/type_info.rs

Line 32 in 4905aa0

name: Ident,

[esdrubal]

#4007 reverted the fix that closed this issue, thus I am reopening it.
When this issue is fixed multiple_enums_with_the_same_name and multiple_structs_with_the_same_name tests should be re-enabled.

[esdrubal]

Closing this issue as the tests mentioned previously are now enabled and passing.