Expose rust-version through env var

[flip1995]

This adds another env var that is exposed by cargo. In Clippy we would like to use that in order to efficiently check if a rust-version is set for the current package: rust-lang/rust-clippy#8774

Currently we either have to parse the Cargo.toml file ourselves or use the cargo_metadata crate which has a notable performance impact when running clippy-driver on single files.

[rust-highfive]

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

[epage]

The thing going through my head is the question: "what principle governs us exposing the manifest as environment variables?".

For myself, exposing CARGO_PKG_RUST_VERSION feels very arbitrary from a cargo-design perspective. Why this field and not many of the others?

[flip1995]

Many of the other fields are already exposed. Like the version field is even exposed through 4 or 5 env vars. Most package metadata is exposed. I think this makes it more consistent.

[ghost]

Also see #8758 where CARGO_PRIMARY_PACKAGE was added "primarily for Clippy".

[epage]

Many of the other fields are already exposed. Like the version field is even exposed through 4 or 5 env vars. Most package metadata is exposed. I think this makes it more consistent.

Except for CARGO_PRIMARY_PACKAGE , those all look targeted at the code being built, either so a crate can access its own metadata or build.rs / test purposes.

Also see #8758 where CARGO_PRIMARY_PACKAGE was added "primarily for Clippy".

Thanks for pointing out precedence on this. I am still interested in seeing if there is an answer to my question or if there is interest in creating an answer to my question. Until that time, I am not comfortable merging this though maybe others of the cargo team would be.

[flip1995]

I'm good with waiting on other team members. Just wanted to bring this up through a PR again, because my question some time ago on Zulip didn't get any replies 😅

[joshtriplett]

Suggested change

	* `CARGO_PKG_RUST_VERSION` — The Rust version from the manifest of your package.
	* `CARGO_PKG_RUST_VERSION` — The Rust version from the manifest of your package. Note that this is the minimum Rust version supported by the package, not the current Rust version.

(People regularly want Rust compiler version information, so I want to make sure people don't think this is the thing they're looking for for that.)

[epage]

We discussed this in the cargo team meeting and we have no guiding principle on what env variables should exist and are intending to be responsive to what different teams needs are. I'm fine merging this after the documentation gets improved.

[epage]

I'm surprised we don't link any of these fields to their manifest field's documentation. That would also be a natural way to clarify what this is intending to contain

[est31]

Alternative to an environment variable, one could also think about a flag that is passed to the clippy invocation. @epage would you be more okay with a flag?

[est31]

Also, the flag would be useful in rustc too. For example, it could give a warning if you are using a library feature that was stabilized after the specified minimum rust version.

[epage]

Alternative to an environment variable, one could also think about a flag that is passed to the clippy invocation. @epage would you be more okay with a flag?

Not dealt enough with that machinery to have much of an opinion.

Also, the flag would be useful in rustc too. For example, it could give a warning if you are using a library feature that was stabilized after the specified minimum rust version.

That'd be slick

[hellow554]

Also, the flag would be useful in rustc too. For example, it could give a warning if you are using a library feature that was stabilized after the specified minimum rust version.

Not only library features, but deprecated methods/struts/... Would be cool.
Currently you have to write #[allow(deprecated)] which is unpleasant, because you may forget to remove that and use a deprecated function for ever.

I would be down to implement that.

[epage]

To double check my understanding, are we interested in changing the direction of how to solve clippy's problem away from reading a cargo-specific env variable to a general flag for the driver (sorry if I've used the wrong terms) so rustc can also take advantage of this?

[est31]

The env var would be available to both rustc and clippy, from my understanding. Of course unless it's implemented in a way that it's only set for cargo clippy. But you could make rustc read the env var as well.

I guess my comment reads like if a flag would enable that, and I'm sorry for the bad formulation. I meant it as general support for having cargo pass it some way to rustc/clippy.

I think a flag would still be better. The env vars are mostly useful for people who want to do env! in their code or want to read it in build.rs, but I don't see any use cases for reading the MSRV. I'm also not sure of precedent in rustc for reading env vars in such a way.

[epage]

. I'm also not sure of precedent in rustc for reading env vars in such a way.

This was my reason for assuming an alternative solution would be needed for rustc. Let us know either way you all go with this.

[est31]

Yeah idk if compiler maintainers will accept something that uses an env var. I wonder why clippy needs an env var at all, what are the advantages over an argument from clippy's point of view?

[ghost]

The advantage of the env var is that it doesn't require changing the compiler and only needs minimal changes to Cargo.

Keep in mind that it took something like 4 years just to get rust-version into the Cargo manifest. The flag can get added when the compiler wants/needs it.

[hellow554]

I'm also in favor of the environment variable for now.

Maybe the documentation should be adjusted, but all in all, for the current situation, an env variable is the simplest solution.

I'm more than willing to adjust it to a command line argument, if needed.

The reason for having it in the first place is pointed out by @flip1995:

Currently we either have to parse the Cargo.toml file ourselves or use the cargo_metadata crate which has a notable performance impact when running clippy-driver on single files.

So in fact, we have to parse Cargo.toml twice which can be avoided and lead to a slight performance improvement. I think adding an environment variable should not be a big performance impact.

[epage]

The downside I see to an environment variable, especially one defined in cargo and not in the compiler/clippy, is that it makes the API less clear for people building without cargo. Granted, I don't know how much of a problem that already is and how much we care.

If you all feel this is the route to go, if you get the documentation updated and tests passing, we can merge this.

[est31]

I've originally believed that the presence of an environment variable was an unchangeable fact due to stability guarantees, but given that programs have to be prepared to the environment variable not being present at all, like when there is no rust-version in the manifest, I guess that it should be possible to remove it in the future in favour of a flag. So there's less pressure to "do it right" I guess...

How do people feel about me submitting a PR to add flags for it instead of an environment variable?

[flip1995]

How do people feel about me submitting a PR to add flags for it instead of an environment variable?

A flag would be a way bigger commitment if added to stable Rust. And a -Z flag doesn't really help, because you couldn't pass this to stable Clippy AFAIK. And I think most people use stable toolchains with stable Clippy now.

[epage]

@bors r+

[bors]

📌 Commit 0852f54 has been approved by epage

[bors]

⌛ Testing commit 0852f54 with merge 7d289b1...

[bors]

☀️ Test successful - checks-actions
Approved by: epage
Pushing 7d289b1 to master...