Skip to content
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.

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

Minimum supported version of Rust (MSVR) config and support #6097

Closed
ghost opened this issue Sep 29, 2020 · 17 comments · Fixed by #6465
Closed

Minimum supported version of Rust (MSVR) config and support #6097

ghost opened this issue Sep 29, 2020 · 17 comments · Fixed by #6465
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages good-first-issue These issues are a good way to get started with Clippy

Comments

@ghost
Copy link

ghost commented Sep 29, 2020

Many lints suggest using new features in the std lib like clippy::manual_strip. I think these are great because they help people upgrade their code to take advantage of these features. However, there are many projects that support old versions of Rust that can't use these. These lints only make more work for those projects because they have to allows to fix their CI. See this recent commit in serde-yaml for example.

I'd like to add a key to clippy.toml to specify the minimum version of rust (and the stdlib) that a project supports. We can conditionally register these lints that target features not supported on that version.

I understand that eventually proper support for this will be added to Cargo and rustc to support this but at that stage we can just deprecate the setting and use the new stuff.

@flip1995
Copy link
Member

flip1995 commented Oct 1, 2020

I think this makes sense. This should be possible by just adding a "lint configuration" and passing that to the relevant lint passes.

@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages labels Oct 1, 2020
@suyashb95
Copy link
Contributor

@flip1995 I'd like to pick this up!

@djc
Copy link
Contributor

djc commented Oct 9, 2020

This would be awesome to have. I know we have a bunch of these allows in the url crate already at least.

@flip1995
Copy link
Member

flip1995 commented Oct 9, 2020

@djc Can you post a list of them here, so we can apply this configuration option to those lints? That would be really helpful!

@djc
Copy link
Contributor

djc commented Oct 9, 2020

In url we have these:

#[allow(clippy::option_as_ref_deref)] // introduced in 1.40, MSRV is 1.36
#[allow(clippy::manual_non_exhaustive)] // introduced in 1.40, MSRV is 1.36

The matches!() macro is also an important one that I ran into before.

@alex
Copy link
Member

alex commented Oct 9, 2020

clippy::match-like-matches-macro requires matches!() which is new in 1.42

@suyashb95
Copy link
Contributor

suyashb95 commented Oct 13, 2020

@flip1995 do we need to add an msvr attribute to all the lints to denote the targeted Rust version?

@flip1995
Copy link
Member

No, just the lints mentioned here. And then, we can iteratively add it to lints in the future, if a issue for them comes up.

@llogiq
Copy link
Contributor

llogiq commented Oct 17, 2020

Just a heads up: manual_range_contains should have a msrv of 1.35.0.

@Kixunil
Copy link

Kixunil commented Oct 18, 2020

THIS! Just got bitten by matches!() being used in postgres-protocol motivated by clippy.

Can we perhaps also add a lint that will suggest setting the msrv key for projects that don't have it? Explicit latest would turn it off. This would motivate people to at least think about supporting older versions.

Also, oddly enough, Debian stable contains rustc 1.41.1, but oddly enough cargo 1.42.1, so please make sure the version is compared against rustc not against cargo.

@flip1995
Copy link
Member

Besides making this lint only configurable in the config file, we may want to also make it configurable with an attribute, like #[clippy::msrv = "1.42.0"]. This is also done in the cognitive_complexity lint.

Can we perhaps also add a lint that will suggest setting the msrv key for projects that don't have it?

This could be implemented once cargo supports setting it, aka once the bikeshedding in rust-lang/rust#65262 is over and someone implements it ;)

Otherwise I don't think a lint that always triggers on every crate to mention the msrv Clippy configuration is helpful.

suyashb95 added a commit to suyashb95/rust-clippy that referenced this issue Oct 28, 2020
suyashb95 added a commit to suyashb95/rust-clippy that referenced this issue Nov 2, 2020
…#6097

add configuration option for minimum supported rust version
add msrv attribute to  lints listed in rust-lang#6097
generify `LimitStack` for use in other lints
update the `cognitive_complexity` lint to use a `LimitStack<u64>`
update existing tests with the msrv attribute
suyashb95 added a commit to suyashb95/rust-clippy that referenced this issue Nov 17, 2020
…#6097

add configuration option for minimum supported rust version
add msrv attribute to  lints listed in rust-lang#6097
generify `LimitStack` for use in other lints
update the `cognitive_complexity` lint to use a `LimitStack<u64>`
update existing tests with the msrv attribute
@djc
Copy link
Contributor

djc commented Nov 20, 2020

manual_strip depends on methods first added in 1.45.

@taiki-e
Copy link
Member

taiki-e commented Nov 20, 2020

Some have already been mentioned, but I have a list of (13) lints that requires newer compilers:

  • redundant_field_names requires Rust 1.17 due to suggest feature stablized in that version.
  • redundant_static_lifetimes requires Rust 1.17 due to suggest feature stablized in that version.
  • filter_map_next requires Rust 1.30 due to suggest Iterator::find_map.
  • checked_conversions requires Rust 1.34 due to suggest TryFrom.
  • manual_range_contains requires Rust 1.35 due to suggest Range*::contains.
  • use_self requires Rust 1.37 due to suggest Self::Variant on enum.
  • mem_replace_with_default requires Rust 1.40 due to suggest mem::take.
  • manual_non_exhaustive requires Rust 1.40 due to suggest #[non_exhaustive]. Addressed in Add support for minimum supported rust version #6201
  • option_as_ref_deref requires Rust 1.40 due to suggest Option::{as_deref, as_deref_mut}. Addressed in Add support for minimum supported rust version #6201
  • map_unwrap_or requires Rust 1.41 due to suggest Result::{map_or, map_or_else}.
  • match_like_matches_macro requires Rust 1.42 due to suggest matches!. Addressed in Add support for minimum supported rust version #6201
  • manual_strip requires Rust 1.45 due to suggest str::{strip_prefix, strip_suffix}. Addressed in Add support for minimum supported rust version #6201
  • missing_const_for_fn requires Rust 1.46 due to match/if/loop in const fn needs that version.

(I have not checked the versions older than 1.31. I knew about filter_map_next and some lints because I had encountered it before.)

@flip1995
Copy link
Member

cc @suyash458 ^

I would not add them in #6201. This PR is already big enough as it is. Let's add the remaining ones in a separate PR after #6201 is merged.

suyashb95 added a commit to suyashb95/rust-clippy that referenced this issue Nov 24, 2020
…#6097

add configuration option for minimum supported rust version
add msrv attribute to some lints listed in rust-lang#6097
add tests
flip1995 pushed a commit to suyashb95/rust-clippy that referenced this issue Nov 25, 2020
…#6097

add configuration option for minimum supported rust version
add msrv attribute to some lints listed in rust-lang#6097
add tests
flip1995 pushed a commit to suyashb95/rust-clippy that referenced this issue Nov 25, 2020
add configuration option for minimum supported rust version
add msrv attribute to some lints listed in rust-lang#6097
add tests
bors added a commit that referenced this issue Nov 25, 2020
Add support for minimum supported rust version

add configuration option for minimum supported rust version
add msrv attribute to some lints listed in #6097
add tests
addresses #6097

changelog: Add `msrv` configuration to Clippy. This should get a longer changelog entry.
bors added a commit that referenced this issue Nov 25, 2020
update readme for specifying msrv

add some documentation for the `msrv` feature (#6097)
related PR: #6201
bors added a commit that referenced this issue Nov 25, 2020
update readme for specifying msrv

changelog: add some documentation for the `msrv` feature (#6097)
related PR: #6201
suyashb95 added a commit to suyashb95/rust-clippy that referenced this issue Dec 5, 2020
suyashb95 added a commit to suyashb95/rust-clippy that referenced this issue Dec 8, 2020
suyashb95 added a commit to suyashb95/rust-clippy that referenced this issue Dec 9, 2020
suyashb95 added a commit to suyashb95/rust-clippy that referenced this issue Dec 11, 2020
bors added a commit that referenced this issue Dec 11, 2020
Add MSRV to more lints specified in #6097

add MSRV to more lints specified in #6097
add instructions for adding msrv in other lints
update tests

 - [x] `redundant_field_names` requires Rust 1.17 due to suggest feature stablized in that version.
 - [x] `redundant_static_lifetimes` requires Rust 1.17 due to suggest feature stablized in that version.
 - [x] `filter_map_next` requires Rust 1.30 due to suggest `Iterator::find_map`.
 - [x] `checked_conversions` requires Rust 1.34 due to suggest `TryFrom`.
 - [x] `match_like_matches_macro` requires Rust 1.42 due to suggest `matches!`. Addressed in #6201
 - [x] `manual_strip` requires Rust 1.45 due to suggest `str::{strip_prefix, strip_suffix}`. Addressed in #6201
 - [x] `option_as_ref_deref` requires Rust 1.40 due to suggest `Option::{as_deref, as_deref_mut}`. Addressed in #6201
 - [x] `manual_non_exhaustive` requires Rust 1.40 due to suggest `#[non_exhaustive]`. Addressed in #6201
 - [x] `manual_range_contains` requires Rust 1.35 due to suggest `Range*::contains`.
 - [x] `use_self` requires Rust 1.37 due to suggest `Self::Variant on enum`.
 - [x] `mem_replace_with_default` requires Rust 1.40 due to suggest `mem::take`.
 - [x] `map_unwrap_or` requires Rust 1.41 due to suggest `Result::{map_or, map_or_else}`.
 - [x] `missing_const_for_fn` requires Rust 1.46 due to `match/if/loop in const fn` needs that version.

changelog: Add MSRV config to more lints. ^This is now the complete list, AFAWK
@suyashb95
Copy link
Contributor

@flip1995 is this good to be closed now?

@Kixunil
Copy link

Kixunil commented Dec 17, 2020

Hmm, the msrv list in docs mentions only four lints, but there are more in #6097 (comment)

@flip1995
Copy link
Member

@suyash458 The lints also have to be added here:

/// Lint: MANUAL_NON_EXHAUSTIVE, MANUAL_STRIP, OPTION_AS_REF_DEREF, MATCH_LIKE_MATCHES_MACRO. The minimum rust version that the project supports
Can you do this please in a follow up PR? I totally missed that during review. Thanks for pointing that out @Kixunil!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages good-first-issue These issues are a good way to get started with Clippy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants