Add support for minimum supported rust version #6201

suyashb95 · 2020-10-21T12:49:37Z

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.

rust-highfive · 2020-10-21T12:49:39Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

suyashb95 · 2020-10-21T15:41:49Z

@flip1995 I was thinking we'd add an MSRV attribute to the lints mentioned in the issue and then check against the MSRV provided in the lint configuration before registering all the lints here. Is that the right way to go?

flip1995 · 2020-10-25T09:35:08Z

The right way to do this is to add a configuration option here:

rust-clippy/clippy_lints/src/utils/conf.rs

Lines 108 to 110 in a675778

    
           define_Conf! { 
        
               /// Lint: BLACKLISTED_NAME. The list of blacklisted names to lint about. NB: `bar` is not here since it has legitimate uses 
        
               (blacklisted_names, "blacklisted_names": Vec<String>, ["foo", "baz", "quux"].iter().map(ToString::to_string).collect()),

In the doc comment above each item, you can list lints, this configuration option applies to, like here:

rust-clippy/clippy_lints/src/utils/conf.rs

Lines 141 to 142 in a675778

    
               /// Lint: BOXED_LOCAL, USELESS_VEC. The maximum size of objects (in bytes) that will be linted. Larger objects are ok on the heap 
        
               (too_large_for_stack, "too_large_for_stack": u64, 200),

Then, you can pass those configuration options to the LintPasses like here:

rust-clippy/clippy_lints/src/lib.rs

Lines 956 to 957 in a675778

    
           let type_complexity_threshold = conf.type_complexity_threshold; 
        
           store.register_late_pass(move || box types::TypeComplexity::new(type_complexity_threshold));

flip1995

Just adding the configuration option and then to the structs, won't do anything. You have to also prevent the lint from triggering, if the MSRV is lower than the applicable MSRV for the specific lints.

Also there should be a way to set the MSRV with an attribute, like #![clippy::msrv = "1.40.0"]. See the cognitive_complexity attribute on how to do this.

clippy_lints/src/manual_non_exhaustive.rs

clippy_lints/src/utils/conf.rs

suyashb95 · 2020-10-29T14:32:11Z

Just adding the configuration option and then to the structs, won't do anything. You have to also prevent the lint from triggering, if the MSRV is lower than the applicable MSRV for the specific lints.

Also there should be a way to set the MSRV with an attribute, like #![clippy::msrv = "1.40.0"]. See the cognitive_complexity attribute on how to do this.

@flip1995 Yep, I'm still working on that. The semver crate is already in the list of dependencies so I'll use that for the version comparisons. I also wanted to check on how we should test these changes

flip1995 · 2020-10-29T14:43:03Z

See the tests/ui-toml directory on how to test configuration options.

bors · 2020-10-29T15:56:33Z

☔ The latest upstream changes (presumably #6227) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

suyashb95 · 2020-10-30T09:42:13Z

Also there should be a way to set the MSRV with an attribute, like #![clippy::msrv = "1.40.0"]. See the cognitive_complexity attribute on how to do this.

@flip1995 From what I saw, the cognitive_complexity lint uses a LimitStack to store attributes in a Vec<u64>. Since it contains a few utility methods to handle attributes, should we make it generic so that it can be used by other lints as well?

clippy_lints/src/utils/attrs.rs

suyashb95 · 2020-11-02T19:35:32Z

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

bors · 2020-11-03T16:23:11Z

☔ The latest upstream changes (presumably #6101) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

clippy_lints/src/manual_non_exhaustive.rs

clippy_lints/src/manual_strip.rs

clippy_lints/src/methods/mod.rs

clippy_lints/src/utils/conf.rs

ghost · 2020-11-07T06:51:46Z

If the config setting or attribute is not present Clippy should function as before i.e. everything should be linted.
Is that how this works? I see the default is 1.0.0...

suyashb95 · 2020-11-07T15:18:15Z

If the config setting or attribute is not present Clippy should function as before i.e. everything should be linted.
Is that how this works? I see the default is 1.0.0...

@mikerite if the attribute is not specified in the configuration a lot of lints will be skipped since the minimum supported version defaults to 1.0.0

suyashb95 · 2020-11-24T18:50:20Z

@suyash458 Still some dogfood errors. I can fix them for you, if you want?

Yeah if it's not a lot of trouble. Unfortunately I don't have access to a Linux environment atm

suyashb95 · 2020-11-24T18:52:29Z

@flip1995 where can we add some documentation about this feature?

flip1995 · 2020-11-24T19:48:20Z

Yeah if it's not a lot of trouble. Unfortunately I don't have access to a Linux environment atm

Alright, will do tomorrow.

where can we add some documentation about this feature?

I thought about doing a blog post in the Rust internals forum, once this hits nightly (so roughly 2 weeks). For the documentation, I would add it to the README.md.

suyashb95 · 2020-11-25T04:30:28Z

Yeah if it's not a lot of trouble. Unfortunately I don't have access to a Linux environment atm

Alright, will do tomorrow.

Thank you so much! :)

where can we add some documentation about this feature?

I thought about doing a blog post in the Rust internals forum, once this hits nightly (so roughly 2 weeks). For the documentation, I would add it to the README.md.

That sounds good, I'll add some docs about this feature to the readme today

flip1995 · 2020-11-25T09:50:12Z

I'll add some docs about this feature to the readme today

Can you do this in a separate PR please? I'll merge this once I fixed the dogfood errors.

suyashb95 · 2020-11-25T09:57:50Z

I'll add some docs about this feature to the readme today

Can you do this in a separate PR please? I'll merge this once I fixed the dogfood errors.

Sure thing!

flip1995 · 2020-11-25T11:04:26Z

clippy_lints/src/utils/mod.rs

-#[macro_export]
 macro_rules! extract_msrv_attr {
    (LateContext) => {
-        fn enter_lint_attrs(&mut self, cx: &rustc_lint::LateContext<'tcx>, attrs: &'tcx [Attribute]) {
-            match get_inner_attr(cx.sess(), attrs, "msrv") {
-                Some(msrv_attr) => {
-                    if let Some(msrv) = msrv_attr.value_str() {
-                        self.msrv = crate::utils::parse_msrv(&msrv.to_string(), Some(cx.sess()), Some(msrv_attr.span));
-                    } else {
-                        cx.sess().span_err(msrv_attr.span, "bad clippy attribute");
-                    }
-                },
-                _ => (),
-            }
-        }
+        extract_msrv_attr!(@LateContext, ());
    };
    (EarlyContext) => {
-        fn enter_lint_attrs(&mut self, cx: &rustc_lint::EarlyContext<'tcx>, attrs: &'tcx [Attribute]) {
-            match get_inner_attr(cx.sess, attrs, "msrv") {
+        extract_msrv_attr!(@EarlyContext);
+    };
+    (@$context:ident$(, $call:tt)?) => {
+        fn enter_lint_attrs(&mut self, cx: &rustc_lint::$context<'tcx>, attrs: &'tcx [rustc_ast::ast::Attribute]) {
+            use $crate::utils::get_unique_inner_attr;


@suyash458 This change might be interesting to you, for learning about macro magic. I did 3 things here:

I removed macro_export, because we don't want this macro to be used outside of Clippy

I added the use $crate::utils::get_unique_inner_attr; statement, so that this function does not have to be imported in very module this is used.

I added an "internal rule". Note that there isn't a thing like an "internal rule" officially, but there is a convention that rules starting with @ should only be used inside the macro definition. With this and the (, $call:tt)? thing I was able to deduplicate the code. The ? operator in a macro means, that this either should appear once or not at all. tt means, that $call is a token tree, so basically everything that is recognized as a token by the Rust lexer is allowed. In combination this makes passing () into the macro possible and can be added anywhere.

@flip1995 Thanks, this is interesting! I tried making this generic using trait objects but quickly ran into a lot of issues with lifetimes

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

flip1995 · 2020-11-25T11:39:25Z

@bors r+ rollup=never p=10

@suyash458 Thanks for all your work on this! What's left to do is:

Write README.md entry about this feature
Write a blog post about this feature
Add the rest of the lints listed in Minimum supported version of Rust (MSVR) config and support #6097 to the msrv configuration.

bors · 2020-11-25T11:39:26Z

📌 Commit d06076c has been approved by flip1995

bors · 2020-11-25T11:39:35Z

⌛ Testing commit d06076c with merge 32c51d2...

bors · 2020-11-25T11:59:57Z

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

suyashb95 · 2020-11-25T12:30:03Z

@bors r+ rollup=never p=10

@suyash458 Thanks for all your work on this! What's left to do is:
1. Write README.md entry about this feature

2. Write a blog post about this feature

3. Add the rest of the lints listed in #6097 to the msrv configuration.

Sounds great! I can work on 1 and 3 in separate PRs

suyashb95 · 2020-11-25T12:43:05Z

clippy_lints/src/utils/attrs.rs

@@ -130,7 +130,9 @@ pub fn get_unique_inner_attr(sess: &Session, attrs: &[ast::Attribute], name: &'s
        match attr.style {
            ast::AttrStyle::Inner if unique_attr.is_none() => unique_attr = Some(attr.clone()),


This is pretty neat, didn't realize match arms can have conditions

Oh, matches are really powerful, take a look at some examples here, to see what you can do with match expressions: https://doc.rust-lang.org/rust-by-example/flow_control/match.html (This section has multiple nested subsections)

update readme for specifying msrv add some documentation for the `msrv` feature (#6097) related PR: #6201

update readme for specifying msrv changelog: add some documentation for the `msrv` feature (#6097) related PR: #6201

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

rust-highfive assigned flip1995 Oct 21, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 21, 2020

flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 25, 2020

flip1995 reviewed Oct 29, 2020

View reviewed changes

clippy_lints/src/manual_non_exhaustive.rs Show resolved Hide resolved

clippy_lints/src/utils/conf.rs Show resolved Hide resolved

suyashb95 force-pushed the master branch from 61f3b6f to 6d22f60 Compare November 2, 2020 19:10

suyashb95 commented Nov 2, 2020

View reviewed changes

clippy_lints/src/utils/attrs.rs Outdated Show resolved Hide resolved

suyashb95 commented Nov 2, 2020

View reviewed changes

clippy_lints/src/utils/attrs.rs Outdated Show resolved Hide resolved

suyashb95 commented Nov 2, 2020

View reviewed changes

clippy_lints/src/utils/attrs.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Nov 2, 2020

suyashb95 force-pushed the master branch 2 times, most recently from f725450 to b0c826f Compare November 3, 2020 03:37