From 6e6a0d7a8f16945d91a8a22fff7c23ee0ed0f6e2 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 1 Apr 2022 21:02:42 -0400 Subject: [PATCH 1/3] Minimal implementation of `expr_sugg` macro --- clippy_lints/Cargo.toml | 1 + clippy_macros/Cargo.toml | 20 + clippy_macros/src/lib.rs | 15 + clippy_macros/src/sugg.rs | 756 +++++++++++++++++++++++++++++++ clippy_macros/tests/expr_sugg.rs | 113 +++++ clippy_utils/src/lib.rs | 259 +++++++++++ tests/dogfood.rs | 9 +- 7 files changed, 1172 insertions(+), 1 deletion(-) create mode 100644 clippy_macros/Cargo.toml create mode 100644 clippy_macros/src/lib.rs create mode 100644 clippy_macros/src/sugg.rs create mode 100644 clippy_macros/tests/expr_sugg.rs diff --git a/clippy_lints/Cargo.toml b/clippy_lints/Cargo.toml index 9a3f042ffc03..a8c5665c24a9 100644 --- a/clippy_lints/Cargo.toml +++ b/clippy_lints/Cargo.toml @@ -11,6 +11,7 @@ edition = "2021" [dependencies] cargo_metadata = "0.14" clippy_utils = { path = "../clippy_utils" } +clippy_macros = { path = "../clippy_macros" } if_chain = "1.0" itertools = "0.10.1" pulldown-cmark = { version = "0.9", default-features = false } diff --git a/clippy_macros/Cargo.toml b/clippy_macros/Cargo.toml new file mode 100644 index 000000000000..cb184bbf7a65 --- /dev/null +++ b/clippy_macros/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "clippy_macros" +version = "0.1.0" +edition = "2021" +publish = false + +[lib] +proc-macro = true + +[dev-dependencies] +clippy_utils = { path = "../clippy_utils" } + +[dependencies] +proc-macro2 = "1.0.32" +quote = "1.0.10" + +[dependencies.syn] +version = "1" +features = ["parsing", "proc-macro"] +default-features = false diff --git a/clippy_macros/src/lib.rs b/clippy_macros/src/lib.rs new file mode 100644 index 000000000000..e4724c9204d5 --- /dev/null +++ b/clippy_macros/src/lib.rs @@ -0,0 +1,15 @@ +#![feature(concat_idents)] +#![feature(exact_size_is_empty)] +#![feature(explicit_generic_args_with_impl_trait)] +#![feature(let_else)] + +mod sugg; + +extern crate proc_macro; +use proc_macro::TokenStream; +use syn::parse_macro_input; + +#[proc_macro] +pub fn expr_sugg(input: TokenStream) -> TokenStream { + parse_macro_input!(input as sugg::ExprSugg).0.into() +} diff --git a/clippy_macros/src/sugg.rs b/clippy_macros/src/sugg.rs new file mode 100644 index 000000000000..43402faa800b --- /dev/null +++ b/clippy_macros/src/sugg.rs @@ -0,0 +1,756 @@ +use core::cmp; +use core::fmt::Write; +use core::iter; +use proc_macro2::{Literal, Span, TokenStream, TokenTree}; +use quote::{quote, ToTokens}; +use syn::parse::discouraged::Speculative; +use syn::parse::{Nothing, Parse, ParseBuffer, ParseStream, Result}; +use syn::{braced, bracketed, parenthesized, token, Error, Ident, Token}; + +trait Token: token::Token + Parse { + fn write(&self, w: &mut String); +} +impl Token for Ident { + fn write(&self, w: &mut String) { + let _ = write!(w, "{}", self); + } +} +impl Token for Literal { + fn write(&self, w: &mut String) { + let _ = write!(w, "{}", self); + } +} +macro_rules! impl_token { + ($($tokens:tt)*) => { + impl Token for Token![$($tokens)*] { + fn write(&self, w: &mut String) { + w.push_str(concat!($(stringify!($tokens)),*)); + } + } + } +} +impl_token!(+); +impl_token!(+=); +impl_token!(-); +impl_token!(-=); +impl_token!(*); +impl_token!(*=); +impl_token!(/); +impl_token!(/=); +impl_token!(%); +impl_token!(%=); +impl_token!(&); +impl_token!(&&); +impl_token!(&=); +impl_token!(|); +impl_token!(||); +impl_token!(|=); +impl_token!(^); +impl_token!(^=); +impl_token!(<); +impl_token!(<=); +impl_token!(<<); +impl_token!(<<=); +impl_token!(>); +impl_token!(>=); +impl_token!(>>); +impl_token!(>>=); +impl_token!(.); +impl_token!(..); +impl_token!(..=); +impl_token!(!); +impl_token!(!=); +impl_token!(=); +impl_token!(==); +impl_token!(,); +impl_token!(?); +impl_token!(;); +impl_token!(:); +impl_token!(::); +impl_token!(mut); +impl_token!(const); +impl_token!(as); +impl_token!(return); +impl_token!(yield); +impl_token!(box); + +macro_rules! op_precedence { + ($($name:ident => $variant:ident $(($($args:tt)*))?,)*) => { + #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] + enum ExprPrec { + $($name),* + } + impl ToTokens for ExprPrec { + fn to_tokens(&self, tokens: &mut TokenStream) { + match *self { + $(Self::$name => tokens.extend( + quote::quote!(rustc_ast::util::parser::ExprPrecedence::$variant $(($($args)*))?) + ),)* + } + } + } + }; +} +op_precedence! { + Closure => Closure, + Range => Range, + Assign => Assign, + Or => Binary(rustc_ast::BinOpKind::Or), + And => Binary(rustc_ast::BinOpKind::And), + Eq => Binary(rustc_ast::BinOpKind::Eq), + BitOr => Binary(rustc_ast::BinOpKind::BitOr), + BitXor => Binary(rustc_ast::BinOpKind::BitXor), + BitAnd => Binary(rustc_ast::BinOpKind::BitAnd), + Shift => Binary(rustc_ast::BinOpKind::Shl), + Add => Binary(rustc_ast::BinOpKind::Add), + Mul => Binary(rustc_ast::BinOpKind::Mul), + Cast => Cast, + Prefix => Unary, + // Essentially the same as `Suffix`, except method calls will take precedence over calling a field. + Field => Field, + Suffix => Call, +} +impl ExprPrec { + fn merge_with(&self, other: Self) -> Self { + match (*self, other) { + (Self::Suffix | Self::Field, Self::Suffix | Self::Field) => other, + _ => core::cmp::min(*self, other), + } + } +} + +macro_rules! op_position { + ($($name:ident,)*) => { + /// The position in the ast an expression can be placed. Needed to distinguish the rhs and lhs of a binary + /// operator. + #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] + enum ExprPos { + $($name),* + } + impl quote::ToTokens for ExprPos { + fn to_tokens(&self, tokens: &mut proc_macro2::TokenStream) { + let name = match *self { + $(Self::$name => stringify!($name),)* + }; + let name = proc_macro2::Ident::new(name, proc_macro2::Span::call_site()); + tokens.extend(quote::quote!(clippy_utils::_internal::ExprPosition::#name)); + } + } + }; +} +op_position! { + Closure, + AssignRhs, + AssignLhs, + RangeLhs, + RangeRhs, + OrLhs, + OrRhs, + AndLhs, + AndRhs, + EqLhs, + EqRhs, + BitOrLhs, + BitOrRhs, + BitXorLhs, + BitXorRhs, + BitAndLhs, + BitAndRhs, + ShiftLhs, + ShiftRhs, + AddLhs, + AddRhs, + MulLhs, + MulRhs, + Cast, + Prefix, + Suffix, + // Method calls take priority of field access + Callee, +} + +macro_rules! bin_op { + ($($name:ident($prec:ident)[$($op_tt:tt)*],)*) => { + struct BinOp { + prec: ExprPrec, + pos: ExprPos, + } + impl SuggBuilder { + fn parse_bin_op(&mut self, input: ParseStream<'_>, var: Option) -> Option { + use ExprPos::*; + $(if self.consume_op_token_space_prefixed::(input, var, concat_idents!($prec, Lhs)) { + self.next_string.push(' '); + Some(BinOp { + prec: ExprPrec::$prec, + pos: concat_idents!($prec, Rhs), + }) + } else)* { + None + } + } + } + }; +} +bin_op! { + ShlAssign(Assign)[<<=], + ShrAssign(Assign)[>>=], + AddAssign(Assign)[+=], + SubAssign(Assign)[-=], + MulAssign(Assign)[*=], + DivAssign(Assign)[/=], + ModAssign(Assign)[%=], + AndAssign(Assign)[&=], + OrAssign(Assign)[|=], + XorAssign(Assign)[^=], + And(And)[&&], + Or(Or)[&&], + Add(Add)[+], + Sub(Add)[-], + Mul(Mul)[*], + Div(Mul)[/], + Mod(Mul)[%], + Shl(Shift)[<<], + Shr(Shift)[>>], + BitAnd(BitAnd)[&], + BitOr(BitOr)[|], + BitXor(BitXor)[^], + LtEq(Eq)[<=], + GtEq(Eq)[>=], + Lt(Eq)[<], + Gt(Eq)[>], + Eq(Eq)[==], + NotEq(Eq)[!=], + Assign(Assign)[=], + RangeIn(Range)[..=], + RangeEx(Range)[..], +} + +mod kw { + syn::custom_keyword!(path); + syn::custom_keyword!(expr); + syn::custom_keyword!(ty); + syn::custom_keyword!(ident); +} + +macro_rules! var_kind { + ($($name:ident($($path_tt:tt)*),)*) => { + #[derive(Clone, Copy, PartialEq, Eq)] + enum VarKind { Default $(, $name)* } + impl Parse for VarKind { + fn parse(input: ParseStream<'_>) -> Result { + let kind = $(if input.peek($($path_tt)*) { + input.parse::<$($path_tt)*>()?; + Self::$name + } else)* { + Self::Default + }; + input.parse::().map(|_| kind) + } + } + } +} +var_kind! { + Mut(token::Mut), + Expr(kw::expr), + Path(kw::path), + Ty(kw::ty), + Ident(kw::ident), +} + +struct Var { + span: Span, + kind: VarKind, +} +impl Parse for Var { + fn parse(input: ParseStream<'_>) -> Result { + let content; + Ok(Self { + span: braced!(content in input).span, + kind: content.parse()?, + }) + } +} + +fn parse_var(input: ParseStream) -> Option { + parse_var_if(input, |_| true) +} + +fn parse_var_if(input: ParseStream, f: impl FnOnce(&Var) -> bool) -> Option { + let fork = input.fork(); + fork.parse().ok().filter(f).map(|x| { + input.advance_to(&fork); + x + }) +} + +trait Group { + fn content(input: ParseStream<'_>) -> Result; + const OPEN: char; + const CLOSE: char; +} +macro_rules! impl_group { + ($ty:ty, $mac:ident, $open:literal, $close:literal) => { + impl Group for $ty { + fn content(input: ParseStream<'_>) -> Result { + let content; + $mac!(content in input); + Ok((content)) + } + const OPEN: char = $open; + const CLOSE: char = $close; + } + } +} +impl_group!(token::Paren, parenthesized, '(', ')'); +impl_group!(token::Bracket, bracketed, '[', ']'); +impl_group!(token::Brace, braced, '{', '}'); + +enum Output { + Tokens(String), + Var(Span, VarOutput), +} +enum VarOutput { + Mut, + PtrMut, + Ident, + Path, + Ty, + Expr(ExprPos), +} + +type ExprVar = (Span, ExprPos); + +#[derive(Default)] +struct SuggBuilder { + output: Vec, + next_string: String, +} +impl SuggBuilder { + fn consume_token_and(&mut self, input: ParseStream<'_>, pre_write: impl FnOnce(&mut Self)) -> bool { + input.parse::().map_or(false, |t| { + pre_write(self); + t.write(&mut self.next_string); + true + }) + } + + fn require_token(&mut self, input: ParseStream<'_>, msg: &str) -> Result<()> { + self.consume_token::(input).then(|| ()).ok_or_else(|| input.error(msg)) + } + + fn consume_token(&mut self, input: ParseStream<'_>) -> bool { + self.consume_token_and::(input, |_| ()) + } + + fn consume_token_space_prefixed(&mut self, input: ParseStream<'_>) -> bool { + self.consume_token_and::(input, |self_| self_.next_string.push(' ')) + } + + fn consume_op_token(&mut self, input: ParseStream<'_>, var: Option, pos: ExprPos) -> bool { + self.consume_token_and::(input, |self_| self_.push_expr_var(var, pos)) + } + + fn consume_op_token_space_prefixed( + &mut self, + input: ParseStream<'_>, + var: Option, + pos: ExprPos, + ) -> bool { + self.consume_token_and::(input, |self_| { + self_.push_expr_var(var, pos); + self_.next_string.push(' '); + }) + } + + fn consume_group( + &mut self, + input: ParseStream<'_>, + pre_write: impl FnOnce(&mut Self), + parse: impl FnOnce(&mut Self, ParseStream) -> Result<()>, + ) -> Result { + if let Ok(content) = G::content(input) { + pre_write(self); + self.next_string.push(G::OPEN); + parse(self, &content)?; + content.parse::()?; + self.next_string.push(G::CLOSE); + Ok(true) + } else { + Ok(false) + } + } + + fn push_expr_var(&mut self, var: Option, pos: ExprPos) { + if let Some((span, pos2)) = var { + self.push_var(span, VarOutput::Expr(cmp::max(pos, pos2))); + } + } + + fn require( + &mut self, + input: ParseStream<'_>, + f: impl FnOnce(&mut Self, ParseStream<'_>) -> Result>, + msg: &str, + ) -> Result { + match f(self, input) { + Ok(Some(x)) => Ok(x), + Ok(None) => Err(input.error(msg)), + Err(e) => Err(e), + } + } + + fn parse_list( + &mut self, + input: ParseStream<'_>, + f: impl Fn(&mut Self, ParseStream<'_>) -> Result>, + ) -> Result<()> { + while f(self, input)?.is_some() && self.consume_token::(input) { + self.next_string.push(' '); + } + Ok(()) + } + + fn push_var(&mut self, span: Span, var: VarOutput) { + if !self.next_string.is_empty() { + self.output.push(Output::Tokens(self.next_string.clone())); + self.next_string.clear(); + } + self.output.push(Output::Var(span, var)); + } + + fn parse_ty(&mut self, input: ParseStream) -> Result> { + if self.consume_token::(input) { + self.parse_mutability(input); + } else if self.consume_token::(input) { + self.parse_ptr_mutability(input)?; + } else { + return self.parse_ty_body(input); + } + + self.require(input, Self::parse_ty, "expected a type").map(Some) + } + + fn parse_ty_body(&mut self, input: ParseStream) -> Result> { + if let Some(ends_with_ident) = self.parse_path_root(input)? { + if ends_with_ident && self.consume_token::(input) { + self.parse_list(input, Self::parse_ty)?; + self.require_token::]>(input, "expected `>`")?; + } + } else if self.consume_group::( + input, + |_| (), + |self_, input| { + self_.require(&input, Self::parse_ty, "expected a type")?; + if self_.consume_token::(&input) { + self_.next_string.push(' '); + self_.require(&input, Self::parse_expr, "expected an expression")?; + } + Ok(()) + }, + )? { + // Nothing to do + } else if self.consume_group::( + input, + |_| (), + |self_, input| self_.parse_list(&input, Self::parse_ty), + )? { + // Nothing to do + } else if let Some(var) = parse_var(input) { + if matches!(var.kind, VarKind::Ty | VarKind::Default) { + self.push_var(var.span, VarOutput::Ty); + } else { + return Err(Error::new(var.span, "expected a `ty`, `ident` or `path` variable")); + } + } else { + return Ok(None); + } + + Ok(Some(())) + } + + fn parse_path(&mut self, input: ParseStream) -> Result { + let ends_with_ident = if self.consume_token::(input) { + true + } else if self.consume_token::(input) { + self.parse_list(input, Self::parse_ty)?; + self.require_token::]>(input, "expected `>`")?; + false + } else if let Some(var) = parse_var(input) { + if matches!(var.kind, VarKind::Default | VarKind::Path | VarKind::Ident) { + self.push_var(var.span, VarOutput::Path); + } else { + return Err(Error::new(var.span, "expected a `path` or `ident` variable")); + } + true + } else { + return Err(input.error("expected an ident, generic arguments, or a `path` or `ident` variable")); + }; + + if self.consume_token::(input) { + self.parse_path(input) + } else { + Ok(ends_with_ident) + } + } + + fn parse_mutability(&mut self, input: ParseStream<'_>) { + if self.consume_token::(input) { + self.next_string.push(' '); + } else if let Some(var) = parse_var_if(input, |var| var.kind == VarKind::Mut) { + self.push_var(var.span, VarOutput::Mut); + } + } + + fn parse_ptr_mutability(&mut self, input: ParseStream<'_>) -> Result<()> { + if self.consume_token::(input) || self.consume_token::(input) { + self.next_string.push(' '); + } else if let Some(var) = parse_var(input) { + if matches!(var.kind, VarKind::Mut | VarKind::Default) { + self.push_var(var.span, VarOutput::PtrMut); + } else { + return Err(Error::new(var.span, "expected a `mut` variable")); + } + } else { + return Err(input.error("expected `mut`, `const` or a `mut` variable")); + } + Ok(()) + } + + fn parse_expr(&mut self, input: ParseStream<'_>) -> Result> { + self.parse_expr_prefix(input, ExprPos::Closure, ExprPrec::Suffix) + } + + fn parse_expr_prefix(&mut self, input: ParseStream<'_>, pos: ExprPos, prec: ExprPrec) -> Result> { + let (pos, prec) = if self.consume_token::(input) { + self.parse_mutability(input); + (ExprPos::Prefix, prec.merge_with(ExprPrec::Prefix)) + } else if self.consume_token::(input) || self.consume_token::(input) { + (ExprPos::Prefix, prec.merge_with(ExprPrec::Prefix)) + } else if self.consume_token::(input) { + self.next_string.push(' '); + (ExprPos::Prefix, prec.merge_with(ExprPrec::Prefix)) + } else if self.consume_token::(input) || self.consume_token::(input) { + self.next_string.push(' '); + (ExprPos::Closure, ExprPrec::Closure) + } else if self.consume_token::(input) { + self.parse_list(input, Self::parse_closure_arg)?; + self.require_token::(input, "expected `|`")?; + (ExprPos::Closure, ExprPrec::Closure) + } else { + return self.parse_expr_body(input, pos, prec); + }; + + self.require( + input, + |self_, input| self_.parse_expr_prefix(input, pos, prec), + "expected an expression", + ) + .map(Some) + } + + fn parse_expr_body(&mut self, input: ParseStream<'_>, pos: ExprPos, prec: ExprPrec) -> Result> { + if self.consume_token::(input) { + // Nothing to do + } else if self.consume_group::( + input, + |_| (), + |self_, input| self_.parse_list(&input, Self::parse_expr).map(|_| ()), + )? { + // Nothing to do + } else if self.parse_path_root(input)?.is_some() { + // Nothing to do + } else if let Some(var) = parse_var(input) { + return if matches!(var.kind, VarKind::Expr | VarKind::Default) { + self.parse_expr_suffix(input, prec, Some((var.span, pos))).map(Some) + } else { + Err(Error::new(var.span, "expected an `expr`, `ident` or `path` variable")) + }; + } else { + return Ok(None); + }; + + self.parse_expr_suffix(input, prec, None).map(Some) + } + + fn parse_expr_suffix(&mut self, input: ParseStream<'_>, prec: ExprPrec, var: Option) -> Result { + let prec = if let Some(bin_op) = self.parse_bin_op(input, var) { + return self.require( + input, + |self_, input| self_.parse_expr_prefix(input, bin_op.pos, prec.merge_with(bin_op.prec)), + "expected an expression", + ); + } else if self.consume_op_token::(input, var, ExprPos::Cast) + || self.consume_op_token_space_prefixed::(input, var, ExprPos::Cast) + { + self.next_string.push(' '); + self.require(input, Self::parse_ty, "expected a type")?; + prec.merge_with(ExprPrec::Cast) + } else if self.consume_op_token::(input, var, ExprPos::Suffix) { + prec.merge_with(ExprPrec::Suffix) + } else if self.consume_group::( + input, + |self_| self_.push_expr_var(var, ExprPos::Suffix), + |self_, input| self_.require(&input, Self::parse_expr, "expected an expression").map(|_| ()), + )? { + prec.merge_with(ExprPrec::Suffix) + } else if self.consume_group::( + input, + |self_| self_.push_expr_var(var, ExprPos::Callee), + |self_, input| self_.parse_list(&input, Self::parse_expr).map(|_| ()), + )? { + prec.merge_with(ExprPrec::Suffix) + } else if self.consume_op_token::(input, var, ExprPos::Suffix) { + if self.consume_token::(input) { + // Nothing to do + } else if let Some(var) = parse_var(input) { + if matches!(var.kind, VarKind::Default | VarKind::Ident) { + self.push_var(var.span, VarOutput::Ident); + } else { + return Err(Error::new(var.span, "expected an `ident` variable")); + } + } else { + return Err(input.error("expected an identifier or an `ident` variable")); + } + if self.consume_token::(input) { + self.require_token::(input, "expected `<`")?; + self.parse_list(input, Self::parse_ty)?; + self.require_token::]>(input, "expected `>`")?; + } + prec.merge_with(ExprPrec::Field) + } else { + self.push_expr_var(var, ExprPos::Closure); + return Ok(prec); + }; + + self.parse_expr_suffix(input, prec, None) + } + + fn parse_closure_arg(&mut self, input: ParseStream<'_>) -> Result> { + if self.consume_token::(input) { + // Nothing to do + } else if let Some(var) = parse_var(input) { + if matches!(var.kind, VarKind::Default | VarKind::Ident) { + self.push_var(var.span, VarOutput::Ident); + } else { + return Err(Error::new(var.span, "expected an `ident` variable")); + } + } else { + return Ok(None); + } + if self.consume_token::(input) { + self.require(input, Self::parse_ty, "expected a type")?; + } + Ok(Some(())) + } + + fn parse_path_root(&mut self, input: ParseStream<'_>) -> Result> { + let parsed = if self.consume_token::(input) { + true + } else if self.consume_token::(input) { + self.require(input, Self::parse_ty, "expected a type")?; + if self.consume_token_space_prefixed::(input) { + self.next_string.push(' '); + self.require(input, Self::parse_ty, "expected a type")?; + } + self.require_token::]>(input, "expected `>`")?; + self.require_token::(input, "expected `::`")?; + return Ok(Some(self.parse_path(input)?)); + } else if let Some(var) = parse_var_if(input, |var| matches!(var.kind, VarKind::Ident | VarKind::Path)) { + self.push_var(var.span, VarOutput::Path); + true + } else { + false + }; + + if self.consume_token::(input) { + self.parse_path(input).map(Some) + } else { + Ok(parsed.then(|| true)) + } + } + + fn build(&self, prec: ExprPrec, args: &[TokenStream]) -> Result { + let mut args = args.iter(); + let mut body = TokenStream::new(); + for part in &self.output { + match part { + Output::Tokens(x) => body.extend(iter::once(quote!(sugg.push_str(#x);))), + &Output::Var(span, ref kind) => { + let Some(arg) = args.next() else { + return Err(Error::new(span, "no argument given for variable")); + }; + match kind { + VarOutput::Mut => body.extend(iter::once(quote!(match #arg { + rustc_ast::ast::Mutability::Mut => sugg.push_str("mut "), + rustc_ast::ast::Mutability::Not => (), + }))), + VarOutput::PtrMut => body.extend(iter::once(quote!(match #arg { + rustc_ast::ast::Mutability::Mut => sugg.push_str("mut "), + rustc_ast::ast::Mutability::Not => sugg.push_str("const "), + }))), + VarOutput::Expr(pos) => body.extend(iter::once(quote!( + sugg.push_str(&clippy_utils::_internal::snip(cx, #arg, #pos, ctxt, app)); + ))), + _ => body.extend(iter::once(quote!(sugg.push_str(&format!("{}", #arg));))), + } + }, + } + } + if !self.next_string.is_empty() { + let s = &self.next_string; + body.extend(iter::once(quote!(sugg.push_str(#s);))); + } + if prec != ExprPrec::Suffix { + body.extend(iter::once(quote!( + if clippy_utils::_internal::needs_parens(#prec, clippy_utils::_internal::expr_position(cx, e)) { + format!("({})", sugg) + } else { + sugg + } + ))); + } else { + body.extend(iter::once(quote!(sugg))); + } + Ok(quote!(|cx: &rustc_lint::LateContext<'_>, e: &rustc_hir::Expr<'_>, app: &mut rustc_errors::Applicability| { + let ctxt = e.span.ctxt(); + let mut sugg = String::new(); + #body + })) + } +} + +fn split_args(input: ParseStream) -> Result> { + let mut args = Vec::new(); + + loop { + let mut arg = TokenStream::default(); + while !input.peek(Token![,]) { + if let Ok(tt) = input.parse::() { + arg.extend(iter::once(tt)); + } else { + if !arg.is_empty() { + args.push(arg); + } + return Ok(args); + } + } + if arg.is_empty() { + return Err(input.error("expected an argument")); + } + input.parse::()?; + args.push(arg); + } +} + +pub struct ExprSugg(pub TokenStream); +impl Parse for ExprSugg { + fn parse(input: ParseStream<'_>) -> Result { + let mut builder = SuggBuilder::default(); + let prec = builder.require(input, SuggBuilder::parse_expr, "expected an expression")?; + let args = match input.parse::() { + Ok(_) => split_args(input)?, + Err(_) => Vec::new(), + }; + builder.build(prec, &args).map(Self) + } +} diff --git a/clippy_macros/tests/expr_sugg.rs b/clippy_macros/tests/expr_sugg.rs new file mode 100644 index 000000000000..dd6c15b38ab9 --- /dev/null +++ b/clippy_macros/tests/expr_sugg.rs @@ -0,0 +1,113 @@ +#![feature(rustc_private)] + +extern crate clippy_macros; +extern crate rustc_ast; +extern crate rustc_errors; + +mod rustc_lint { + pub struct LateContext<'a>(pub &'a ()); +} + +mod rustc_span { + #[derive(Clone, Copy)] + pub struct SyntaxContext; + + #[derive(Clone, Copy)] + pub struct Span; + impl Span { + pub fn ctxt(self) -> SyntaxContext { + SyntaxContext + } + } +} + +mod rustc_hir { + use crate::clippy_utils::_internal::ExprPosition; + use crate::rustc_span::Span; + + pub struct Expr<'a> { + pub value: &'a str, + pub pos: ExprPosition, + pub span: Span, + } + impl<'a> Expr<'a> { + pub fn new(value: &'a str, pos: ExprPosition) -> Self { + Self { value, pos, span: Span } + } + } +} + +mod clippy_utils { + pub mod _internal { + extern crate clippy_utils; + use crate::rustc_hir::Expr; + use crate::rustc_lint::LateContext; + pub use clippy_utils::_internal::{needs_parens, ExprPosition}; + + pub fn expr_position(_: &LateContext<'_>, e: &Expr<'_>) -> ExprPosition { + e.pos + } + + pub fn snip( + _: &LateContext<'_>, + e: &Expr<'_>, + position: ExprPosition, + _: crate::rustc_span::SyntaxContext, + _app: &mut rustc_errors::Applicability, + ) -> String { + if position > e.pos { + format!("({})", e.value) + } else { + e.value.to_string() + } + } + } +} + +use crate::clippy_utils::_internal::ExprPosition; +use clippy_macros::expr_sugg; +use rustc_ast::Mutability; +use rustc_hir::Expr; + +#[test] +fn test() { + let cx = &rustc_lint::LateContext(&()); + let mut app = rustc_errors::Applicability::MachineApplicable; + let app = &mut app; + let closure = Expr::new("", ExprPosition::Closure); + let closure = &closure; + let prefix = Expr::new("", ExprPosition::Prefix); + let prefix = &prefix; + let callee = Expr::new("", ExprPosition::Callee); + let callee = &callee; + + assert_eq!(expr_sugg!(x)(cx, closure, app), "x"); + + let arg = Expr::new("|| ()", ExprPosition::Closure); + assert_eq!(expr_sugg!(x({}), &arg)(cx, closure, app), "x(|| ())"); + assert_eq!(expr_sugg!(x({}), &arg)(cx, prefix, app), "x(|| ())"); + + let arg = Expr::new("foo", ExprPosition::Suffix); + assert_eq!(expr_sugg!(x + {}, &arg)(cx, closure, app), "x + foo"); + assert_eq!(expr_sugg!(x + {}, &arg)(cx, prefix, app), "(x + foo)"); + + let arg = Expr::new("foo + bar", ExprPosition::AddLhs); + assert_eq!(expr_sugg!({} + x, &arg)(cx, closure, app), "foo + bar + x"); + assert_eq!(expr_sugg!(x + {}, &arg)(cx, closure, app), "x + (foo + bar)"); + + assert_eq!(expr_sugg!(foo.bar)(cx, callee, app), "(foo.bar)"); + + let arg = Expr::new("foo + bar", ExprPosition::AddLhs); + assert_eq!( + expr_sugg!({} as {}, &arg, "u32")(cx, closure, app), + "(foo + bar) as u32" + ); + + let arg = Expr::new("0", ExprPosition::Suffix); + assert_eq!( + expr_sugg!(<&{mut} Foo<{}>>::bar::<*{} u32>({}), Mutability::Not, "&str", Mutability::Not, &arg)( + cx, closure, app + ), + "<&Foo<&str>>::bar::<*const u32>(0)" + ) +} diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 46c5c2eef567..53f6f9b4c628 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -57,6 +57,265 @@ pub mod ty; pub mod usage; pub mod visitors; +pub mod _internal { + use crate::diagnostics::span_lint_and_then; + use crate::get_parent_expr; + use crate::source::snippet_with_context; + use rustc_ast::{util::parser::ExprPrecedence, BinOpKind}; + use rustc_errors::Applicability; + use rustc_hir::{Expr, ExprKind}; + use rustc_lint::{LateContext, Lint}; + use rustc_span::SyntaxContext; + use std::borrow::Cow; + + #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] + pub enum ExprPosition { + Closure, + AssignRhs, + AssignLhs, + RangeLhs, + RangeRhs, + OrLhs, + OrRhs, + AndLhs, + AndRhs, + Let, + EqLhs, + EqRhs, + BitOrLhs, + BitOrRhs, + BitXorLhs, + BitXorRhs, + BitAndLhs, + BitAndRhs, + ShiftLhs, + ShiftRhs, + AddLhs, + AddRhs, + MulLhs, + MulRhs, + Cast, + Prefix, + Suffix, + Callee, + } + + pub trait ExprSugg { + fn into_sugg<'tcx>(self, cx: &LateContext<'tcx>, replace: &'tcx Expr<'tcx>, app: &mut Applicability) -> String; + } + impl FnOnce(&LateContext<'tcx>, &'tcx Expr<'tcx>, &mut Applicability) -> String> ExprSugg for T { + fn into_sugg<'tcx>(self, cx: &LateContext<'tcx>, replace: &'tcx Expr<'tcx>, app: &mut Applicability) -> String { + self(cx, replace, app) + } + } + impl ExprSugg for String { + fn into_sugg(self, _: &LateContext<'_>, _: &Expr<'_>, _: &mut Applicability) -> String { + self + } + } + + pub fn lint_expr_and_sugg<'tcx>( + cx: &LateContext<'tcx>, + lint: &'static Lint, + msg: &str, + expr: &'tcx Expr<'tcx>, + help_msg: &str, + sugg: impl ExprSugg, + mut app: Applicability, + ) { + span_lint_and_then(cx, lint, expr.span, msg, |diag| { + let sugg = sugg.into_sugg(cx, expr, &mut app); + diag.span_suggestion(expr.span, help_msg, sugg, app); + }); + } + + pub struct Sugg { + pub sugg: String, + pub precedence: ExprPrecedence, + } + impl Sugg { + pub fn for_position(self, position: ExprPosition) -> String { + if needs_parens(self.precedence, position) { + format!("({})", self.sugg) + } else { + self.sugg + } + } + + pub fn for_expr_position(self, cx: &LateContext<'_>, e: &Expr<'_>) -> String { + self.for_position(expr_position(cx, e)) + } + + pub fn into_string(self) -> String { + self.sugg + } + } + impl From for String { + fn from(from: Sugg) -> Self { + from.sugg + } + } + + pub fn snip( + cx: &LateContext<'_>, + expr: &Expr<'_>, + position: ExprPosition, + ctxt: SyntaxContext, + app: &mut Applicability, + ) -> String { + let snip = match snippet_with_context(cx, expr.span, ctxt, "(..)", app) { + (Cow::Owned(s), true) => return s, + (Cow::Owned(s), false) if has_enclosing_paren(&s) => return s, + (Cow::Owned(s), false) => s, + (Cow::Borrowed(s), _) => return s.into(), + }; + if needs_parens(expr.precedence(), position) { + format!("({})", snip) + } else { + snip + } + } + + pub fn needs_parens(expr: ExprPrecedence, position: ExprPosition) -> bool { + match expr { + ExprPrecedence::Closure | ExprPrecedence::Ret | ExprPrecedence::Break | ExprPrecedence::Yield => { + position > ExprPosition::Closure + }, + ExprPrecedence::Assign | ExprPrecedence::AssignOp => position > ExprPosition::AssignRhs, + ExprPrecedence::Range => position >= ExprPosition::RangeLhs, + ExprPrecedence::Binary(BinOpKind::Or) => position > ExprPosition::OrLhs, + ExprPrecedence::Binary(BinOpKind::And) => position > ExprPosition::AndLhs, + ExprPrecedence::Let => position > ExprPosition::Let, + ExprPrecedence::Binary( + BinOpKind::Eq | BinOpKind::Ne | BinOpKind::Gt | BinOpKind::Lt | BinOpKind::Ge | BinOpKind::Le, + ) => position > ExprPosition::EqLhs, + ExprPrecedence::Binary(BinOpKind::BitAnd) => position > ExprPosition::BitAndLhs, + ExprPrecedence::Binary(BinOpKind::BitXor) => position > ExprPosition::BitXorLhs, + ExprPrecedence::Binary(BinOpKind::BitOr) => position > ExprPosition::BitOrLhs, + ExprPrecedence::Binary(BinOpKind::Shl | BinOpKind::Shr) => position > ExprPosition::ShiftLhs, + ExprPrecedence::Binary(BinOpKind::Add | BinOpKind::Sub) => position > ExprPosition::AddLhs, + ExprPrecedence::Binary(BinOpKind::Mul | BinOpKind::Div | BinOpKind::Rem) => position > ExprPosition::MulLhs, + ExprPrecedence::Cast | ExprPrecedence::Type => position > ExprPosition::Cast, + ExprPrecedence::Unary | ExprPrecedence::AddrOf | ExprPrecedence::Box => position > ExprPosition::Prefix, + ExprPrecedence::Field => position == ExprPosition::Callee, + ExprPrecedence::Continue + | ExprPrecedence::Call + | ExprPrecedence::MethodCall + | ExprPrecedence::Index + | ExprPrecedence::Try + | ExprPrecedence::InlineAsm + | ExprPrecedence::Mac + | ExprPrecedence::Array + | ExprPrecedence::Repeat + | ExprPrecedence::Tup + | ExprPrecedence::Lit + | ExprPrecedence::Path + | ExprPrecedence::Paren + | ExprPrecedence::If + | ExprPrecedence::While + | ExprPrecedence::ForLoop + | ExprPrecedence::Loop + | ExprPrecedence::Match + | ExprPrecedence::ConstBlock + | ExprPrecedence::Block + | ExprPrecedence::TryBlock + | ExprPrecedence::Struct + | ExprPrecedence::Async + | ExprPrecedence::Await + | ExprPrecedence::Err => false, + } + } + + pub fn expr_position(cx: &LateContext<'_>, e: &Expr<'_>) -> ExprPosition { + #[allow(clippy::enum_glob_use)] + use rustc_hir::BinOpKind::*; + match get_parent_expr(cx, e) { + Some(parent) => match parent.kind { + ExprKind::Call(child, _) if child.hir_id == e.hir_id => ExprPosition::Callee, + ExprKind::Field(..) => ExprPosition::Suffix, + ExprKind::MethodCall(_, [child, ..], _) | ExprKind::Index(child, _) if child.hir_id == e.hir_id => { + ExprPosition::Suffix + }, + ExprKind::Box(_) | ExprKind::AddrOf(..) | ExprKind::Unary(..) => ExprPosition::Prefix, + ExprKind::Cast(..) | ExprKind::Type(..) => ExprPosition::Cast, + ExprKind::Binary(op, child, _) if child.hir_id == e.hir_id => match op.node { + Mul | Div | Rem => ExprPosition::MulLhs, + Add | Sub => ExprPosition::AddLhs, + Shl | Shr => ExprPosition::ShiftLhs, + BitAnd => ExprPosition::BitAndLhs, + BitXor => ExprPosition::BitXorLhs, + BitOr => ExprPosition::BitOrLhs, + Eq | Ne | Gt | Lt | Ge | Le => ExprPosition::EqLhs, + And => ExprPosition::AndLhs, + Or => ExprPosition::OrLhs, + }, + ExprKind::Binary(op, ..) => match op.node { + Mul | Div | Rem => ExprPosition::MulRhs, + Add | Sub => ExprPosition::AddRhs, + Shl | Shr => ExprPosition::ShiftRhs, + BitAnd => ExprPosition::BitAndRhs, + BitXor => ExprPosition::BitXorRhs, + BitOr => ExprPosition::BitOrRhs, + Eq | Ne | Gt | Lt | Ge | Le => ExprPosition::EqRhs, + And => ExprPosition::AndRhs, + Or => ExprPosition::OrRhs, + }, + ExprKind::Let(..) => ExprPosition::Let, + ExprKind::Assign(child, ..) | ExprKind::AssignOp(_, child, _) => { + if child.hir_id == e.hir_id { + ExprPosition::AssignLhs + } else { + ExprPosition::AssignRhs + } + }, + ExprKind::Array(_) + | ExprKind::Block(..) + | ExprKind::Call(..) + | ExprKind::MethodCall(..) + | ExprKind::Closure(..) + | ExprKind::Break(..) + | ExprKind::ConstBlock(..) + | ExprKind::Continue(_) + | ExprKind::DropTemps(_) + | ExprKind::Err + | ExprKind::If(..) + | ExprKind::InlineAsm(_) + | ExprKind::Lit(_) + | ExprKind::Loop(..) + | ExprKind::Match(..) + | ExprKind::Path(_) + | ExprKind::Repeat(..) + | ExprKind::Ret(_) + | ExprKind::Struct(..) + | ExprKind::Tup(_) + | ExprKind::Index(..) + | ExprKind::Yield(..) => ExprPosition::Closure, + }, + None => ExprPosition::Closure, + } + } + + fn has_enclosing_paren(expr: &str) -> bool { + let mut chars = expr.chars(); + if chars.next() == Some('(') { + let mut depth = 1; + for c in &mut chars { + if c == '(' { + depth += 1; + } else if c == ')' { + depth -= 1; + } + if depth == 0 { + break; + } + } + chars.next().is_none() + } else { + false + } + } +} + pub use self::attrs::*; pub use self::hir_utils::{ both, count_eq, eq_expr_value, hash_expr, hash_stmt, over, HirEqInterExpr, SpanlessEq, SpanlessHash, diff --git a/tests/dogfood.rs b/tests/dogfood.rs index 5697e8680cd6..c5da22493f4f 100644 --- a/tests/dogfood.rs +++ b/tests/dogfood.rs @@ -20,7 +20,14 @@ fn dogfood_clippy() { } // "" is the root package - for package in &["", "clippy_dev", "clippy_lints", "clippy_utils", "rustc_tools_util"] { + for package in &[ + "", + "clippy_dev", + "clippy_lints", + "clippy_macros", + "clippy_utils", + "rustc_tools_util", + ] { run_clippy_for_package(package, &["-D", "clippy::all", "-D", "clippy::pedantic"]); } } From 7e5f357ffbb4bb1d09acad25e05c99af297cb29b Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 1 Apr 2022 21:53:41 -0400 Subject: [PATCH 2/3] Use `expr_sugg` on some lints --- clippy_lints/src/from_str_radix_10.rs | 21 +++++++------------ clippy_lints/src/methods/bytes_nth.rs | 19 +++++++---------- clippy_lints/src/methods/iter_count.rs | 18 +++++++--------- clippy_lints/src/methods/iter_nth.rs | 8 +++---- clippy_lints/src/methods/iter_nth_zero.rs | 20 +++++++++++------- .../src/methods/unwrap_or_else_default.rs | 19 ++++++----------- clippy_macros/src/lib.rs | 1 - clippy_utils/src/lib.rs | 8 ++++--- 8 files changed, 48 insertions(+), 66 deletions(-) diff --git a/clippy_lints/src/from_str_radix_10.rs b/clippy_lints/src/from_str_radix_10.rs index 57b075132052..9bfc6accca98 100644 --- a/clippy_lints/src/from_str_radix_10.rs +++ b/clippy_lints/src/from_str_radix_10.rs @@ -1,5 +1,5 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::sugg::Sugg; +use clippy_macros::expr_sugg; +use clippy_utils::_internal::lint_expr_and_sugg; use clippy_utils::ty::is_type_diagnostic_item; use if_chain::if_chain; use rustc_errors::Applicability; @@ -44,7 +44,7 @@ declare_clippy_lint! { declare_lint_pass!(FromStrRadix10 => [FROM_STR_RADIX_10]); impl<'tcx> LateLintPass<'tcx> for FromStrRadix10 { - fn check_expr(&mut self, cx: &LateContext<'tcx>, exp: &Expr<'tcx>) { + fn check_expr(&mut self, cx: &LateContext<'tcx>, exp: &'tcx Expr<'_>) { if_chain! { if let ExprKind::Call(maybe_path, arguments) = &exp.kind; if let ExprKind::Path(QPath::TypeRelative(ty, pathseg)) = &maybe_path.kind; @@ -76,21 +76,14 @@ impl<'tcx> LateLintPass<'tcx> for FromStrRadix10 { &arguments[0] }; - let sugg = Sugg::hir_with_applicability( - cx, - expr, - "", - &mut Applicability::MachineApplicable - ).maybe_par(); - - span_lint_and_sugg( + lint_expr_and_sugg( cx, FROM_STR_RADIX_10, - exp.span, "this call to `from_str_radix` can be replaced with a call to `str::parse`", + exp, "try", - format!("{}.parse::<{}>()", sugg, prim_ty.name_str()), - Applicability::MaybeIncorrect + expr_sugg!({}.parse::<{}>(), expr, prim_ty.name_str()), + Applicability::MaybeIncorrect, ); } } diff --git a/clippy_lints/src/methods/bytes_nth.rs b/clippy_lints/src/methods/bytes_nth.rs index 44857d61fef8..a1d3da9c030f 100644 --- a/clippy_lints/src/methods/bytes_nth.rs +++ b/clippy_lints/src/methods/bytes_nth.rs @@ -1,5 +1,5 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::snippet_with_applicability; +use clippy_macros::expr_sugg; +use clippy_utils::_internal::lint_expr_and_sugg; use clippy_utils::ty::is_type_diagnostic_item; use rustc_errors::Applicability; use rustc_hir::Expr; @@ -8,7 +8,7 @@ use rustc_span::sym; use super::BYTES_NTH; -pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>, recv: &'tcx Expr<'tcx>, n_arg: &'tcx Expr<'tcx>) { +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, recv: &'tcx Expr<'_>, n_arg: &'tcx Expr<'_>) { let ty = cx.typeck_results().expr_ty(recv).peel_refs(); let caller_type = if ty.is_str() { "str" @@ -17,18 +17,13 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>, recv: &'tcx E } else { return; }; - let mut applicability = Applicability::MachineApplicable; - span_lint_and_sugg( + lint_expr_and_sugg( cx, BYTES_NTH, - expr.span, &format!("called `.bytes().nth()` on a `{}`", caller_type), + expr, "try", - format!( - "{}.as_bytes().get({})", - snippet_with_applicability(cx, recv.span, "..", &mut applicability), - snippet_with_applicability(cx, n_arg.span, "..", &mut applicability) - ), - applicability, + expr_sugg!({}.as_bytes().get({}), recv, n_arg), + Applicability::MachineApplicable, ); } diff --git a/clippy_lints/src/methods/iter_count.rs b/clippy_lints/src/methods/iter_count.rs index 052be3d8ee7c..3f6788603fcb 100644 --- a/clippy_lints/src/methods/iter_count.rs +++ b/clippy_lints/src/methods/iter_count.rs @@ -1,6 +1,6 @@ use super::utils::derefs_to_slice; -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::snippet_with_applicability; +use clippy_macros::expr_sugg; +use clippy_utils::_internal::lint_expr_and_sugg; use clippy_utils::ty::is_type_diagnostic_item; use rustc_errors::Applicability; use rustc_hir::Expr; @@ -9,7 +9,7 @@ use rustc_span::sym; use super::ITER_COUNT; -pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>, recv: &'tcx Expr<'tcx>, iter_method: &str) { +pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, recv: &'tcx Expr<'_>, iter_method: &str) { let ty = cx.typeck_results().expr_ty(recv); let caller_type = if derefs_to_slice(cx, recv, ty).is_some() { "slice" @@ -32,17 +32,13 @@ pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>, recv: &'tcx E } else { return; }; - let mut applicability = Applicability::MachineApplicable; - span_lint_and_sugg( + lint_expr_and_sugg( cx, ITER_COUNT, - expr.span, &format!("called `.{}().count()` on a `{}`", iter_method, caller_type), + expr, "try", - format!( - "{}.len()", - snippet_with_applicability(cx, recv.span, "..", &mut applicability), - ), - applicability, + expr_sugg!({}.len(), recv), + Applicability::MachineApplicable, ); } diff --git a/clippy_lints/src/methods/iter_nth.rs b/clippy_lints/src/methods/iter_nth.rs index 80ca4c94219f..758704b4c143 100644 --- a/clippy_lints/src/methods/iter_nth.rs +++ b/clippy_lints/src/methods/iter_nth.rs @@ -10,10 +10,10 @@ use super::ITER_NTH; pub(super) fn check<'tcx>( cx: &LateContext<'tcx>, - expr: &hir::Expr<'_>, - iter_recv: &'tcx hir::Expr<'tcx>, - nth_recv: &hir::Expr<'_>, - nth_arg: &hir::Expr<'_>, + expr: &'tcx hir::Expr<'_>, + iter_recv: &'tcx hir::Expr<'_>, + nth_recv: &'tcx hir::Expr<'_>, + nth_arg: &'tcx hir::Expr<'_>, is_mut: bool, ) { let mut_str = if is_mut { "_mut" } else { "" }; diff --git a/clippy_lints/src/methods/iter_nth_zero.rs b/clippy_lints/src/methods/iter_nth_zero.rs index 68d906c3ea39..8bf1f0e1d994 100644 --- a/clippy_lints/src/methods/iter_nth_zero.rs +++ b/clippy_lints/src/methods/iter_nth_zero.rs @@ -1,7 +1,7 @@ +use clippy_macros::expr_sugg; +use clippy_utils::_internal::lint_expr_and_sugg; use clippy_utils::consts::{constant, Constant}; -use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::is_trait_method; -use clippy_utils::source::snippet_with_applicability; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir as hir; @@ -10,20 +10,24 @@ use rustc_span::sym; use super::ITER_NTH_ZERO; -pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>, arg: &hir::Expr<'_>) { +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx hir::Expr<'_>, + recv: &'tcx hir::Expr<'_>, + arg: &'tcx hir::Expr<'_>, +) { if_chain! { if is_trait_method(cx, expr, sym::Iterator); if let Some((Constant::Int(0), _)) = constant(cx, cx.typeck_results(), arg); then { - let mut applicability = Applicability::MachineApplicable; - span_lint_and_sugg( + lint_expr_and_sugg( cx, ITER_NTH_ZERO, - expr.span, "called `.nth(0)` on a `std::iter::Iterator`, when `.next()` is equivalent", + expr, "try calling `.next()` instead of `.nth(0)`", - format!("{}.next()", snippet_with_applicability(cx, recv.span, "..", &mut applicability)), - applicability, + expr_sugg!({}.next(), recv), + Applicability::MachineApplicable, ); } } diff --git a/clippy_lints/src/methods/unwrap_or_else_default.rs b/clippy_lints/src/methods/unwrap_or_else_default.rs index f3af281d6cac..46cbc8d2cae3 100644 --- a/clippy_lints/src/methods/unwrap_or_else_default.rs +++ b/clippy_lints/src/methods/unwrap_or_else_default.rs @@ -1,10 +1,8 @@ //! Lint for `some_result_or_option.unwrap_or_else(Default::default)` use super::UNWRAP_OR_ELSE_DEFAULT; -use clippy_utils::{ - diagnostics::span_lint_and_sugg, is_default_equivalent_call, source::snippet_with_applicability, - ty::is_type_diagnostic_item, -}; +use clippy_macros::expr_sugg; +use clippy_utils::{_internal::lint_expr_and_sugg, is_default_equivalent_call, ty::is_type_diagnostic_item}; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_lint::LateContext; @@ -27,19 +25,14 @@ pub(super) fn check<'tcx>( if is_option || is_result; if is_default_equivalent_call(cx, u_arg); then { - let mut applicability = Applicability::MachineApplicable; - - span_lint_and_sugg( + lint_expr_and_sugg( cx, UNWRAP_OR_ELSE_DEFAULT, - expr.span, "use of `.unwrap_or_else(..)` to construct default value", + expr, "try", - format!( - "{}.unwrap_or_default()", - snippet_with_applicability(cx, recv.span, "..", &mut applicability) - ), - applicability, + expr_sugg!({}.unwrap_or_default(), recv), + Applicability::MachineApplicable, ); } } diff --git a/clippy_macros/src/lib.rs b/clippy_macros/src/lib.rs index e4724c9204d5..951917b874cb 100644 --- a/clippy_macros/src/lib.rs +++ b/clippy_macros/src/lib.rs @@ -1,6 +1,5 @@ #![feature(concat_idents)] #![feature(exact_size_is_empty)] -#![feature(explicit_generic_args_with_impl_trait)] #![feature(let_else)] mod sugg; diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 53f6f9b4c628..fa8cf186c396 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -178,9 +178,11 @@ pub mod _internal { pub fn needs_parens(expr: ExprPrecedence, position: ExprPosition) -> bool { match expr { - ExprPrecedence::Closure | ExprPrecedence::Ret | ExprPrecedence::Break | ExprPrecedence::Yield => { - position > ExprPosition::Closure - }, + ExprPrecedence::Closure + | ExprPrecedence::Ret + | ExprPrecedence::Break + | ExprPrecedence::Yield + | ExprPrecedence::Yeet => position > ExprPosition::Closure, ExprPrecedence::Assign | ExprPrecedence::AssignOp => position > ExprPosition::AssignRhs, ExprPrecedence::Range => position >= ExprPosition::RangeLhs, ExprPrecedence::Binary(BinOpKind::Or) => position > ExprPosition::OrLhs, From a47fe3b5fddd148b9a5934a8a70a5e70e3d630cc Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 1 Apr 2022 22:33:36 -0400 Subject: [PATCH 3/3] Dogfood fixes --- clippy_macros/src/sugg.rs | 94 ++++++++++++++++++++++----------------- 1 file changed, 52 insertions(+), 42 deletions(-) diff --git a/clippy_macros/src/sugg.rs b/clippy_macros/src/sugg.rs index 43402faa800b..8d4672bc63d4 100644 --- a/clippy_macros/src/sugg.rs +++ b/clippy_macros/src/sugg.rs @@ -111,10 +111,10 @@ op_precedence! { Suffix => Call, } impl ExprPrec { - fn merge_with(&self, other: Self) -> Self { - match (*self, other) { + fn merge_with(self, other: Self) -> Self { + match (self, other) { (Self::Suffix | Self::Field, Self::Suffix | Self::Field) => other, - _ => core::cmp::min(*self, other), + _ => core::cmp::min(self, other), } } } @@ -178,7 +178,11 @@ macro_rules! bin_op { impl SuggBuilder { fn parse_bin_op(&mut self, input: ParseStream<'_>, var: Option) -> Option { use ExprPos::*; - $(if self.consume_op_token_space_prefixed::(input, var, concat_idents!($prec, Lhs)) { + $(if self.consume_op_token_space_prefixed::( + input, + var, + concat_idents!($prec, Lhs) + ) { self.next_string.push(' '); Some(BinOp { prec: ExprPrec::$prec, @@ -335,7 +339,9 @@ impl SuggBuilder { } fn require_token(&mut self, input: ParseStream<'_>, msg: &str) -> Result<()> { - self.consume_token::(input).then(|| ()).ok_or_else(|| input.error(msg)) + self.consume_token::(input) + .then(|| ()) + .ok_or_else(|| input.error(msg)) } fn consume_token(&mut self, input: ParseStream<'_>) -> bool { @@ -430,6 +436,7 @@ impl SuggBuilder { self.require(input, Self::parse_ty, "expected a type").map(Some) } + #[allow(clippy::blocks_in_if_conditions)] fn parse_ty_body(&mut self, input: ParseStream) -> Result> { if let Some(ends_with_ident) = self.parse_path_root(input)? { if ends_with_ident && self.consume_token::(input) { @@ -440,19 +447,17 @@ impl SuggBuilder { input, |_| (), |self_, input| { - self_.require(&input, Self::parse_ty, "expected a type")?; - if self_.consume_token::(&input) { + self_.require(input, Self::parse_ty, "expected a type")?; + if self_.consume_token::(input) { self_.next_string.push(' '); - self_.require(&input, Self::parse_expr, "expected an expression")?; + self_.require(input, Self::parse_expr, "expected an expression")?; } Ok(()) }, - )? { - // Nothing to do - } else if self.consume_group::( + )? || self.consume_group::( input, |_| (), - |self_, input| self_.parse_list(&input, Self::parse_ty), + |self_, input| self_.parse_list(input, Self::parse_ty), )? { // Nothing to do } else if let Some(var) = parse_var(input) { @@ -549,15 +554,14 @@ impl SuggBuilder { } fn parse_expr_body(&mut self, input: ParseStream<'_>, pos: ExprPos, prec: ExprPrec) -> Result> { - if self.consume_token::(input) { - // Nothing to do - } else if self.consume_group::( - input, - |_| (), - |self_, input| self_.parse_list(&input, Self::parse_expr).map(|_| ()), - )? { - // Nothing to do - } else if self.parse_path_root(input)?.is_some() { + if self.consume_token::(input) + || self.consume_group::( + input, + |_| (), + |self_, input| self_.parse_list(input, Self::parse_expr).map(|_| ()), + )? + || self.parse_path_root(input)?.is_some() + { // Nothing to do } else if let Some(var) = parse_var(input) { return if matches!(var.kind, VarKind::Expr | VarKind::Default) { @@ -585,19 +589,22 @@ impl SuggBuilder { self.next_string.push(' '); self.require(input, Self::parse_ty, "expected a type")?; prec.merge_with(ExprPrec::Cast) - } else if self.consume_op_token::(input, var, ExprPos::Suffix) { - prec.merge_with(ExprPrec::Suffix) - } else if self.consume_group::( - input, - |self_| self_.push_expr_var(var, ExprPos::Suffix), - |self_, input| self_.require(&input, Self::parse_expr, "expected an expression").map(|_| ()), - )? { - prec.merge_with(ExprPrec::Suffix) - } else if self.consume_group::( - input, - |self_| self_.push_expr_var(var, ExprPos::Callee), - |self_, input| self_.parse_list(&input, Self::parse_expr).map(|_| ()), - )? { + } else if self.consume_op_token::(input, var, ExprPos::Suffix) + || self.consume_group::( + input, + |self_| self_.push_expr_var(var, ExprPos::Suffix), + |self_, input| { + self_ + .require(input, Self::parse_expr, "expected an expression") + .map(|_| ()) + }, + )? + || self.consume_group::( + input, + |self_| self_.push_expr_var(var, ExprPos::Callee), + |self_, input| self_.parse_list(input, Self::parse_expr).map(|_| ()), + )? + { prec.merge_with(ExprPrec::Suffix) } else if self.consume_op_token::(input, var, ExprPos::Suffix) { if self.consume_token::(input) { @@ -700,7 +707,9 @@ impl SuggBuilder { let s = &self.next_string; body.extend(iter::once(quote!(sugg.push_str(#s);))); } - if prec != ExprPrec::Suffix { + if prec == ExprPrec::Suffix { + body.extend(iter::once(quote!(sugg))); + } else { body.extend(iter::once(quote!( if clippy_utils::_internal::needs_parens(#prec, clippy_utils::_internal::expr_position(cx, e)) { format!("({})", sugg) @@ -708,14 +717,14 @@ impl SuggBuilder { sugg } ))); - } else { - body.extend(iter::once(quote!(sugg))); } - Ok(quote!(|cx: &rustc_lint::LateContext<'_>, e: &rustc_hir::Expr<'_>, app: &mut rustc_errors::Applicability| { - let ctxt = e.span.ctxt(); - let mut sugg = String::new(); - #body - })) + Ok( + quote!(|cx: &rustc_lint::LateContext<'_>, e: &rustc_hir::Expr<'_>, app: &mut rustc_errors::Applicability| { + let ctxt = e.span.ctxt(); + let mut sugg = String::new(); + #body + }), + ) } } @@ -742,6 +751,7 @@ fn split_args(input: ParseStream) -> Result> { } } +#[allow(clippy::module_name_repetitions)] pub struct ExprSugg(pub TokenStream); impl Parse for ExprSugg { fn parse(input: ParseStream<'_>) -> Result {