New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type expanders for TR #604

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@takikawa
Contributor

takikawa commented Mar 27, 2014

This PR adds type expanders ala match expanders to TR.

I implemented this so I could add a shorthand notation for mixin types. See the second commit.

This still needs tests and docs, but I thought I'd run it by to see if (1) people thought this was a good addition at all, and (2) that the design looks palatable.

takikawa added some commits Mar 27, 2014

Add type expanders for Typed Racket
Analogous to match expanders but for types
@endobson

This comment has been minimized.

Contributor

endobson commented Mar 27, 2014

Why is define-type not enough here?

@samth

This comment has been minimized.

Member

samth commented Mar 27, 2014

This will let you abstract over parts that aren't themselves types, as with
the Mixin constructor.
On Mar 26, 2014 10:48 PM, "Eric Dobson" notifications@github.com wrote:

Why is define-type not enough here?


Reply to this email directly or view it on GitHubhttps://github.com//pull/604#issuecomment-38764071
.

@endobson

This comment has been minimized.

Contributor

endobson commented Mar 27, 2014

Ah, I didn't get that the argument to implements has to be an id and cannot be an actual class form.

@takikawa

This comment has been minimized.

Contributor

takikawa commented Mar 27, 2014

More generally, there are other type notations that could be made into type expanders instead of built-in. For example, the new ->* type constructor could just be a type expander. Users could define their own function type syntaxes.

Note: making ->* a type expander is actually not a good idea because we want them to print nicely. That's one disadvantage of type expanders.

@endobson

This comment has been minimized.

I don't think this module belongs in the types directory. The rest of the directory is implementing the internals of the type system not syntax that is exported to the user.

This comment has been minimized.

Owner

takikawa replied Mar 27, 2014

Maybe base-env then?

This comment has been minimized.

endobson replied Mar 27, 2014

At least for define-type-expander. The struct definitions might not want to belong there though. Currently our file layout is a mess, and the fact that we have a ton of lazy-requires doesn't make it any better.

This comment has been minimized.

Collaborator

samth replied Mar 27, 2014

It seems to belong in the same place as parse-type.rkt, which is in private.

@endobson

This comment has been minimized.

Can we document what the valid values are. I assume it is either the index of the field that is the procedure or a procedure itself.

@takikawa

This comment has been minimized.

Note, just realized this contract is wrong. The result is (U accessor Procedure).

@endobson

This comment has been minimized.

Nit: expander-expr should be an expr syntax class.

@endobson

This comment has been minimized.

Likely this should be a syntax/loc.

@endobson

This comment has been minimized.

This seems like if I define a TypeExpander it acts like a normal macro in expression positions. Why is that desired? I can see have a different transformer procedure for that, but don't see why I would want the same one as the type expansion.

This comment has been minimized.

Owner

takikawa replied Mar 27, 2014

Yes, you're right, this should use an optional extra transformer expression in define-type-expander (like define-match-expander) or just be removed.

This comment has been minimized.

Collaborator

samth replied Mar 27, 2014

I think it should just error in expression position. match expanders work that way so that you can have a parallel between construction and pattern matching, but that doesn't apply here.

@endobson

This comment has been minimized.

Wouldn't this be simpler if the actual type-expander was the argument not the id that has it as a binding.

This comment has been minimized.

Owner

takikawa replied Mar 27, 2014

Agreed, and it's easy to do if we use the static syntax class like you suggested. (more awkward without it)

@endobson

This comment has been minimized.

If this module is going to apply the procedure to 1 argument shouldn't it at least do an arity check to ensure it doesn't get blamed for bad things, maybe even a real contract.

@endobson

This comment has been minimized.

Should these be ids and maybe even check that they are bound to type aliases (if that is possible at this stage of parsing).

This comment has been minimized.

Owner

takikawa replied Mar 27, 2014

Yes on the first. The latter isn't possible at this point, I think.

This comment has been minimized.

Collaborator

samth replied Mar 27, 2014

I think we actually want something more general than just single ids. If you look at the case studies, @takikawa, you'll see that a bunch of them use multiple inlined members where out goes.

This comment has been minimized.

Owner

takikawa replied Mar 27, 2014

Instead of changing those to non-ids, we can optional clauses at the end for extra members and/or new members. (and maybe make the ids themselves optional?)

@endobson

This comment has been minimized.

@takikawa

This comment has been minimized.

Contributor

takikawa commented Mar 27, 2014

I think I've addressed the PR feedback so far (except making Mixin more interesting), but I've discovered a problem: I had only been testing this at the REPL (where it works fine) but define-type-expander does not work in a module. Though it does work if the expander is defined in prims.rkt like Mixin.

e.g., this does not work:

#lang typed/racket

(define-type-expander Empty (lambda (stx) #''()))

(: x Empty)
(define x '())

Any ideas why? (the syntax-local-value is presumably failing to get the data)

Edit: Ryan gave me an idea of what's going on, so I think I can fix it but we'll see.

@samth

This comment has been minimized.

Member

samth commented Mar 31, 2014

Note: you can see the problem by running this gist: https://gist.github.com/samth/9834239

@takikawa

This comment has been minimized.

Contributor

takikawa commented Dec 8, 2014

Couldn't get it working, will reopen on TR repo if I can make it work later.

@takikawa takikawa closed this Dec 8, 2014

@samth

This comment has been minimized.

Member

samth commented Dec 8, 2014

We very much want this, so talk to @mflatt if you can't figure it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment