Skip to content
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

feat(compiler)!: Change -> to => in type signatures #1855

Merged
merged 2 commits into from
Jun 15, 2023

Conversation

alex-snezhko
Copy link
Member

@alex-snezhko alex-snezhko commented Jun 11, 2023

Closes #1179 (supersedes)

Copy link
Member

@spotandjake spotandjake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

I don't fully understand the code in the lever hack that checks the data types but what I did understand looks good.

Copy link
Member

@ospencer ospencer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work here!

Copy link
Member

@marcusroberts marcusroberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Oscar's happy with the parsing, this looks great.

One question, can we use the formatter or provide a utility to help people bulk change their code to the new style?

Copy link
Member

@peblair peblair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments:

  1. I would love to see some stress tests/weird cases which should not parse (let f = (foo, bar) => Number) in the test suite
  2. Extremely minor concern: just want to double-check other Core Team members (cc @ospencer and @phated especially) are cool with this possibly introducing some potential confusion for new users (e.g. let lexer : (stream) => List<Token> might look to a new user like stream is supposed to be the name of the function argument and not a placeholder type).

@spotandjake
Copy link
Member

  1. Extremely minor concern: just want to double-check other Core Team members (cc @ospencer and @phated especially) are cool with this possibly introducing some potential confusion for new users (e.g. let lexer : (stream) => List<Token> might look to a new user like stream is supposed to be the name of the function argument and not a placeholder type).

I feel like the issue of the confusion was already there before the arrow change.

@peblair
Copy link
Member

peblair commented Jun 12, 2023

I feel like the issue of the confusion was already there before the arrow change.

Yes, true, but the -> syntax made it a bit more clear when reading quickly that one was looking at a type, not a function definition. Maybe I am wording this scenario wrong, but I am just envisioning some potential weird interaction with the unified => syntax and placeholder type names

@alex-snezhko
Copy link
Member Author

possibly introducing some potential confusion for new users (e.g. let lexer : (stream) => List might look to a new user like stream is supposed to be the name of the function argument and not a placeholder type).

@peblair A potential point of relief is that if the user is using VSCode or possibly another editor we may support integration with in the future, the LSP & syntax highlighting may allow for a form of distinguishability between parameter names and placeholder types. For example with the code

let a: stream => Number = myStream => 1

VSCode should highlight/format stream differently from myStream (assuming the color scheme they are using prints them differently of course). They will also get this code lens:

(myStream: stream) => Number

which can be useful for figuring out what is going on. Not perfect, but it's something at least

@alex-snezhko
Copy link
Member Author

@peblair I added some parsing tests to types.re; are those the sort of cases you had in mind?

@phated
Copy link
Member

phated commented Jun 14, 2023

Going to merge this, we can always add more tests later 🎉

@phated phated added this pull request to the merge queue Jun 14, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Jun 14, 2023
@alex-snezhko alex-snezhko added this pull request to the merge queue Jun 14, 2023
Merged via the queue into grain-lang:main with commit b3d68a4 Jun 15, 2023
@alex-snezhko alex-snezhko deleted the return-type-arrows branch June 15, 2023 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Syntax: Change -> to => in type signatures
7 participants