Hacker News Reader

Comment by gtirloni | Hacker News Reader

> new contributors make PRs. they make silly simple mistakes due to lack of experience; you point them out and they get fixed. this can be fun, for a time. what it’s teaching you is that you personally are responsible for catching mistakes.

Every team I've worked with that was feeling exhausted about code contributions actually needed to focus on better documenting guidelines but most importantly, improve CI. Catching silly mistakes is the textbook definition where automating stuff can help the most. This should improve reviewer's peace of mind and speed things up because people will work by themselves to fix things before asking for a review.

I don't know what's the CI situation in the Rust project but if it's anything like what I'm used to, it probably needs improvement. Adding more human-hours to reviewing things isn't sustainable.

View on HN

rust's CI is quite extensive actually; there's a talk on it here: https://www.pietroalbini.org/blog/shipping-a-compiler-every-...

CI is good. more tests are good. but there's a limit to how much they can catch.

here is an example: there is an internal type checking API called [`resolve_vars_if_possible`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_infer/...). this needs to be called on the output of almost any call to `.normalize()`, or you will get an internal compiler error. the conditions under which you get an ICE are quite complicated; sometimes you need 6 or more items in the crate with `impl Trait` or type_alias_impl_trait or const generics involved. but it's very easy to see there's a bug just by looking at the code and seeing the missing `resolve_vars()` call.

how can we automate this? well, rustc has a mechanism for "internal lints", but they're somewhat annoying to write - you're basically writing a new compiler pass, and it needs to not have false positives. it's much easier to just catch this one instance. so the reviewer, who's already burned out, does this quick manual review instead.

Just tell that verbatim to ChatGPT and it will catch it for you.

hm, is it possible to ... add these trivial-by-look-but-hard-to-script things to a checklist as an auto-comment to PRs? is there a list of these?

anyway, thanks for writing this post, and for featuring that awesome message/art/logo/thing at the end \m/

yes, it's possible! that list doesn't exist today but i would love to create it. i wrote a draft a few years ago before shifting to other work; someone recently expressed interest in reviving that project: https://github.com/rust-lang/rustc-dev-guide/pull/1463

<3 i'm glad you enjoyed it

This is true, but then the corrollary is that new PRs need to come with this higher, rigorous level of test coverage.

And then that becomes a bit of a barrier to contribution -- it's a tradeoff

I often write an entirely new test harnesses for a feature, e.g. for https://www.oilshell.org, particularly using other implementations as "oracles". All of these run in the CI - https://www.oilshell.org/release/latest/quality.html

The good thing is that it definitely helps me accept PRs faster. Current contributors are good at this kind of exhaustive testing, but many PRs aren't

Even so, I do find myself simply adding test coverage from time-to-time -- that's an important job of a maintainer

I agree

At the same time lint tools sometimes move into the 'pedantic unhelpful' field because those things are the easiest to check (no, nobody likes pep8 and its brain-dead char limit - Black is much better)

But yes, if it can be automated it should be automated

Reply on news.ycombinator.com