Fix #934, though Gavin might not like it

[tombentley]

No description provided.

[gavinking]

You're right, I hate it. This is a total layer breaker. There's no way I can possibly know I'm not breaking things when I make changes to the type checker. This kind of logic has to be handled in the back end.

[tombentley]

I can see your point.

However, I don't know how the backend can reasonably deal with it. Once the type of the attribute/variable has been inferred the typechecker will go about using it in the rest of its analysis, and that underlying type will be copied all over the model. How am I supposed to handle it in the backend when I don't know where that incorrect type has propagated to? I really don't want to have to reproduce half the logic of the ExpressionVisitor to do that. Even if I did it would be buggy and fragile.

It seems to me we should infer the correct type and in the case of the JVM that means without the underyling type, since we say that Integer means 64 bit long and the underyling type could only ever be narrower than that.

[gavinking]

Well look I told you guys way back when that this was the wrong way to do it, that ProducedType just wasn't the place where you could put this information, for precisely this reason, and ya'll decided to ignore me! I mean do you want me to dig up the email where I predicted that this problem would arise?

[gavinking]

I mean, looking at your proposed fix, it seems to me that this change is required precisely because of totally realsonable recent changes I made to the typechecker that had the totally unexpectable side-effect of breaking the handling of underlyingTypes, which is all essentially based on the unfounded assumption that the typechecker never creates new ProducedTypes and throws away the old ones.

[gavinking]

Oh, sorry, that's not right, I misunderstood. The issue is in fact the opposite: that it is remembering the underlying type when it shouldn't be. But the point still stands: the typechecker is incapable of reasoning about underlying types, and engineering this stuff into ProducedType was a mistake. It should have been a totally independent infrastructure in the backend.

[quintesse]

engineering this stuff into ProducedType was a mistake

It's the only reasonable place we could have put this! The only common thing that is used all over the place is the type checker tree. Even if we would have made this infrastructure you talk about, how would we make the link between ProducedTypes in the tree and whatever we would have come up with? We would have needed a way to be able to say: all these PTs in the tree all come from one single source. That's something we cannot easily reason about in the backend and even then we'd still need something in the tree to relate thing in the tree to our infrastracture and back.
Trying to do that after the fact would just mean re-implementing lots of the reasoning that the typechecker already does,

What I do agree with is that we should either a) solve this on our own in the backend or b) come up with some way that backends can inject callbacks to do this kind of work in a completely consistent and transparant way to the typechecker. Because we can't just go arounud inserting random withoutUnderlyingType() calls in the typechecker. To me that seems completely wrong (even if it "works").

[FroMage]

Well, like it or not, the underlying type is part of the type, and as such has to be dealt with in the typechecker, because it is tied to ProducedType and not just declarations. It trickles down the AST tree, like ProducedType does. It's just one of those things the typechecker has to deal with, like overloading, even though Gavin may not like it, there is simply no other choice, and certainly not anything saner. This is the only approach that makes sense until someone proves otherwise and we're given this some thought back at the time. Not every typechecker problem is solved better by making it harder to solve in the backend.

[gavinking]

It's the only reasonable place we could have put this!

I don't see how that's true.

The only common thing that is used all over the place is the type checker tree.

Correct. So put the underlying type there.

That's something we cannot easily reason about in the backend and even then we'd still need something in the tree to relate thing in the tree to our infrastracture and back.

It's totally mysterious to me why you can't have your own Visitor which manages underlying types, just like ExpressionVisitor manages types.

[FroMage]

It's totally mysterious to me why you can't have your own Visitor which manages underlying types, just like ExpressionVisitor manages types.

Because there's no point in duplicating that class?

[gavinking]

Because there's no point in duplicating that class?

Whhhaaaa? ExpressionVisitor klocs in at 6000 lines. Surely an UnderlyingTypeVisitor could be implemented in like way <1000 lines. Probably more like 500.

And the point is: separation of concerns, and testability.

[quintesse]

separation of concerns

I do agree here. I mean there no "logic" to this change, no seemingly obvious reason why suddenly the typechecker in these particular 4 lines needs to insert a call it knows nothing about but not in dozens of other similar locations.

[FroMage]

There is logic: it's only for declarations of type value/function. That's very logical to me.

If we pull underlyingType from ProducedType we need to add it to every tree node and every model element, including a new List<String> for method parameters.

[tombentley]

The logic is simple: When we infer the type (i.e. copy the type from the specifier expression to the attribute declaration) we have to drop the underlying type because the declaration (being a Ceylon declaration) should not have an underlying type (by definition, underlying types should only be coming from stuff loaded by the model loader).

The fact is that ProducedType.underlyingType is the "abstraction" we have today for dealing with types that the typechecker cannot reason about. Yet here we have a situation were it must know in order to function correctly. If Gavin's rejecting this patch, that's his decision, but we still have a bug to fix, so it would seem the alternatives are:

• Provide a way for the backend to be involved with the type inference -- for example a subclass of ExpressionVisitor which overrides a method or two or
• Completely rewrite how the compiler deals with what are today called underlying types.

[quintesse]

That's not logic, at least not from the point of the type checker. It's just something that happens to work. But someone working on the typechecker without any knowledge about backend can't know what to do here.

And I'm not suggesting changing anything about the current system, I actually agree that PT is the best place for this information for us, but I also think we can't pollute the typechekcer with behaviour that it knows and cares nothing about.

Provide a way for the backend to be involved with the type inference

Which was one of my suggestions yes

Or maybe there is a way to nicely abstract this information away into PT that makes it official that model loaders (in general) can add extra information to PTs and that from there define it as a rule that that information is lost when assigning or passing it as a parameter, etc.

But I can't help but agree with @gavinking that the current fix from his perspective is horrible. (But @gavinking please help us to come up with something that works for you and for us)

[gavinking]

My prediction is that if you rewrite this as a separate visitor, you will discover that suddenly you understand the logic for handling underlying types much better than you do today, and will discover and fix several bugs in the process. I also suspect that it's a very easy job. You would not need to reproduce any of the functionality of ExpressionVisitor because you would already have the output of ExpressionVisitor readily available to you, right there in the tree.

[quintesse]

(I'll repeat this here because I don't know if anyone besides Tom reads the original issue):
Is another solution maybe that the type checker never copies the underlying type at all? And that we convert from the underyling type to long/double/etc at the earliest possible opportunity?
(and if copying is needed it might be enough to do it with a visitor as @gavinking suggests, it does seem similar to the other things we need to propagate around the tree like erasure info etc)

[FroMage]

This appears to have been merged manually so I'll close it.