roc-lang / roc

A fast, friendly, functional language.
https://roc-lang.org
Universal Permissive License v1.0
3.86k stars 284 forks source link

feat: allow ResultTryQuestion suffix in AST #6844

Open sekerez opened 5 days ago

sekerez commented 5 days ago

Makes progress toward https://github.com/roc-lang/roc/issues/6828, based on this excellent guide!

No tests required changes, I'll add new automated tests in the desugar followup unless there are recommendations.

lukewilliamboswell commented 5 days ago

What do you think about adding a couple of snapshot tests too?

What you have so far looks good to me.

sekerez commented 4 days ago

What do you think about adding a couple of snapshot tests too?

Great point, working on it rn.

sekerez commented 4 days ago

@lukewilliamboswell added snapshot testing by adding analogous examples in files that previously contained only the task bang suffix, lmk if you'd rather I split out into different files

smores56 commented 3 days ago

@sekerez I'm not Luke Boswell, but I'd vote for splitting out into different files. It makes the testing more single responsibility, and we may have the ? and ! behave differently in the future, though I don't find it likely.

However, I'd understand wanting to keep them in the same file, in that we want any tests updated for ! to also get updated for ?, as their behavior is currently expected to be basically identical.

kdziamura commented 1 day ago

Just a heads-up: I'm changing the parsing logic a little bit here: https://github.com/roc-lang/roc/pull/6851 In particular, EmptyDefsFinal will be dropped

kdziamura commented 17 hours ago

their behavior is currently expected to be basically identical

What about ResultTryQuestion as a statement? Should it be forbidden or implement early return?

I mean:

and = \resultA, resultB -> 
    resultA? # return resultA if it's Result.err
    resultB? # otherwise return resultB
sekerez commented 16 hours ago

What about ResultTryQuestion as a statement? Should it be forbidden or implement early return?

@kdziamura great point - I feel like it should be forbidden, as if resultB is of type Result T, then returning resultB? would return Err T if it's an error, and T otherwise... whereas it should instead return Ok T in that case, hence the ? should be omitted from the return.

Just to check my understanding, returning a value with ? should result in a type error, right? And that's downstream from AST generation? Hence the AST should still generate, right?

edit: I'm a little surprised that code like this compiles:

main =
    Stdout.line! "Hello, World!" # returns {} | Task.Err [...] 

and behaves identically to

main =
    Stdout.line "Hello, World!" # returns Task.ok | Task.Err [...] 

given that in my mind the return type differs, based on the same reasoning for ?.

Is my understanding incorrect? Should we as a matter of convention allow returning resultB?, and wrap it in an Ok if it's not Err?

sekerez commented 16 hours ago

@smores56 that makes sense - I'll split out into separate files, and changes the original files' name to taskbang from suffixed.

@kdziamura also rebasing on your changes.

kdziamura commented 16 hours ago
and = \resultA, resultB ->
    resultA? # return resultA if it's Result.err
    resultB? # otherwise return resultB

It will be desugared into

and = \resultA, resultB ->
    Result.try resultA \{} ->
        resultB

Which makes sense for consistency (! and ? are interchangeable, it’s only one for Tasks and the other for Results).

So, we can indeed fully piggyback the TaskAwaitBang implementation