Anthony Bullard (Jan 04 2025 at 22:49): Anthony Bullard (Jan 04 2025 at 22:49):Is there any reason someone can think of why this line inside of the expr_to_pattern_help function in parse/src/expr.rs could not or should not be implemented?
Expr::SpaceBefore(..) | Expr::SpaceAfter(..) | Expr::ParensAround(..) => unreachable!(),
Seems easy enough to translate into a pattern, but I want to make sure there isn't an invariant that I'm missing
Anthony Bullard (Jan 04 2025 at 22:49):I ask because this is often hit in the fuzzer and it causes a panic
Anthony Bullard (Jan 05 2025 at 00:16):I see why now.
Anthony Bullard (Jan 05 2025 at 00:17):For now, I'm going to wrap the fuzz_module function in a std::panic::catch_unwind so that it doesn't blow up on parse errors
Anthony Bullard (Jan 05 2025 at 01:14):Ok, that doesn't seem to be helping. But it seems like, as Joshua warned me, the problem is trying to extract spaces when there is a SpacesBefore inside a SpacesBefore or a SpacesAfter inside a SpacesAfter....
Anthony Bullard (Jan 05 2025 at 01:16):It seems to me that to do that would require the ExtractSpaces trait's extract_spaces method to also take an arena as an arg.
Anthony Bullard (Jan 05 2025 at 01:16):So that they can be merged
Anthony Bullard (Jan 05 2025 at 16:26):Alright, I have this compiling. This is either my best idea, or the worst idea. Got rid of several todo!()s though
Anthony Bullard (Jan 05 2025 at 16:26):Running the fuzzer
Anthony Bullard (Jan 05 2025 at 16:43):Does this make sense to anyone:
* * * Source code before formatting:
dbg(2ii/fi&fh
)
i
* * * Source code after formatting:
dbg 2ii / fi &fh
i
* * * AST before formatting:
Expr(
@0-15 SpaceAfter(
DbgStmt {
first: @4-13 SpaceAfter(
BinOps(
[
(
@4-7 Num(
"2ii",
),
@7-8 Slash,
),
],
@8-13 Apply(
@8-10 Var {
module_name: "",
ident: "fi",
},
[
@10-13 RecordUpdater(
"fh",
),
],
Space,
),
),
[
Newline,
],
),
extra_args: [],
continuation: @17-18 SpaceBefore(
Var {
module_name: "",
ident: "i",
},
[
Newline,
Newline,
],
),
},
[
Newline,
],
),
)
* * * AST after formatting:
Expr(
@0-19 Defs(
Defs {
tags: [
EitherIndex(2147483648),
],
regions: [
@0-16,
],
space_before: [
Slice<roc_parse::ast::CommentOrNewline> { start: 0, length: 0 },
],
space_after: [
Slice<roc_parse::ast::CommentOrNewline> { start: 0, length: 0 },
],
spaces: [],
type_defs: [],
value_defs: [
Stmt(
@0-16 BinOps(
[
(
@0-7 Apply(
@0-3 Dbg,
[
@4-7 Num(
"2ii",
),
],
Space,
),
@8-9 Slash,
),
],
@10-16 Apply(
@10-12 Var {
module_name: "",
ident: "fi",
},
[
@13-16 RecordUpdater(
"fh",
),
],
Space,
),
),
),
],
},
@18-19 SpaceBefore(
Var {
module_name: "",
ident: "i",
},
[
Newline,
Newline,
],
),
),
)
I don't think I understand what a dbg statement continuation is
Ayaz Hafiz (Jan 05 2025 at 16:45):it's the rest of the program after the dbg. for example
dbg(x)
1 + 2
1 + 2 is the continuation
Anthony Bullard (Jan 05 2025 at 16:46):Sure....why is that not just a separate expression?
Ayaz Hafiz (Jan 05 2025 at 16:47):is the AST not structured as nested expressions
Ayaz Hafiz (Jan 05 2025 at 16:47):it used to be, at least
Anthony Bullard (Jan 05 2025 at 16:48):It is, but I don't understand why a debug would contain a continuation....
Anthony Bullard (Jan 05 2025 at 16:48):I would think the grammar for a Dbg expression would be "dbg" <Expr>
Anthony Bullard (Jan 05 2025 at 16:50):I get it
Ayaz Hafiz (Jan 05 2025 at 16:50):because you need something to indicate what the rest of the expression after the debug is. So it's like dbg <Expr> <Expr>. otherwise you would need a list as you mention but a nice property of nesting the next expression is that you can get the type of the whole expression just by looking at one node.
Anthony Bullard (Jan 05 2025 at 16:51):We don't have a separate Statement type. So something that is a statement needs to have a continuation that has the rest of the function
Anthony Bullard (Jan 05 2025 at 16:52):The problem here is we didn't print the parens around the binop when we formatted
Anthony Bullard (Jan 05 2025 at 16:53):So now it thinks we are applying dbg to the number 2ii and then that is the left-hand side of the binop
Anthony Bullard (Jan 05 2025 at 16:53):And that's because the original parse didn't capture the ParensAround
Anthony Bullard (Jan 05 2025 at 16:54):Let me know if that sounds correct to you @Ayaz Hafiz and thanks for talking sense into me
Anthony Bullard (Jan 05 2025 at 17:23):FIgured it out
Anthony Bullard (Jan 05 2025 at 17:23):One last issue and this should fix the last major fuzzer issue with PNC
Joshua Warner (Jan 05 2025 at 19:18):I've made a few fixes for PNC issues locally (didn't notice this thread until after I them!)
Joshua Warner (Jan 05 2025 at 19:19):Pushed a few of those here: https://github.com/roc-lang/roc/pull/7468
Joshua Warner (Jan 05 2025 at 19:23):Anyway, one thing that I think can't be preserved in the current AST is comments in an empty PNC apply, e.g.:
foo(
#comment
)
I think this will require using a different variant for PncApply in the AST, something like:
PncApply(&'a Expr<'a>, Collection<'a, &'a Expr<'a>>),
Well the args use the collection helper so wouldn’t this apply to empty collections too?
Anthony Bullard (Jan 05 2025 at 19:25):Oh use the collection type instead of a slice?
Anthony Bullard (Jan 05 2025 at 19:25):That would be helpful
Joshua Warner (Jan 05 2025 at 19:25):Yeah, collections explicitly keep trailing comments
Anthony Bullard (Jan 05 2025 at 19:25):Couldn’t we just move apply to use collection
Anthony Bullard (Jan 05 2025 at 19:25):Since WS apply is going away?
Joshua Warner (Jan 05 2025 at 19:25):That adds a bunch of complexity to the ws apply case
Anthony Bullard (Jan 05 2025 at 19:26):That’s the exact issue I was trying to fix later
Joshua Warner (Jan 05 2025 at 19:26):I would rather just have a new Expr variant for Pnc, and eventually delete the old WS one
Anthony Bullard (Jan 05 2025 at 19:26):Ok. That’s a bigger refactor than I’m willing to do for this PR
Anthony Bullard (Jan 05 2025 at 19:26):Especially after my extract spaces refactor
Joshua Warner (Jan 05 2025 at 19:27):Which PR are you talking about?
Anthony Bullard (Jan 05 2025 at 19:27):The one I’m working on
Joshua Warner (Jan 05 2025 at 19:27):Ah got it, not submitted yet
Anthony Bullard (Jan 05 2025 at 19:28):Not yet. I’m at the museum with my kids, but I’ll be able to push up what I got in a couple of hours
Anthony Bullard (Jan 05 2025 at 19:31):You’ll either love me or hate me for it
Joshua Warner (Jan 05 2025 at 19:32):Nah no hating here :)
Anthony Bullard (Jan 05 2025 at 19:33):Read the above for context. Just made extract spaces take an arena so I could merge spaces recursively from SpacesBefore/SpacesAfter nodes
Anthony Bullard (Jan 05 2025 at 19:35):And get rid of six todos in source. Probably 16 after macro expansion
Joshua Warner (Jan 05 2025 at 19:35):Great!
Joshua Warner (Jan 05 2025 at 19:36):I think that probably resolves one of the panics I was looking at
Joshua Warner (Jan 05 2025 at 19:36):I'm going to pull back the PNC-related fuzzing fixes I had in the PR I linked, to avoid conflicting with you
Anthony Bullard (Jan 05 2025 at 19:38):Thank you!
Anthony Bullard (Jan 05 2025 at 23:18):Joshua Warner said:
Anyway, one thing that I think can't be preserved in the current AST is comments in an empty PNC apply, e.g.:
foo( #comment )I think this will require using a different variant for PncApply in the AST, something like:
PncApply(&'a Expr<'a>, Collection<'a, &'a Expr<'a>>),
I'm going to start on this tonight if you haven't
Joshua Warner (Jan 06 2025 at 00:35):Go for it!
Anthony Bullard (Jan 07 2025 at 20:48):I am in a horrible back and forth where I have all of the tests passing, and then exceptions gone, but the fuzzer still fails relatively quickly
Anthony Bullard (Jan 07 2025 at 20:48):The latest thing is dbg followed immediately by parens
Sam Mohr (Jan 07 2025 at 20:50):Suggestion: the fuzzer is really useful for making sure we have coherent coverage for our syntax when it's stable, but is blocking us from progressing when we're making tons of syntax changes
Sam Mohr (Jan 07 2025 at 20:50):Can we disable it until after PNC + || args and all are merged?
Anthony Bullard (Jan 07 2025 at 20:50):That's true
Sam Mohr (Jan 07 2025 at 20:51):Not remove it, just comment it out in the CI
Anthony Bullard (Jan 07 2025 at 20:51):Here's an example that fails:
(L5(L5
0)
(
5)
(L5
0)
e
0)
dbg(L22
0)
Because we now have two separate nodes for Whitespace Apply and PNC apply
Brendan Hansknecht (Jan 07 2025 at 20:55):Up to you guys, but often I find it a mistake to disable fuzzers temporarily. Turning them back on is a huge pain and often never happens.
Brendan Hansknecht (Jan 07 2025 at 20:55):The fuzzer is catching real bugs in the new syntax. Even if they are ridiculous bugs.
Brendan Hansknecht (Jan 07 2025 at 20:57):As long as there is a definite plan of when work will be put in to re-enable it, I think it could be reasonable to disable temporarily. Especially if after removing old syntax it will be easier to fix.
Sam Mohr (Jan 07 2025 at 20:58):I just don't want Anthony Bullard to put his computer in the garbage disposal one piece at a time
Sam Mohr (Jan 07 2025 at 20:58):I'm avoiding touching parsing right now because it's no fun at the moment
Anthony Bullard (Jan 07 2025 at 21:02):Here's another beaut:
(L5(L5
0)
(
0)
(L5
0)
e
0)
dbg(L(
L5
0)
+
0asebg,L(
5is
if1)
e
0)
(21
0)
@Joshua Warner
Anthony Bullard (Jan 07 2025 at 21:03):@Anton already disabled it I thought?
Luke Boswell (Jan 07 2025 at 21:03):I thought Josh was standing by with some fuzzer fixes until Anthony lands his change
Luke Boswell (Jan 07 2025 at 21:03):I think we just ignore the fuzzer for now... if @Joshua Warner is ok with that. We can follow up with fixes in a separate PR
Luke Boswell (Jan 07 2025 at 21:04):And by we I am thinking Josh... he seems to fix these bugs faster than I can find them (and I'm just running an automated tool)
Sam Mohr (Jan 07 2025 at 21:05):As Brendan said, if we have a concrete plan to disable the fuzzer and how we can improve the parsing impl such that re-enabling the fuzzer puts us in a better spot, I think disabling it is good
Joshua Warner (Jan 07 2025 at 21:05):If we have folks actively working on this (and we do!), I think temporarily disabling in CI is fine
Sam Mohr (Jan 07 2025 at 21:05):But yes, if we just disable in indefinitely, that's a recipe for dead code
Anthony Bullard (Jan 07 2025 at 21:05):(L5(L5 0)(0)(L5 0)e 0)
dbg (L(L5 0) + 0asebg, L(5is ifl) e 0)
(21 0)
We should minimize these examples
Anthony Bullard (Jan 07 2025 at 21:06):I do
Joshua Warner (Jan 07 2025 at 21:06):Hmm they don’t look very minimal?
Joshua Warner (Jan 07 2025 at 21:06):What tool are you using to minimize?
Luke Boswell (Jan 07 2025 at 21:07):I just ran cargo run --bin minimize expr on the above and concur with Anthony... this is "minimal" according to our tool
Joshua Warner (Jan 07 2025 at 21:08):Interesting
Anthony Bullard (Jan 07 2025 at 21:08):Here's this minimization
dbg()
:rofl::rofl::rofl::rofl::rofl::rofl::rofl::rofl::rofl::rofl::rofl::rofl::rofl::rofl:
Joshua Warner (Jan 07 2025 at 21:08):Oh yeah that looks more minimal!
Joshua Warner (Jan 07 2025 at 21:08):I have a fix for that one locally
Anthony Bullard (Jan 07 2025 at 21:08):Care to share?
Anthony Bullard (Jan 07 2025 at 21:09):So i can get past this?
Anthony Bullard (Jan 07 2025 at 21:09):I was thinking of returning a Malformed if the args were empty
Joshua Warner (Jan 07 2025 at 21:12):I think this commit has the fix I made for that: https://github.com/roc-lang/roc/commit/8f48ced87974ff4b6b4a9fd703b437ac27ef8943
Anthony Bullard (Jan 07 2025 at 21:13):Ok, this isn't really compatible with the changes I've made
Joshua Warner (Jan 07 2025 at 21:13):FWIW I eventually want to get rid of DbgStmt as a separate node (i.e. so this is just an apply in the syntax tree), so I would rather not go down the road of making a special Malformed node for this
Joshua Warner (Jan 07 2025 at 21:14):Interesting; what incompatibilities are there?
Anthony Bullard (Jan 07 2025 at 21:14):I got rid of the ParensAndCommas CalledVia variant
Anthony Bullard (Jan 07 2025 at 21:16):I really dislike DbgStmt
Anthony Bullard (Jan 07 2025 at 21:16):Ayaz seemed to disagree with me about it
Joshua Warner (Jan 07 2025 at 21:17):Yeah it's caused a bunch of trouble with making the formatter consistent
Anthony Bullard (Jan 07 2025 at 21:17):I just wish it was just collapsed into the Dbg Expr node
Anthony Bullard (Jan 07 2025 at 21:18):And have that just be a tuple variant that takes the Expr to be inspected
Joshua Warner (Jan 07 2025 at 21:19):Anyway, I agree this commit will have conflicts with your PncApply refactor, but I think it should be possible to adapt
Anthony Bullard (Jan 07 2025 at 21:19):So should I just push it up with all the tests passing and go from there
Anthony Bullard (Jan 07 2025 at 21:20):This has been the most harrrowing PR I've done on this project
Joshua Warner (Jan 07 2025 at 21:20):I don't quite follow?
Anthony Bullard (Jan 07 2025 at 21:20):Should I push my current PR?
Anthony Bullard (Jan 07 2025 at 21:20):Get it merged, and then get yours in after revving
Joshua Warner (Jan 07 2025 at 21:20):Oh yeah sharing early+often sounds valuable
Joshua Warner (Jan 07 2025 at 21:20):Don't have to fix everything all at once
Anthony Bullard (Jan 07 2025 at 21:21):Yeah, I was just trying to see if this approach was going to solve any problems
Anthony Bullard (Jan 07 2025 at 21:21):And it did fix the "final comments" issue with PNC
Joshua Warner (Jan 07 2025 at 21:22):I think that's the primary and possibly only thing this fixes
Anthony Bullard (Jan 07 2025 at 21:22):One thing that will make you happy is that I did split a fmt_pnc_apply from fmt_apply
Anthony Bullard (Jan 07 2025 at 21:23):It was so much simpler since I could use the format_expr_collection
Sam Mohr (Jan 07 2025 at 21:23):Oh thanks for that
Joshua Warner (Jan 07 2025 at 21:23):Nice!
Anthony Bullard (Jan 07 2025 at 21:23):Yeah fmt_apply goes back to the way it was before PNC
Sam Mohr (Jan 07 2025 at 21:23):I was running into issues formatting abilities that such a change would have solved
Anthony Bullard (Jan 07 2025 at 21:23):Sorry for all the dust
Joshua Warner (Jan 07 2025 at 21:23):NP!
Joshua Warner (Jan 07 2025 at 21:23):Thanks for helping out here!
Sam Mohr (Jan 07 2025 at 21:23):I don't give a hoot, you're doing the hard work
Joshua Warner (Jan 07 2025 at 21:24):Glad to see PNC live!
Anthony Bullard (Jan 07 2025 at 21:24):Collection and all the utilities around it are very nice
Anthony Bullard (Jan 07 2025 at 21:25):Threading the last arg after and the final_comments together consistently made me so mad - solved problem with collection
Anthony Bullard (Jan 07 2025 at 23:23):@Joshua Warner https://github.com/roc-lang/roc/pull/7480
Joshua Warner (Jan 08 2025 at 03:07):Nice; left a few review comments. Most (maybe all?) are things that can be fixed up in follow-up PRs.
Joshua Warner (Jan 08 2025 at 03:08):I'm not too worried about this stuff going stale / not being actioned, so I'm generally fine either way.
Joshua Warner (Jan 08 2025 at 03:09):There are a bunch of different changes we have out-standing right now and keeping the momentum feels important - so maybe bias towards action now?
Joshua Warner (Jan 08 2025 at 03:10):You will have to fix those conflicts tho :/
Joshua Warner (Jan 08 2025 at 03:10):(LMK if I can help with that!)
Anton (Jan 08 2025 at 09:49):Anthony Bullard said:
Anton already disabled it I thought?
Yeah, to give some context; it was failing the ubuntu_x86_64.yml workflow very often, but you can never be sure that nothing else failed so you need to check every time. Also, the ubuntu_x86_64.yml is a required workflow so basically all PRs needed to be force merged. I think the fuzzer should be moved to a new standalone workflow that is not required to pass. So it can inform us but does not require a force merge by an admin.
Luke Boswell (Jan 08 2025 at 09:52):I saw that and thought it was a good idea.. but wasn't sure how to implement it.
Sam Mohr (Jan 08 2025 at 09:53):https://github.com/actions/runner/issues/2347
Sam Mohr (Jan 08 2025 at 09:54):There's an issue to allow CI actions to fail and not block merging, but GH doesn't want to implement it, apparently
Sam Mohr (Jan 08 2025 at 09:54):So we can probably just do run_fuzzer || true in the shell command in the GH action definition
Anton (Jan 08 2025 at 09:54):I'll take care of it, there's a way around it, you still get the red x but it's fine to merge
Sam Mohr (Jan 08 2025 at 09:54):Awesome!
Sam Mohr (Jan 08 2025 at 09:54):That's perfect
Anton (Jan 08 2025 at 15:19):Anton said:
I'll take care of it, there's a way around it, you still get the red x but it's fine to merge
Last updated: Jul 06 2025 at 12:14 UTC