Skip to content

Commit

Permalink
Change more usages of SemanticModel::is_builtin to use `resolve_bui…
Browse files Browse the repository at this point in the history
…ltin_symbol` or `match_builtin_expr` (#10982)

## Summary

This PR switches more callsites of `SemanticModel::is_builtin` to move
over to the new methods I introduced in #10919, which are more concise
and more accurate. I missed these calls in the first PR.
  • Loading branch information
AlexWaygood committed Apr 17, 2024
1 parent 2882604 commit f48a794
Show file tree
Hide file tree
Showing 23 changed files with 191 additions and 252 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,12 @@ pub(crate) fn blind_except(
let Some(type_) = type_ else {
return;
};
let Expr::Name(ast::ExprName { id, .. }) = &type_ else {
return;
};

if !matches!(id.as_str(), "BaseException" | "Exception") {
let semantic = checker.semantic();
let Some(builtin_exception_type) = semantic.resolve_builtin_symbol(type_) else {
return;
}

if !checker.semantic().is_builtin(id) {
};
if !matches!(builtin_exception_type, "BaseException" | "Exception") {
return;
}

Expand Down Expand Up @@ -121,7 +118,7 @@ pub(crate) fn blind_except(
Expr::Attribute(ast::ExprAttribute { attr, .. }) => {
if logging::is_logger_candidate(
func,
checker.semantic(),
semantic,
&checker.settings.logger_objects,
) {
match attr.as_str() {
Expand All @@ -138,9 +135,8 @@ pub(crate) fn blind_except(
}
}
Expr::Name(ast::ExprName { .. }) => {
if checker
.semantic()
.resolve_qualified_name(func.as_ref())
if semantic
.resolve_qualified_name(func)
.is_some_and(|qualified_name| match qualified_name.segments() {
["logging", "exception"] => true,
["logging", "error"] => {
Expand Down Expand Up @@ -170,7 +166,7 @@ pub(crate) fn blind_except(

checker.diagnostics.push(Diagnostic::new(
BlindExcept {
name: id.to_string(),
name: builtin_exception_type.to_string(),
},
type_.range(),
));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,40 +60,35 @@ impl AlwaysFixableViolation for UnnecessaryCallAroundSorted {
pub(crate) fn unnecessary_call_around_sorted(
checker: &mut Checker,
expr: &Expr,
func: &Expr,
outer_func: &Expr,
args: &[Expr],
) {
let Some(outer) = func.as_name_expr() else {
let Some(Expr::Call(ast::ExprCall {
func: inner_func, ..
})) = args.first()
else {
return;
};
if !matches!(outer.id.as_str(), "list" | "reversed") {
return;
}
let Some(arg) = args.first() else {
return;
};
let Expr::Call(ast::ExprCall { func, .. }) = arg else {
return;
};
let Some(inner) = func.as_name_expr() else {
let semantic = checker.semantic();
let Some(outer_func_name) = semantic.resolve_builtin_symbol(outer_func) else {
return;
};
if inner.id != "sorted" {
if !matches!(outer_func_name, "list" | "reversed") {
return;
}
if !checker.semantic().is_builtin(&inner.id) || !checker.semantic().is_builtin(&outer.id) {
if !semantic.match_builtin_expr(inner_func, "sorted") {
return;
}
let mut diagnostic = Diagnostic::new(
UnnecessaryCallAroundSorted {
func: outer.id.to_string(),
func: outer_func_name.to_string(),
},
expr.range(),
);
diagnostic.try_set_fix(|| {
Ok(Fix::applicable_edit(
fixes::fix_unnecessary_call_around_sorted(expr, checker.locator(), checker.stylist())?,
if outer.id == "reversed" {
if outer_func_name == "reversed" {
Applicability::Unsafe
} else {
Applicability::Safe
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,10 @@ pub(crate) fn unnecessary_collection_call(
if !call.arguments.args.is_empty() {
return;
}
let Some(func) = call.func.as_name_expr() else {
let Some(builtin) = checker.semantic().resolve_builtin_symbol(&call.func) else {
return;
};
let collection = match func.id.as_str() {
let collection = match builtin {
"dict"
if call.arguments.keywords.is_empty()
|| (!settings.allow_dict_calls_with_keyword_arguments
Expand All @@ -87,13 +87,10 @@ pub(crate) fn unnecessary_collection_call(
}
_ => return,
};
if !checker.semantic().is_builtin(func.id.as_str()) {
return;
}

let mut diagnostic = Diagnostic::new(
UnnecessaryCollectionCall {
obj_type: func.id.to_string(),
obj_type: builtin.to_string(),
},
call.range(),
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,6 @@ pub(crate) fn unnecessary_comprehension_in_call(
if !keywords.is_empty() {
return;
}

let Expr::Name(ast::ExprName { id, .. }) = func else {
return;
};
if !(matches!(id.as_str(), "any" | "all")
|| (checker.settings.preview.is_enabled() && matches!(id.as_str(), "sum" | "min" | "max")))
{
return;
}
let [arg] = args else {
return;
};
Expand All @@ -110,7 +101,13 @@ pub(crate) fn unnecessary_comprehension_in_call(
if contains_await(elt) {
return;
}
if !checker.semantic().is_builtin(id) {
let Some(builtin_function) = checker.semantic().resolve_builtin_symbol(func) else {
return;
};
if !(matches!(builtin_function, "any" | "all")
|| (checker.settings.preview.is_enabled()
&& matches!(builtin_function, "sum" | "min" | "max")))
{
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,24 +70,15 @@ impl AlwaysFixableViolation for UnnecessaryDoubleCastOrProcess {
pub(crate) fn unnecessary_double_cast_or_process(
checker: &mut Checker,
expr: &Expr,
func: &Expr,
outer_func: &Expr,
args: &[Expr],
outer_kw: &[Keyword],
) {
let Some(outer) = func.as_name_expr() else {
return;
};
if !matches!(
outer.id.as_str(),
"list" | "tuple" | "set" | "reversed" | "sorted"
) {
return;
}
let Some(arg) = args.first() else {
return;
};
let Expr::Call(ast::ExprCall {
func,
func: inner_func,
arguments: Arguments {
keywords: inner_kw, ..
},
Expand All @@ -96,16 +87,23 @@ pub(crate) fn unnecessary_double_cast_or_process(
else {
return;
};
let Some(inner) = func.as_name_expr() else {
let semantic = checker.semantic();
let Some(outer_func_name) = semantic.resolve_builtin_symbol(outer_func) else {
return;
};
if !checker.semantic().is_builtin(&inner.id) || !checker.semantic().is_builtin(&outer.id) {
if !matches!(
outer_func_name,
"list" | "tuple" | "set" | "reversed" | "sorted"
) {
return;
}
let Some(inner_func_name) = semantic.resolve_builtin_symbol(inner_func) else {
return;
};

// Avoid collapsing nested `sorted` calls with non-identical keyword arguments
// (i.e., `key`, `reverse`).
if inner.id == "sorted" && outer.id == "sorted" {
if inner_func_name == "sorted" && outer_func_name == "sorted" {
if inner_kw.len() != outer_kw.len() {
return;
}
Expand All @@ -122,15 +120,15 @@ pub(crate) fn unnecessary_double_cast_or_process(
// Ex) `list(tuple(...))`
// Ex) `set(set(...))`
if matches!(
(outer.id.as_str(), inner.id.as_str()),
(outer_func_name, inner_func_name),
("set" | "sorted", "list" | "tuple" | "reversed" | "sorted")
| ("set", "set")
| ("list" | "tuple", "list" | "tuple")
) {
let mut diagnostic = Diagnostic::new(
UnnecessaryDoubleCastOrProcess {
inner: inner.id.to_string(),
outer: outer.id.to_string(),
inner: inner_func_name.to_string(),
outer: outer_func_name.to_string(),
},
expr.range(),
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,22 +73,18 @@ pub(crate) fn unnecessary_map(
func: &Expr,
args: &[Expr],
) {
let Some(func) = func.as_name_expr() else {
let Some(builtin_name) = checker.semantic().resolve_builtin_symbol(func) else {
return;
};

let object_type = match func.id.as_str() {
let object_type = match builtin_name {
"map" => ObjectType::Generator,
"list" => ObjectType::List,
"set" => ObjectType::Set,
"dict" => ObjectType::Dict,
_ => return,
};

if !checker.semantic().is_builtin(&func.id) {
return;
}

match object_type {
ObjectType::Generator => {
// Exclude the parent if already matched by other arms.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,6 @@ pub(crate) fn unnecessary_subscript_reversal(checker: &mut Checker, call: &ast::
let Some(first_arg) = call.arguments.args.first() else {
return;
};
let Some(func) = call.func.as_name_expr() else {
return;
};
if !matches!(func.id.as_str(), "reversed" | "set" | "sorted") {
return;
}
if !checker.semantic().is_builtin(&func.id) {
return;
}
let Expr::Subscript(ast::ExprSubscript { slice, .. }) = first_arg else {
return;
};
Expand Down Expand Up @@ -89,9 +80,15 @@ pub(crate) fn unnecessary_subscript_reversal(checker: &mut Checker, call: &ast::
if *val != 1 {
return;
};
let Some(function_name) = checker.semantic().resolve_builtin_symbol(&call.func) else {
return;
};
if !matches!(function_name, "reversed" | "set" | "sorted") {
return;
}
checker.diagnostics.push(Diagnostic::new(
UnnecessarySubscriptReversal {
func: func.id.to_string(),
func: function_name.to_string(),
},
call.range(),
));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,24 +65,26 @@ pub(crate) fn reimplemented_container_builtin(checker: &mut Checker, expr: &Expr
range: _,
} = expr;

if parameters.is_none() {
let container = match body.as_ref() {
Expr::List(ast::ExprList { elts, .. }) if elts.is_empty() => Some(Container::List),
Expr::Dict(ast::ExprDict { values, .. }) if values.is_empty() => Some(Container::Dict),
_ => None,
};
if let Some(container) = container {
let mut diagnostic =
Diagnostic::new(ReimplementedContainerBuiltin { container }, expr.range());
if checker.semantic().is_builtin(container.as_str()) {
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
container.to_string(),
expr.range(),
)));
}
checker.diagnostics.push(diagnostic);
}
if parameters.is_some() {
return;
}

let container = match &**body {
Expr::List(ast::ExprList { elts, .. }) if elts.is_empty() => Container::List,
Expr::Dict(ast::ExprDict { values, .. }) if values.is_empty() => Container::Dict,
_ => return,
};
let mut diagnostic = Diagnostic::new(ReimplementedContainerBuiltin { container }, expr.range());
diagnostic.try_set_fix(|| {
let (import_edit, binding) = checker.importer().get_or_import_builtin_symbol(
container.as_str(),
expr.start(),
checker.semantic(),
)?;
let binding_edit = Edit::range_replacement(binding, expr.range());
Ok(Fix::safe_edits(binding_edit, import_edit))
});
checker.diagnostics.push(diagnostic);
}

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
Expand All @@ -92,7 +94,7 @@ enum Container {
}

impl Container {
fn as_str(self) -> &'static str {
const fn as_str(self) -> &'static str {
match self {
Container::List => "list",
Container::Dict => "dict",
Expand All @@ -102,9 +104,6 @@ impl Container {

impl std::fmt::Display for Container {
fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
match self {
Container::List => fmt.write_str("list"),
Container::Dict => fmt.write_str("dict"),
}
fmt.write_str(self.as_str())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,7 @@ impl fmt::Display for ExprType {
/// Return the [`ExprType`] of an [`Expr]` if it is a builtin type (e.g. `int`, `bool`, `float`,
/// `str`, `bytes`, or `complex`).
fn match_builtin_type(expr: &Expr, semantic: &SemanticModel) -> Option<ExprType> {
let name = expr.as_name_expr()?;
let result = match name.id.as_str() {
let result = match semantic.resolve_builtin_symbol(expr)? {
"int" => ExprType::Int,
"bool" => ExprType::Bool,
"str" => ExprType::Str,
Expand All @@ -139,9 +138,6 @@ fn match_builtin_type(expr: &Expr, semantic: &SemanticModel) -> Option<ExprType>
"complex" => ExprType::Complex,
_ => return None,
};
if !semantic.is_builtin(name.id.as_str()) {
return None;
}
Some(result)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,12 +90,15 @@ pub(crate) fn deprecated_type_alias(checker: &mut Checker, expr: &Expr) {
"long" => "int",
_ => type_name,
};
if checker.semantic().is_builtin(type_name) {
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
type_name.to_string(),
expr.range(),
)));
}
diagnostic.try_set_fix(|| {
let (import_edit, binding) = checker.importer().get_or_import_builtin_symbol(
type_name,
expr.start(),
checker.semantic(),
)?;
let binding_edit = Edit::range_replacement(binding, expr.range());
Ok(Fix::safe_edits(binding_edit, import_edit))
});
checker.diagnostics.push(diagnostic);
}
}

0 comments on commit f48a794

Please sign in to comment.