racket / typed-racket

Typed Racket
Other
527 stars 104 forks source link

since 6.1.1, `require/typed` seems to have lost its ability to import partial class interfaces #64

Open mbutterick opened 9 years ago

mbutterick commented 9 years ago

Comparing behavior of 6.1.1 build vs. git build from yesterday, require/typed is no longer allowing partial import of class interfaces. (IMO showstopper bug for existing code)

For instance, in 6.1.1, this code works without error:

#lang typed/racket
(require/typed racket/draw [record-dc%  (Class (init-field))])
(define dc (new record-dc%))

But in the current git build, it now throws an error:

record-dc%: broke its contract;
 method glyph-exists? not specified in contract
  in: g5
  contract from: (interface for record-dc%)
  blaming: (interface for record-dc%)
   (assuming the contract is correct)

If I add glyph-exists? to the require/typed spec, then I get a new error asking for another class method. And so forth. I confess I did not get to the end of the list to find out if require/typed would cooperate.

mbutterick commented 9 years ago

Doesn’t help much narrowing it down, but the bug is present in snapshot 20150322-785fb57 also.

mbutterick commented 9 years ago

My last full git build was about a month ago. This code worked then. So I’m supposing the wrinkle has been added in the last 4–6 weeks.

takikawa commented 9 years ago

This issue is unfortunately not a bug, but an intentional design decision that was just not fully implemented in the type->contract translation until the upcoming release. There was a warning note to this effect in the docs, but I would agree that this is not a very helpful response if you already have code that relies on the old behavior.

The issue is that if you allow such a partial specification, it is possible for Typed Racket to end up with a class value whose type claims it doesn't have a particular method or field.

When that happens, Typed Racket would let you add that method or field via define/public or field in a subclass of the imported class. But this should be a type error because you're trying to add a conflicting method/field (and ordinary Racket will error).

For the particular example you've given, I can add record-dc% to typed/racket/draw and then you should be able to use that library without specifying your own type for the class. The fact that it's not there right now is just an oversight.

In retrospect, it would have been better if I had made this type->contract translation more conservative to begin with to avoid breaking your code. Sorry about that.

samth commented 9 years ago

Could we write a contract that prohibited subclassing entirely, but allowed new? That would make @mbutterick's example work, while avoiding the unsoundness.

takikawa commented 9 years ago

@samth we could hypothetically (though the implementation would require changing the class system and not just class contracts), but how would we decide to compile to that contract vs. the usual contract? Are you thinking of making a non-subclassable Class type?

takikawa commented 9 years ago

@mbutterick I added record-dc% to typed/racket/draw in commit f758a6ed42474f38498f41f5f49a7e384b1309c6, so you should be able to import it from there without a require/typed.

mbutterick commented 9 years ago

OK, I now see the warning at the top of Typed Classes. (I was only working with require/typed so I didn’t think to look over there.)

I respect the design decision. Certainly you should not turn anything into a pretzel for backward compatibility with a small universe of source files (least of all mine).

Thank you for adding record-dc% to typed/racket/draw.

To @samth’s point, however, there’s still a broader ergonomic consideration. The programmer relying on a class has no control over its interface. If the class designer puts in ω methods and fields, it’s going to be a giant drag for the person who needs to use it in TR for one indispensable method. (This is certainly true of the draw library; you can’t get anything done without resort to classes.) IOW it seems a little unfair to export interface complexity onto the person who wants to keep things simple.

Perhaps you could package @samth’s proposed semantics in a new form like require/typed/final that would let a programmer trade subclassability for an interface that’s no larger than it needs to be.

samth commented 9 years ago

@takikawa I was thinking that if there were insufficient method specs, then we give a contract error on subclassing (with something like the current error message).

takikawa commented 9 years ago

@samth ah, that makes sense. I think that is implementable but a fairly invasive change (since reflection on classes doesn't always give you much info, so it has to be in the class internals. Also since restricting subclassing is not something the API really provides right now).

samth commented 9 years ago

This isn't necessarily what I'd prefer as an original design, but backwards compat constraints are real (despite the warning).

rfindler commented 9 years ago

I think it would not be too hard to add a notion of a "final class" to racket/class and then to make one of those in this case. You'd have to add one bit of information somewhere into the class struct and then a check in the subclass creation code. It seems doable?

rfindler commented 9 years ago

(The point being that you wouldn't use the contract system-- you'd just make a subclass of whatever it was and then make that subclass be a final class)