Kiryl Dziamura (May 28 2023 at 12:31): Kiryl Dziamura (May 28 2023 at 12:31):I think I need some help with Pipes ignore parenthesis wrapping an application
I could blindly fix the exact problem outlined in the issue but the fix is not correct (I'll explain why below) and the biggest problem I'm struggling with is I don't know how to debug the compiler.
All the time I encounter the problem, probably because the changes I make lead to infinite loops but I don't know how to check it. I also added println to the can module to log intermediate values but it logs std which is very distracting. Ah, and I use repl to quickly test/experiment with my assumptions.
Now regarding the issue itself. In the case of pizza, both desugaring for lefts and right matter. Both of the following examples should be invalid (because parens produce partial application which is not allowed):
2 |> (Num.sub 3) # removing desugar from the `right` helps in this case...
2 |> (Num.sub 3) |> Num.sub 3 # but in this case, desugar is applied to the `left`, but removing it breaks many other places
The idea is to match loc_op and desugar based on it, but it feels incorrect because it makes the pizza a special case.
Richard Feldman (May 28 2023 at 13:39):hm, this is interesting!
Richard Feldman (May 28 2023 at 13:39):an idea: what if we just didn't desugar parens? :thinking:
Richard Feldman (May 28 2023 at 13:41):in other words, leave them in when we're doing the desugar pass in operator.rs and then later, in can::expr, remove them at the last minute (using the same strategy we currently do during desugaring)
Richard Feldman (May 28 2023 at 13:42):that way, when we're desugaring the pizza operator, we would see the parens and not treat it as a function call because https://github.com/roc-lang/roc/blob/ec21f1982611915bfbfffb4b89eece4cdc043ebd/crates/compiler/can/src/operator.rs#L32 wouldn't pass
Richard Feldman (May 28 2023 at 13:42):so it would take the _ => branch there and (I think) do what we want?
Ayaz Hafiz (May 28 2023 at 14:04):With regards to debugging the compiler - when building/testing, you can use the environment variable ROC_SKIP_SUBS_CACHE=1, which will avoid building the std during compile time and hopefully give you better messages at runtime
Ayaz Hafiz (May 28 2023 at 14:06):Actually I wonder, should 2 |> (Num.sub 3) be disallowed? Because there is only one way to interpret this, which is Num.sub 2 3 (so it always breaks in to a call, you cannot claim a partial application)
Ayaz Hafiz (May 28 2023 at 14:07):Another alternative to Richard's suggestion is to disallow parens around top-level expressions on the RHS and before LHS of a pipe. So 2 |> (Num.sub 3) is disallowed but e.g. 2 |> (if Bool.true then Num.add else Num.sub) 3 may be admitted.
Richard Feldman (May 28 2023 at 14:09):I wonder, should
2 |> (Num.sub 3)be disallowed? Because there is only one way to interpret this, which isNum.sub 2 3(so it always breaks in to a call, you cannot claim a partial application)
I think it should be allowed, but should actually apply the function - e.g. I think this should be allowed, and should work:
curriedSub = \num1 -> \num2 -> Num.sub num1 num2
expect 2 |> Num.sub 3 == 2 |> (curriedSub 3)
Num.sub specifically isn't a curried function, so it shouldn't be allowed for that function, right?
Brendan Hansknecht (May 28 2023 at 14:17):As in, the error should be that Num.sub didn't get enough arcs if you do |> (Num.sub 2)
Richard Feldman (May 28 2023 at 14:17):I'm saying I think the code I wrote there should be valid and the expect should pass
Richard Feldman (May 28 2023 at 14:17):in other words, that x |> (foo y) should be allowed because foo could be curried
Richard Feldman (May 28 2023 at 14:18):and if foo is curried, then x |> foo y should be a type mismatch because that desugars to foo x y, which is the wrong number of arguments to give foo if it's curried (since it should only be given 1 argument!)
Ayaz Hafiz (May 28 2023 at 14:23):I see, I misunderstood - thanks!
Ayaz Hafiz (May 28 2023 at 14:25):What if we passed a precedence level while parsing expressions in a pipe and increased the level each time you go under a parenthesis? Then the top level |> Num.sub 3 has Apply(Num.sub, 3) at precedence-level 0, and |> (Num.sub 3) has Apply(Num.sub, 3) at precedence-level 1. During desugaring, only expressions at the same precedence-level can be chained, or else they must be nested Applys.
Richard Feldman (May 28 2023 at 14:40):hm, what would the difference be between that and the idea of deferring removing parens until after operator desugaring? :thinking:
Ayaz Hafiz (May 28 2023 at 15:07):I guess maybe not much?
Ayaz Hafiz (May 28 2023 at 15:08):maybe just separation of concerns so we don’t have to keep parents around as semantic information?
Richard Feldman (May 28 2023 at 15:17):oh they're still not kept around
Richard Feldman (May 28 2023 at 15:17):it's just a question of whether they get removed before or after desugaring
Richard Feldman (May 28 2023 at 15:17):(desugaring operators I mean)
Richard Feldman (May 28 2023 at 15:18):right now it's:
and I think if we change this to the following, it would (hopefully!) solve the problem:
Richard Feldman (May 28 2023 at 15:19):the critical distinction being that during operator desugaring the parens are still there, so |> sees a distinction between parenthesized and non-parenthesized expressions (whereas today it can't see a distinction bc they've already been removed by the in-progress desugaring)
Kiryl Dziamura (May 28 2023 at 15:30):Btw, the result of compiler/parse/src/expr.rs#parse_expr_final
for this expr
2 |> (Num.sub 3) |> Num.sub 3
BinOps([(@66-67 Num("2"), @68-70 Pizza), (@72-81 ParensAround(Apply(@72-79 Var { module_name: "Num", ident: "sub" }, [@80-81 Num("3")], Space)), @83-85 Pizza)], @86-95 Apply(@86-93 Var { module_name: "Num", ident: "sub" }, [@94-95 Num("3")], Space))
and
2 |> (Num.sub 3)
BinOps([(@66-67 Num("2"), @68-70 Pizza)], @72-81 ParensAround(Apply(@72-79 Var { module_name: "Num", ident: "sub" }, [@80-81 Num("3")], Space)))
so parser doesn't remove parens. or do you mean something else?
Richard Feldman (May 28 2023 at 18:05):ah, yeah! So in operator.rs (the "operator desugaring step" - which takes a parse AST as its input and then ouputs a desugared parse AST), there are two places where we explicitly drop parens from the parse AST:
so my first suggestion is that in these cases, we should change these (just the ParensAround cases - so they would need to be moved out into their own branches in the match, because SpaceBefore and SpaceAfter should continue to be handled the way they are today) and make it so that ParensAround desugars the sub-expression but then re-adds the ParensAround, so that we aren't actually dropping the ParensAround anymore.
Richard Feldman (May 28 2023 at 18:06):so if you do this, it will cause some things to break, because this desugaring step runs before we convert between parse AST and canonical IR
Richard Feldman (May 28 2023 at 18:06):and today, when we convert to canonical IR, we have a check that says "hey if we run into any parens, that must be a bug, because a previous step should have gotten rid of those" - https://github.com/roc-lang/roc/blob/7cabaecd6ec711371c24889445e9b3a00f7a528d/crates/compiler/can/src/expr.rs#L1399
Richard Feldman (May 28 2023 at 18:07):so we'd want to change that code to no longer throw an error, and instead silently discard the parens right there
Richard Feldman (May 28 2023 at 18:07):does that make sense? Happy to elaborate on any part of that!
Kiryl Dziamura (May 28 2023 at 18:42):ah, now I see. makes sense. let me try
Kiryl Dziamura (May 30 2023 at 14:24):A simple pr (but it solves the problem. somehow) https://github.com/roc-lang/roc/pull/5481
Brendan Hansknecht (Jun 20 2023 at 04:23):Just wondering @Kiryl Dziamura , what is the status of this? Will it be submitted soon?
Ran into it in something I am working on and would be quite convenient to take advantage of this change if it can be submitted soon.
Kiryl Dziamura (Jun 22 2023 at 21:14):Every day I think about back to the issue, but unfortunately, last three weeks I hardly find time even for my primary job :confounded:
Having small child, fulltime job and passion for opensource contribution is a very fragile construction.
I apologise for the delay and lack of status update. Will find some time next week, and put the status here if I have any difficulties. If it's a blocker - feel free to fix it, it doesn't look difficult at all. I would be happy to review and learn something new.
(omg, it’s the monthly meeting in two days. time flies)
Brendan Hansknecht (Jun 22 2023 at 21:54):No worries, i totally understand.
Richard Feldman (Jun 22 2023 at 22:58):no need to apologize! We appreciate your time, and no pressure :heart:
Kiryl Dziamura (Jun 22 2023 at 22:59):ok, found a way to unhang the load(build) step: just commented https://github.com/roc-lang/roc/blob/a9f7961b524b11f34038d96e262e5f68f3ecc86d/crates/compiler/load/build.rs#L17-L28
It helped a lot, so I could locate the problem I was struggling with almost immediately.
I updated the pr, but some tests failed with stack overflow:
https://github.com/roc-lang/roc/pull/5481#issuecomment-1603413829
Kiryl Dziamura (Jun 24 2023 at 21:03):any ideas on what I can do with the error?
thread 'glue_cli_run::basic_record' panicked at '`roc glue` command had unexpected stderr:
thread 'main' has overflowed its stack
fatal runtime error: stack overflow
not sure how to debug it
Brendan Hansknecht (Jun 24 2023 at 21:10):I would make sure to run it in release just to check if it is a correctness issue or an optimization issue
Kiryl Dziamura (Jun 24 2023 at 21:32):ha! in release glue code tests are passed
Kiryl Dziamura (Jun 24 2023 at 22:10):faced fatal in the following scenario:
» id = (\x -> x)
thread 'main' panicked at 'function symbol `#UserApp.id` not in set LambdaSet { set: [( Test.2, [])], args: [InLayout(VOID)], ret: InLayout(VOID), representation: InLayout(21), full_layout: InLayout(22) }', crates/compiler/mono/src/layout.rs:1472:9
nice! Check out the with_larger_debug_stack test helper - it's what we use in the situation where a test works with --release but overflows the stack without --release (basically it just increases the test's stack size in non---release builds)
Kiryl Dziamura (Jun 25 2023 at 06:14):I’ll do that, thanks. But there’s another problem with the parens around lambda now. I probably should fix that first.
Kiryl Dziamura (Jun 28 2023 at 19:41):An update:
since the parens aren't dropped, this code id = (\x -> x) fails because for closures there's a special case in canonicalize_pending_body (lambda sets are involved there), and a closure wrapped into parens is not recognized. I pushed an update, all tests are passed (at least in --release) but I'm not sure about the changes. looking for insights
speaking of the with_larger_debug_stack helper - it lives in test_gen crate but as I understand, I can't just add it to the roc_glue crate?
Kiryl Dziamura (Jun 30 2023 at 07:20):@Richard Feldman @Ayaz Hafiz any thoughts here?
Richard Feldman (Jul 01 2023 at 00:22):I left some comments on the PR - great catch! :smiley:
Last updated: Jul 06 2025 at 12:14 UTC