Type annotations for suffixed sugar #6868

kdziamura · 2024-07-04T16:54:18Z

In this implementation, type annotations are propagated this way:

Annotated assignments:

f = \x ->
    r : A
    r = x!
    Task.ok r

f = \x ->
    Task.await
        (
            # Type annotation goes here as the first Task arg
            a0 : Task A _
            a0 = x 
            a0
        )
        \r -> Task.ok r

Statements:

f = \x ->
    x!
    Task.ok {}

f = \x ->
    # Stmt desugared to AnnotatedBody with unit type
    _ : {}
    _ = x!
    Task.ok {}

f = \x ->
    Task.await
        (
            # Type annotation goes here as the first Task arg
            a0 : Task {} _
            a0 = x
            a0
        )
        \_ -> Task.ok {}

crates/compiler/can/src/desugar.rs

lukewilliamboswell · 2024-07-05T00:43:02Z

@kdziamura -- I've added to the zulip discussion seeking clarification on the intended design.

I think the current behaviour is correct as it restricts the use of statements to Task {} _, while the proposed change would permit using a task that returns anything and is not correct.

If a user would like to ignore the output they can be explicit and use a _ = assignment, otherwise this should be a type error.

Does this explanation help with your original problem?

kdziamura · 2024-07-05T06:36:08Z

It makes sense. I'll change _ back to {}. The other part of the fix seems to be correct:

ignoreResult = \task ->
    Task.await (Task.result task) \{} ->
        Task.ok {}

# 3. drop destructuring because input and output are the same. it's incorrect
ignoreResult = \task ->
    Task.result task

this step hides the intention of returning Task.ok {}, so the actual fix is to drop it

kdziamura · 2024-07-05T07:10:15Z

@lukewilliamboswell I updated the fix. Now it works as expected and generates a type error:

ignoreResult = \task ->
    Task.result! task
    Task.ok {}

── TYPE MISMATCH in Bug.roc ────────────────────────────────────────────────────

This 2nd argument to this function has an unexpected type:

8│      Task.result! task
                    ^^^^^

The argument is an anonymous function of type:

    {}a -> Task {} *

But this function needs its 2nd argument to be:

    Result ok err -> Task {} *

crates/compiler/can/tests/test_suffixed.rs

lukewilliamboswell · 2024-07-19T09:25:30Z

crates/compiler/can/src/desugar.rs

+            // _ = stmt_expr!
+
+            let region = stmt_expr.region;
+            let new_pat = arena.alloc(Loc::at(region, Pattern::Underscore("#!stmt")));


Do these patterns ever conflict with each other?

Nope. At least not with the current design. The underscore pattern is not an identifier and it has no unique name restriction

lukewilliamboswell · 2024-07-19T09:28:10Z

crates/compiler/can/src/suffixed.rs

+    /// Suffixed sub expression
+    /// e.g. x = first! (second! 42)
+    /// In this example, the second unwrap (after unwrapping the top level `first!`) will produce
+    /// `UnwrappedSubExpr<{ sub_arg: second 42, sub_pat: #!a0, sub_new: #!a0 }>`


Minor nit. We change the unique prefix above, so these comments are now not quite right.

Otherwise these comments are a helpful addition.

Nice. I updated the other comments, seems to miss this one

fixed in 3b50a83

lukewilliamboswell · 2024-07-19T09:28:47Z

crates/compiler/can/src/suffixed.rs

@@ -60,19 +70,17 @@ fn init_unwrapped_err<'a>(
            // Provide an intermediate answer expression and pattern when unwrapping a
            // (sub) expression.
            // e.g. `x = foo (bar!)` unwraps to `x = Task.await (bar) \#!a0 -> foo #!a0`
-            let answer_ident = arena.alloc(next_unique_suffixed_ident());
+            let ident = arena.alloc(format!("{}_arg", next_unique_suffixed_ident()));


Why did we add the _arg?

Just for clearer purpose of the variable. The value comes from the argument

lukewilliamboswell · 2024-07-19T09:35:18Z

crates/compiler/can/src/suffixed.rs

+                Defs(arena.alloc(defs), new_var),
+            ))
+        }
+        _ => {


Is it clearer if this is None?

Agree, will change

fixed in 3b50a83

lukewilliamboswell

I think this looks good, and I've done some testing also.

The only thing I'm unsure of is the #!stmt's and if they should also have unique identifiers/numbers with them?

Also we are switching in comments between the new and old syntax. Might be helpful to do a find-all and replace the old versions.

joshuawarner32 · 2024-07-24T04:41:56Z

crates/compiler/can/src/suffixed.rs

+            let new_ident = match loc_pat.value {
+                Pattern::Underscore("#!stmt") => format!("{}_stmt", new_ident),
+                Pattern::Identifier { ident }
+                    if ident.starts_with('#') && ident.ends_with("_stmt") =>


This seems a little fishy; why do we need to do string parsing/matching like this? This is mildly concerning since it introduces a very subtle dependency, where someone changing a string that shouldn't matter, can accidentally have broader consequences in the future.

Is there a way to get this same information without trying to parse identifier strings like this?

Sure. Need to change the existing enum structure or introduce another one. I just didn't want to have too many changes in this pr to keep it simple. I can refactor it as a follow-up

joshuawarner32 · 2024-07-24T04:46:41Z

crates/reporting/src/error/type.rs

            use roc_types::types::AnnotationSource::*;

+            let is_suffixed = match &name.value {
+                Pattern::Identifier(symbol) => symbol.as_str(alloc.interns).starts_with("#!"),


Similarly here - is there a way that we can get the info we need here, without inspecting a string like this?

Probably due to roc-lang/roc#6868

kdziamura self-assigned this Jul 4, 2024

kdziamura force-pushed the suffixed-stmt-desugaring branch from 3a329ba to c337853 Compare July 4, 2024 17:07

kdziamura changed the title ~~Suffixed stmt desugaring~~ Fix suffixed stmt desugaring Jul 4, 2024

kdziamura enabled auto-merge July 4, 2024 17:08

kdziamura requested review from lukewilliamboswell and joshuawarner32 July 4, 2024 19:37

kdziamura commented Jul 4, 2024

View reviewed changes

crates/compiler/can/src/desugar.rs Outdated Show resolved Hide resolved

kdziamura force-pushed the suffixed-stmt-desugaring branch from 76a16b5 to cb172c5 Compare July 5, 2024 07:06

kdziamura force-pushed the suffixed-stmt-desugaring branch from cb172c5 to 3bcaeae Compare July 5, 2024 07:11

kdziamura marked this pull request as draft July 5, 2024 15:16

auto-merge was automatically disabled July 5, 2024 15:16
Pull request was converted to draft

lukewilliamboswell reviewed Jul 6, 2024

View reviewed changes

crates/compiler/can/tests/test_suffixed.rs Outdated Show resolved Hide resolved

lukewilliamboswell reviewed Jul 6, 2024

View reviewed changes

crates/compiler/can/tests/test_suffixed.rs Outdated Show resolved Hide resolved

lukewilliamboswell reviewed Jul 6, 2024

View reviewed changes

crates/compiler/can/tests/test_suffixed.rs Outdated Show resolved Hide resolved

lukewilliamboswell reviewed Jul 6, 2024

View reviewed changes

crates/compiler/can/tests/test_suffixed.rs Outdated Show resolved Hide resolved

kdziamura force-pushed the suffixed-stmt-desugaring branch 3 times, most recently from d0155ea to a76f987 Compare July 8, 2024 19:42

kdziamura changed the title ~~Fix suffixed stmt desugaring~~ Respect type annotations in suffixed statements Jul 8, 2024

kdziamura changed the title ~~Respect type annotations in suffixed statements~~ Respect type annotations for suffixed statements Jul 8, 2024

kdziamura requested a review from lukewilliamboswell July 8, 2024 19:49

kdziamura changed the title ~~Respect type annotations for suffixed statements~~ wip: Respect type annotations for suffixed statements Jul 8, 2024

kdziamura force-pushed the suffixed-stmt-desugaring branch from a76f987 to f9fad17 Compare July 9, 2024 17:08

kdziamura changed the title ~~wip: Respect type annotations for suffixed statements~~ Respect type annotations for suffixed statements Jul 9, 2024

kdziamura marked this pull request as ready for review July 9, 2024 17:08

kdziamura commented Jul 9, 2024

View reviewed changes

crates/compiler/can/tests/test_suffixed.rs Show resolved Hide resolved

kdziamura force-pushed the suffixed-stmt-desugaring branch from f9fad17 to 8baec48 Compare July 12, 2024 15:05

kdziamura marked this pull request as ready for review July 16, 2024 09:51

kdziamura changed the title ~~Respect type annotations for suffixed statements~~ Type annotations for suffixed sugar Jul 16, 2024

kdziamura force-pushed the suffixed-stmt-desugaring branch from a03cdb1 to a06574e Compare July 17, 2024 11:37

lukewilliamboswell reviewed Jul 19, 2024

View reviewed changes

kdziamura force-pushed the suffixed-stmt-desugaring branch 2 times, most recently from 80c99c1 to 3b50a83 Compare July 22, 2024 18:04

kdziamura enabled auto-merge July 22, 2024 19:11

kdziamura requested a review from lukewilliamboswell July 22, 2024 20:08

kdziamura added 12 commits July 23, 2024 23:16

propagate type annotations to desugared ast

56eae5d

print formatted desugared code instead of ast

3e17168

unwrap identity call

a7ca02d

simplify identity unwrap

4d5586b

fix types propagation

9a37aeb

better desugared idents

10bdabf

update type annotation errors

7a8a5bc

update suffixed tests snapshots

1f65d95

remove obsolete test

863fb3b

remove redundant dependency

692f8e8

add todo tests

0086a53

code cleanup

7aa31a1

kdziamura force-pushed the suffixed-stmt-desugaring branch from 3b50a83 to 7aa31a1 Compare July 23, 2024 21:16

joshuawarner32 reviewed Jul 24, 2024

View reviewed changes

joshuawarner32 approved these changes Jul 24, 2024

View reviewed changes

kdziamura merged commit d5db313 into roc-lang:main Jul 24, 2024
17 checks passed

Anton-4 added a commit to roc-lang/basic-cli that referenced this pull request Jul 24, 2024

import Task.Task

2a24301

Probably due to roc-lang/roc#6868

kdziamura deleted the suffixed-stmt-desugaring branch July 29, 2024 11:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Type annotations for suffixed sugar #6868

Type annotations for suffixed sugar #6868

kdziamura commented Jul 4, 2024 •

edited

Loading

lukewilliamboswell commented Jul 5, 2024 •

edited

Loading

kdziamura commented Jul 5, 2024 •

edited

Loading

kdziamura commented Jul 5, 2024 •

edited

Loading

lukewilliamboswell Jul 19, 2024

kdziamura Jul 21, 2024 •

edited

Loading

lukewilliamboswell Jul 19, 2024

kdziamura Jul 21, 2024

kdziamura Jul 22, 2024

lukewilliamboswell Jul 19, 2024

kdziamura Jul 21, 2024

lukewilliamboswell Jul 19, 2024

kdziamura Jul 21, 2024

kdziamura Jul 22, 2024

lukewilliamboswell left a comment

joshuawarner32 Jul 24, 2024

kdziamura Jul 24, 2024

joshuawarner32 Jul 24, 2024

Type annotations for suffixed sugar #6868

Type annotations for suffixed sugar #6868

Conversation

kdziamura commented Jul 4, 2024 • edited Loading

lukewilliamboswell commented Jul 5, 2024 • edited Loading

kdziamura commented Jul 5, 2024 • edited Loading

kdziamura commented Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

kdziamura Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukewilliamboswell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdziamura commented Jul 4, 2024 •

edited

Loading

lukewilliamboswell commented Jul 5, 2024 •

edited

Loading

kdziamura commented Jul 5, 2024 •

edited

Loading

kdziamura commented Jul 5, 2024 •

edited

Loading

kdziamura Jul 21, 2024 •

edited

Loading