Typechecker error messages don't use type abbreviations

[msullivan]

Since we got rid of cnames (for good reason, as I understand it), typechecker error messages always use the full structural types, which is sucky. Patrick says he has a plan for fixing this that isn't high priority right now. Creating this bug to track.

We probably want this fixed for a release?

[graydon]

Wow. I .. did not realize we got rid of cnames. Why?

[nikomatsakis]

Marijn, I am somewhat concerned about your fix (which introduces the ty_named() variant into sty; basically a layer of indirection into ty::t that preserves the user type). It bothers me that the intended type system is structural but our representation is nominal, seems like it's asking for subtle bugs (basically, the interning is no longer computing type equality).

To demonstrate my point, I did a quick search through typeck for cases where we compared types using == or !=. I found one (checking for method override equality). Thus the following program no longer type checks:

type S = uint;
type T = uint;

fn main() {
    obj a() {
        fn foo() -> S { ret 2u; }
    }
    let my_a = a();

    let my_b = obj() {
        fn foo() -> T { ret 3u; } // Type checker reports an error
        with my_a
    };

    assert my_b.foo() == 3u;
}

We can fix this case (use unification) and we can convert any other places where == or != is used, but the problem is that it's hard to find all those places or guarantee that no new ones crop up. (As an aside, it would be nice if users could declare types non-comparable, though it complicates generics).

(On the other hand, the existence of ty_var in the type hierarchy already means that comparing with == was a bit dubious, but it used to be safe after all variables had been substituted away)

I would rather see the names kept alongside the true, interned type as side information. This either means that we have a side table tracking, for each interned type, the shortest ast::ty that was resolved to it (isn't this kind of what we used to do?). Alternatively, we could pass around a (ty::t, ast::ty) pair outside of the ty module, but that's more work. Either way we get user-defined names for print-outs while keeping our interning meaningful.

Alternatively, nominal records would basically make this problem go away with no effort at all (modulo objects).

[marijnh]

I suggest we simply fix code that uses == on ty::t (it's a trivial fix, adding an unpack step before the compare). Both of your solutions boil down to assigning a canonical name to a hashed type, which is not correct. Module A might define type point = {x: int, y: int}, and module B can define the same struct type to be called vector. You want the right name in the right context, based not on which module you are in, but on the source of the actual type. I think this means that named types do have to be different ty::t values, and thus have to be hashed separately.

[nikomatsakis]

Keep in mind, the unpack must be a deep unpack, because you might types like:

type S = uint;
type T = uint;
type U = { f: S };
type V = { f: T };

where U and V are equivalent. Basically, if we do that, I am not sure what the value is to interning types at all. Also, regarding == and !=, as I said it's hard to find all the possible places such a bug could occur, you have to check through all the modules. You also have to find anywhere that types are used in hashtables to make sure it's always the unpacked version that gets hashed, and so forth.

Regarding user-visible names, yes, the names will be approximate. We could keep the mapping from canonical type to user-defined name per-module or, better yet, per-scope, but in the end we may not print the name precisely how the user wrote it. I am not concerned about this personally, it already happens with C compilers and typedef and so forth. Mildly annoying but best effort is probably good enough.

[nikomatsakis]

Also, the intention of passing around (ty::t, ast::ty) was to say that the ast::ty represents the type as the user wrote it in this instance, not some canonical name for the type. That said, this seems more complex to implement (certainly a larger delta) than keeping some sort of table associated with the scope with user-friendly names and then consulting this table when stringifying a type within a particular context.

[marijnh]

That wouldn't cut it though, as you'd have to also make the values used internally in other types somehow carry the name.

Let's try to replace equality tests with a unify-based compare and see how it performs. Yes, someone might introduce bugs by doing compares. I'm starting to feel that our code would actually perform better without interned types. We're unpacking them all over the place, again and again. Interning to reuse structure is a good idea, but the thing passed around might as well be an @ box rather than the interned id.

[nikomatsakis]

Yes, I agree that passing around the pair is a bad idea. I think having
per-scope lookups is a better idea. I think having nominal records,
classes, and interfaces is a better idea still (though clearly not
happening for 0.1...).

Anyway, if we keep this scheme, then yes we need to look through the
code for all places where == or != is used with types or types are
placed in hashtables and the like. In those cases, we need to either
normalize or have some sort of deep equality routine.

Also, keep in mind that putting names into the types does not produce
names that are necessarily valid for the point where an error occurred,
as the names are not adjusted when the scope changes. For example, a
program like:

mod Foo {
     type t = uint;
     fn foo() -> t { ret 32u; }
}

mod Bar {
     type t = int;
     fn bar() {
         let x: t = Foo::foo(); // Error: uint vs int mismatch
     }
}

fn main() {
}

yields the not-particularly-helpful error message mismatched types: expected t but found t (types differ).

[nikomatsakis]

Also: I am inclined to agree that there are better ways to represent types than what we're doing now... interning to an @sty might be one of those. We do use struct a lot.

[nikomatsakis]

Also, re-reading your previous comment, I see that you wanted the stringified version of a type to be based on the source of the type, and not on the module where the error or type mismatch occurs. I am not sure this is desirable at all! I usually expect the type to be written in terms of the line where the error message is pointing.

[nikomatsakis]

All that said, I know I am complaining a lot, but ultimately I am not completely opposed to this fix, although I am not persuaded it's the best approach for the reasons I enumerated. Still, almost anything is better than the current situation, as long as it's not buggy..

[marijnh]

The above patch fixes the way method types (and a few other things) are compared to use proper unification.

As for having different types with the same name in different modules, I think we should use the full path of the definition in error messages to handle that.

[nikomatsakis]

Looks good. I have to admit, yesterday I was building something and I was so pleased to see the user-defined names in place of gigantic structure definitions. :)

[pcwalton]

Considering this fixed for now.