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

Warn if an item coming from more recent version than MSRV is used #12160

Merged
merged 1 commit into from
Jan 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5198,6 +5198,7 @@ Released 2018-09-13
[`implied_bounds_in_impls`]: https://rust-lang.github.io/rust-clippy/master/index.html#implied_bounds_in_impls
[`impossible_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#impossible_comparisons
[`imprecise_flops`]: https://rust-lang.github.io/rust-clippy/master/index.html#imprecise_flops
[`incompatible_msrv`]: https://rust-lang.github.io/rust-clippy/master/index.html#incompatible_msrv
[`inconsistent_digit_grouping`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_digit_grouping
[`inconsistent_struct_constructor`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_struct_constructor
[`incorrect_clone_impl_on_copy_type`]: https://rust-lang.github.io/rust-clippy/master/index.html#incorrect_clone_impl_on_copy_type
Expand Down
11 changes: 11 additions & 0 deletions clippy_config/src/msrvs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use rustc_semver::RustcVersion;
use rustc_session::Session;
use rustc_span::{sym, Symbol};
use serde::Deserialize;
use std::fmt;

macro_rules! msrv_aliases {
($($major:literal,$minor:literal,$patch:literal {
Expand Down Expand Up @@ -58,6 +59,16 @@ pub struct Msrv {
stack: Vec<RustcVersion>,
}

impl fmt::Display for Msrv {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
if let Some(msrv) = self.current() {
write!(f, "{msrv}")
} else {
f.write_str("1.0.0")
}
}
}

impl<'de> Deserialize<'de> for Msrv {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::implicit_saturating_add::IMPLICIT_SATURATING_ADD_INFO,
crate::implicit_saturating_sub::IMPLICIT_SATURATING_SUB_INFO,
crate::implied_bounds_in_impls::IMPLIED_BOUNDS_IN_IMPLS_INFO,
crate::incompatible_msrv::INCOMPATIBLE_MSRV_INFO,
crate::inconsistent_struct_constructor::INCONSISTENT_STRUCT_CONSTRUCTOR_INFO,
crate::index_refutable_slice::INDEX_REFUTABLE_SLICE_INFO,
crate::indexing_slicing::INDEXING_SLICING_INFO,
Expand Down
133 changes: 133 additions & 0 deletions clippy_lints/src/incompatible_msrv.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
use clippy_config::msrvs::Msrv;
use clippy_utils::diagnostics::span_lint;
use rustc_attr::{StabilityLevel, StableSince};
use rustc_data_structures::fx::FxHashMap;
use rustc_hir::{Expr, ExprKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::TyCtxt;
use rustc_semver::RustcVersion;
use rustc_session::impl_lint_pass;
use rustc_span::def_id::DefId;
use rustc_span::Span;

declare_clippy_lint! {
/// ### What it does
///
/// This lint checks that no function newer than the defined MSRV (minimum
/// supported rust version) is used in the crate.
///
/// ### Why is this bad?
///
/// It would prevent the crate to be actually used with the specified MSRV.
///
/// ### Example
/// ```no_run
/// // MSRV of 1.3.0
/// use std::thread::sleep;
/// use std::time::Duration;
///
/// // Sleep was defined in `1.4.0`.
/// sleep(Duration::new(1, 0));
/// ```
///
/// To fix this problem, either increase your MSRV or use another item
/// available in your current MSRV.
#[clippy::version = "1.77.0"]
pub INCOMPATIBLE_MSRV,
Copy link

Choose a reason for hiding this comment

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

I'm a bit hesitant about using "msrv" because (1) it is an acronym and (2) not everyone knows what it means.

Would "incompatible_rust_version` work for a lint name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum... What do you think about incompatible_minimum_rust_version?

Copy link

Choose a reason for hiding this comment

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

No meaningful preference either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll go for the more explicit one then.

suspicious,
Copy link
Member

Choose a reason for hiding this comment

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

So... What do you think about putting this lint in correctness?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's pretty low risk (ie, warning will very likely always be right) to keep it into suspicious. Happened to me a few times and I would have loved to know earlier on that I was doing something wrong.

"ensures that all items used in the crate are available for the current MSRV"
}

pub struct IncompatibleMsrv {
msrv: Msrv,
is_above_msrv: FxHashMap<DefId, RustcVersion>,
}

impl_lint_pass!(IncompatibleMsrv => [INCOMPATIBLE_MSRV]);

impl IncompatibleMsrv {
pub fn new(msrv: Msrv) -> Self {
Self {
msrv,
is_above_msrv: FxHashMap::default(),
}
}

#[allow(clippy::cast_lossless)]
fn get_def_id_version(&mut self, tcx: TyCtxt<'_>, def_id: DefId) -> RustcVersion {
if let Some(version) = self.is_above_msrv.get(&def_id) {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this light caching, it does indeed work! yeehaw 🤠

Copy link
Member Author

Choose a reason for hiding this comment

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

😃

return *version;
}
let version = if let Some(version) = tcx
.lookup_stability(def_id)
.and_then(|stability| match stability.level {
StabilityLevel::Stable {
since: StableSince::Version(version),
..
} => Some(RustcVersion::new(
Comment on lines +61 to +67
Copy link

Choose a reason for hiding this comment

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

In case stable gets stabilized, should we guard this to be exclusive to certain crates? Or should we wait until that wished for day to arrive?

Copy link

Choose a reason for hiding this comment

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

(and yes, when its stabilized, I would love to tell clippy or rustc the minimum version for every dependency and have a lint to catch crate dependencies and not just std/alloc/core)

version.major as _,
version.minor as _,
version.patch as _,
)),
Comment on lines +67 to +71
Copy link
Member

Choose a reason for hiding this comment

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

Meow (does this work?)

Suggested change
} => Some(RustcVersion::new(
version.major as _,
version.minor as _,
version.patch as _,
)),
} => Some(version),

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't work unfortunately:

error[E0308]: `if` and `else` have incompatible types
  --> clippy_lints/src/incompatible_msrv.rs:72:16
   |
62 |            let version = if let Some(version) = tcx
   |  ________________________-
63 | |              .lookup_stability(def_id)
64 | |              .and_then(|stability| match stability.level {
65 | |                  StabilityLevel::Stable {
...  |
71 | |              version
   | |              ------- expected because of this
72 | |          } else if let Some(parent_def_id) = tcx.opt_parent(def_id) {
   | | ________________^
73 | ||             self.get_def_id_version(tcx, parent_def_id)
74 | ||         } else {
75 | ||             RustcVersion::new(1, 0, 0)
76 | ||         };
   | ||         ^
   | ||_________|
   |  |_________`if` and `else` have incompatible types
   |            expected `RustcVersion`, found `rustc_semver::RustcVersion`
   |
   = note: `rustc_semver::RustcVersion` and `RustcVersion` have similar names, but are actually distinct types
note: `rustc_semver::RustcVersion` is defined in crate `rustc_semver`
  --> /home/imperio/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-semver-1.1.0/src/lib.rs:58:1
   |
58 | pub enum RustcVersion {
   | ^^^^^^^^^^^^^^^^^^^^^
note: `RustcVersion` is defined in crate `rustc_session`

_ => None,
}) {
version
} else if let Some(parent_def_id) = tcx.opt_parent(def_id) {
self.get_def_id_version(tcx, parent_def_id)
} else {
RustcVersion::new(1, 0, 0)
};
self.is_above_msrv.insert(def_id, version);
version
}

fn emit_lint_if_under_msrv(&mut self, cx: &LateContext<'_>, def_id: DefId, span: Span) {
if def_id.is_local() {
// We don't check local items since their MSRV is supposed to always be valid.
return;
}
let version = self.get_def_id_version(cx.tcx, def_id);
if self.msrv.meets(version) {
return;
}
self.emit_lint_for(cx, span, version);
}

fn emit_lint_for(&self, cx: &LateContext<'_>, span: Span, version: RustcVersion) {
span_lint(
cx,
INCOMPATIBLE_MSRV,
span,
&format!(
"current MSRV (Minimum Supported Rust Version) is `{}` but this item is stable since `{version}`",
self.msrv
),
);
}
}

impl<'tcx> LateLintPass<'tcx> for IncompatibleMsrv {
extract_msrv_attr!(LateContext);

fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
if self.msrv.current().is_none() {
// If there is no MSRV, then no need to check anything...
return;
}
match expr.kind {
ExprKind::MethodCall(_, _, _, span) => {
if let Some(method_did) = cx.typeck_results().type_dependent_def_id(expr.hir_id) {
self.emit_lint_if_under_msrv(cx, method_did, span);
}
},
ExprKind::Call(call, [_]) => {
if let ExprKind::Path(qpath) = call.kind
&& let Some(path_def_id) = cx.qpath_res(&qpath, call.hir_id).opt_def_id()
{
self.emit_lint_if_under_msrv(cx, path_def_id, call.span);
}
},
_ => {},
}
}
}
3 changes: 3 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ extern crate rustc_abi;
extern crate rustc_arena;
extern crate rustc_ast;
extern crate rustc_ast_pretty;
extern crate rustc_attr;
extern crate rustc_data_structures;
extern crate rustc_driver;
extern crate rustc_errors;
Expand Down Expand Up @@ -153,6 +154,7 @@ mod implicit_return;
mod implicit_saturating_add;
mod implicit_saturating_sub;
mod implied_bounds_in_impls;
mod incompatible_msrv;
mod inconsistent_struct_constructor;
mod index_refutable_slice;
mod indexing_slicing;
Expand Down Expand Up @@ -1094,6 +1096,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
store.register_late_pass(move |_| {
Box::new(thread_local_initializer_can_be_made_const::ThreadLocalInitializerCanBeMadeConst::new(msrv()))
});
store.register_late_pass(move |_| Box::new(incompatible_msrv::IncompatibleMsrv::new(msrv())));
// add lints here, do not remove this comment, it's used in `new_lint`
}

Expand Down
2 changes: 1 addition & 1 deletion tests/ui-toml/min_rust_version/min_rust_version.fixed
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![allow(clippy::redundant_clone, clippy::unnecessary_operation)]
#![allow(clippy::redundant_clone, clippy::unnecessary_operation, clippy::incompatible_msrv)]
#![warn(clippy::manual_non_exhaustive, clippy::borrow_as_ptr, clippy::manual_bits)]

use std::mem::{size_of, size_of_val};
Expand Down
2 changes: 1 addition & 1 deletion tests/ui-toml/min_rust_version/min_rust_version.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![allow(clippy::redundant_clone, clippy::unnecessary_operation)]
#![allow(clippy::redundant_clone, clippy::unnecessary_operation, clippy::incompatible_msrv)]
#![warn(clippy::manual_non_exhaustive, clippy::borrow_as_ptr, clippy::manual_bits)]

use std::mem::{size_of, size_of_val};
Expand Down
23 changes: 23 additions & 0 deletions tests/ui/incompatible_msrv.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#![warn(clippy::incompatible_msrv)]
#![feature(custom_inner_attributes)]
#![clippy::msrv = "1.3.0"]

use std::collections::hash_map::Entry;
use std::collections::HashMap;
use std::thread::sleep;
use std::time::Duration;

fn foo() {
let mut map: HashMap<&str, u32> = HashMap::new();
assert_eq!(map.entry("poneyland").key(), &"poneyland");
//~^ ERROR: is `1.3.0` but this item is stable since `1.10.0`
if let Entry::Vacant(v) = map.entry("poneyland") {
v.into_key();
//~^ ERROR: is `1.3.0` but this item is stable since `1.12.0`
}
// Should warn for `sleep` but not for `Duration` (which was added in `1.3.0`).
sleep(Duration::new(1, 0));
//~^ ERROR: is `1.3.0` but this item is stable since `1.4.0`
}

fn main() {}
23 changes: 23 additions & 0 deletions tests/ui/incompatible_msrv.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
error: current MSRV (Minimum Supported Rust Version) is `1.3.0` but this item is stable since `1.10.0`
--> $DIR/incompatible_msrv.rs:12:39
|
LL | assert_eq!(map.entry("poneyland").key(), &"poneyland");
| ^^^^^
|
= note: `-D clippy::incompatible-msrv` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::incompatible_msrv)]`

error: current MSRV (Minimum Supported Rust Version) is `1.3.0` but this item is stable since `1.12.0`
--> $DIR/incompatible_msrv.rs:15:11
|
LL | v.into_key();
| ^^^^^^^^^^

error: current MSRV (Minimum Supported Rust Version) is `1.3.0` but this item is stable since `1.4.0`
--> $DIR/incompatible_msrv.rs:19:5
|
LL | sleep(Duration::new(1, 0));
| ^^^^^

error: aborting due to 3 previous errors