dylan-lang / opendylan

Open Dylan compiler and IDE
http://opendylan.org/
Other
458 stars 69 forks source link

Type unions treated incorrectly by subtype? #773

Open cgay opened 10 years ago

cgay commented 10 years ago

Testworks has code in do-check-condition like this:

let condition-class :: subclass(<condition>) = f();

If f() returns type-union(c1, c2) where c1 and c2 are both subclasses of <condition> Testworks gets a runtime error similar to this:

parse-request-line-values(request-line, request-line.size) signals condition error-class crashed [Error evaluating assertion expression: {<union>: <bad-request-error>, <moved-permanently-redirect>} is not of type {<subclass>}]

This seems like something the compiler ought to be able to figure out.

See this work-around for reference: https://github.com/dylan-lang/testworks/pull/58

hannesm commented 10 years ago

now the crucial question is whether subtype?, ^subtype? or type-estimate-subtype? is responsible (and whether they're in sync). Also, is there an ordering of typing rules? I'd appreciate if someone sits down and documents the current behaviour (and/or expected) by writing a test suite... and due to Dylan's metaprogramming facilities, we should actually generate all the subtype functions from a single set of rules (typist already uses some meta-programming for its rules); and maybe also instance and known-disjoint etc!?

waywardmonkeys commented 10 years ago

I have a branch that I'm working on that may address this eventually ... right now, I see that adding stuff to sources/dylan and sources/dfmc/modeling isn't sufficient. I probably have to add some stuff to the type estimate code in the typist as well.

waywardmonkeys commented 10 years ago

I believe that it is type-estimate-subtype? and the corresponding disjointness code that is required to be fixed for this. That's the part that I didn't do yet.

The DEP that added subclass() in the first place doesn't adequately describe the rules for handling this, so I'm going with what is logical and plan to document it somewhere / somehow once we get something working.

waywardmonkeys commented 10 years ago

To be more clear, there were no existing rules implemented for handling relations between <union> and subclass(). I'll work on the type-estimate-subtype? stuff this week and hopefully that'll resolve this in conjunction with what I've already done.

BarAgent commented 10 years ago

The applicable rule from DEP-0005 is the INSTANCE-1 rule, I think. It specifically says that the object you are testing must be an instance of <class>. A type-union is not a class, but rather a non-class type. They designed it that way intentionally. The purpose of subclass is to "return a type describing all the subclasses of its argument class".

In effect, the code fragment @cgay gave above should be read as:

let condition-class :: type-union(singleton(<condition>), singleton(<error>), etc.) = f()

This means that if f() were to return either <bad-request-error> or <moved-permanently-redirect>, the type check would clearly work. But it actually returns a type-union object, not one of the singleton objects, so it fails.

That seems to be the correct behavior to me.

The DEP authors acknowledged the limited scope of the construct. The original problem description asked for "a type which allows us to express applicability to a type and all its subtypes". Instead they designed something that does not "address everything in the problem description…but are simpler to understand than subtype types and still very useful…".

But even if subclass did return a type describing a type and all its subtypes, that wouldn't help here, because specialization type-checks are described in terms of instance? not subtype?. That is, the type-check asks "is X an instance of Y". And asking "is type-union(<c1>, <c2>) an instance of any of the subtypes of <c>" makes about as much sense as asking "is the type <rgb> a direct instance of the type <color>".

I do think subclass is poorly named, though. It should have been subclasses. But that ship has sailed.

waywardmonkeys commented 10 years ago

I still plan to get back to my branch for this and make it work. I've just been distracted by 100 other things as well. My approach looks solid and theoretically right / sound.