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

Prefer foo() to !foo #5114

Merged
merged 5 commits into from
Jun 25, 2024
Merged

Prefer foo() to !foo #5114

merged 5 commits into from
Jun 25, 2024

Conversation

pchiusano
Copy link
Member

@pchiusano pchiusano commented Jun 20, 2024

From recent discussions, there's a feeling that ! isn't really paying its way as special syntax. It...

  • looks weird...
  • isn't suggestive of what it is (passing () to a function), and...
  • it's in the "wrong" spot, needing to go before the symbol being forced - by time I think to write ! I've already generally typed the symbol and have to backtrack in the buffer.
  • adding confusion, identifiers can end with a !, a loose convention used for a variant of a function that runs its effects rather than returning a thunk, so you sometimes have expressions like object.at! !qux (quaffle 1 2) and it makes me sad to think about trying to explain why the ! appears at the end of object.at but at the start of !qux...

This PR changes things. Before, you'd write !foo to mean foo () with high precedence. Now you can just use foo() with no spaces after the foo to get the same precedence and meaning as foo (). The pretty-printer uses this syntax as well. The old !foo syntax is still supported in the parser but is now unused by the pretty-printer.

Here's a few examples (which parse and pretty-print like this):

deployHttp Environment.default() hello.logic

forkAt pool() do 1 + 1

The lack of any space after Environment.default and pool makes this bind tighter than the normal function application syntax. If you put any spaces, the parenthesized thing is treated like a tuple, just like now.


(possibly controversial, ended up being removed from this PR, but including for posterity) It was easy to generalize this syntax to have it work for higher-precedence function application, just like in C, Java, etc:

  • foo sqrt(2.0) parses as foo (sqrt 2.0)
  • foo max(x, y) parses as foo (max x y)

The pretty-printer still prefers the foo (max x y) syntax in these cases though, so consider this more of a "I'm typing and don't feel like going backwards to add parens" or "I forgot that I'm not programming in Java, but this still parses the way Java would".

Implementation notes

A simple, surgical change to the term parser and pretty-printer.

Interesting/controversial decisions

Possibly this whole thing is controversial. :)

Some possibilities I considered:

  • We could disallow foo(x, y, z) for function application, instead just supporting foo() for forcing. I didn't see the harm in including the more general syntax. I'll probably use it myself when I'm feeling lazy about going back and adding parens, and it might make folks coming from other languages more comfortable. (I have seen people accidentally using foo(x,y,z) for function application many times.)
  • Don't use blah foo() arg2 in the pretty-printer - print this as blah (foo ()) arg2. So basically - this PR, but don't use the syntax in the pretty-printer. I don't like this as much. I feel like forkAt pool() do ... is pretty easy to parse even if you don't know Unison. You tend to assume that pool()
  • Remove ! syntax entirely. I'd rather keep it around for a while. We can remove it after people have adjusted and we're confident in the change.

This is also easy to back out anytime later.

Test coverage

Existing transcripts provide pretty good coverage, and I added a round trip test. I think coverage is good.

Loose ends

None

@aryairani
Copy link
Contributor

Good discussion —

On first read, I hated giving foo() a different meaning from foo (), it feels hard to explain.

Generalizing it to multi-arg calls, introducing a full, second, function-calling syntax,

<id> '(' [args,..] ')'

vs

<id> [args..]

is much better IMO than introducing a single, special case.

The fact that the pretty-printer uses both forms though means that everyone has to learn both forms.

So, I think it's an improvement, but it's worth thinking about the end-game too, to know which direction we should be heading in, and would like to hear other feedback.

@pchiusano
Copy link
Member Author

Okay, after hearing the discussion, I'm going to axe the general function application syntax for now (which many thought was iffy given the pretty-printer doesn't use it) and just have the PR be about replacing !foo with foo(). Thank you very much to everyone who weighed in.

Note that changing function call syntax to C-style more generally is a bigger deal. I don't think it plays nicely with the rest of Unison's syntax, its use of layout, curried functions, etc, and I consider it out of scope for this PR, more something we'd consider as part of a comprehensive rethink of Unison's syntax. Or if we were supporting #499. So, not now. :)

@aryairani one note on your comment above, there actually isn't a consistent rule between the two -

  • foo() means (foo ()) with high precedence.
  • foo(x,y) means (foo x y), aka left nested function application where the comma separated elements are the list of args.

The base case of the second interpretation would have that foo() means the same as foo (it's a left fold starting with foo, with the empty list of arguments), not that it's passing Unit to foo. You'd need to write foo(()) to get (foo ()) without a special case.

Aside: I think it could be cool to let users supply their own syntax sugar since then it's opt in, but I tend to agree it's a little weird and possibly confusing to add sugar for everyone when that isn't going to be used by the pretty-printer.

I'll get this cleaned up and all the transcripts passing.

@pchiusano pchiusano changed the title foo() instead of !foo and allow foo(x,y,z) function application syntax Prefer foo() to !foo Jun 21, 2024
@aryairani
Copy link
Contributor

@pchiusano Oh, I see, thanks for pointing that out. I glossed over it, because it looks like a function with no args.

I guess I'm not convinced then that this special case helps, short of a comprehensive rethink, which we should consider undertaking. (A comprehensive rethink doesn't necessarily mean radical changes, but who knows.) I'm not sure who'd be best to lead it but I'd be happy to try.

I don't have any concerns about just eliminating the ! syntax though, calling foo () or bar (foo ()) or bar (force foo) as needed.

@pchiusano
Copy link
Member Author

pchiusano commented Jun 21, 2024

K, this is all set, I reduced the scope as discussed. I ack that some people might prefer removing any special treatment for thunk forcing (or at least phasing it out of the pretty-printer). These things are always going to be a judgement call / personal preference which is okay. :)

Also just noting again how easy it is to change our minds on this anytime later. (For instance, we could easily drop the special case from the pretty-printer and have it print with an extra level of parens, and we could also drop the special case from the parser, forcing extra parens there as well.) This PR will not be the last time we get to bikeshed Unison's syntax. :)

@aryairani
Copy link
Contributor

aryairani commented Jun 24, 2024

Paul and I discussed on Slack, and decided to give this PR a shot; gambling that users will not, in reality, be confused by this change, even without requiring any immediate update to the language docs, since the parser continues to accept the old syntax.

Any "told-you-so"s will be dealt with on a case-by-case basis.

# Conflicts:
#	unison-src/transcripts-round-trip/main.output.md
#	unison-src/transcripts-round-trip/reparses-with-same-hash.u
@aryairani aryairani merged commit 53c3a16 into trunk Jun 25, 2024
35 checks passed
@aryairani aryairani deleted the topic/force-syntax branch June 25, 2024 14:34
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