Auto merge of #4018 - rust-lang:or_fn_call_variants, r=oli-obk

Ignore non-const ctor expressions in or_fn_call

Fixes https://github.com/rust-lang/rust-clippy/issues/1338

Should have been fixed by #919, however that focuses on const ctor expressions only, and `.or(Some(local))` isn't const.

This also automatically ignores things like `.or(Some(local.clone())` which we don't actually want to do; I need to figure out what to do here.

changelog: Fixed false positive in [`or_fn_call`] pertaining to enum variant constructors

r? @oli-obk @phansch

3 files changed, 41 insertions, 5 deletions

diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
index 16a83604c21..d8595ea9004 100644
--- a/clippy_lints/src/methods/mod.rs
+++ b/clippy_lints/src/methods/mod.rs
@@ -20,11 +20,11 @@ use syntax::symbol::LocalInternedString;
 use crate::utils::paths;
 use crate::utils::sugg;
 use crate::utils::{
-    get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, implements_trait, in_macro, is_copy, is_expn_of,
-    is_self, is_self_ty, iter_input_pats, last_path_segment, match_path, match_qpath, match_trait_method, match_type,
-    match_var, method_calls, method_chain_args, remove_blocks, return_ty, same_tys, single_segment_path, snippet,
-    snippet_with_applicability, snippet_with_macro_callsite, span_lint, span_lint_and_sugg, span_lint_and_then,
-    span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq,
+    get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, implements_trait, in_macro, is_copy,
+    is_ctor_function, is_expn_of, is_self, is_self_ty, iter_input_pats, last_path_segment, match_path, match_qpath,
+    match_trait_method, match_type, match_var, method_calls, method_chain_args, remove_blocks, return_ty, same_tys,
+    single_segment_path, snippet, snippet_with_applicability, snippet_with_macro_callsite, span_lint,
+    span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq,
 };
 
 declare_clippy_lint! {
@@ -1072,6 +1072,11 @@ fn lint_or_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: Spa
             return;
         }
 
+        // ignore enum and struct constructors
+        if is_ctor_function(cx, &arg) {
+            return;
+        }
+
         // don't lint for constant values
         let owner_def = cx.tcx.hir().get_parent_did_by_hir_id(arg.hir_id);
         let promotable = cx.tcx.rvalue_promotable_map(owner_def).contains(&arg.hir_id.local_id);
diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs
index 31f6658b4e8..e95acd343bf 100644
--- a/clippy_lints/src/utils/mod.rs
+++ b/clippy_lints/src/utils/mod.rs
@@ -742,6 +742,19 @@ pub fn is_copy<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty<'tcx>) -> bool {
     ty.is_copy_modulo_regions(cx.tcx.global_tcx(), cx.param_env, DUMMY_SP)
 }
 
+/// Checks if an expression is constructing a tuple-like enum variant or struct
+pub fn is_ctor_function(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
+    if let ExprKind::Call(ref fun, _) = expr.node {
+        if let ExprKind::Path(ref qp) = fun.node {
+            return matches!(
+                cx.tables.qpath_def(qp, fun.hir_id),
+                def::Def::Variant(..) | def::Def::Ctor(..)
+            );
+        }
+    }
+    false
+}
+
 /// Returns `true` if a pattern is refutable.
 pub fn is_refutable(cx: &LateContext<'_, '_>, pat: &Pat) -> bool {
     fn is_enum_variant(cx: &LateContext<'_, '_>, qpath: &QPath, id: HirId) -> bool {
diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs
index 3a8aedcd659..395271b37eb 100644
--- a/tests/ui/methods.rs
+++ b/tests/ui/methods.rs
@@ -268,3 +268,21 @@ fn main() {
     let opt = Some(0);
     let _ = opt.unwrap();
 }
+
+struct Foo(u8);
+#[rustfmt::skip]
+fn test_or_with_ctors() {
+    let opt = Some(1);
+    let opt_opt = Some(Some(1));
+    // we also test for const promotion, this makes sure we don't hit that
+    let two = 2;
+
+    let _ = opt_opt.unwrap_or(Some(2));
+    let _ = opt_opt.unwrap_or(Some(two));
+    let _ = opt.ok_or(Some(2));
+    let _ = opt.ok_or(Some(two));
+    let _ = opt.ok_or(Foo(2));
+    let _ = opt.ok_or(Foo(two));
+    let _ = opt.or(Some(2));
+    let _ = opt.or(Some(two));
+}