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

Fix FnCall #5363

Merged
merged 6 commits into from
May 9, 2024
Merged

Fix FnCall #5363

merged 6 commits into from
May 9, 2024

Conversation

OceanOak
Copy link
Collaborator

@OceanOak OceanOak commented Apr 30, 2024

we accidentally got away from how we normally represent function calls in writtenTypes.dark, this pr fixes it.

  • Grammar: Remove parens around the function call
  • Grammar: Rename function_call to apply?
  • WrittenTypes: use EApply and EFnName instead of EFncall
  • Fix parser
  • Get tests to pass
  • Fix semantic tokens

#5321

@OceanOak OceanOak changed the title WIP: fix fncall Fix FnCall May 3, 2024
@OceanOak OceanOak marked this pull request as ready for review May 7, 2024 15:32
@OceanOak OceanOak requested a review from StachuDotNet May 7, 2024 15:34
Copy link
Member

@StachuDotNet StachuDotNet left a comment

Choose a reason for hiding this comment

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

This is looking great!

Two thoughts:

  1. it'd be great to include, in the grammar.js, some /** commentary */ on expression and simple_expression noting why they exist and how they're different

  2. to that end, I thought the logic there would be "some expressions are really clear where they start/end, and never need parens, and others might need parens () around them to help disambiguate.

But, these three being in simple-expression gave me pause:
$.dict_literal,, $.record_literal, $.enum_literal,

Ok well for dicts, the {} make it clear I suppose. and for records, they have the {} and just a quick label to the left. and enum literal, similar to record literal (esp since even for 1 arg we require the () to wrap, for now). So, in writing this, I've sort of reconsidered. But, it gave me pause - "isn't there chance for ambiguity when passing one of these into a fn argument?"


Anyway, this is mostly a request for some such commentary in grammar.js, and wanted to think through those 3 cases.

@StachuDotNet
Copy link
Member

also, I think the answer to

Grammar: Rename function_call to apply?

is yes

@OceanOak OceanOak marked this pull request as draft May 8, 2024 08:41
@OceanOak
Copy link
Collaborator Author

OceanOak commented May 8, 2024

isn't there chance for ambiguity when passing one of these into a fn argument?

I've added more tests around this, and a todo to reconsider having enums as a simple_expression. maybe once we remove the parentheses around enum field, it could cause ambiguity (not sure). Thanks for pointing it out

@OceanOak OceanOak marked this pull request as ready for review May 8, 2024 09:07
@OceanOak OceanOak requested a review from StachuDotNet May 8, 2024 09:11
@StachuDotNet
Copy link
Member

great work here!

@StachuDotNet StachuDotNet merged commit 2c82e40 into darklang:main May 9, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants