try reading rust-version from Cargo.toml #8774

hellow554 · 2022-05-02T09:58:56Z

Cargo.toml can contain a field rust-version, that acts like a MSRV of
clippy.toml file: https://doc.rust-lang.org/cargo/reference/manifest.html#the-rust-version-field
This will try to read that field and use it, if the clippy.toml config
has no msrv entry

changelog: respect rust-version from Cargo.toml

closes #8746
closes #7765

rust-highfive · 2022-05-02T09:58:59Z

r? @giraffate

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

flip1995 · 2022-05-02T10:43:00Z

This sounds familiar. I think I also worked on that before 🤔

....

I did: rust-lang/cargo#9967 and I claimed #7765 back then. I probably forgot about finishing it 😄

I think one issue I couldn't resolve with this is that MetadataCommand is a quite expensive operation. This isn't really noticeable if compiling a single crate, but for example when running Clippy on rustc test suite, this can increase the time it takes immensely. See #5505 (comment)

cc @matthiaskrgr Do you still have a way to run Clippy on rustc's test suite to test this branch and whether it brings back the issue linked above?

hellow554 · 2022-05-02T10:51:03Z

I really tried searching for a duplicate, I'm sorry :D
But thanks for bringing it up my attention.
Should I add a closes #7765 as well?

flip1995 · 2022-05-02T10:54:23Z

Should I add a closes #7765 as well?

Yes, please.

flip1995

I'm also not sure about the precedence. Should the rust-version in the Cargo.toml file be valued more or the one in the clippy.toml file? Should we deprecate the msrv conf option and point to the rust-version instead?

flip1995 · 2022-05-02T10:55:38Z

clippy_lints/src/lib.rs

+            Err(e) => {
+                sess.struct_err(&format!("could not read cargo metadata: {}", e)).emit();


I don't think this is a hard error. For example when using clippy-driver outside of a cargo project, no Cargo.toml file can be found. I would just silently keep conf.msrv = None here.

hellow554 · 2022-05-02T11:02:10Z

I'm also not sure about the precedence. Should the rust-version in the Cargo.toml file be valued more or the one in the clippy.toml file? Should we deprecate the msrv conf option and point to the rust-version instead?

Good question. I thought, that the clippy.toml file should have a higher precedence, than the cargo.toml file. Deprecating it would be another idea, yes... But that's above my paygrade 😇

flip1995 · 2022-05-02T11:10:07Z

Let's talk about this in tomorrows meeting 👍

ghost · 2022-05-03T06:56:12Z

I think one issue I couldn't resolve with this is that MetadataCommand is a quite expensive operation. This isn't really noticeable if compiling a single crate, but for example when running Clippy on rustc test suite, this can increase the time it takes immensely. See #5505 (comment)

Shouldn't Cargo provide rust-version as an environment variable? They already provide a lot of other manifest data in that way - cargo docs.

We already use CARGO_PRIMARY_PACKAGE in the driver.

rust-clippy/src/driver.rs

Line 338 in 95f8b26

let in_primary_package = env::var("CARGO_PRIMARY_PACKAGE").is_ok();

flip1995 · 2022-05-03T16:19:57Z

We discussed this in today's meeting. We came to the agreement that:

We shouldn't deprecate Clippy's MSRV config option (yet)
The clippy.toml MSRV config option should have precedence.
If both clippy.toml MSRV and Cargo.toml rust-version are defined, but differ, a warning should be emitted.

I think the current implementation covers the first two points. Do you mind adding the warning? If you need help with that let me know.

Also we should answer the perf question and if cargo providing this through a env var is and option. Maybe we can even get that from rustc itself?

hellow554 · 2022-05-04T06:04:58Z

Thanks for the discussion on this topic.

Currently, I only use the expensive MetadataCommand call if clippy.toml hasn't that msrv option set. If I want to check them both, I need to make that call every time. I can do that, but that will very much likely make things worse.

I can open an issue/pr @ https://github.com/rust-lang/cargo to insert a CARGO_PKG_RUST_VERSION field to the environment.
I'm not sure what you mean by "getting it from rustc itself"? rustc doesn't read a Cargo.toml IIRC, only cargo does.

flip1995 · 2022-05-04T09:10:38Z

If I want to check them both, I need to make that call every time. I can do that, but that will very much likely make things worse.

Not really, because the issue only occurs if you check multiple single files, not a cargo project. But if you don't have a Cargo.toml for those single files you also won't have a clippy.toml. Maybe it's a none-issue. Let me check that.

'm not sure what you mean by "getting it from rustc itself"? rustc doesn't read a Cargo.toml IIRC, only cargo does.

For some reason I had in mind that a #[rust-version] attribute exists and that therefore rustc must have some information about that. But that's not the case, so nevermind.

flip1995 · 2022-05-04T09:55:13Z

Checking this with flamegraph, the MetadataCommand takes up 3-4% of the runtime, when running on a single file.

Looking at cargo uitest this has some more impact:

Without this patch:

cargo uitest > /dev/null  75.74s user 26.33s system 886% cpu 11.511 total
cargo uitest > /dev/null  75.96s user 26.91s system 891% cpu 11.545 total
cargo uitest > /dev/null  76.83s user 26.69s system 879% cpu 11.768 total

With this patch:

cargo uitest > /dev/null  90.80s user 30.94s system 883% cpu 13.785 total
cargo uitest > /dev/null  91.13s user 30.94s system 880% cpu 13.864 total
cargo uitest > /dev/null  91.20s user 31.08s system 892% cpu 13.697 total

(meassured on Intel Core i7-9750H @ 12x 4.5GHz)

It isn't too bad, but definitely a noticable regression.

I think creating an issue in cargo about the env var or (maybe better) asking on Zulip about it would be good. I'm open to merge this with MetadataCommand if the cargo team doesn't want to add the env var.

hellow554 · 2022-05-04T10:02:13Z

Thanks for the measurement!
I'm not really firm with zulip, would you mind asking there? I would try to prepare a PR for that?

flip1995 · 2022-05-04T10:17:08Z

Zulip thread: https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/env.20var.20for.20rust-version.20metadata/near/281135045

flip1995 · 2022-05-04T10:23:08Z

If no such env var should get added we could just check if CARGO_MANIFEST_DIR is set before running MetadataCommand. That way we can be sure, that Clippy is run through cargo and not through clippy-driver. But that's only a workaround.

hellow554 · 2022-05-05T08:58:07Z

So I did things™.

Instead of using CargoMetadata I use what @bjorn3 suggested and reading the $CARGO_MANIFEST_DIR/Cargo.toml file directly as toml file and parse the package.rust-version field from it.
This should give a little speed advantage over the cargo metadata call.

What I wanted next, but couldn't achieve because of the way the Conf struct is created, is to have the RustcVersion directly, instead of storing a string and parsing it all over again. But for this to work I either need to implement Deserialize directly for that struct (looking @flip1995 ;) ) or finding a clever workaround...

hellow554 · 2022-05-05T11:21:39Z

I really would like to add some tests, but I'm not really sure how to integrate them into the clippy testsuite. After all, I would like to have at least 7 different testcases and therefore 7 different cargo projects.

file-attr	clippy	cargo
None	None	None
None	None	Some
None	Some	None
...

flip1995

But for this to work I either need to implement Deserialize directly for that struct (looking @flip1995 ;) ) or finding a clever workaround

I intentionally wrote the rustc-version crate without deps. So adding serde for this isn't really an option.

You can add tests for this in the ~~tests/ui-toml or~~ tests/ui-cargo dirs. See for example: https://github.com/rust-lang/rust-clippy/tree/master/tests/ui-cargo/cargo_common_metadata/pass which has both, a Cargo.toml and a clippy.toml file in the test.

clippy_lints/src/lib.rs

flip1995 · 2022-05-05T12:21:58Z

Instead of using CargoMetadata I use what @bjorn3 suggested and reading the $CARGO_MANIFEST_DIR/Cargo.toml file directly as toml file and parse the package.rust-version field from it.

Not sure if we should re-parse it by hand or just check for the CARGO_MANIFEST_DIR env var and then call MetadataCommand.

hellow554 · 2022-05-05T15:19:43Z

Not sure if we should re-parse it by hand or just check for the CARGO_MANIFEST_DIR env var and then call MetadataCommand.

I mean, in the end, the two options should be equal in their outcome.
Where they can differ is the speed. As said, I avoided using MetadataCommand, because of the speed concerns. Parsing a toml file should be relativly easy and in the end, I'm doing the same as cargo does, am I right? :/

flip1995 · 2022-05-05T16:40:53Z

Where they can differ is the speed. As said, I avoided using MetadataCommand, because of the speed concerns. Parsing a toml file should be relativly easy and in the end, I'm doing the same as cargo does, am I right? :/

Yeah I guess this is only style preference in the end. I don't think the perf difference matters much when linting a whole crate with cargo clippy. The perf concern is only with clippy-driver single-file.rs.

Let's keep the toml parsing code for now 👍

matthiaskrgr · 2022-05-05T19:53:54Z

@flip1995 sorry, just randomly saw the mention (I'm getting like 30 github mails a day and don't check them all tbh 😓 )
I think the iterations were tacked in here: #3142
(from bash for-loop to a semi-fledged rust program (icemaker))
icemaker has a --clippy mode but the setup might be a bit tricky (you'll need RTIM master toolchain + clippy installed with the name "master" and systemd-run at least)

hellow554 · 2022-05-09T06:37:28Z

I did the suggested changes and rebased everything into one commit.

Hope this is ready for merging :)

flip1995

LGTM. Do you want to add some tests to tests/ui-cargo?

clippy_lints/src/lib.rs

hellow554 · 2022-06-18T19:26:18Z

Hey @flip1995 I need your help again.

I seem to be stuck to the same problem as last time, when I tried to parse Cargo.toml manually, namely that when running tests I don't get the rust-version from the tests Cargo.toml, but from the project one.

Running pass_both_same yields an empty string, but when I change the Cargo.toml file in the clippy root directory to something like 1.62.51 I get exactly that, but not the one from the tests Cargo.toml file.

Is there a way around this? Else I would have to use cargo-metadata again and the whole point failed :(
Is that a problem in cargo uitest?

flip1995 · 2022-06-18T21:20:49Z

We may want to look at the UI-tests and if we can change something there to test it. We should definitely use the env var and not cargo_metadata. I'll have to look more in depth into that though...

ghost · 2022-06-19T07:27:19Z

Would making a UI test with compiletest header to set the environment variable work?

hellow554 · 2022-06-27T09:14:32Z

I think I finally found the solution. It passes on my machine™.

Problem is, that compiletest_rs does not parse the Cargo.toml file in the tests folder itself.
I see two options: either use the compiletest-header as @mikerite suggests, or, to stick with the current theme, we just parse the Cargo.toml file ourself with the toml crate and set the environment variable ourself.

I know, this isn't a perfect solution, but this is the best I can do :)

hellow554 · 2022-06-27T09:16:56Z

tests/ui-cargo/multiple_config_files/warn/src/main.stderr

-Using config file `$SRC_DIR/tests/ui-cargo/multiple_config_files/warn/.clippy.toml`
-Warning: `$SRC_DIR/tests/ui-cargo/multiple_config_files/warn/clippy.toml` will be ignored.
+Using config file `$SRC_DIR/.clippy.toml`
+Warning: `$SRC_DIR/clippy.toml` will be ignored.


This is a "fallout" by setting CARGO_MANIFEST_DIR, but I think this is the proper way

flip1995

Impl LGTM and also the solution to the tests. Only some minor things left.

Cargo.toml

README.md

clippy_lints/src/lib.rs

flip1995 · 2022-06-27T12:42:40Z

tests/ui-cargo/cargo_rust_version/fail_file_attr/src/main.rs

@@ -0,0 +1,13 @@
+#![feature(custom_inner_attributes)]
+#![clippy::msrv = "1.58.0"]


So, this doesn't produce a warning. That is not optimal, but I wouldn't try to address this in this PR.

Can you add a FIXME, that this should produce some kind of warning, that it overrides the value from one of the toml files?

Maybe abandon that attribute in the long run? ;)

hellow554 · 2022-06-27T13:00:31Z

Done

Cargo.toml can contain a field `rust-version`, that acts like a MSRV of clippy.toml file: https://doc.rust-lang.org/cargo/reference/manifest.html#the-rust-version-field This will try to read that field and use it, if the clippy.toml config has no `msrv` entry

compiletest_rs is not meant to test full cargo projects, but instead only files. So we need to parse the `Cargo.toml` file ourself and set the corresponding environment variable. In this case we just set `CARGO_PKG_RUST_VERSION`, nothing more. But, of course, this can be extended.

hellow554 · 2022-06-28T06:14:53Z

CI is green and I think this is (finally) ready to merge

flip1995 · 2022-06-28T07:27:03Z

Thanks so much for this and your patience with all the back and forth!

@bors r+

bors · 2022-06-28T07:27:05Z

📌 Commit f0a1cd5 has been approved by flip1995

bors · 2022-06-28T07:27:11Z

⌛ Testing commit f0a1cd5 with merge b776fb8...

bors · 2022-06-28T07:42:29Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing b776fb8 to master...

Clippy now respects the rust-version field in Cargo manifests: rust-lang/rust-clippy#8774

rust-highfive assigned giraffate May 2, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 2, 2022

flip1995 reviewed May 2, 2022

View reviewed changes

flip1995 added the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label May 2, 2022

hellow554 force-pushed the cargo-rust-version branch from 2a8dc2f to 56224e3 Compare May 5, 2022 09:01

flip1995 reviewed May 5, 2022

View reviewed changes

clippy_lints/src/lib.rs Outdated Show resolved Hide resolved

clippy_lints/src/lib.rs Outdated Show resolved Hide resolved

hellow554 force-pushed the cargo-rust-version branch from 56224e3 to 08ea04b Compare May 9, 2022 06:36

hellow554 force-pushed the cargo-rust-version branch from 08ea04b to 3cebb1b Compare May 9, 2022 06:49

flip1995 reviewed May 9, 2022

View reviewed changes

clippy_lints/src/lib.rs Outdated Show resolved Hide resolved

flip1995 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work labels Jun 18, 2022

hellow554 mentioned this pull request Jun 27, 2022

add rustdoc and clippy to github workflow rust-itertools/itertools#618

Closed

hellow554 force-pushed the cargo-rust-version branch from bab6b4a to 7ce853d Compare June 27, 2022 09:12

hellow554 commented Jun 27, 2022

View reviewed changes

hellow554 force-pushed the cargo-rust-version branch from 7ce853d to d8e6c33 Compare June 27, 2022 09:24

flip1995 reviewed Jun 27, 2022

View reviewed changes

hellow554 force-pushed the cargo-rust-version branch from 5099e74 to 3d0f73e Compare June 27, 2022 12:59

hellow554 force-pushed the cargo-rust-version branch 3 times, most recently from 4883039 to f0a1cd5 Compare June 28, 2022 06:01

hellow554 added 4 commits June 28, 2022 08:01

update README.md to reflect rust-version in cargo.toml

83511d1

add uitests for cargo rust-version field

f0a1cd5

bors merged commit b776fb8 into rust-lang:master Jun 28, 2022

This was referenced Jun 28, 2022

Use clippy's MSRV support when available jonasbb/serde_with#227

Closed

respect CARGO_PKG_RUST_VERSION when issuing deprecation warnings rust-lang/rust#98893

Closed

jplatte added a commit to ruma/ruma that referenced this pull request Jul 19, 2022

Remove msrv from .clippy.toml

2c2a48b

Clippy now respects the rust-version field in Cargo manifests: rust-lang/rust-clippy#8774

jplatte mentioned this pull request Jul 19, 2022

Remove msrv from .clippy.toml ruma/ruma#1246

Merged

		Err(e) => {
		sess.struct_err(&format!("could not read cargo metadata: {}", e)).emit();

		@@ -0,0 +1,13 @@
		#![feature(custom_inner_attributes)]
		#![clippy::msrv = "1.58.0"]

try reading rust-version from Cargo.toml #8774

try reading rust-version from Cargo.toml #8774

Conversation

hellow554 commented May 2, 2022 • edited Loading

rust-highfive commented May 2, 2022

flip1995 commented May 2, 2022 • edited Loading

hellow554 commented May 2, 2022

flip1995 commented May 2, 2022

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 May 2, 2022 • edited Loading

Choose a reason for hiding this comment

hellow554 commented May 2, 2022

flip1995 commented May 2, 2022

ghost commented May 3, 2022

flip1995 commented May 3, 2022

hellow554 commented May 4, 2022 • edited Loading

flip1995 commented May 4, 2022

flip1995 commented May 4, 2022 • edited Loading

hellow554 commented May 4, 2022

flip1995 commented May 4, 2022

flip1995 commented May 4, 2022

hellow554 commented May 5, 2022

hellow554 commented May 5, 2022

flip1995 left a comment • edited Loading

Choose a reason for hiding this comment

flip1995 commented May 5, 2022

hellow554 commented May 5, 2022

flip1995 commented May 5, 2022 • edited Loading

matthiaskrgr commented May 5, 2022

hellow554 commented May 9, 2022

flip1995 left a comment

Choose a reason for hiding this comment

hellow554 commented Jun 18, 2022

flip1995 commented Jun 18, 2022

ghost commented Jun 19, 2022

hellow554 commented Jun 27, 2022

hellow554 Jun 27, 2022

Choose a reason for hiding this comment

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 Jun 27, 2022

Choose a reason for hiding this comment

hellow554 Jun 27, 2022

Choose a reason for hiding this comment

hellow554 commented Jun 27, 2022

hellow554 commented Jun 28, 2022

flip1995 commented Jun 28, 2022

bors commented Jun 28, 2022

bors commented Jun 28, 2022

bors commented Jun 28, 2022

hellow554 commented May 2, 2022 •

edited

Loading

flip1995 commented May 2, 2022 •

edited

Loading

flip1995 May 2, 2022 •

edited

Loading

hellow554 commented May 4, 2022 •

edited

Loading

flip1995 commented May 4, 2022 •

edited

Loading

flip1995 left a comment •

edited

Loading

flip1995 commented May 5, 2022 •

edited

Loading