Skip to content

Commit

Permalink
Auto merge of #8802 - smoelius:allow-expect-unwrap-in-tests, r=llogiq
Browse files Browse the repository at this point in the history
Optionally allow `expect` and `unwrap` in tests

This addresses #1015, except it makes the new behavior optional.

The reason for the msrv-related changes is as follows.

Rather than expand `check_methods` list of arguments, it seemed easier to make `check_methods` a method of `Methods`, so that `check_methods` could access `Methods`' fields.

`check_methods` had an `msrv` parameter, which I consequently made a field of `Methods`. But, to avoid adding a lifetime parameter to `Methods`, I made the field type `Option<RustcVersion>` instead of the parameter's existing type, `Option<&RustcVersion>`. This seemed sensible since `RustcVersion` implements `Copy`. But this broke a lot of code that expected an `Option<&RustcVersion>` or `&Option<RustcVersion>`. I changed all of those occurrences to `Option<RustcVersion>`. IMHO, the code is better as a result of these changes, though.

The msrv-related changes are in their own commit to (hopefully) ease review.

Closes #1015

changelog: optionally allow `expect` and `unwrap` in tests

r? `@llogiq`
  • Loading branch information
bors committed May 8, 2022
2 parents 9c78883 + 597f61b commit 4667198
Show file tree
Hide file tree
Showing 47 changed files with 610 additions and 250 deletions.
4 changes: 1 addition & 3 deletions clippy_lints/src/approx_const.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,7 @@ impl ApproxConstant {
let s = s.as_str();
if s.parse::<f64>().is_ok() {
for &(constant, name, min_digits, msrv) in &KNOWN_CONSTS {
if is_approx_const(constant, s, min_digits)
&& msrv.as_ref().map_or(true, |msrv| meets_msrv(self.msrv.as_ref(), msrv))
{
if is_approx_const(constant, s, min_digits) && msrv.map_or(true, |msrv| meets_msrv(self.msrv, msrv)) {
span_lint_and_help(
cx,
APPROX_CONSTANT,
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,7 @@ fn check_empty_line_after_outer_attr(cx: &EarlyContext<'_>, item: &rustc_ast::It

fn check_deprecated_cfg_attr(cx: &EarlyContext<'_>, attr: &Attribute, msrv: Option<RustcVersion>) {
if_chain! {
if meets_msrv(msrv.as_ref(), &msrvs::TOOL_ATTRIBUTES);
if meets_msrv(msrv, msrvs::TOOL_ATTRIBUTES);
// check cfg_attr
if attr.has_name(sym::cfg_attr);
if let Some(items) = attr.meta_item_list();
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/borrow_as_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ impl BorrowAsPtr {

impl<'tcx> LateLintPass<'tcx> for BorrowAsPtr {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if !meets_msrv(self.msrv.as_ref(), &msrvs::BORROW_AS_PTR) {
if !meets_msrv(self.msrv, msrvs::BORROW_AS_PTR) {
return;
}

Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/casts/cast_abs_to_unsigned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ pub(super) fn check(
cast_expr: &Expr<'_>,
cast_from: Ty<'_>,
cast_to: Ty<'_>,
msrv: &Option<RustcVersion>,
msrv: Option<RustcVersion>,
) {
if_chain! {
if meets_msrv(msrv.as_ref(), &msrvs::UNSIGNED_ABS);
if meets_msrv(msrv, msrvs::UNSIGNED_ABS);
if cast_from.is_integral();
if cast_to.is_integral();
if cast_from.is_signed();
Expand Down
6 changes: 3 additions & 3 deletions clippy_lints/src/casts/cast_lossless.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ pub(super) fn check(
cast_op: &Expr<'_>,
cast_from: Ty<'_>,
cast_to: Ty<'_>,
msrv: &Option<RustcVersion>,
msrv: Option<RustcVersion>,
) {
if !should_lint(cx, expr, cast_from, cast_to, msrv) {
return;
Expand Down Expand Up @@ -68,7 +68,7 @@ fn should_lint(
expr: &Expr<'_>,
cast_from: Ty<'_>,
cast_to: Ty<'_>,
msrv: &Option<RustcVersion>,
msrv: Option<RustcVersion>,
) -> bool {
// Do not suggest using From in consts/statics until it is valid to do so (see #2267).
if in_constant(cx, expr.hir_id) {
Expand All @@ -95,7 +95,7 @@ fn should_lint(
};
!is_isize_or_usize(cast_from) && from_nbits < to_nbits
},
(false, true) if matches!(cast_from.kind(), ty::Bool) && meets_msrv(msrv.as_ref(), &msrvs::FROM_BOOL) => true,
(false, true) if matches!(cast_from.kind(), ty::Bool) && meets_msrv(msrv, msrvs::FROM_BOOL) => true,
(_, _) => {
matches!(cast_from.kind(), ty::Float(FloatTy::F32)) && matches!(cast_to.kind(), ty::Float(FloatTy::F64))
},
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/casts/cast_slice_different_sizes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ use rustc_semver::RustcVersion;

use super::CAST_SLICE_DIFFERENT_SIZES;

pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, msrv: &Option<RustcVersion>) {
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, msrv: Option<RustcVersion>) {
// suggestion is invalid if `ptr::slice_from_raw_parts` does not exist
if !meets_msrv(msrv.as_ref(), &msrvs::PTR_SLICE_RAW_PARTS) {
if !meets_msrv(msrv, msrvs::PTR_SLICE_RAW_PARTS) {
return;
}

Expand Down
10 changes: 5 additions & 5 deletions clippy_lints/src/casts/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ impl_lint_pass!(Casts => [
impl<'tcx> LateLintPass<'tcx> for Casts {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if !in_external_macro(cx.sess(), expr.span) {
ptr_as_ptr::check(cx, expr, &self.msrv);
ptr_as_ptr::check(cx, expr, self.msrv);
}

if expr.span.from_expansion() {
Expand Down Expand Up @@ -562,18 +562,18 @@ impl<'tcx> LateLintPass<'tcx> for Casts {
cast_possible_wrap::check(cx, expr, cast_from, cast_to);
cast_precision_loss::check(cx, expr, cast_from, cast_to);
cast_sign_loss::check(cx, expr, cast_expr, cast_from, cast_to);
cast_abs_to_unsigned::check(cx, expr, cast_expr, cast_from, cast_to, &self.msrv);
cast_abs_to_unsigned::check(cx, expr, cast_expr, cast_from, cast_to, self.msrv);
}
cast_lossless::check(cx, expr, cast_expr, cast_from, cast_to, &self.msrv);
cast_lossless::check(cx, expr, cast_expr, cast_from, cast_to, self.msrv);
cast_enum_constructor::check(cx, expr, cast_expr, cast_from);
}
}

cast_ref_to_mut::check(cx, expr);
cast_ptr_alignment::check(cx, expr);
char_lit_as_u8::check(cx, expr);
ptr_as_ptr::check(cx, expr, &self.msrv);
cast_slice_different_sizes::check(cx, expr, &self.msrv);
ptr_as_ptr::check(cx, expr, self.msrv);
cast_slice_different_sizes::check(cx, expr, self.msrv);
}

extract_msrv_attr!(LateContext);
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/casts/ptr_as_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ use rustc_semver::RustcVersion;

use super::PTR_AS_PTR;

pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, msrv: &Option<RustcVersion>) {
if !meets_msrv(msrv.as_ref(), &msrvs::POINTER_CAST) {
pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, msrv: Option<RustcVersion>) {
if !meets_msrv(msrv, msrvs::POINTER_CAST) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/checked_conversions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ impl_lint_pass!(CheckedConversions => [CHECKED_CONVERSIONS]);

impl<'tcx> LateLintPass<'tcx> for CheckedConversions {
fn check_expr(&mut self, cx: &LateContext<'_>, item: &Expr<'_>) {
if !meets_msrv(self.msrv.as_ref(), &msrvs::TRY_FROM) {
if !meets_msrv(self.msrv, msrvs::TRY_FROM) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/from_over_into.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ impl_lint_pass!(FromOverInto => [FROM_OVER_INTO]);

impl<'tcx> LateLintPass<'tcx> for FromOverInto {
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) {
if !meets_msrv(self.msrv.as_ref(), &msrvs::RE_REBALANCING_COHERENCE) {
if !meets_msrv(self.msrv, msrvs::RE_REBALANCING_COHERENCE) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/if_then_some_else_none.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ impl_lint_pass!(IfThenSomeElseNone => [IF_THEN_SOME_ELSE_NONE]);

impl<'tcx> LateLintPass<'tcx> for IfThenSomeElseNone {
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'tcx Expr<'_>) {
if !meets_msrv(self.msrv.as_ref(), &msrvs::BOOL_THEN) {
if !meets_msrv(self.msrv, msrvs::BOOL_THEN) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/index_refutable_slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ impl<'tcx> LateLintPass<'tcx> for IndexRefutableSlice {
if !expr.span.from_expansion() || is_expn_of(expr.span, "if_chain").is_some();
if let Some(IfLet {let_pat, if_then, ..}) = IfLet::hir(cx, expr);
if !is_lint_allowed(cx, INDEX_REFUTABLE_SLICE, expr.hir_id);
if meets_msrv(self.msrv.as_ref(), &msrvs::SLICE_PATTERNS);
if meets_msrv(self.msrv, msrvs::SLICE_PATTERNS);

let found_slices = find_slice_values(cx, let_pat);
if !found_slices.is_empty();
Expand Down
11 changes: 10 additions & 1 deletion clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -582,8 +582,17 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
});

let avoid_breaking_exported_api = conf.avoid_breaking_exported_api;
let allow_expect_in_tests = conf.allow_expect_in_tests;
let allow_unwrap_in_tests = conf.allow_unwrap_in_tests;
store.register_late_pass(move || Box::new(approx_const::ApproxConstant::new(msrv)));
store.register_late_pass(move || Box::new(methods::Methods::new(avoid_breaking_exported_api, msrv)));
store.register_late_pass(move || {
Box::new(methods::Methods::new(
avoid_breaking_exported_api,
msrv,
allow_expect_in_tests,
allow_unwrap_in_tests,
))
});
store.register_late_pass(move || Box::new(matches::Matches::new(msrv)));
store.register_early_pass(move || Box::new(manual_non_exhaustive::ManualNonExhaustiveStruct::new(msrv)));
store.register_late_pass(move || Box::new(manual_non_exhaustive::ManualNonExhaustiveEnum::new(msrv)));
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/manual_bits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ impl_lint_pass!(ManualBits => [MANUAL_BITS]);

impl<'tcx> LateLintPass<'tcx> for ManualBits {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if !meets_msrv(self.msrv.as_ref(), &msrvs::MANUAL_BITS) {
if !meets_msrv(self.msrv, msrvs::MANUAL_BITS) {
return;
}

Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/manual_non_exhaustive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ impl_lint_pass!(ManualNonExhaustiveEnum => [MANUAL_NON_EXHAUSTIVE]);

impl EarlyLintPass for ManualNonExhaustiveStruct {
fn check_item(&mut self, cx: &EarlyContext<'_>, item: &ast::Item) {
if !meets_msrv(self.msrv.as_ref(), &msrvs::NON_EXHAUSTIVE) {
if !meets_msrv(self.msrv, msrvs::NON_EXHAUSTIVE) {
return;
}

Expand Down Expand Up @@ -150,7 +150,7 @@ impl EarlyLintPass for ManualNonExhaustiveStruct {

impl<'tcx> LateLintPass<'tcx> for ManualNonExhaustiveEnum {
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) {
if !meets_msrv(self.msrv.as_ref(), &msrvs::NON_EXHAUSTIVE) {
if !meets_msrv(self.msrv, msrvs::NON_EXHAUSTIVE) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/manual_strip.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ enum StripKind {

impl<'tcx> LateLintPass<'tcx> for ManualStrip {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if !meets_msrv(self.msrv.as_ref(), &msrvs::STR_STRIP_PREFIX) {
if !meets_msrv(self.msrv, msrvs::STR_STRIP_PREFIX) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/map_clone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ impl MapClone {
fn lint_explicit_closure(&self, cx: &LateContext<'_>, replace: Span, root: Span, is_copy: bool) {
let mut applicability = Applicability::MachineApplicable;

let (message, sugg_method) = if is_copy && meets_msrv(self.msrv.as_ref(), &msrvs::ITERATOR_COPIED) {
let (message, sugg_method) = if is_copy && meets_msrv(self.msrv, msrvs::ITERATOR_COPIED) {
("you are using an explicit closure for copying elements", "copied")
} else {
("you are using an explicit closure for cloning elements", "cloned")
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/matches/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
}
if !contains_cfg_arm(cx, expr, ex, arms) {
if source == MatchSource::Normal {
if !(meets_msrv(self.msrv.as_ref(), &msrvs::MATCHES_MACRO)
if !(meets_msrv(self.msrv, msrvs::MATCHES_MACRO)
&& match_like_matches::check_match(cx, expr, ex, arms))
{
match_same_arms::check(cx, arms);
Expand All @@ -685,7 +685,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
match_wild_err_arm::check(cx, ex, arms);
wild_in_or_pats::check(cx, arms);
} else {
if meets_msrv(self.msrv.as_ref(), &msrvs::MATCHES_MACRO) {
if meets_msrv(self.msrv, msrvs::MATCHES_MACRO) {
match_like_matches::check(cx, expr);
}
redundant_pattern_match::check(cx, expr);
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/mem_replace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ impl<'tcx> LateLintPass<'tcx> for MemReplace {
then {
check_replace_option_with_none(cx, src, dest, expr.span);
check_replace_with_uninit(cx, src, dest, expr.span);
if meets_msrv(self.msrv.as_ref(), &msrvs::MEM_TAKE) {
if meets_msrv(self.msrv, msrvs::MEM_TAKE) {
check_replace_with_default(cx, src, dest, expr.span);
}
}
Expand Down
6 changes: 3 additions & 3 deletions clippy_lints/src/methods/cloned_instead_of_copied.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,16 @@ use rustc_span::{sym, Span};

use super::CLONED_INSTEAD_OF_COPIED;

pub fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span: Span, msrv: Option<&RustcVersion>) {
pub fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span: Span, msrv: Option<RustcVersion>) {
let recv_ty = cx.typeck_results().expr_ty_adjusted(recv);
let inner_ty = match recv_ty.kind() {
// `Option<T>` -> `T`
ty::Adt(adt, subst)
if cx.tcx.is_diagnostic_item(sym::Option, adt.did()) && meets_msrv(msrv, &msrvs::OPTION_COPIED) =>
if cx.tcx.is_diagnostic_item(sym::Option, adt.did()) && meets_msrv(msrv, msrvs::OPTION_COPIED) =>
{
subst.type_at(0)
},
_ if is_trait_method(cx, expr, sym::Iterator) && meets_msrv(msrv, &msrvs::ITERATOR_COPIED) => {
_ if is_trait_method(cx, expr, sym::Iterator) && meets_msrv(msrv, msrvs::ITERATOR_COPIED) => {
match get_iterator_item_ty(cx, recv_ty) {
// <T as Iterator>::Item
Some(ty) => ty,
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/methods/err_expect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,15 @@ pub(super) fn check(
cx: &LateContext<'_>,
_expr: &rustc_hir::Expr<'_>,
recv: &rustc_hir::Expr<'_>,
msrv: Option<&RustcVersion>,
msrv: Option<RustcVersion>,
expect_span: Span,
err_span: Span,
) {
if_chain! {
if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), sym::Result);
// Test the version to make sure the lint can be showed (expect_err has been
// introduced in rust 1.17.0 : https://github.com/rust-lang/rust/pull/38982)
if meets_msrv(msrv, &msrvs::EXPECT_ERR);
if meets_msrv(msrv, msrvs::EXPECT_ERR);

// Grabs the `Result<T, E>` type
let result_type = cx.typeck_results().expr_ty(recv);
Expand Down
7 changes: 6 additions & 1 deletion clippy_lints/src/methods/expect_used.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use clippy_utils::diagnostics::span_lint_and_help;
use clippy_utils::is_in_test_function;
use clippy_utils::ty::is_type_diagnostic_item;
use rustc_hir as hir;
use rustc_lint::LateContext;
Expand All @@ -7,7 +8,7 @@ use rustc_span::sym;
use super::EXPECT_USED;

/// lint use of `expect()` for `Option`s and `Result`s
pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>) {
pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>, allow_expect_in_tests: bool) {
let obj_ty = cx.typeck_results().expr_ty(recv).peel_refs();

let mess = if is_type_diagnostic_item(cx, obj_ty, sym::Option) {
Expand All @@ -18,6 +19,10 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr
None
};

if allow_expect_in_tests && is_in_test_function(cx.tcx, expr.hir_id) {
return;
}

if let Some((lint, kind, none_value)) = mess {
span_lint_and_help(
cx,
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/methods/filter_map_next.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ pub(super) fn check<'tcx>(
expr: &'tcx hir::Expr<'_>,
recv: &'tcx hir::Expr<'_>,
arg: &'tcx hir::Expr<'_>,
msrv: Option<&RustcVersion>,
msrv: Option<RustcVersion>,
) {
if is_trait_method(cx, expr, sym::Iterator) {
if !meets_msrv(msrv, &msrvs::ITERATOR_FIND_MAP) {
if !meets_msrv(msrv, msrvs::ITERATOR_FIND_MAP) {
return;
}

Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/methods/is_digit_ascii_radix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ pub(super) fn check<'tcx>(
expr: &'tcx Expr<'_>,
self_arg: &'tcx Expr<'_>,
radix: &'tcx Expr<'_>,
msrv: Option<&RustcVersion>,
msrv: Option<RustcVersion>,
) {
if !meets_msrv(msrv, &msrvs::IS_ASCII_DIGIT) {
if !meets_msrv(msrv, msrvs::IS_ASCII_DIGIT) {
return;
}

Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/methods/map_unwrap_or.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ pub(super) fn check<'tcx>(
recv: &'tcx hir::Expr<'_>,
map_arg: &'tcx hir::Expr<'_>,
unwrap_arg: &'tcx hir::Expr<'_>,
msrv: Option<&RustcVersion>,
msrv: Option<RustcVersion>,
) -> bool {
// lint if the caller of `map()` is an `Option`
let is_option = is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), sym::Option);
let is_result = is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), sym::Result);

if is_result && !meets_msrv(msrv, &msrvs::RESULT_MAP_OR_ELSE) {
if is_result && !meets_msrv(msrv, msrvs::RESULT_MAP_OR_ELSE) {
return false;
}

Expand Down
Loading

0 comments on commit 4667198

Please sign in to comment.