Let clippy check MSRV

[robertbastian]

#7068

[gemini-code-assist]

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

[Manishearth]

I'm a little wary, because it's nice for us to say "we know we build on MSRV", whereas I would not trust Clippy to be 100% correct when it comes to stuff like this. Especially when macros/etc are involved.

[Manishearth]

I am okay with getting rid of msrv-utils and trusting Clippy for that part.

I just don't want to get rid of the MSRV job.

[robertbastian]

let's set the toolchain in the GHA job then, so that locally it doesn't use a different toolchain, and so that this job also uses beta and nightly in CI

[Manishearth]

I like that!

[Manishearth]

Still a little concerned about increasing the number of CI-specific things that are not accessible via the makefile. I have a suggestion.

[Manishearth]

issue: So ci-job-foo is intended to, as much as possible, match what happens in CI; so that you can replicate CI failures locally without having to go spelunking in GHA setup files.

I think we should have ci-job-msrv-features (or whatever), and internally it sets the toolchain and calls some named task like check-all-features or ci-job-features

[robertbastian]

I like that!

so you don't like that after all?

If the features CI job fails, clippy will also fail, except in the very unlikely case where that MSRV issue is a false negative in clippy. fixing clippy will fix the features job as well, so there isn't really any spelunking necessary.

[Manishearth]

I didn't fully understand the specific approach being proposed in that comment. What I am proposing also matches that comment I think: I'm proposing an underlying makefile task for local invocation, distinct from

That said, I don't consider "get MSRV from CI" to be too much spelunking (if the MSRV job fails, you probably should set MSRV when testing locally, that's obvious), and we've never had perfect replicability. So I don't consider this blocking.

[sffc]

I trust @Manishearth to make sure this works. Please make sure that:

1. We still test MSRV in CI. It sounds like that means building with MSRV, not just asking Clippy to check for us.
2. Specific crates can continue to opt-out of global MSRV and use their own, and this is also tested in CI.

[Manishearth]

We still test MSRV in CI. It sounds like that means building with MSRV, not just asking Clippy to check for us.

Yes, I asked that the MSRV job still do this: it explicitly sets the MSRV first.

Specific crates can continue to opt-out of global MSRV and use their own, and this is also tested in CI.

this part is now handled by the clippy check. I think that's overall fine, personally? We could revive the msrv-utils task if we really want.

[sffc]

I have been the delegate who pushes most strongly for strict MSRV conformance, so my prior is to not make the check weaker, but I trust you both to make a good decision here that complies with previous TC consensus on supporting older MSRVs for specific utils crates.

[Manishearth]

@sffc I'm actually deferring to you on the msrv-utils task here, sorry if that was unclear.

I think it's mostly fine to use the clippy check for the older util MSRV, but lints can have false negatives.

Here's most of the information you need:

incompatible_msrv is not a full check, it checks item availability and constness. It does not, for example, check new Rust syntax, or macros. By and large we don't use new syntax/macros, so IMO it's not a big deal, and they're both things that can be undone in a utils patch release if we have to.

The nice thing about the clippy check is that it's automatic and we don't have to do additional steps to decouple util MSRV from ICU4X MSRV, it's just the Cargo.toml rust-version change. I think a lower barrier for playing with util MSRV is great.

[robertbastian]

I don't understand what changes you guys want

[Manishearth]

@robertbastian the change in my issue above is an optional one where:

• we have various check-all-features (etc) make targets. I don't know what they should be called, but they operate on the default toolchain
• ci-job-msrv explicitly calls cargo +msrv make check-all-features

This way running cargo make check-all-features does the right thing locally, and you can use ci-job-msrv if you have just seen an MSRV error in the MSRV CI job and want to just replicate it locally without fiddling with things.

Again, this is optional.

---

Regarding msrv-utils: I don't think there's a requested change yet. This PR replaces msrv-utils with the clippy check, which is a degradation in fidelity. A guaranteed "does it compile" check that works by checking if it compiles is replaced by a heuristic based lint that only checks item references and constness. This is probably fine, but I want to highlight it and give Shane a chance to think about that tradeoff. I find that the upside of not needing to write a bunch of makefile stuff for each util to be really nice, personally. Retaining the msrv-utils task in this PR would make it immediately mergeable in my eye. I tend towards removing it though.

[robertbastian]

so no action on my end, just an approval from @sffc?

[Manishearth]

Correct.

[Manishearth]

And Shane can use the contents of this comment to provide his opinion: #7069 (comment)

[sffc]

incompatible_msrv is not a full check, it checks item availability and constness. It does not, for example, check new Rust syntax, or macros. By and large we don't use new syntax/macros, so IMO it's not a big deal, and they're both things that can be undone in a utils patch release if we have to.

If we're fairly confident that we can always undo MSRV breakages via patch releases, then relying on the clippy check seems fine when scoped to smaller crates like zerofrom. I would worry that a more complex crate like yoke or zerovec might have problems that slip through and might not be easy to undo. For example, things like GATs are evolving, and we could end up in a situation where we rely on a GAT feature that Clippy doesn't catch.

The nice thing about the clippy check is that it's automatic and we don't have to do additional steps to decouple util MSRV from ICU4X MSRV, it's just the Cargo.toml rust-version change. I think a lower barrier for playing with util MSRV is great.

Yes, this is a really nice benefit.

[Manishearth]

Okay, in that case @robertbastian go ahead and merge.

Perhaps we can resurrect msrv-utils as a smarter CI task that doesn't require setting things in the CI file.