Auto merge of rust-lang#6917 - MysteryJump:fix-manual-unwrap-or-const…

…-fn, r=giraffate

Fix false-positive `manual_unwrap_or` inside const fn

Fixes rust-lang#6898

changelog:  Fix false-positive for manual_unwrap_or in const fn.

clippy_lints/src/manual_unwrap_or.rs

@@ -1,6 +1,6 @@
 use crate::consts::constant_simple;
 use crate::utils;
-use crate::utils::{path_to_local_id, sugg};
+use crate::utils::{in_constant, path_to_local_id, sugg};
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::{indent_of, reindent_multiline, snippet_opt};
 use clippy_utils::ty::is_type_diagnostic_item;
@@ -45,7 +45,7 @@ declare_lint_pass!(ManualUnwrapOr => [MANUAL_UNWRAP_OR]);
 
 impl LateLintPass<'_> for ManualUnwrapOr {
  fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
- if in_external_macro(cx.sess(), expr.span) {
+ if in_external_macro(cx.sess(), expr.span) || in_constant(cx, expr.hir_id) {
  return;
  }
  lint_manual_unwrap_or(cx, expr);

clippy_lints/src/methods/mod.rs

@@ -1837,10 +1837,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
  let item = cx.tcx.hir().expect_item(parent);
  let self_ty = cx.tcx.type_of(item.def_id);
 
- // if this impl block implements a trait, lint in trait definition instead
- if let hir::ItemKind::Impl(hir::Impl { of_trait: Some(_), .. }) = item.kind {
- return;
- }
+ let implements_trait = matches!(item.kind, hir::ItemKind::Impl(hir::Impl { of_trait: Some(_), .. }));
 
  if_chain! {
  if let hir::ImplItemKind::Fn(ref sig, id) = impl_item.kind;
@@ -1855,7 +1852,8 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
  if let Some(first_arg_ty) = first_arg_ty;
 
  then {
- if cx.access_levels.is_exported(impl_item.hir_id()) {
+ // if this impl block implements a trait, lint in trait definition instead
+ if !implements_trait && cx.access_levels.is_exported(impl_item.hir_id()) {
  // check missing trait implementations
  for method_config in &TRAIT_METHODS {
  if name == method_config.method_name &&
@@ -1891,11 +1889,17 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
  item.vis.node.is_pub(),
  self_ty,
  first_arg_ty,
- first_arg.pat.span
+ first_arg.pat.span,
+ false
  );
  }
  }
 
+ // if this impl block implements a trait, lint in trait definition instead
+ if implements_trait {
+ return;
+ }
+
  if let hir::ImplItemKind::Fn(_, _) = impl_item.kind {
  let ret_ty = return_ty(cx, impl_item.hir_id());
 
@@ -1947,7 +1951,8 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
  false,
  self_ty,
  first_arg_ty,
- first_arg_span
+ first_arg_span,
+ true
  );
  }
  }

clippy_lints/src/methods/wrong_self_convention.rs

@@ -1,5 +1,6 @@
 use crate::methods::SelfKind;
 use clippy_utils::diagnostics::span_lint_and_help;
+use clippy_utils::ty::is_copy;
 use rustc_lint::LateContext;
 use rustc_middle::ty::TyS;
 use rustc_span::source_map::Span;
@@ -9,32 +10,40 @@ use super::WRONG_PUB_SELF_CONVENTION;
 use super::WRONG_SELF_CONVENTION;
 
 #[rustfmt::skip]
-const CONVENTIONS: [(&[Convention], &[SelfKind]); 8] = [
+const CONVENTIONS: [(&[Convention], &[SelfKind]); 9] = [
  (&[Convention::Eq("new")], &[SelfKind::No]),
  (&[Convention::StartsWith("as_")], &[SelfKind::Ref, SelfKind::RefMut]),
  (&[Convention::StartsWith("from_")], &[SelfKind::No]),
  (&[Convention::StartsWith("into_")], &[SelfKind::Value]),
  (&[Convention::StartsWith("is_")], &[SelfKind::Ref, SelfKind::No]),
  (&[Convention::Eq("to_mut")], &[SelfKind::RefMut]),
  (&[Convention::StartsWith("to_"), Convention::EndsWith("_mut")], &[SelfKind::RefMut]),
- (&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut")], &[SelfKind::Ref]),
+
+ // Conversion using `to_` can use borrowed (non-Copy types) or owned (Copy types) input.
+ // Source: https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv
+ (&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(false), Convention::IsTraitItem(false)], &[SelfKind::Ref]),
+ (&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(true), Convention::IsTraitItem(false)], &[SelfKind::Value]),
 ];
 
 enum Convention {
  Eq(&'static str),
  StartsWith(&'static str),
  EndsWith(&'static str),
  NotEndsWith(&'static str),
+ IsSelfTypeCopy(bool),
+ IsTraitItem(bool),
 }
 
 impl Convention {
  #[must_use]
- fn check(&self, other: &str) -> bool {
+ fn check<'tcx>(&self, cx: &LateContext<'tcx>, self_ty: &'tcx TyS<'tcx>, other: &str, is_trait_def: bool) -> bool {
  match *self {
  Self::Eq(this) => this == other,
  Self::StartsWith(this) => other.starts_with(this) && this != other,
  Self::EndsWith(this) => other.ends_with(this) && this != other,
- Self::NotEndsWith(this) => !Self::EndsWith(this).check(other),
+ Self::NotEndsWith(this) => !Self::EndsWith(this).check(cx, self_ty, other, is_trait_def),
+ Self::IsSelfTypeCopy(yes) => yes == is_copy(cx, self_ty),
+ Self::IsTraitItem(yes) => yes == is_trait_def,
  }
  }
 }
@@ -46,6 +55,8 @@ impl fmt::Display for Convention {
  Self::StartsWith(this) => this.fmt(f).and_then(|_| '*'.fmt(f)),
  Self::EndsWith(this) => '*'.fmt(f).and_then(|_| this.fmt(f)),
  Self::NotEndsWith(this) => '~'.fmt(f).and_then(|_| this.fmt(f)),
+ Self::IsSelfTypeCopy(is_true) => format!("Self type is{}Copy", if is_true { " " } else { " not " }).fmt(f),
+ Self::IsTraitItem(is_true) => format!("Is{}trait item", if is_true { " " } else { " not " }).fmt(f),
  }
  }
 }
@@ -57,45 +68,44 @@ pub(super) fn check<'tcx>(
  self_ty: &'tcx TyS<'tcx>,
  first_arg_ty: &'tcx TyS<'tcx>,
  first_arg_span: Span,
+ is_trait_item: bool,
 ) {
  let lint = if is_pub {
  WRONG_PUB_SELF_CONVENTION
  } else {
  WRONG_SELF_CONVENTION
  };
- if let Some((conventions, self_kinds)) = &CONVENTIONS
- .iter()
- .find(|(convs, _)| convs.iter().all(|conv| conv.check(item_name)))
- {
+ if let Some((conventions, self_kinds)) = &CONVENTIONS.iter().find(|(convs, _)| {
+ convs
+ .iter()
+ .all(|conv| conv.check(cx, self_ty, item_name, is_trait_item))
+ }) {
  if !self_kinds.iter().any(|k| k.matches(cx, self_ty, first_arg_ty)) {
  let suggestion = {
  if conventions.len() > 1 {
- let special_case = {
- // Don't mention `NotEndsWith` when there is also `StartsWith` convention present
- if conventions.len() == 2 {
- match conventions {
- [Convention::StartsWith(starts_with), Convention::NotEndsWith(_)]
- | [Convention::NotEndsWith(_), Convention::StartsWith(starts_with)] => {
- Some(format!("methods called `{}`", Convention::StartsWith(starts_with)))
- },
- _ => None,
- }
- } else {
- None
- }
- };
-
- if let Some(suggestion) = special_case {
- suggestion
- } else {
- let s = conventions
+ // Don't mention `NotEndsWith` when there is also `StartsWith` convention present
+ let cut_ends_with_conv = conventions
+ .iter()
+ .find(|conv| matches!(conv, Convention::StartsWith(_)))
+ .is_some()
+ && conventions
  .iter()
- .map(|c| format!("`{}`", &c.to_string()))
- .collect::<Vec<_>>()
- .join(" and ");
+ .find(|conv| matches!(conv, Convention::NotEndsWith(_)))
+ .is_some();
+
+ let s = conventions
+ .iter()
+ .filter_map(|conv| {
+ if cut_ends_with_conv && matches!(conv, Convention::NotEndsWith(_)) {
+ None
+ } else {
+ Some(format!("`{}`", &conv.to_string()))
+ }
+ })
+ .collect::<Vec<_>>()
+ .join(" and ");
 
- format!("methods called like this: ({})", &s)
- }
+ format!("methods with the following characteristics: ({})", &s)
  } else {
  format!("methods called `{}`", &conventions[0])
  }

tests/ui/manual_unwrap_or.fixed

@@ -136,4 +136,19 @@ fn result_unwrap_or() {
  };
 }
 
+// don't lint in const fn
+const fn const_fn_option_unwrap_or() {
+ match Some(1) {
+ Some(s) => s,
+ None => 0,
+ };
+}
+
+const fn const_fn_result_unwrap_or() {
+ match Ok::<&str, &str>("Alice") {
+ Ok(s) => s,
+ Err(_) => "Bob",
+ };
+}
+
 fn main() {}

tests/ui/manual_unwrap_or.rs

@@ -175,4 +175,19 @@ fn result_unwrap_or() {
  };
 }
 
+// don't lint in const fn
+const fn const_fn_option_unwrap_or() {
+ match Some(1) {
+ Some(s) => s,
+ None => 0,
+ };
+}
+
+const fn const_fn_result_unwrap_or() {
+ match Ok::<&str, &str>("Alice") {
+ Ok(s) => s,
+ Err(_) => "Bob",
+ };
+}
+
 fn main() {}

tests/ui/wrong_self_convention.rs

@@ -163,3 +163,34 @@ mod issue6307 {
  fn to_mut(&mut self);
  }
 }
+
+mod issue6727 {
+ trait ToU64 {
+ fn to_u64(self) -> u64;
+ }
+
+ #[derive(Clone, Copy)]
+ struct FooCopy;
+
+ impl ToU64 for FooCopy {
+ fn to_u64(self) -> u64 {
+ 1
+ }
+ // trigger lint
+ fn to_u64(&self) -> u64 {
+ 1
+ }
+ }
+
+ struct FooNoCopy;
+
+ impl ToU64 for FooNoCopy {
+ // trigger lint
+ fn to_u64(self) -> u64 {
+ 2
+ }
+ fn to_u64(&self) -> u64 {
+ 2
+ }
+ }
+}