Joshua Warner (Nov 18 2024 at 00:07): Joshua Warner (Nov 18 2024 at 00:07):If there's a weird way for features to interact, fuzzing seems to reliably find it. So, brain teaser:
When parsed and formatted, dbg dbg g g yields: dbg (dbg g g)
Is that the same thing? Why or why not?
Luke Boswell (Nov 18 2024 at 00:12):Why does it add the parens?
Luke Boswell (Nov 18 2024 at 00:12):Wouldn't that parse as a function call with three arguments?
Luke Boswell (Nov 18 2024 at 00:13):I would expect that to fail typechecking or something later, but for the sake of parsing/formatting it should be no different to say foo 1 2 3 right?
Joshua Warner (Nov 18 2024 at 00:21):It _is_ different, because there are two different ways dbg is parsed :/
Joshua Warner (Nov 18 2024 at 00:21):The same thing doesn't happen with foo foo g g
Joshua Warner (Nov 18 2024 at 00:22):* * * AST before formatting:
Expr(
SpaceAfter(
Apply(
@0-11 Dbg,
[
@4-11 Apply(
@4-7 Dbg,
[
@8-9 Var {
module_name: "",
ident: "g",
},
@10-11 Var {
module_name: "",
ident: "g",
},
],
Space,
),
],
Space,
),
[
Newline,
],
),
)
* * * AST after formatting:
Expr(
Apply(
Dbg,
[
Apply(
Dbg,
[
Apply(
Var {
module_name: "",
ident: "g",
},
[
Var {
module_name: "",
ident: "g",
},
],
Space,
),
],
Space,
),
],
Space,
),
)
The key is that dbg in the outer context is parsing as a dbg statement, so it gobbles up the rest of the string - and then prior to formatting (haven't quite identified where...) this gets converted to a Dbg expression, which of course must follow the usual rules for how calls work - and so we introduce parens in the formatter to (try to) preserve that nesting order.
Joshua Warner (Nov 18 2024 at 00:24):... but unbeknownst to the formatter, adding the parens triggers that inner thing to be a new "defs" context, which allows the inner dbg to parse as a dbg statement instead of a dbg expression.
Luke Boswell (Nov 18 2024 at 00:26):I guess it needs to be parsed as a special keyword because we don't have statements in roc, like user space function call on a line by itself.
Luke Boswell (Nov 18 2024 at 00:28):I'm looking at
fn dbg_kw<'a>() -> impl Parser<'a, Expr<'a>, EExpect<'a>> {
(move |arena: &'a Bump, state: State<'a>, min_indent: u32| {
let (_, _, next_state) =
parser::keyword(keyword::DBG, EExpect::Dbg).parse(arena, state, min_indent)?;
Ok((MadeProgress, Expr::Dbg, next_state))
})
.trace("dbg_kw")
}
In the parser::keyword should we prevent two keywords in sequence, like if if or dbg dbg?
Richard Feldman (Nov 18 2024 at 01:17):Joshua Warner said:
If there's a weird way for features to interact, fuzzing seems to reliably find it. So, brain teaser:
When parsed and formatted,
dbg dbg g gyields:dbg (dbg g g)Is that the same thing? Why or why not?
that seems correct, but I don't like that it seems correct :laughing:
Richard Feldman (Nov 18 2024 at 01:18):in the sense that dbg doesn't accept multiple arguments, so whatever comes after it must be its own self-contained expression (hence the parens)
Richard Feldman (Nov 18 2024 at 01:18):but although it's logical, I don't like it :laughing:
Joshua Warner (Nov 18 2024 at 01:31):The core of the weirdness here is that we have two fundamental the different precedence levels for dbg statements: at the statement level, it has a very low precedence, but if the expression level it has the same precedence as other function applications.
Joshua Warner (Nov 18 2024 at 01:32):One way to fix that would be to standardize on dbg always having function level precedents - effectively, eliminating the statement level dbg.
Joshua Warner (Nov 18 2024 at 01:32):On the other hand, we could move even the expression level dbg to have a very low precedence, and operate the same way as the statement level dbg.
Joshua Warner (Nov 18 2024 at 01:34):Or, if we just want to fix this one particular thing, we can teach the formatter not to introduce parentheses in the case where it would matter and change how this inner statement is parsed. But that feels pretty hacky?
Joshua Warner (Nov 18 2024 at 03:03):Actually not sure that last option will be very feasible...
Richard Feldman (Nov 18 2024 at 03:10):personally I'm okay with the status quo
Richard Feldman (Nov 18 2024 at 03:10):on the grounds that I don't think anyone is likely to ever write it on purpose
Richard Feldman (Nov 18 2024 at 03:11):also in the parens-and-commas world you'd have to write it like dbg(dbg(g, g)) or dbg(dbg, g, g) anyway so there wouldn't be any strangeness
Joshua Warner (Nov 18 2024 at 03:12):I would like to fix this invariant violation somehow
Joshua Warner (Nov 18 2024 at 03:12):I do eventually want to get "fuzz-clean", which then means we have strong properties that you can trust of the parser and formatter
Joshua Warner (Nov 18 2024 at 03:13):And after that, move on to fuzzing other areas of the compiler ;)
Joshua Warner (Nov 18 2024 at 03:14):IMO the status quo is a bit hard to understand because there are two different rules for how you have to parenthesize things in dbg, depending on where it is
Joshua Warner (Nov 18 2024 at 03:17):Concretely, dbg f g is fine at the top level, and operates like dbg (f g) - but if it's inside an expression somewhere you have to do dbg (f g).
Joshua Warner (Nov 18 2024 at 03:17):This fuzzer-found bug is a very round about way of describing that weirdness
Richard Feldman (Nov 18 2024 at 03:39):a straightforward fix would be to have dbg "accept" multiple arguments, like a function call, at the parsing level
Richard Feldman (Nov 18 2024 at 03:39):and then give a canonicalization error for too many args passed to dbg
Joshua Warner (Nov 18 2024 at 03:40):Yep, and that's how dbg works at the "expression" level
Richard Feldman (Nov 18 2024 at 03:40):yeah so we could do the same at the statement level
Joshua Warner (Nov 18 2024 at 03:40):Aha yeah; that's what I was thinking as well :)
Joshua Warner (Nov 18 2024 at 03:41):A slightly further change would be to make us parse dbg identically regardless of the context
Joshua Warner (Nov 18 2024 at 03:42):Given the thing we just agreed upon above, the remaining difference is that dbg at the statement level can currently introduce a block - e.g.:
dbg
x = 1
x
That seems much less useful than it is in the expect context, so my initial instinct is to go ahead and remove that ability
Joshua Warner (Nov 18 2024 at 03:42):Note that you can always do:
dbg (
x = 1
x
)
... if you really need to
Richard Feldman (Nov 18 2024 at 03:50):why couldn't you do it in the expression? :thinking:
Joshua Warner (Nov 18 2024 at 04:02):(moved discussion to #compiler development > Defs in arbitrary expressions )
Luke Boswell (Nov 18 2024 at 04:25):Joshua Warner said:
I do eventually want to get "fuzz-clean", which then means we have strong properties that you can trust of the parser and formatter
It would be cool to have a "hey did you run the fuzzer" as a requirement whenever someone updates anything syntax related. I'm guessing at the moment they would immediately run into these kind of issues, and so it wouldn't be as helpful for them?
Brendan Hansknecht (Nov 18 2024 at 04:29):You could have CI run the fuzzer for 30 seconds or a minute on each pr. Or do it a bit longer each nightly.
Anton (Nov 18 2024 at 09:11):Nightly seems best, we don't want to confuse people with failures unrelated to their changes
Anton (Nov 18 2024 at 09:12):Do we have a script or single command for the fuzzing?
Luke Boswell (Nov 18 2024 at 10:11):$ cd crates/compiler/test_syntax/fuzz
$ cargo +nightly fuzz run -j4 fuzz_expr --sanitizer=none -- -dict=dict.txt
It's described in crates/compiler/test_syntax/fuzz/README.md
Anton (Nov 18 2024 at 10:20):Thanks :)
Brendan Hansknecht (Nov 18 2024 at 16:59):Assuming we iron out the current bugs, I think it is much nicer to do a bit per PR. Cause if it caches something it will almost certainly be a real bug from the PR.
Brendan Hansknecht (Nov 18 2024 at 16:59):Can always bisect instead, but it is more work
Brendan Hansknecht (Nov 18 2024 at 17:00):Also, if you can, make sure to save the corpus (and probably want to minimize it at the end of runs)
Joshua Warner (Nov 19 2024 at 04:23):One caveat that I didn't realize immediately with moving _everything_ to use the expr-level dbg is that things like dbg 1 + 1 will cease to work, and you'll have to write dbg (1 + 1).
Thoughts?
Anton (Nov 19 2024 at 09:09):seems reasonable, we'll see if people complain about it :big_smile:
Isaac Van Doren (Nov 19 2024 at 15:10):I was also surprised when I first used debug that you didn’t have add parens around expressions like that so I think this just decreases the amount of surprise :+1:
Brendan Hansknecht (Nov 19 2024 at 16:20):Given debug works as an expression, I would expect this to print 1:
x = dbg 1 + 1
This prints 2
x = dbg (1 + 1)
That said, as a statement, I would expect different precedence. Though same precedence is ok. I would expect 2 from this
dbg 1 + 1
Could we make those use a different syntax?
Richard Feldman (Nov 19 2024 at 17:30):I think parens-and-commas fixes this, right?
Brendan Hansknecht (Nov 19 2024 at 20:03):Yeah, I think so
Richard Feldman (Nov 19 2024 at 20:28):in that case I'd say we can go with whatever design most easily fixes the fuzzing case :big_smile:
Richard Feldman (Nov 19 2024 at 20:28):can always revisit later
Joshua Warner (Nov 22 2024 at 03:37):Here's the diff to do that, with some refactoring to make the required transformations in can easier: https://github.com/roc-lang/roc/pull/7239
Joshua Warner (Nov 22 2024 at 03:37):This relegates Expr::Defs to only be used between desugaring and the main canonicalization phase, and instead uses Expr::Stmts as the representation that comes out of the parser.
Joshua Warner (Nov 22 2024 at 03:39):Note that the list of stmts already existed as an intermediate representation in the parser - so this isn't introducing a new step, it's just moving it around.
Joshua Warner (Nov 22 2024 at 03:41):One of the ancillary benefits here is that some things that used to be a parse errors, and thus completely blocked running the code (e.g. missing a final expression in a defs), are now canonicalization errors where they can be non-fatal
Joshua Warner (Nov 29 2024 at 05:33):Ok, with that out of the way, I was able to fix a bunch of other bugs found in fuzzing here: https://github.com/roc-lang/roc/pull/7267
Not yet at the end of it, but I at least feel like it's still finding interesting things that'd be possible for users to hit, rather than trivial problems.
Was hoping to get fully to "fuzz clean" before submitting the next PR - but these fixes have been piling up for too long now.
Joshua Warner (Dec 03 2024 at 04:50):...
#1863400: cov: 47740 ft: 42904 corp: 11904 exec/s 1197 oom/timeout/crash: 0/0/0 time: 397s job: 52 dft_time: 0
#1936387: cov: 47842 ft: 42918 corp: 11911 exec/s 1351 oom/timeout/crash: 0/0/0 time: 412s job: 53 dft_time: 0
#1985095: cov: 47873 ft: 42933 corp: 11917 exec/s 885 oom/timeout/crash: 0/0/0 time: 427s job: 54 dft_time: 0
#2068034: cov: 47874 ft: 42945 corp: 11925 exec/s 1481 oom/timeout/crash: 0/0/0 time: 441s job: 55 dft_time: 0
#2105228: cov: 47882 ft: 42948 corp: 11928 exec/s 652 oom/timeout/crash: 0/0/0 time: 455s job: 56 dft_time: 0
#2184903: cov: 47914 ft: 42959 corp: 11937 exec/s 1373 oom/timeout/crash: 0/0/0 time: 471s job: 57 dft_time: 0
#2262528: cov: 47917 ft: 42960 corp: 11938 exec/s 1315 oom/timeout/crash: 0/0/0 time: 486s job: 58 dft_time: 0
#2335873: cov: 47986 ft: 42969 corp: 11944 exec/s 1222 oom/timeout/crash: 0/0/0 time: 501s job: 59 dft_time: 0
#2388073: cov: 48069 ft: 42995 corp: 11954 exec/s 855 oom/timeout/crash: 0/0/0 time: 517s job: 60 dft_time: 0
#2468634: cov: 48092 ft: 43009 corp: 11962 exec/s 1299 oom/timeout/crash: 0/0/0 time: 534s job: 61 dft_time: 0
#2548301: cov: 48139 ft: 43018 corp: 11970 exec/s 1264 oom/timeout/crash: 0/0/0 time: 550s job: 62 dft_time: 0
#2623369: cov: 48172 ft: 43021 corp: 11972 exec/s 1172 oom/timeout/crash: 0/0/0 time: 566s job: 63 dft_time: 0
#2701742: cov: 48202 ft: 43039 corp: 11983 exec/s 1205 oom/timeout/crash: 0/0/0 time: 583s job: 64 dft_time: 0
#2748245: cov: 48243 ft: 43046 corp: 11987 exec/s 704 oom/timeout/crash: 0/0/0 time: 601s job: 65 dft_time: 0
INFO: fuzzed for 601 seconds, wrapping up soon
INFO: exiting: 0 time: 602s
And there is the sweet sweet sound of the fuzzer not having found any problems in 10 minutes of fuzzing.
Joshua Warner (Dec 03 2024 at 04:50):Yay for arbitrary goals
Joshua Warner (Dec 03 2024 at 04:53):PR here: https://github.com/roc-lang/roc/pull/7301
Joshua Warner (Dec 03 2024 at 04:53):Note that one of the changes I'm including here is removing the ability for joining neighboring annotations+bodies if the patterns are not "equivalent", discussed here: https://roc.zulipchat.com/#narrow/channel/395097-compiler-development/topic/Merging.20non-equivalent.20annotations.20.2B.20bodies.3F/near/485732867
Joshua Warner (Dec 03 2024 at 04:53):That much is probably worth discussing prior to actually merging. I can fairly easily back that commit out and fix that fuzzing issue separately if need be.
Luke Boswell (Dec 03 2024 at 04:57):Well done! This is great to see. Feels good
Joshua Warner (Dec 03 2024 at 04:57):Indeed
Joshua Warner (Dec 03 2024 at 04:57):That was a long time coming
Joshua Warner (Dec 03 2024 at 05:04):If I could quickly TL;DR the #1 learning here, it's: spaces are "slippery", and if they have any room to slide around, chaos ensues.
For example, and eliding some of the fields of these things, you can have these two trees that are logically equivalent:
SpaceBefore(Apply(Dbg, [...]))
Apply(SpaceBefore(Dbg), [...])
i.e. both represent having placed the spaces at the same place in the file.
Or similarly, where each element in a series of Defs has both "spaces_before" and "spaces_after".
I'm not sure if the parser would ever produce that exact example, but there definitely are a bunch of other possible cases where spaces can "slide" around while still being in logically the same place.
That's what all the expr_lift_spaces, pattern_lift_spaces, etc stuff in these recent diffs have been: trying to normalize the placing of those spaces, "lifting" it up to the highest part of the tree it can be.
Joshua Warner (Dec 03 2024 at 05:08):This "sliding" is particularly prone to happening if the formatter decides to add or remove parentheses
Joshua Warner (Dec 03 2024 at 05:12):Another footgun in the formatter is that is_multiline() must correctly predict what .format_with_options(...) will do, and if we have some subtle logic that sometimes adds a newline to the output in the .format_with_options(...) impl, it's easy to forget to make .is_multiline() account for that. The rules to format a string as a block string if it contains a newline or quote char have been particularly annoying here.
Joshua Warner (Dec 03 2024 at 05:14):The solution to both of those that I'm very slowly working towards is to add an intermediate stage of processing that gives a heavily-normalized version of the expression from which is_multiline can be calculated trivially and 100% correctly.
Joshua Warner (Dec 03 2024 at 05:17):Today that's taking the form of expr_lift_spaces/etc - but there are hints of my eventual plans with ann_lift_to_node: that's the barest sketch of that normalized form. I added that in this case in order to make sure the decision of whether a type annotation needs parens consistent in a couple key spots.
Joshua Warner (Dec 07 2024 at 01:42):I put up an initial sketch of doing fuzzing in CI here: https://github.com/roc-lang/roc/pull/7316
Would love some help getting this properly set up since I really don't know what I'm doing, and this is me just flailing around.
In particular, I'm guessing that we'll have problems with making sure the nightly version of cargo is installed. There may also be problems with nix/
Luke Boswell (Dec 07 2024 at 01:57):Does the fuzzer work inside nix? I'd just add it as another step for one of our existing nix runs
Luke Boswell (Dec 07 2024 at 01:57):It doesn't need to run on all the different os/archs
Joshua Warner (Dec 07 2024 at 01:58):Yeah, no need for different architectures.
Luke Boswell (Dec 07 2024 at 01:58):Like in .github/workflows/nix_linux_x86_64.yml
Joshua Warner (Dec 07 2024 at 01:58):The only thing I'm not sure about is how to get the nightly compiler working in CI with nix. Fuzzing requires running nightly compiler, and doesn't work on stable.
Joshua Warner (Dec 07 2024 at 01:59):The rust compiler I mean.
Luke Boswell (Dec 07 2024 at 01:59):Just copy the commands already there I think
Luke Boswell (Dec 07 2024 at 01:59):nix develop -c ... maybe we add a bash.sh script that runs the fuzzer
Luke Boswell (Dec 07 2024 at 02:00):I'd give this a try
- name: run fuzz tests
run: |
cd crates/compiler/test_syntax/fuzz
nix develop -c cargo +nightly fuzz run -j4 fuzz_expr --sanitizer=none -- -dict=dict.txt -max_total_time=60
@Anton may not have the nightly toolchain on the self-hosted machine.. but that should be an easy fix
Joshua Warner (Dec 07 2024 at 02:07):One reason we might want this to be part of a separate job is so that we can configure it to be not blocking merging of PRs until we're confident that it's very stable.
Luke Boswell (Dec 07 2024 at 02:08):I'm just throwing ideas around for how to get it working... I think @Anton will definitely have some things to say on this. But the more we can do to set it up and have something working for him, the easier to get to the desired end state is my thinking.
Joshua Warner (Dec 07 2024 at 19:06):Here's the current failure mode:
path '/home/small-ci-user/actions-runner/_work/roc/roc/crates/compiler/test_syntax/fuzz' does not contain a 'flake.nix', searching up
error: no such command: `+nightly`
Cargo does not handle `+toolchain` directives.
Did you mean to invoke `cargo` through `rustup` instead?
It appears the cargo installed there is actual cargo instead of rustup, so it doesn't understand the +nightly thing.
Brendan Hansknecht (Dec 07 2024 at 19:18):Can you do rustup +nightly cargo (that might not be the right command, but something like that)
Brendan Hansknecht (Dec 07 2024 at 19:18):Cause I know we use rustup and the tool chain file
Luke Boswell (Dec 07 2024 at 19:39):It's Anton's machine, we should just wait for him to jump online.
Luke Boswell (Dec 07 2024 at 19:40):It's a self hosted runner
Joshua Warner (Dec 07 2024 at 22:02):Ok, my most recent attempt seems to be getting closest... except the nightly toolchain isn't installed it looks like
Config:
- name: run fuzz tests
run: |
cd crates/compiler/test_syntax/fuzz
nix develop -c rustup run nightly cargo fuzz run -j4 fuzz_expr --sanitizer=none -- -dict=dict.txt -max_total_time=60
Output:
error: toolchain 'nightly-x86_64-unknown-linux-gnu' is not installed
Our nix rust version comes from here, getting the nightly in there too would require some fiddling. I recommend starting out with a new workflow using the runner runs-on: [self-hosted, i7-6700K]. I'll install nightly(rust toolchain) nightly-2024-02-03 on there, that one matches our current rust version and that way we don't need to install the latest every day.
Anton (Dec 09 2024 at 10:49):It's installed, I expect this will work for your command cargo +nightly-2024-02-03 fuzz ...
Joshua Warner (Dec 10 2024 at 16:08):I think we'll also need a cargo +nightly-2024-02-03 install cargo-fuzz command as a preparatory step. Or should I put that in the job?
Anton (Dec 10 2024 at 16:29):Yeah, you can put that in the job, it'll basically be zero cost if it is already installed
Joshua Warner (Dec 11 2024 at 02:54):Ok, seems to be working! I currently have this in ubuntu_x86_64.yml, which may not be the ideal spot.
Anton (Dec 11 2024 at 10:04):that file should be fine :+1:
Luke Boswell (Dec 12 2024 at 01:25):We got our first fuzzer bug in the wild! :bug: :ladybug:
In this CI run https://github.com/roc-lang/roc/actions/runs/12287783837/job/34290444406?pr=7335
INFO: exiting: 77 time: 33s
────────────────────────────────────────────────────────────────────────────────
Failing input:
artifacts/fuzz_expr/crash-7f53e1d94350d5255f7f9bfdbedaff7665f10a0b
Output of `std::fmt::Debug`:
[50, 45, 52, 10, 46, 116, 10, 10, 33, 10, 10, 38, 112, 122, 50, 112, 122, 46, 116, 10, 10, 33, 10, 38, 112, 114, 118, 111, 105, 100, 101, 115, 33, 61, 61, 101, 74]
Reproduce with:
cargo fuzz run --sanitizer=none fuzz_expr artifacts/fuzz_expr/crash-7f53e1d94350d5255f7f9bfdbedaff7665f10a0b
Minimize test case with:
cargo fuzz tmin --sanitizer=none fuzz_expr artifacts/fuzz_expr/crash-7f53e1d94350d5255f7f9bfdbedaff7665f10a0b
────────────────────────────────────────────────────────────────────────────────
Error: Fuzz target exited with exit status: 77
Error: Process completed with exit code 1.
2-4
.t
!
&pz2pz.t
!
&prvoides!==eJ
What's our plan if CI fails on a fuzzer bug... and it passes everything else? Merge the PR and log an issue with a repro?
Luke Boswell (Dec 12 2024 at 01:30):Nvm, it looks like it cancels the rest of the run immediately
Joshua Warner (Dec 12 2024 at 01:48):We should move fuzzing to the last step in that job, so we can be sure everything else succeeded
Joshua Warner (Dec 12 2024 at 01:49):And obviously if this gets too noisy we should remove it or move it to a non-blocking job
Joshua Warner (Dec 12 2024 at 01:49):Will take a look later tonight
Joshua Warner (Dec 12 2024 at 02:19):The minimal repro here is:
4
!
&z.t
(found by putting that in a file called todo and then running cargo run --bin minimize expr todo)
Joshua Warner (Dec 12 2024 at 02:39):Fixed here: https://github.com/roc-lang/roc/pull/7340
Luke Boswell (Dec 12 2024 at 02:43):It'll still cancel all the other jobs early in CI Manager though right if this fails? I had a little investigation how to make it not do that, and I wasn't confident I could do it.
Joshua Warner (Dec 12 2024 at 02:56):I didn't realize it canceled other jobs
Joshua Warner (Dec 12 2024 at 02:58):Although, I think this is one of the longer running jobs, so maybe it will work out fine.
Richard Feldman (Dec 12 2024 at 04:08):here's another! https://github.com/roc-lang/roc/actions/runs/12289072630/job/34293972798?pr=7337#step:8:5297
Joshua Warner (Dec 12 2024 at 04:08):Ooof
Joshua Warner (Dec 12 2024 at 04:09):This is a little noisier than I was hoping for :/
Joshua Warner (Dec 12 2024 at 04:09):I think I'm going to go ahead and disable it for now
Brendan Hansknecht (Dec 12 2024 at 04:09):That was before merging the other fix into main
Brendan Hansknecht (Dec 12 2024 at 04:10):could they both be hitting the same failure?
Brendan Hansknecht (Dec 12 2024 at 04:10):oh, nvm
Brendan Hansknecht (Dec 12 2024 at 04:10):missed the last commit being merging main
Luke Boswell (Dec 12 2024 at 04:11):I'll run it for a bit while I'm in a meeting and see if I can find anything
Brendan Hansknecht (Dec 12 2024 at 04:12):actually my first statement is correct. That failure is before the fix on main. So main may be clean for fuzzing.
Joshua Warner (Dec 12 2024 at 04:12):I just found a couple more things locally
Joshua Warner (Dec 12 2024 at 04:12):Definitely not clean
Brendan Hansknecht (Dec 12 2024 at 04:12):ok
Joshua Warner (Dec 12 2024 at 04:14):Actually, interesting fuzzing question here
Joshua Warner (Dec 12 2024 at 04:14):Or perhaps language design question
Luke Boswell (Dec 12 2024 at 04:18):Here's another
x
!&
I've been running into a bunch of problems with this logic that sees what would usually be an Alias followed by a body and turns that into an annotation.
It's very sensitive to where exactly the whitespace is attached, as well as some details like whether we're adding/removing parens around the pattern in the alias turned annotation.
I'm not sure I've seen this logic kick in on actual example code. Is that needed/valuable?
Luke Boswell (Dec 12 2024 at 04:23):Found another
1)
:thinking: Maybe my 10 minute no-bugs-found run was lucky. And/or because I'm running it on relatively modest hardware (M1 macbook air)
Luke Boswell (Dec 12 2024 at 04:24):I can make issues instead if you'd prefer @Joshua Warner
Luke Boswell (Dec 12 2024 at 04:25):I had to go into the office, for some meetings. I can sit here all afternoon poking at the fuzzer :smiley:
Luke Boswell (Dec 12 2024 at 04:25):This is a task I can excel at
Joshua Warner (Dec 12 2024 at 04:25):@Luke Boswell what were the errors those were failing with?
Luke Boswell (Dec 12 2024 at 04:25):I'll make issues and copy it all in, with the minimisation too
Luke Boswell (Dec 12 2024 at 04:29):crash-d8c7b906b169f03d93286cac954573468ff5aae9
I lost the history (can't scroll back far enough) for this one.
Richard Feldman (Dec 12 2024 at 04:36):Joshua Warner said:
I've been running into a bunch of problems with this logic that sees what would usually be an Alias followed by a body and turns that into an annotation.
It's very sensitive to where exactly the whitespace is attached, as well as some details like whether we're adding/removing parens around the pattern in the alias turned annotation.
I'm not sure I've seen this logic kick in on actual example code. Is that needed/valuable?
I don't think this is something people mess up in any significant amount in practice, so I'd say in this situation it sounds reasonable to change the parser to be more resilient to failure - even if that makes the grammar stricter
Luke Boswell (Dec 12 2024 at 04:42):Ok, I'm going to try and not post ones that look like duplicates. Just noticing some of these might be for the same thing
Joshua Warner (Dec 12 2024 at 04:49):it sounds reasonable to change the parser to be more resilient to failure
@Richard Feldman What would you say to requiring parens around the pattern in this case?
e.g.
(UserId x) : [ UserId I64 ]
UserId x = UserId 42
... and have the non-parens equivalent parse strictly as an Alias?
Richard Feldman (Dec 12 2024 at 05:04):that seems fine for now...I don't think I've ever seen anyone write an annotation like that in practice :big_smile:
Brendan Hansknecht (Dec 12 2024 at 05:05):that is a valid annotation?
Brendan Hansknecht (Dec 12 2024 at 05:06):Can we just ban the parens there?
Joshua Warner (Dec 12 2024 at 05:07):Not sure what you mean by ban?
Joshua Warner (Dec 12 2024 at 05:08):Like, give a parse error?
Brendan Hansknecht (Dec 12 2024 at 05:10):yeah
Brendan Hansknecht (Dec 12 2024 at 05:11):Though I guess roc is permissive...so remove with the formatter I guess...
Joshua Warner (Dec 12 2024 at 05:11):I don't think that actually helps in this situation, but I could be misunderstanding
Richard Feldman (Dec 12 2024 at 05:12):well there ought to be some way to annotate that if you really want to
Brendan Hansknecht (Dec 12 2024 at 05:12):I guess, in my mind:
This is normal code to see:
(UserId x) = UserId 42
This is abnormal code to see:
(UserId x) : [ UserId I64 ]
Why do you have to be able to annotate a pattern match?
Joshua Warner (Dec 12 2024 at 05:14):Right; I think that's what I was getting at earlier...
Brendan Hansknecht (Dec 12 2024 at 05:14):Like you don't annotate branches of a when ... is
Brendan Hansknecht (Dec 12 2024 at 05:14):I don't think you should be able to annotate this
Richard Feldman (Dec 12 2024 at 05:15):hm, that's interesting :thinking:
Brendan Hansknecht (Dec 12 2024 at 05:15):Or you have to annotate it indirectly:
y : [ UserId I64 ]
y = UserId 42
UserId x = y
I hadn't thought about that perspective, but it would certainly simplify things!
Richard Feldman (Dec 12 2024 at 05:15):so you can only annotate plain identifiers, not destructures
Richard Feldman (Dec 12 2024 at 05:15):so you also couldn't annotate like (a, b) =
Brendan Hansknecht (Dec 12 2024 at 05:15):yeah, that would be my take
Brendan Hansknecht (Dec 12 2024 at 05:16):do people annotate destructures? I don't think I have ever seen that happen.
Richard Feldman (Dec 12 2024 at 05:16):given that people seem not to do that anyway, and that it definitely creates parsing problems, I'm on board with that plan
Richard Feldman (Dec 12 2024 at 05:16):I think there's been plenty enough time of having it be supported to know that it hasn't seen significant use in practice :big_smile:
Brendan Hansknecht (Dec 12 2024 at 05:17):Also, when I initially saw this code, I thought it was a weird way to write a type alias.
(UserId x) : [ UserId I64 ]
Thought it was the same as:
UserId x : [ UserId I64 ]
FWIW when I first encountered Roc, I was very confused that aliases look so much like annotations.
Luke Boswell (Dec 12 2024 at 05:24):I didn't know you could annotate a pattern match. My mental model was; Uppercase is an Alias, Lowercase is an Annotation
Luke Boswell (Jan 10 2025 at 05:51):~30mins on commit 9f395e033dfb9ade5c0642567f0dda8bbd1f2e5a
@Joshua Warner
Luke Boswell (Jan 10 2025 at 19:49):~12 hours on commit 9f395e033dfb9ade5c0642567f0dda8bbd1f2e5a
Luke Boswell (Jan 11 2025 at 11:37):~6 hours on commit 0471993428739cc754516c0e6c8a432895dbdaff
Joshua Warner (Jan 11 2025 at 19:07):https://github.com/roc-lang/roc/pull/7500 now has fixes for all these :)
Luke Boswell (Jan 11 2025 at 21:38):~2 hours on commit 875e355b68d15334f13fad616d7fe5d0dbc055ce
Joshua Warner (Jan 11 2025 at 22:17):These are all slow-unit- or oom- results; i.e. the test framework thought those inputs took excessively long or consumed too much memory.
... but when I run those, I don't see either thing happen :thinking:
Anthony Bullard (Jan 11 2025 at 22:20):I think in light of Josh's change....I might just throw away my current PR and focus on || lambda syntax
Joshua Warner (Jan 11 2025 at 22:22):Do you mean this one? https://github.com/roc-lang/roc/pull/7470
Anthony Bullard (Jan 11 2025 at 22:22):No, https://github.com/roc-lang/roc/pull/7490
Anthony Bullard (Jan 11 2025 at 22:23):And maybe 7470 too
Anthony Bullard (Jan 11 2025 at 22:23):But 7490 on my local is _much larger_ and ambitious
Anthony Bullard (Jan 11 2025 at 22:24):And I think after your change, and Sam's from a day or two ago, it might be hard to rebase and a lot of the assumptions I'm making might not work out
Anthony Bullard (Jan 11 2025 at 22:25):Basically I'm trying to move ALL PncApply likes to Collections, and also Pattern:RecordDestructure to use assignedfield and deprecate the RequiredField and OptionalFiled variants of the Pattern enum.
Anthony Bullard (Jan 11 2025 at 22:25):And just moving to align everything between Pattern and Expr to be more consistent
Joshua Warner (Jan 11 2025 at 22:28):I definitely like the direction
Joshua Warner (Jan 11 2025 at 22:33):Anything I can do to help?
Joshua Warner (Jan 11 2025 at 22:33):Happy to work on fixing conflicts or something
Luke Boswell (Jan 11 2025 at 22:53):@Joshua Warner maybe you could help merge this https://github.com/roc-lang/roc/pull/7470
Luke Boswell (Jan 11 2025 at 22:53):That sounds like the work Anthony is referring to
Luke Boswell (Jan 11 2025 at 22:54):And I guess the other one too?? https://github.com/roc-lang/roc/pull/7490
Anthony Bullard (Jan 12 2025 at 02:21):@Luke Boswell @Joshua Warner I have a lot more going on on my local
Anthony Bullard (Jan 12 2025 at 02:22):The weekend has been hectic, but I’ll try to make it pushable tomorrow morning
Joshua Warner (Jan 12 2025 at 04:27):Started finding & fixing some problems in can now!
https://github.com/roc-lang/roc/pull/7504
Luke Boswell (Jan 12 2025 at 04:44):~6 hours on commit 9ffc671659c2c2fec55e9b0475535e6cd34eb278
Luke Boswell (Jan 12 2025 at 04:49):That's a big tuple :smirk:
@0-21 AccessorFunction(
TupleIndex(
"18888888888888888888",
),
),
Honestly I think we should give the same error for anything more than say 32 or something. In that range it’ll still work with the warning. We can always raise the limit if someone autogenerates code that needs that or something.
Luke Boswell (Jan 12 2025 at 20:33):~12 hours on commit 5ebd6e0884db65f2d099dfb0ebf255d0bdf80c2b
Anthony Bullard (Jan 13 2025 at 03:03):@Joshua Warner I push up where I'm at right now.
Anthony Bullard (Jan 13 2025 at 03:03):Still some snapshot failures I'm not happy with, but don't have the brain power to fix right now
Anthony Bullard (Jan 13 2025 at 03:04):But the changes that are in there I'm happy with - for now. I still need to implement the "short single-arg" collapsing
Anthony Bullard (Jan 13 2025 at 03:04):And obviously rebase and figure out some of the new newlines I have
Anthony Bullard (Jan 13 2025 at 03:05):Probably just an artifact of some of the pattern-only variance I removed (Some may need to be added back, some may need to be done in a different way).
Anthony Bullard (Jan 14 2025 at 15:31):stat::number_of_executed_units: 567703
stat::average_exec_per_sec: 15343
stat::new_units_added: 9912
stat::slowest_unit_time_sec: 0
stat::peak_rss_mb: 602
INFO: exiting: 19712 time: 240s
AST changed after formatting...
Before formatting:
MM//(#
z
(#
w)#\"
w)#\",/
!\"\"aC-\"\"\"!\"a\"\"!CCa\"a\"\"\"(
#w)##,(
interface
##w,(
w)?
After formatting:
MM
// ( #
z
#
w # "
w) # ",/
!""
aC
-
"""
!"a""!CCa"a
"""(
# w)##,(
interface
## w,(
w,
)?
AST diff
Expr(
Defs(
Defs {
tags: [
EitherIndex(2147483648),
],
regions: [
…,
],
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(
BinOps(
[
(
Tag(
"MM",
),
DoubleSlash,
),
],
Defs(
Defs {
tags: [
EitherIndex(2147483648),
EitherIndex(2147483649),
],
regions: [
…,
…,
],
space_before: [
Slice<roc_parse::ast::CommentOrNewline> { start: 0, length: 0 },
Slice<roc_parse::ast::CommentOrNewline> { start: 0, length: 0 },
],
space_after: [
Slice<roc_parse::ast::CommentOrNewline> { start: 0, length: 0 },
Slice<roc_parse::ast::CommentOrNewline> { start: 0, length: 0 },
],
spaces: [],
type_defs: [],
value_defs: [
Stmt(
Var {
module_name: "",
ident: "z",
},
),
Stmt(
Var {
module_name: "",
ident: "w",
},
),
],
},
Var {
module_name: "",
ident: "w",
},
),
),
),
],
},
- Apply(
- UnaryOp(
- Str(
- PlainLine(
- "",
- ),
- ),
- Not,
- ),
+ BinOps(
[
- Var {
- module_name: "",
- ident: "aC",
- },
- UnaryOp(
- TrySuffix(
- PncApply(
+ (
+ Apply(
+ UnaryOp(
Str(
PlainLine(
- "!\"a\"\"!CCa\"a",
+ "",
),
),
- [
- Apply(
- Var {
- module_name: "",
- ident: "interface",
- },
- [
- Var {
- module_name: "",
- ident: "w",
- },
- ],
- Space,
- ),
- ],
+ Not,
),
+ [
+ Var {
+ module_name: "",
+ ident: "aC",
+ },
+ ],
+ Space,
),
- Negate,
+ Minus,
),
],
- Space,
+ TrySuffix(
+ PncApply(
+ Str(
+ PlainLine(
+ "!\"a\"\"!CCa\"a",
+ ),
+ ),
+ [
+ Apply(
+ Var {
+ module_name: "",
+ ident: "interface",
+ },
+ [
+ Var {
+ module_name: "",
+ ident: "w",
+ },
+ ],
+ Space,
+ ),
+ ],
+ ),
+ ),
),
),
)
crash-e5c5f2279bb018c4128ff6a25ca5e1130aae4952
Joshua Warner (Jan 14 2025 at 16:08):That one's fixed in https://github.com/roc-lang/roc/pull/7510
Luke Boswell (Jan 14 2025 at 20:39):~12 hours on 9c340302e2b83246ad3857337b7f5d22fcd58a7f
Luke Boswell (Jan 17 2025 at 10:37):~ 1 min on commit d42af0b763440176e829d67f0646b5c59c3f0f6d
Joshua Warner (Jan 19 2025 at 03:47):That was all one bug; pushed a fix to https://github.com/roc-lang/roc/pull/7510
Joshua Warner (Jan 19 2025 at 03:48):(pretty easy-to-hit problem with pipe-based closures)
Joshua Warner (Jan 19 2025 at 03:48):@Anthony Bullard (or perhaps @Luke Boswell) - that could use a re-review if you have some time :)
Luke Boswell (Jan 19 2025 at 07:40):~3 hours on commit 300412a4da46d81c6f40d196d968045fb0ad973f
Joshua Warner (Jan 21 2025 at 01:01):That, plus a pretty key oversight in the implementation of and/or, are fixed in https://github.com/roc-lang/roc/pull/7535
Joshua Warner (Jan 21 2025 at 01:01):I think >= 50% of the recent bugs I've fixed have been regressions introduced with recent parser changes
Joshua Warner (Jan 21 2025 at 01:03):Some of them have been caught either directly on the fuzzing job on the PR itself, or in a fuzzing job on some subsequent PR
Joshua Warner (Jan 21 2025 at 01:03):Maybe it's time to start paying more attention to the fuzzing happening in CI?
Luke Boswell (Jan 21 2025 at 01:33):Ahk that's good to know. I thought we might be mostly ignoring that until we get a solid run out of the fuzzer without issues.
Luke Boswell (Jan 21 2025 at 01:33):I haven't seen it run for more than 10mins yet without a crash - edit after the recent PNC changes etc
Luke Boswell (Jan 21 2025 at 01:34):But I guess I'm only running each time you've been putting a new syntax related PR in.
Joshua Warner (Jan 21 2025 at 01:34):There was a stretch of time where it was going a few hours without a crash
Luke Boswell (Jan 21 2025 at 01:34):Yeah, we were really close
Luke Boswell (Jan 21 2025 at 01:34):It definitely feels like we're almost back there again.
Anthony Bullard (Jan 21 2025 at 01:34):Yeah we need to stabilize again after some pretty rapid
Joshua Warner (Jan 21 2025 at 01:35):Maybe if there is a crash we should make it a policy to:
Joshua Warner (Jan 21 2025 at 01:35):A fuzzing crash found in CI, I mean
Luke Boswell (Jan 21 2025 at 01:37):@Joshua Warner would you prefer pushing a fix into a PR for new syntax, or landing that PR in main and then following up with any syntax/fuzzer related fixes?
Joshua Warner (Jan 21 2025 at 01:37):If it does look related to the PR, we should probably be bugging the PR author to look at it :)
Luke Boswell (Jan 21 2025 at 01:38):We've been moving a lot faster than usual and trying to land the breaking changes staged to unblock things.
Joshua Warner (Jan 21 2025 at 01:38):Keeping master bug-free-ish is a nice-to-have. Agree rapid collaboration is important (particularly recently)
Joshua Warner (Jan 21 2025 at 01:39):I'm not too picky if the fix goes in the same PR or a following one
Joshua Warner (Jan 21 2025 at 01:39):(at least, supposing the breakage isn't to something fairly obvious!)
Luke Boswell (Jan 21 2025 at 01:40):I've been a little more relaxed because I know the nightlies are paused, and you've been very effective at cleaning things up. So it's been a way to collaborate faster, by merging things into main and not backing up merge conflicts
Anthony Bullard (Jan 21 2025 at 01:41):Yeah I think for better or worse we’ve been in move fast and break things mode - but I think that’s going to slow down now. We can work as a team and smash the fuzzer issues
Anthony Bullard (Jan 21 2025 at 01:42):I think there is a lot of dissonance (much of it surely caused by me) between the parse grammar and the formatter
Anthony Bullard (Jan 21 2025 at 01:42):I’m happy to put down my current projects and jump on to help with that
Anthony Bullard (Jan 21 2025 at 01:43):If we see a legitimate crash - it shouldn’t be on you alone Josh, despite being a wizard - write up issues for these and mark them as P:Medium
Anthony Bullard (Jan 21 2025 at 01:44):And maybe tag them as nightly blocker
Joshua Warner (Jan 21 2025 at 01:44):Haha fair point :)
Anthony Bullard (Jan 21 2025 at 01:44):And feel free to assign the guilty party
Anthony Bullard (Jan 21 2025 at 01:44):I await the flood of issues…
Joshua Warner (Jan 21 2025 at 05:55):Adding auto-minimization to the fuzzer in ci: https://github.com/roc-lang/roc/pull/7537
Luke Boswell (Jan 21 2025 at 11:04):crash-fd9fabc49ffdbd93271a9f8d8e75ad416b7f9d83
Luke Boswell (Jan 21 2025 at 20:00):~12 hours on commit 5b4c8e70d873e7f5e30f995aedea0199bf1f99b1
Last updated: Jul 06 2025 at 12:14 UTC