roc-lang / roc

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

Support passing values into `dbg` with the pipe operator #7058

Closed mulias closed 2 months ago

mulias commented 2 months ago

This PR (hopefully) wraps up the work started in #7038 and continued in #7054, fully implementing https://github.com/roc-lang/roc/issues/5894, dbg as an expression.

In the feedback I got for #7038 there was general agreement that the functions in desugar.rs could use some refactoring to consolidate params into a shared struct. My initial intuition was that this should be a new struct, independent of the existing can::Env struct. Once I started implementing the desugaring for |> dbg I realized I would have to pass desugar specific args through the rest of canonicalization, which means the two parts essentially need the same set of args and it makes more sense to share one struct.

Use a shared env for desugaring and the rest of canonicalization

This refactor simplifies the desugar pass by reducing the number of arguments threaded through each recursive function call.

Support passing values into dbg with the pipe operator

In order to desugar dbg in a pipeline we need to allow a bare dbg node in desugaring and only report it as an error if the bare node survives to the next step of canonicalization. This means we move the error code out of desugar_expr and into canonicalize_expr. This is much simpler to do now that these functions use the same env struct, since previously we would have had to pass down extra args to canonicalize_expr. Sharing the env struct means that we also don't have to worry about calculating line_info more than once.

smores56 commented 2 months ago

This is ready to merge, just waiting for whether @mulias agrees with my PR comments or not. Once that's resolved, we're good to go

mulias commented 2 months ago

@smores56 I added two commits to address your feedback. I think the doc comment is more helpful where it is now, but let me know what you think.

smores56 commented 2 months ago

Uh oh, it looks like you didn't propagate the function arg change...

error[E0061]: this function takes 3 arguments but 4 arguments were supplied
    --> crates/compiler/can/src/desugar.rs:1310:17
     |
1310 |         value: *desugar_dbg_stmt(env, scope, tmp_var, tmp_var),
     |                 ^^^^^^^^^^^^^^^^    -------
     |                                     | |
     |                                     | unexpected argument of type `&mut scope::Scope`
     |                                     help: remove the extra argument
     |
note: function defined here
    --> crates/compiler/can/src/desugar.rs:1333:4
     |
1333 | fn desugar_dbg_stmt<'a>(
     |    ^^^^^^^^^^^^^^^^
1334 |     env: &mut Env<'a>,
     |     -----------------
1335 |     condition: &'a Loc<Expr<'a>>,
     |     ----------------------------
1336 |     continuation: &'a Loc<Expr<'a>>,
     |     -------------------------------
mulias commented 2 months ago

Never pays to rush a change!