update readme for specifying msrv #6379
Conversation
|
r? @ebroto (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @flip1995 |
There was a problem hiding this comment.
I would put this section below the section about allowing/denying lints.
Please also add a link to the supported lints: https://rust-lang.github.io/rust-clippy/master/index.html#msrv
|
If you add the other lints mentioned in #6097, can you write a step-by-step instruction documentation inside |
Sure, I'll add that along in a different PR |
|
@bors r+ Thanks! |
|
📌 Commit b2eb55b has been approved by |
|
💔 Test failed - checks-action_test |
|
@bors retry |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
| specifying the minimum supported Rust version (MSRV) in the clippy configuration file. | ||
|
|
||
| ```toml | ||
| msrv = "1.30.0" |
There was a problem hiding this comment.
IIUC the #6201 implementation does not consider the exact patch version, so I think it is preferable to always omit the patch version in the documentation.
There was a problem hiding this comment.
It does, if you specify it. So if you specify msrv = 1.30.0 and a lint is marked as 1.30.1 this lint will not trigger. The thing is, that features don't usually get stabilized in a patch version, so no lint will probably ever have a patch version >0
There was a problem hiding this comment.
It does, if you specify it. So if you specify
msrv = 1.30.0and a lint is marked as1.30.1this lint will not trigger.
Oh, I didn't realize it was possible with the current implementation. Thanks for pointing it out.
The thing is, that features don't usually get stabilized in a patch version, so no lint will probably ever have a patch version
>0
Yeah, this is what I actually wanted to say. (There was a case where stabilization was reverted in a patch version due to a security vulnerability, but I don't think there were any cases where new APIs were stabilized.)
| } | ||
| ``` | ||
|
|
||
| Tilde/Caret version requirements (like `^1.0` or `~1.2`) can be specified as well. |
There was a problem hiding this comment.
IIRC semver::VersionReq treats 1.1 as ^1.1 without ^, right? If so, these requirements are verbose and I recommend not to mention them. Also, I don't think it is necessary to mention ~. MSRV is a specific version and is not a range. (I think it means strictly "it can be compiled with all stable versions after the specified version", but ~x.y and some requirements are not the correct requirements to express this so I think should ideally be rejected.)
There was a problem hiding this comment.
~x.yand some requirements are not the correct requirements to express this so I think should ideally be rejected.
I looked at how rustc's cfg(version) parses this, but it is worse: rust-lang/rust#79436
I've commented about an alternative parser implementation, but I think that is probably sufficient for Clippy's use cases as well. If so, it may be preferable to use that instead of VersionReq.
(I'm not the maintainer of Clippy, so I'll leave the decision to maintainers, but I believe we shouldn't accept the extra requirement syntax that doesn't really make sense here.)
There was a problem hiding this comment.
I don't see why the additional options to specify a version should be problematic. If you want to misuse those, that's on you. But I agree, that we should not mention that this is possible, otherwise it invites people to misuse it.
There was a problem hiding this comment.
@taiki-e yeah, the MSRV should definitely not be a range so tilde requirements don't make sense but, I don't think VersionReq treats 1.1 and ^1.1 the same way
e.g. In the snippet below, switching between 1.30 and ^1.30 produces different results
use semver::{Version, VersionReq};
fn main() {
const MANUAL_STRIP_MSRV: Version = Version {
major: 1,
minor: 45,
patch: 0,
pre: Vec::new(),
build: Vec::new(),
};
match VersionReq::parse("1.30") {
Ok(vReq) => {
if !vReq.matches(&MANUAL_STRIP_MSRV) {
println!("Will lint!")
} else {
println!("Won't lint!")
}
},
Err(_) => println!("Error")
}
}There was a problem hiding this comment.
@suyash458: Good catch! I didn't know that behavior. Given that cargo treats them as the same requirements, I think it's preferable to treat the same as cargo.
@flip1995: I agree with it is a user issue, but the more things that can be accepted, the more complicated the test will be. I think it is preferable to accept only the minimum necessary things if possible.
In fact, @suyash458 found that msrv = "major.minor" didn't seem to be working as expected. (Seems #6201 had tests for msrv = "major.minor.patch", msrv = "=major.minor.patch", msrv = "^major.minor", etc., but not for msrv = "major.minor".)
There was a problem hiding this comment.
the more complicated the test will be. I think it is preferable to accept only the minimum necessary things if possible.
Well we use the semver crate, so this crate should have the tests, that the version matching works, so we can just assume, that it works. (And semver has indeed many unit tests)
Contrary to that: The semver documentation for VersionReq claims, that 1.30 is just an alias for ^1.30 and should therefore behave the same 😅
Remove mention of possibility to specify the MSRV with a tilde/caret As `@taiki-e` explained in #6379 (comment), mentioning this might be problematic. changelog: none
changelog: add some documentation for the
msrvfeature (#6097)related PR: #6201