Skip to content

update readme for specifying msrv #6379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 25, 2020
Merged

update readme for specifying msrv #6379

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
85a17b5
update README.md for specifying msrv
suyashb95 Nov 25, 2020
94a6832
update README.md
suyashb95 Nov 25, 2020
b2eb55b
Fix formatting in README.md
flip1995 Nov 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,32 @@ cargo clippy -- -A clippy::all -W clippy::useless_format -W clippy::...
```
Note that if you've run clippy before, this may only take effect after you've modified a file or ran `cargo clean`.

### Specifying the minimum supported Rust version

Projects that intend to support old versions of Rust can disable lints pertaining to newer features by
specifying the minimum supported Rust version (MSRV) in the clippy configuration file.

```toml
msrv = "1.30.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@flip1995 flip1995 Nov 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.)

```

The MSRV can also be specified as an inner attribute, like below.

```rust
#![feature(custom_inner_attributes)]
#![clippy::msrv = "1.30.0"]

fn main() {
...
}
```

Tilde/Caret version requirements (like `^1.0` or `~1.2`) can be specified as well.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~x.y and 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.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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")
  }
}

Copy link
Member

@taiki-e taiki-e Nov 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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".)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Contributor Author

@suyashb95 suyashb95 Nov 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taiki-e, @flip1995 ^1.30, 1.30 and 1.30.0 are treated the same way in semver v0.9 but this behavior is broken in v0.11. Weird


Note: `custom_inner_attributes` is an unstable feature so it has to be enabled explicitly.

Lints that recognize this configuration option can be found [here](https://rust-lang.github.io/rust-clippy/master/index.html#msrv)

## Contributing

If you want to contribute to Clippy, you can find more information in [CONTRIBUTING.md](https://github.com/rust-lang/rust-clippy/blob/master/CONTRIBUTING.md).
Expand Down