-
-
Notifications
You must be signed in to change notification settings - Fork 309
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
Support passing values into dbg
with the pipe operator
#7058
Conversation
@@ -1,14 1,14 @@ | |||
--- | |||
source: crates/compiler/can/tests/test_suffixed.rs | |||
assertion_line: 459 | |||
assertion_line: 473 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was accidentally committed early as part of a previous PR. I've updated the source code for the test, so the contents of the snapshot have updated, even though it looks like it should be a new test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this out with basic-cli v0.15, all of my dbg
usages were working! My past self would have really appreciated this last year, you've really improved the quality of life of Roc debugging here.
crates/compiler/can/src/desugar.rs
Outdated
} | ||
|
||
/// Desugars a `dbg x` statement into essentially `Inspect.toStr x |> LowLevelDbg` | ||
fn desugar_dbg_stmt<'a>( | ||
arena: &'a Bump, | ||
env: &mut Env<'a>, | ||
_scope: &mut Scope, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: just a callout to remember to remove this, I presume that's why it's underscored here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
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 |
7da9022
to
55ad319
Compare
@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. |
55ad319
to
02219f7
Compare
Uh oh, it looks like you didn't propagate the function arg change...
|
Never pays to rush a change! |
This refactor simplifies the desugar pass by reducing the number of arguments threaded through each recursive function call. - Add the module src string to `Env`. - Add `line_info` to `Env` as a lazy-evaled function. - Refactor desugar functions to take the `can::Env` struct in place of a number of params. This is mostly a find-and-replace, but in a few places `Vec::from_iter_in` was changed to `Vec::with_capacity_in` followed by a `for` loop in order to avoid lifetime issues. - Remove unnecessary linter annotations for `clippy::too_many_arguments`
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.
02219f7
to
85aad0d
Compare
This PR (hopefully) wraps up the work started in #7038 and continued in #7054, fully implementing #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 existingcan::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.
Env
.line_info
toEnv
as a lazy-evaled getter function.can::Env
struct in place of a number of params. This is mostly a find-and-replace, but in a few placesVec::from_iter_in
was changed toVec::with_capacity_in
followed by afor
loop in order to avoid lifetime issues.clippy::too_many_arguments
Support passing values into dbg with the pipe operator
In order to desugar
dbg
in a pipeline we need to allow a baredbg
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 ofdesugar_expr
and intocanonicalize_expr
. This is much simpler to do now that these functions use the sameenv
struct, since previously we would have had to pass down extra args tocanonicalize_expr
. Sharing theenv
struct means that we also don't have to worry about calculatingline_info
more than once.