-
Notifications
You must be signed in to change notification settings - Fork 430
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
"of" word is not needed. #491
Comments
Should exception also change? For example:
would become
|
I think we should also change exception for consistency. |
Exceptions should also change. |
Alright, I want to take a go at this issue. |
So I've been breaking my head on this issue - as this is really unfamiliar territory for me. I made a change to This is the reason_parser.conflicts that I'm getting:
Note that I added I think it's related to tuples - but I'm not 100% sure. Any hints on how to proceed with this would be very welcome. |
@SanderSpies You are right, it is related to tuples. Basically with the proposed syntax, the compiler cannot differentiate these two cases when the look ahead token is the highlighted type a = A When it gets to the highlighted The debug log tells you how the compiler gets to each case. |
@yunxing it's not clear why the presence or removal of "of" would change that ambiguity. |
I'm a little torn on this. Reason has a great syntax to bring new people into the ML fold. Removing the 'of' would be shorter and cleaner, but it will also make it slightly more unreadable for people unfamiliar with ML-like languages. I was surprised when I showed this syntax to a Java developer and he just got it because of the |
@nmn I would not have predicted that. Doesn't it help that the variant tag is capitalized, and the rest are lowercase? |
@jordwalke I personally have no problem reading that myself, but I have experience reading functional ML-like languages. But when I was showing Reason of to not-so-functional developers, the As I said, I'm torn! |
I wonder if those people were confused that expressions were written "Some x" instead of "Some of x" |
I'll do some more research on this. |
@jordwalke So most people agree that the Someone compared it to the On the other hand, a lot of people find |
Okay, I think it's about tied. I will now go ask some tie breakers. I don't think it's a make or break decision but all else equal a kind of poll is a good signal. |
I asked an audience of experienced OCamlers and the unanimous feedback was that |
@jordwalke That sounds good. @SanderSpies Do you want to take a try on this one as a puzzle? I actually have a working version on my local branch but it may be good for you to try to solve it so you can get a good understanding of our We've discussed where to start on IRC yesterday. Feel free to ask any question either on IRC or here. |
@yunxing Yeah, I'd like to work on this one to get a better feeling on how to solve reduce/reduce issues. I expect to be able to focus on this on Friday. |
One of the things I do not like about this style of type declaration (with or without the My suggestion would be to simply remove this style of type declaration from Reason altogether and replace it with the GADT style. With the GADT style, the original example would look like this: type something =
| Constructor: (int, int) -> something
| Another: list int -> something
; I think this is better for a few reasons. First, it doesn’t require learning a different syntax for declaring constructors for algebraic types. It’s just the same syntax you use for functions. Even with the current style, you have to know that the Second, it would make the language simpler since there wouldn’t need to be a separate style for declaring GADT and non-GADTs. In general, I am in favor of simplifying the language and consolidating the syntax and this seems like a good opportunity to do that. In OCaml, removing the non-GADT syntax would be pretty much impossible now because of all the legacy code but Reason doesn’t have this problem yet. The main downside to the GADT syntax is that it is a bit more verbose. Personally I think that is a worthwhile tradeoff. |
@freebroccolo This syntax change makes sense if curried constructors are also added. Otherwise this would make the already initially confusing difference between constructor application and function application more confusing. Making all variants GADTs is an interesting idea. Does removing the difference between ADTs and GADTs cause trouble anywhere for users? |
Of course. We can evolve the syntax fairly rapidly and upgrade all existing source trivially.
I have considered this and thought quite a bit about it actually. I wanted to do this, for exactly the reasons you mentioned (so that learning GADTs aren't such a huge step). The only reason I held off on it was because it's more verbose (I had the same proposal as you gave). I would like to find a syntax that not only unifies them but also is less verbose if possible. Any ideas? (That should probably be another github issue because it's independent of the choice of including For anyone out there watching, the current syntax for GADTs is as follows (which is close to @freebroccolo's suggestion):
The |
Type-based disambiguation of constructors works for ADTs but not
(nearly so well?) for GADTs. But if the parser mapped GADT syntax to ADT abstract syntax where possible, I guess that should not be an issue.
|
That was the thinking - that the syntax merely help people make the connection between ADTs and GADTs, where one is pretty much a special case of the other.
Can you elaborate? Is this true for the subset of GADTs which do not exercise any GADT-ness such as:
|
I'm not positive, but my impression is that the type-checker just |
Changes: remove of from lexer remove of from messages file remove of from tests remove of from printer remove of from parser generator
I think this issue can closed @jordwalke |
Thanks! |
Multiple people have suggested removing the need for the
of
keyword. Sounds great to me, if possible and doesn't introduce grammar conflicts.@kayceesrk
Tagging this as "good first task" for newcomers in the community to give it a shot. It should be relatively straightforward.
The text was updated successfully, but these errors were encountered: