From 09b65a64494c02b55eb2afae6658a635c09710d2 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 22 Jan 2023 23:15:43 -0500 Subject: [PATCH] Remove some usages of default format for expressions (#2100) --- src/ast/hashable.rs | 40 +++++++++++++++++++ src/ast/mod.rs | 1 + .../redundant_tuple_in_exception_handler.rs | 3 +- src/rules/pylint/rules/merge_isinstance.rs | 23 +++++++---- src/source_code/generator.rs | 14 +++---- 5 files changed, 65 insertions(+), 16 deletions(-) create mode 100644 src/ast/hashable.rs diff --git a/src/ast/hashable.rs b/src/ast/hashable.rs new file mode 100644 index 0000000000..2344ef007e --- /dev/null +++ b/src/ast/hashable.rs @@ -0,0 +1,40 @@ +use std::hash::Hash; + +use rustpython_ast::Expr; + +use crate::ast::comparable::ComparableExpr; + +/// Wrapper around `Expr` that implements `Hash` and `PartialEq`. +pub struct HashableExpr<'a>(&'a Expr); + +impl Hash for HashableExpr<'_> { + fn hash(&self, state: &mut H) { + let comparable = ComparableExpr::from(self.0); + comparable.hash(state); + } +} + +impl PartialEq for HashableExpr<'_> { + fn eq(&self, other: &Self) -> bool { + let comparable = ComparableExpr::from(self.0); + comparable == ComparableExpr::from(other.0) + } +} + +impl Eq for HashableExpr<'_> {} + +impl<'a> From<&'a Expr> for HashableExpr<'a> { + fn from(expr: &'a Expr) -> Self { + Self(expr) + } +} + +impl<'a> HashableExpr<'a> { + pub(crate) fn from_expr(expr: &'a Expr) -> Self { + Self(expr) + } + + pub(crate) fn as_expr(&self) -> &'a Expr { + self.0 + } +} diff --git a/src/ast/mod.rs b/src/ast/mod.rs index 9121e23a58..bad57983e2 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -2,6 +2,7 @@ pub mod branch_detection; pub mod cast; pub mod comparable; pub mod function_type; +pub mod hashable; pub mod helpers; pub mod operations; pub mod relocate; diff --git a/src/rules/flake8_bugbear/rules/redundant_tuple_in_exception_handler.rs b/src/rules/flake8_bugbear/rules/redundant_tuple_in_exception_handler.rs index 91d23be6de..479d88c7a0 100644 --- a/src/rules/flake8_bugbear/rules/redundant_tuple_in_exception_handler.rs +++ b/src/rules/flake8_bugbear/rules/redundant_tuple_in_exception_handler.rs @@ -1,5 +1,6 @@ use rustpython_ast::{Excepthandler, ExcepthandlerKind, ExprKind}; +use crate::ast::helpers::unparse_expr; use crate::ast::types::Range; use crate::checkers::ast::Checker; use crate::fix::Fix; @@ -20,7 +21,7 @@ pub fn redundant_tuple_in_exception_handler(checker: &mut Checker, handlers: &[E continue; }; let mut diagnostic = Diagnostic::new( - violations::RedundantTupleInExceptionHandler(elt.to_string()), + violations::RedundantTupleInExceptionHandler(unparse_expr(elt, checker.stylist)), Range::from_located(type_), ); if checker.patch(diagnostic.kind.rule()) { diff --git a/src/rules/pylint/rules/merge_isinstance.rs b/src/rules/pylint/rules/merge_isinstance.rs index a540585b10..a05f3182a2 100644 --- a/src/rules/pylint/rules/merge_isinstance.rs +++ b/src/rules/pylint/rules/merge_isinstance.rs @@ -2,6 +2,8 @@ use itertools::Itertools; use rustc_hash::{FxHashMap, FxHashSet}; use rustpython_ast::{Boolop, Expr, ExprKind}; +use crate::ast::hashable::HashableExpr; +use crate::ast::helpers::unparse_expr; use crate::ast::types::Range; use crate::checkers::ast::Checker; use crate::registry::Diagnostic; @@ -13,7 +15,8 @@ pub fn merge_isinstance(checker: &mut Checker, expr: &Expr, op: &Boolop, values: return; } - let mut obj_to_types: FxHashMap)> = FxHashMap::default(); + let mut obj_to_types: FxHashMap)> = + FxHashMap::default(); for value in values { let ExprKind::Call { func, args, .. } = &value.node else { continue; @@ -25,16 +28,14 @@ pub fn merge_isinstance(checker: &mut Checker, expr: &Expr, op: &Boolop, values: continue; }; let (num_calls, matches) = obj_to_types - .entry(obj.to_string()) + .entry(obj.into()) .or_insert_with(|| (0, FxHashSet::default())); *num_calls += 1; matches.extend(match &types.node { - ExprKind::Tuple { elts, .. } => { - elts.iter().map(std::string::ToString::to_string).collect() - } + ExprKind::Tuple { elts, .. } => elts.iter().map(HashableExpr::from_expr).collect(), _ => { - vec![types.to_string()] + vec![types.into()] } }); } @@ -42,7 +43,15 @@ pub fn merge_isinstance(checker: &mut Checker, expr: &Expr, op: &Boolop, values: for (obj, (num_calls, types)) in obj_to_types { if num_calls > 1 && types.len() > 1 { checker.diagnostics.push(Diagnostic::new( - violations::ConsiderMergingIsinstance(obj, types.into_iter().sorted().collect()), + violations::ConsiderMergingIsinstance( + unparse_expr(obj.as_expr(), checker.stylist), + types + .iter() + .map(HashableExpr::as_expr) + .map(|expr| unparse_expr(expr, checker.stylist)) + .sorted() + .collect(), + ), Range::from_located(expr), )); } diff --git a/src/source_code/generator.rs b/src/source_code/generator.rs index e93e4506d1..87dffb36eb 100644 --- a/src/source_code/generator.rs +++ b/src/source_code/generator.rs @@ -1,6 +1,5 @@ //! Generate Python source code from an abstract syntax tree (AST). -use std::fmt::{self, Write}; use std::ops::Deref; use rustpython_ast::{Excepthandler, ExcepthandlerKind, Suite, Withitem}; @@ -116,10 +115,6 @@ impl<'a> Generator<'a> { self.p_if(!std::mem::take(first), s); } - fn write_fmt(&mut self, f: fmt::Arguments<'_>) { - self.buffer.write_fmt(f).unwrap(); - } - pub fn unparse_suite(&mut self, suite: &Suite) { for stmt in suite { self.unparse_stmt(stmt); @@ -928,7 +923,8 @@ impl<'a> Generator<'a> { self.p_delim(&mut first, ", "); self.unparse_arg(arg); if let Some(i) = i.checked_sub(defaults_start) { - write!(self, "={}", &args.defaults[i]); + self.p("="); + self.unparse_expr(&args.defaults[i], precedence::TEST); } self.p_if(i + 1 == args.posonlyargs.len(), ", /"); } @@ -947,7 +943,8 @@ impl<'a> Generator<'a> { .checked_sub(defaults_start) .and_then(|i| args.kw_defaults.get(i)) { - write!(self, "={default}"); + self.p("="); + self.unparse_expr(default, precedence::TEST); } } if let Some(kwarg) = &args.kwarg { @@ -960,7 +957,8 @@ impl<'a> Generator<'a> { fn unparse_arg(&mut self, arg: &Arg) { self.p(&arg.node.arg); if let Some(ann) = &arg.node.annotation { - write!(self, ": {}", **ann); + self.p(": "); + self.unparse_expr(ann, precedence::TEST); } }