From 2a5c4e998cdeea3d13e9d57209cfd08ce62aa23b Mon Sep 17 00:00:00 2001 From: Chris Denton Date: Fri, 29 Apr 2022 16:31:26 +0100 Subject: [PATCH 1/7] Revert "Rollup merge of #92519 - ChrisDenton:command-maybe-verbatim, r=dtolnay" This reverts commit ba2d5ede70ed7e37d7f13a397b9d554e2386a19c, reversing changes made to 9b701e7eaa08c2b2ef8c6e59b8b33436cb10aa45. --- library/std/src/sys/windows/process.rs | 67 ++++++++------------ library/std/src/sys/windows/process/tests.rs | 7 +- 2 files changed, 30 insertions(+), 44 deletions(-) diff --git a/library/std/src/sys/windows/process.rs b/library/std/src/sys/windows/process.rs index a13585a02224a..fafd1412d4cb3 100644 --- a/library/std/src/sys/windows/process.rs +++ b/library/std/src/sys/windows/process.rs @@ -19,12 +19,12 @@ use crate::path::{Path, PathBuf}; use crate::ptr; use crate::sys::c; use crate::sys::c::NonZeroDWORD; -use crate::sys::cvt; use crate::sys::fs::{File, OpenOptions}; use crate::sys::handle::Handle; use crate::sys::path; use crate::sys::pipe::{self, AnonPipe}; use crate::sys::stdio; +use crate::sys::{cvt, to_u16s}; use crate::sys_common::mutex::StaticMutex; use crate::sys_common::process::{CommandEnv, CommandEnvs}; use crate::sys_common::{AsInner, IntoInner}; @@ -269,13 +269,8 @@ impl Command { None }; let program = resolve_exe(&self.program, || env::var_os("PATH"), child_paths)?; - // Case insensitive "ends_with" of UTF-16 encoded ".bat" or ".cmd" - let is_batch_file = matches!( - program.len().checked_sub(5).and_then(|i| program.get(i..)), - Some([46, 98 | 66, 97 | 65, 116 | 84, 0] | [46, 99 | 67, 109 | 77, 100 | 68, 0]) - ); let mut cmd_str = - make_command_line(&program, &self.args, self.force_quotes_enabled, is_batch_file)?; + make_command_line(program.as_os_str(), &self.args, self.force_quotes_enabled)?; cmd_str.push(0); // add null terminator // stolen from the libuv code. @@ -314,6 +309,7 @@ impl Command { si.hStdOutput = stdout.as_raw_handle(); si.hStdError = stderr.as_raw_handle(); + let program = to_u16s(&program)?; unsafe { cvt(c::CreateProcessW( program.as_ptr(), @@ -370,7 +366,7 @@ fn resolve_exe<'a>( exe_path: &'a OsStr, parent_paths: impl FnOnce() -> Option, child_paths: Option<&OsStr>, -) -> io::Result> { +) -> io::Result { // Early return if there is no filename. if exe_path.is_empty() || path::has_trailing_slash(exe_path) { return Err(io::const_io_error!( @@ -392,19 +388,19 @@ fn resolve_exe<'a>( if has_exe_suffix { // The application name is a path to a `.exe` file. // Let `CreateProcessW` figure out if it exists or not. - return path::maybe_verbatim(Path::new(exe_path)); + return Ok(exe_path.into()); } let mut path = PathBuf::from(exe_path); // Append `.exe` if not already there. path = path::append_suffix(path, EXE_SUFFIX.as_ref()); - if let Some(path) = program_exists(&path) { + if program_exists(&path) { return Ok(path); } else { // It's ok to use `set_extension` here because the intent is to // remove the extension that was just added. path.set_extension(""); - return path::maybe_verbatim(&path); + return Ok(path); } } else { ensure_no_nuls(exe_path)?; @@ -419,7 +415,7 @@ fn resolve_exe<'a>( if !has_extension { path.set_extension(EXE_EXTENSION); } - program_exists(&path) + if program_exists(&path) { Some(path) } else { None } }); if let Some(path) = result { return Ok(path); @@ -435,10 +431,10 @@ fn search_paths( parent_paths: Paths, child_paths: Option<&OsStr>, mut exists: Exists, -) -> Option> +) -> Option where Paths: FnOnce() -> Option, - Exists: FnMut(PathBuf) -> Option>, + Exists: FnMut(PathBuf) -> Option, { // 1. Child paths // This is for consistency with Rust's historic behaviour. @@ -490,18 +486,17 @@ where } /// Check if a file exists without following symlinks. -fn program_exists(path: &Path) -> Option> { +fn program_exists(path: &Path) -> bool { unsafe { - let path = path::maybe_verbatim(path).ok()?; - // Getting attributes using `GetFileAttributesW` does not follow symlinks - // and it will almost always be successful if the link exists. - // There are some exceptions for special system files (e.g. the pagefile) - // but these are not executable. - if c::GetFileAttributesW(path.as_ptr()) == c::INVALID_FILE_ATTRIBUTES { - None - } else { - Some(path) - } + to_u16s(path) + .map(|path| { + // Getting attributes using `GetFileAttributesW` does not follow symlinks + // and it will almost always be successful if the link exists. + // There are some exceptions for special system files (e.g. the pagefile) + // but these are not executable. + c::GetFileAttributesW(path.as_ptr()) != c::INVALID_FILE_ATTRIBUTES + }) + .unwrap_or(false) } } @@ -735,12 +730,7 @@ enum Quote { // Produces a wide string *without terminating null*; returns an error if // `prog` or any of the `args` contain a nul. -fn make_command_line( - prog: &[u16], - args: &[Arg], - force_quotes: bool, - is_batch_file: bool, -) -> io::Result> { +fn make_command_line(prog: &OsStr, args: &[Arg], force_quotes: bool) -> io::Result> { // Encode the command and arguments in a command line string such // that the spawned process may recover them using CommandLineToArgvW. let mut cmd: Vec = Vec::new(); @@ -749,18 +739,17 @@ fn make_command_line( // need to add an extra pair of quotes surrounding the whole command line // so they are properly passed on to the script. // See issue #91991. + let is_batch_file = Path::new(prog) + .extension() + .map(|ext| ext.eq_ignore_ascii_case("cmd") || ext.eq_ignore_ascii_case("bat")) + .unwrap_or(false); if is_batch_file { cmd.push(b'"' as u16); } - // Always quote the program name so CreateProcess to avoid ambiguity when - // the child process parses its arguments. - // Note that quotes aren't escaped here because they can't be used in arg0. - // But that's ok because file paths can't contain quotes. - cmd.push(b'"' as u16); - cmd.extend_from_slice(prog.strip_suffix(&[0]).unwrap_or(prog)); - cmd.push(b'"' as u16); - + // Always quote the program name so CreateProcess doesn't interpret args as + // part of the name if the binary wasn't found first time. + append_arg(&mut cmd, prog, Quote::Always)?; for arg in args { cmd.push(' ' as u16); let (arg, quote) = match arg { diff --git a/library/std/src/sys/windows/process/tests.rs b/library/std/src/sys/windows/process/tests.rs index cd535afb4a338..be3a0f4ed52a9 100644 --- a/library/std/src/sys/windows/process/tests.rs +++ b/library/std/src/sys/windows/process/tests.rs @@ -3,12 +3,11 @@ use super::Arg; use crate::env; use crate::ffi::{OsStr, OsString}; use crate::process::Command; -use crate::sys::to_u16s; #[test] fn test_raw_args() { let command_line = &make_command_line( - &to_u16s("quoted exe").unwrap(), + OsStr::new("quoted exe"), &[ Arg::Regular(OsString::from("quote me")), Arg::Raw(OsString::from("quote me *not*")), @@ -17,7 +16,6 @@ fn test_raw_args() { Arg::Regular(OsString::from("optional-quotes")), ], false, - false, ) .unwrap(); assert_eq!( @@ -30,10 +28,9 @@ fn test_raw_args() { fn test_make_command_line() { fn test_wrapper(prog: &str, args: &[&str], force_quotes: bool) -> String { let command_line = &make_command_line( - &to_u16s(prog).unwrap(), + OsStr::new(prog), &args.iter().map(|a| Arg::Regular(OsString::from(a))).collect::>(), force_quotes, - false, ) .unwrap(); String::from_utf16(command_line).unwrap() From 5046d91e54c5a2f0f31f2986f5e46313077dd877 Mon Sep 17 00:00:00 2001 From: ricked-twice <39213807+ricked-twice@users.noreply.github.com> Date: Tue, 3 May 2022 21:16:03 +0200 Subject: [PATCH 2/7] Quick fix for #96223. --- .../src/traits/error_reporting/suggestions.rs | 8 ++- src/test/ui/suggestions/issue-96223.rs | 53 +++++++++++++++++++ src/test/ui/suggestions/issue-96223.stderr | 28 ++++++++++ 3 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/suggestions/issue-96223.rs create mode 100644 src/test/ui/suggestions/issue-96223.stderr diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs index b49a5f6578f75..3b98b5a802d18 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs @@ -713,7 +713,13 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { return false; } - let orig_ty = old_pred.self_ty().skip_binder(); + // This is a quick fix to resolve an ICE ([issue#96223](https://github.com/rust-lang/rust/issues/96223)). + // This change should probably be deeper. + // As suggested by @jackh726, `mk_trait_obligation_with_new_self_ty` could take a `Binder<(TraitRef, Ty)> + // instead of `Binder` leading to some changes to its call places. + let Some(orig_ty) = old_pred.self_ty().no_bound_vars() else { + return false; + }; let mk_result = |new_ty| { let obligation = self.mk_trait_obligation_with_new_self_ty(param_env, old_pred, new_ty); diff --git a/src/test/ui/suggestions/issue-96223.rs b/src/test/ui/suggestions/issue-96223.rs new file mode 100644 index 0000000000000..501373d96abec --- /dev/null +++ b/src/test/ui/suggestions/issue-96223.rs @@ -0,0 +1,53 @@ +// Test case for #96223. +// An ICE was triggered because of a failed assertion. +// Thanks to @Manishearth for the minimal test case. + +pub trait Foo<'de>: Sized {} + +pub trait Bar<'a>: 'static { + type Inner: 'a; +} + +pub trait Fubar { + type Bar: for<'a> Bar<'a>; +} + +pub struct Baz(pub T); + +impl<'de, T> Foo<'de> for Baz where T: Foo<'de> {} + +struct Empty; + +impl Dummy for Empty +where + M: Fubar, + for<'de> Baz<>::Inner>: Foo<'de>, +{ +} + +pub trait Dummy +where + M: Fubar, +{ +} + +pub struct EmptyBis<'a>(&'a [u8]); + +impl<'a> Bar<'a> for EmptyBis<'static> { + type Inner = EmptyBis<'a>; +} + +pub struct EmptyMarker; + +impl Fubar for EmptyMarker { + type Bar = EmptyBis<'static>; +} + +fn icey_bounds>(p: &D) {} + +fn trigger_ice() { + let p = Empty; + icey_bounds(&p); //~ERROR +} + +fn main() {} diff --git a/src/test/ui/suggestions/issue-96223.stderr b/src/test/ui/suggestions/issue-96223.stderr new file mode 100644 index 0000000000000..2737ebe517bef --- /dev/null +++ b/src/test/ui/suggestions/issue-96223.stderr @@ -0,0 +1,28 @@ +error[E0277]: the trait bound `for<'de> EmptyBis<'de>: Foo<'_>` is not satisfied + --> $DIR/issue-96223.rs:50:17 + | +LL | icey_bounds(&p); + | ----------- ^^ the trait `for<'de> Foo<'_>` is not implemented for `EmptyBis<'de>` + | | + | required by a bound introduced by this call + | + = help: the trait `Foo<'de>` is implemented for `Baz` +note: required because of the requirements on the impl of `for<'de> Foo<'de>` for `Baz>` + --> $DIR/issue-96223.rs:17:14 + | +LL | impl<'de, T> Foo<'de> for Baz where T: Foo<'de> {} + | ^^^^^^^^ ^^^^^^ +note: required because of the requirements on the impl of `Dummy` for `Empty` + --> $DIR/issue-96223.rs:21:9 + | +LL | impl Dummy for Empty + | ^^^^^^^^ ^^^^^ +note: required by a bound in `icey_bounds` + --> $DIR/issue-96223.rs:46:19 + | +LL | fn icey_bounds>(p: &D) {} + | ^^^^^^^^^^^^^^^^^^ required by this bound in `icey_bounds` + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0277`. From 6db7d1bc83ede0d3acee9d9cdec493ab54a128c0 Mon Sep 17 00:00:00 2001 From: ricked-twice <39213807+ricked-twice@users.noreply.github.com> Date: Tue, 3 May 2022 22:23:30 +0200 Subject: [PATCH 3/7] Taking review hints into account. --- .../src/traits/error_reporting/suggestions.rs | 2 +- src/test/ui/suggestions/issue-96223.rs | 7 +++---- src/test/ui/suggestions/issue-96223.stderr | 8 ++++---- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs index 3b98b5a802d18..66ab794f41c4d 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs @@ -713,7 +713,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { return false; } - // This is a quick fix to resolve an ICE ([issue#96223](https://github.com/rust-lang/rust/issues/96223)). + // This is a quick fix to resolve an ICE (#96223). // This change should probably be deeper. // As suggested by @jackh726, `mk_trait_obligation_with_new_self_ty` could take a `Binder<(TraitRef, Ty)> // instead of `Binder` leading to some changes to its call places. diff --git a/src/test/ui/suggestions/issue-96223.rs b/src/test/ui/suggestions/issue-96223.rs index 501373d96abec..85667bb849bd4 100644 --- a/src/test/ui/suggestions/issue-96223.rs +++ b/src/test/ui/suggestions/issue-96223.rs @@ -1,6 +1,5 @@ -// Test case for #96223. -// An ICE was triggered because of a failed assertion. -// Thanks to @Manishearth for the minimal test case. +// Previously ICEd because we didn't properly track binders in suggestions +// check-fail pub trait Foo<'de>: Sized {} @@ -47,7 +46,7 @@ fn icey_bounds>(p: &D) {} fn trigger_ice() { let p = Empty; - icey_bounds(&p); //~ERROR + icey_bounds(&p); //~ERROR the trait bound } fn main() {} diff --git a/src/test/ui/suggestions/issue-96223.stderr b/src/test/ui/suggestions/issue-96223.stderr index 2737ebe517bef..513725d99628c 100644 --- a/src/test/ui/suggestions/issue-96223.stderr +++ b/src/test/ui/suggestions/issue-96223.stderr @@ -1,5 +1,5 @@ error[E0277]: the trait bound `for<'de> EmptyBis<'de>: Foo<'_>` is not satisfied - --> $DIR/issue-96223.rs:50:17 + --> $DIR/issue-96223.rs:49:17 | LL | icey_bounds(&p); | ----------- ^^ the trait `for<'de> Foo<'_>` is not implemented for `EmptyBis<'de>` @@ -8,17 +8,17 @@ LL | icey_bounds(&p); | = help: the trait `Foo<'de>` is implemented for `Baz` note: required because of the requirements on the impl of `for<'de> Foo<'de>` for `Baz>` - --> $DIR/issue-96223.rs:17:14 + --> $DIR/issue-96223.rs:16:14 | LL | impl<'de, T> Foo<'de> for Baz where T: Foo<'de> {} | ^^^^^^^^ ^^^^^^ note: required because of the requirements on the impl of `Dummy` for `Empty` - --> $DIR/issue-96223.rs:21:9 + --> $DIR/issue-96223.rs:20:9 | LL | impl Dummy for Empty | ^^^^^^^^ ^^^^^ note: required by a bound in `icey_bounds` - --> $DIR/issue-96223.rs:46:19 + --> $DIR/issue-96223.rs:45:19 | LL | fn icey_bounds>(p: &D) {} | ^^^^^^^^^^^^^^^^^^ required by this bound in `icey_bounds` From 096a7ca120d36dce5f68bc54bf3af3888a511d17 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Tue, 12 Apr 2022 15:27:28 +0200 Subject: [PATCH 4/7] Prevent infinite (exponential) recursion in only_used_in_recursion This simplifies the visitor code a bit and prevents checking expressions multiple times. I still think this lint should be removed for now, because its code isn't really tested. --- .../src/only_used_in_recursion.rs | 47 +++++++------------ 1 file changed, 16 insertions(+), 31 deletions(-) diff --git a/src/tools/clippy/clippy_lints/src/only_used_in_recursion.rs b/src/tools/clippy/clippy_lints/src/only_used_in_recursion.rs index 8e61f2347767d..f946fc1119281 100644 --- a/src/tools/clippy/clippy_lints/src/only_used_in_recursion.rs +++ b/src/tools/clippy/clippy_lints/src/only_used_in_recursion.rs @@ -8,7 +8,7 @@ use rustc_errors::Applicability; use rustc_hir::def::{DefKind, Res}; use rustc_hir::def_id::DefId; use rustc_hir::definitions::{DefPathData, DisambiguatedDefPathData}; -use rustc_hir::intravisit::{walk_expr, FnKind, Visitor}; +use rustc_hir::intravisit::{walk_expr, walk_stmt, FnKind, Visitor}; use rustc_hir::{ Arm, Block, Body, Expr, ExprKind, Guard, HirId, ImplicitSelfKind, Let, Local, Pat, PatKind, Path, PathSegment, QPath, Stmt, StmtKind, TyKind, UnOp, @@ -145,7 +145,8 @@ impl<'tcx> LateLintPass<'tcx> for OnlyUsedInRecursion { is_method: matches!(kind, FnKind::Method(..)), has_self, ty_res, - ty_ctx: cx.tcx, + tcx: cx.tcx, + visited_exprs: FxHashSet::default(), }; visitor.visit_expr(&body.value); @@ -206,19 +207,13 @@ impl<'tcx> LateLintPass<'tcx> for OnlyUsedInRecursion { } pub fn is_primitive(ty: Ty<'_>) -> bool { - match ty.kind() { - ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Str => true, - ty::Ref(_, t, _) => is_primitive(*t), - _ => false, - } + let ty = ty.peel_refs(); + ty.is_primitive() || ty.is_str() } pub fn is_array(ty: Ty<'_>) -> bool { - match ty.kind() { - ty::Array(..) | ty::Slice(..) => true, - ty::Ref(_, t, _) => is_array(*t), - _ => false, - } + let ty = ty.peel_refs(); + ty.is_array() || ty.is_array_slice() } /// This builds the graph of side effect. @@ -250,40 +245,30 @@ pub struct SideEffectVisit<'tcx> { is_method: bool, has_self: bool, ty_res: &'tcx TypeckResults<'tcx>, - ty_ctx: TyCtxt<'tcx>, + tcx: TyCtxt<'tcx>, + visited_exprs: FxHashSet, } impl<'tcx> Visitor<'tcx> for SideEffectVisit<'tcx> { - fn visit_block(&mut self, b: &'tcx Block<'tcx>) { - b.stmts.iter().for_each(|stmt| { - self.visit_stmt(stmt); - self.ret_vars.clear(); - }); - walk_list!(self, visit_expr, b.expr); - } - fn visit_stmt(&mut self, s: &'tcx Stmt<'tcx>) { match s.kind { StmtKind::Local(Local { pat, init: Some(init), .. }) => { self.visit_pat_expr(pat, init, false); - self.ret_vars.clear(); }, - StmtKind::Item(i) => { - let item = self.ty_ctx.hir().item(i); - self.visit_item(item); - self.ret_vars.clear(); - }, - StmtKind::Expr(e) | StmtKind::Semi(e) => { - self.visit_expr(e); - self.ret_vars.clear(); + StmtKind::Item(_) | StmtKind::Expr(_) | StmtKind::Semi(_) => { + walk_stmt(self, s); }, StmtKind::Local(_) => {}, } + self.ret_vars.clear(); } fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) { + if !self.visited_exprs.insert(ex.hir_id) { + return; + } match ex.kind { ExprKind::Array(exprs) | ExprKind::Tup(exprs) => { self.ret_vars = exprs @@ -307,7 +292,7 @@ impl<'tcx> Visitor<'tcx> for SideEffectVisit<'tcx> { ExprKind::Match(expr, arms, _) => self.visit_match(expr, arms), // since analysing the closure is not easy, just set all variables in it to side-effect ExprKind::Closure(_, _, body_id, _, _) => { - let body = self.ty_ctx.hir().body(body_id); + let body = self.tcx.hir().body(body_id); self.visit_body(body); let vars = std::mem::take(&mut self.ret_vars); self.add_side_effect(vars); From 7d3bb1dd2d187305a740a5acf11f6960d8bd3912 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Tue, 3 May 2022 16:41:12 +0100 Subject: [PATCH 5/7] Move only_used_in_recursion to nursery --- src/tools/clippy/clippy_lints/src/lib.register_all.rs | 1 - .../clippy/clippy_lints/src/lib.register_complexity.rs | 1 - .../clippy/clippy_lints/src/lib.register_nursery.rs | 1 + .../clippy/clippy_lints/src/only_used_in_recursion.rs | 9 ++++++++- 4 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/tools/clippy/clippy_lints/src/lib.register_all.rs b/src/tools/clippy/clippy_lints/src/lib.register_all.rs index 132a466267626..4cd19d99c0856 100644 --- a/src/tools/clippy/clippy_lints/src/lib.register_all.rs +++ b/src/tools/clippy/clippy_lints/src/lib.register_all.rs @@ -237,7 +237,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(non_expressive_names::JUST_UNDERSCORES_AND_DIGITS), LintId::of(non_octal_unix_permissions::NON_OCTAL_UNIX_PERMISSIONS), LintId::of(octal_escapes::OCTAL_ESCAPES), - LintId::of(only_used_in_recursion::ONLY_USED_IN_RECURSION), LintId::of(open_options::NONSENSICAL_OPEN_OPTIONS), LintId::of(option_env_unwrap::OPTION_ENV_UNWRAP), LintId::of(overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL), diff --git a/src/tools/clippy/clippy_lints/src/lib.register_complexity.rs b/src/tools/clippy/clippy_lints/src/lib.register_complexity.rs index a2ce69065f94d..24658f02b052d 100644 --- a/src/tools/clippy/clippy_lints/src/lib.register_complexity.rs +++ b/src/tools/clippy/clippy_lints/src/lib.register_complexity.rs @@ -66,7 +66,6 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(neg_cmp_op_on_partial_ord::NEG_CMP_OP_ON_PARTIAL_ORD), LintId::of(no_effect::NO_EFFECT), LintId::of(no_effect::UNNECESSARY_OPERATION), - LintId::of(only_used_in_recursion::ONLY_USED_IN_RECURSION), LintId::of(overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL), LintId::of(partialeq_ne_impl::PARTIALEQ_NE_IMPL), LintId::of(precedence::PRECEDENCE), diff --git a/src/tools/clippy/clippy_lints/src/lib.register_nursery.rs b/src/tools/clippy/clippy_lints/src/lib.register_nursery.rs index c2fc67afba517..18904a9453890 100644 --- a/src/tools/clippy/clippy_lints/src/lib.register_nursery.rs +++ b/src/tools/clippy/clippy_lints/src/lib.register_nursery.rs @@ -20,6 +20,7 @@ store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![ LintId::of(mutex_atomic::MUTEX_INTEGER), LintId::of(non_send_fields_in_send_ty::NON_SEND_FIELDS_IN_SEND_TY), LintId::of(nonstandard_macro_braces::NONSTANDARD_MACRO_BRACES), + LintId::of(only_used_in_recursion::ONLY_USED_IN_RECURSION), LintId::of(option_if_let_else::OPTION_IF_LET_ELSE), LintId::of(path_buf_push_overwrite::PATH_BUF_PUSH_OVERWRITE), LintId::of(redundant_pub_crate::REDUNDANT_PUB_CRATE), diff --git a/src/tools/clippy/clippy_lints/src/only_used_in_recursion.rs b/src/tools/clippy/clippy_lints/src/only_used_in_recursion.rs index f946fc1119281..beb812793f81c 100644 --- a/src/tools/clippy/clippy_lints/src/only_used_in_recursion.rs +++ b/src/tools/clippy/clippy_lints/src/only_used_in_recursion.rs @@ -1,6 +1,7 @@ use std::collections::VecDeque; use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::is_lint_allowed; use itertools::{izip, Itertools}; use rustc_ast::{walk_list, Label, Mutability}; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; @@ -33,6 +34,9 @@ declare_clippy_lint! { /// and the assigned variables are also only in recursion, it is useless. /// /// ### Known problems + /// Too many code paths in the linting code are currently untested and prone to produce false + /// positives or are prone to have performance implications. + /// /// In some cases, this would not catch all useless arguments. /// /// ```rust @@ -85,7 +89,7 @@ declare_clippy_lint! { /// ``` #[clippy::version = "1.60.0"] pub ONLY_USED_IN_RECURSION, - complexity, + nursery, "arguments that is only used in recursion can be removed" } declare_lint_pass!(OnlyUsedInRecursion => [ONLY_USED_IN_RECURSION]); @@ -100,6 +104,9 @@ impl<'tcx> LateLintPass<'tcx> for OnlyUsedInRecursion { _: Span, id: HirId, ) { + if is_lint_allowed(cx, ONLY_USED_IN_RECURSION, id) { + return; + } if let FnKind::ItemFn(ident, ..) | FnKind::Method(ident, ..) = kind { let def_id = id.owner.to_def_id(); let data = cx.tcx.def_path(def_id).data; From 169374fdf32b42cb198101a30fb96184d0487d03 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Fri, 13 May 2022 10:29:41 -0400 Subject: [PATCH 6/7] Fix UI test --- src/test/ui/suggestions/issue-96223.stderr | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/ui/suggestions/issue-96223.stderr b/src/test/ui/suggestions/issue-96223.stderr index 513725d99628c..b48e99d389c59 100644 --- a/src/test/ui/suggestions/issue-96223.stderr +++ b/src/test/ui/suggestions/issue-96223.stderr @@ -6,7 +6,6 @@ LL | icey_bounds(&p); | | | required by a bound introduced by this call | - = help: the trait `Foo<'de>` is implemented for `Baz` note: required because of the requirements on the impl of `for<'de> Foo<'de>` for `Baz>` --> $DIR/issue-96223.rs:16:14 | From 97f0d7fdbb17729fa0859eee850ac68930744c7c Mon Sep 17 00:00:00 2001 From: Jack Huey <31162821+jackh726@users.noreply.github.com> Date: Sat, 30 Apr 2022 21:00:57 -0400 Subject: [PATCH 7/7] Revert #92191 Prefer projection candidates instead of param_env candidates for Sized predicates --- .../src/traits/select/candidate_assembly.rs | 4 ---- .../src/traits/select/mod.rs | 20 +++++++---------- .../bugs/issue-89352.base.stderr | 22 +++++++++++++++++++ .../{ => bugs}/issue-89352.rs | 14 +++++++++++- .../generic-associated-types/issue-93262.rs | 21 ++++++++++++++++++ 5 files changed, 64 insertions(+), 17 deletions(-) create mode 100644 src/test/ui/generic-associated-types/bugs/issue-89352.base.stderr rename src/test/ui/generic-associated-types/{ => bugs}/issue-89352.rs (58%) create mode 100644 src/test/ui/generic-associated-types/issue-93262.rs diff --git a/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs b/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs index cf472813e9e32..1bd981a69c226 100644 --- a/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs +++ b/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs @@ -175,9 +175,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { let needs_infer = stack.obligation.predicate.has_infer_types_or_consts(); - let sized_predicate = self.tcx().lang_items().sized_trait() - == Some(stack.obligation.predicate.skip_binder().def_id()); - // If there are STILL multiple candidates, we can further // reduce the list by dropping duplicates -- including // resolving specializations. @@ -186,7 +183,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { while i < candidates.len() { let is_dup = (0..candidates.len()).filter(|&j| i != j).any(|j| { self.candidate_should_be_dropped_in_favor_of( - sized_predicate, &candidates[i], &candidates[j], needs_infer, diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index e69bf6eca90ef..9e2d0657029fd 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -1553,7 +1553,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { /// See the comment for "SelectionCandidate" for more details. fn candidate_should_be_dropped_in_favor_of( &mut self, - sized_predicate: bool, victim: &EvaluatedCandidate<'tcx>, other: &EvaluatedCandidate<'tcx>, needs_infer: bool, @@ -1625,16 +1624,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { // Drop otherwise equivalent non-const fn pointer candidates (FnPointerCandidate { .. }, FnPointerCandidate { is_const: false }) => true, - // If obligation is a sized predicate or the where-clause bound is - // global, prefer the projection or object candidate. See issue - // #50825 and #89352. - (ObjectCandidate(_) | ProjectionCandidate(_), ParamCandidate(ref cand)) => { - sized_predicate || is_global(cand) - } - (ParamCandidate(ref cand), ObjectCandidate(_) | ProjectionCandidate(_)) => { - !(sized_predicate || is_global(cand)) - } - // Global bounds from the where clause should be ignored // here (see issue #50825). Otherwise, we have a where // clause so don't go around looking for impls. @@ -1650,8 +1639,15 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { | BuiltinUnsizeCandidate | TraitUpcastingUnsizeCandidate(_) | BuiltinCandidate { .. } - | TraitAliasCandidate(..), + | TraitAliasCandidate(..) + | ObjectCandidate(_) + | ProjectionCandidate(_), ) => !is_global(cand), + (ObjectCandidate(_) | ProjectionCandidate(_), ParamCandidate(ref cand)) => { + // Prefer these to a global where-clause bound + // (see issue #50825). + is_global(cand) + } ( ImplCandidate(_) | ClosureCandidate diff --git a/src/test/ui/generic-associated-types/bugs/issue-89352.base.stderr b/src/test/ui/generic-associated-types/bugs/issue-89352.base.stderr new file mode 100644 index 0000000000000..81125a7d6f320 --- /dev/null +++ b/src/test/ui/generic-associated-types/bugs/issue-89352.base.stderr @@ -0,0 +1,22 @@ +error[E0308]: mismatched types + --> $DIR/issue-89352.rs:36:13 + | +LL | let a = A::reborrow::<'ai, 's>(self.a.clone()); + | ^ lifetime mismatch + | + = note: expected type `<>::Iter<'s> as Sized>` + found type `<>::Iter<'ai> as Sized>` +note: the lifetime `'s` as defined here... + --> $DIR/issue-89352.rs:35:13 + | +LL | fn iter<'s>(&'s self) -> Self::Iter<'s> { + | ^^ +note: ...does not necessarily outlive the lifetime `'ai` as defined here + --> $DIR/issue-89352.rs:30:6 + | +LL | impl<'ai, T: 'ai, A: GenAssoc> GenAssoc for Wrapper<'ai, T, A> + | ^^^ + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0308`. diff --git a/src/test/ui/generic-associated-types/issue-89352.rs b/src/test/ui/generic-associated-types/bugs/issue-89352.rs similarity index 58% rename from src/test/ui/generic-associated-types/issue-89352.rs rename to src/test/ui/generic-associated-types/bugs/issue-89352.rs index d9c656d5f58a9..a9f0dd0a0b4ad 100644 --- a/src/test/ui/generic-associated-types/issue-89352.rs +++ b/src/test/ui/generic-associated-types/bugs/issue-89352.rs @@ -1,4 +1,16 @@ -// check-pass +// revisions: base nll +// ignore-compare-mode-nll +//[nll] compile-flags: -Z borrowck=mir + +//[base] check-fail +//[nll] check-pass +// known-bug + +// This should pass, but we end up with `A::Iter<'ai>: Sized` for some specific +// `'ai`. We also know that `for<'at> A::Iter<'at>: Sized` from the definition, +// but we prefer param env candidates. We changed this to preference in #92191, +// but this led to unintended consequences (#93262). Suprisingly, this passes +// under NLL. So only a bug in migrate mode. #![feature(generic_associated_types)] diff --git a/src/test/ui/generic-associated-types/issue-93262.rs b/src/test/ui/generic-associated-types/issue-93262.rs new file mode 100644 index 0000000000000..adc6aa8fa1afa --- /dev/null +++ b/src/test/ui/generic-associated-types/issue-93262.rs @@ -0,0 +1,21 @@ +// check-pass + +#![feature(generic_associated_types)] + +pub trait Trait { + type Assoc<'a> where Self: 'a; +} + +pub trait Foo +where + for<'a> T::Assoc<'a>: Clone +{} + +pub struct Type; + +impl Foo for Type +where + for<'a> T::Assoc<'a>: Clone +{} + +fn main() {}