Skip to content

Commit

Permalink
Reimplement the high-level abstraction based on a lower-level one
Browse files Browse the repository at this point in the history
  • Loading branch information
AlexWaygood committed Apr 15, 2024
1 parent 4567626 commit e441e77
Show file tree
Hide file tree
Showing 11 changed files with 96 additions and 97 deletions.
14 changes: 5 additions & 9 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -784,17 +784,13 @@ impl<'a> Visitor<'a> for Checker<'a> {
}) => {
let mut handled_exceptions = Exceptions::empty();
for type_ in extract_handled_exceptions(handlers) {
if let Some(qualified_name) = self.semantic.resolve_qualified_name(type_) {
match qualified_name.segments() {
["" | "builtins", "NameError"] => {
handled_exceptions |= Exceptions::NAME_ERROR;
}
["" | "builtins", "ModuleNotFoundError"] => {
if let Some(builtins_name) = self.semantic.resolve_builtin_symbol(type_) {
match builtins_name {
"NameError" => handled_exceptions |= Exceptions::NAME_ERROR,
"ModuleNotFoundError" => {
handled_exceptions |= Exceptions::MODULE_NOT_FOUND_ERROR;
}
["" | "builtins", "ImportError"] => {
handled_exceptions |= Exceptions::IMPORT_ERROR;
}
"ImportError" => handled_exceptions |= Exceptions::IMPORT_ERROR,
_ => {}
}
}
Expand Down
18 changes: 4 additions & 14 deletions crates/ruff_linter/src/rules/flake8_bandit/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,23 +24,13 @@ pub(super) fn is_untyped_exception(type_: Option<&Expr>, semantic: &SemanticMode
if let Expr::Tuple(ast::ExprTuple { elts, .. }) = &type_ {
elts.iter().any(|type_| {
semantic
.resolve_qualified_name(type_)
.is_some_and(|qualified_name| {
matches!(
qualified_name.segments(),
["" | "builtins", "Exception" | "BaseException"]
)
})
.resolve_builtin_symbol(type_)
.is_some_and(|builtin| matches!(builtin, "Exception" | "BaseException"))
})
} else {
semantic
.resolve_qualified_name(type_)
.is_some_and(|qualified_name| {
matches!(
qualified_name.segments(),
["" | "builtins", "Exception" | "BaseException"]
)
})
.resolve_builtin_symbol(type_)
.is_some_and(|builtin| matches!(builtin, "Exception" | "BaseException"))
}
})
}
Original file line number Diff line number Diff line change
Expand Up @@ -95,24 +95,22 @@ pub(crate) fn assert_raises_exception(checker: &mut Checker, items: &[WithItem])
return;
};

let Some(exception) =
checker
.semantic()
.resolve_qualified_name(arg)
.and_then(|qualified_name| match qualified_name.segments() {
["" | "builtins", "Exception"] => Some(ExceptionKind::Exception),
["" | "builtins", "BaseException"] => Some(ExceptionKind::BaseException),
_ => None,
})
else {
let semantic = checker.semantic();

let Some(builtin_symbol) = semantic.resolve_builtin_symbol(arg) else {
return;
};

let exception = match builtin_symbol {
"Exception" => ExceptionKind::Exception,
"BaseException" => ExceptionKind::BaseException,
_ => return,
};

let assertion = if matches!(func.as_ref(), Expr::Attribute(ast::ExprAttribute { attr, .. }) if attr == "assertRaises")
{
AssertionKind::AssertRaises
} else if checker
.semantic()
} else if semantic
.resolve_qualified_name(func)
.is_some_and(|qualified_name| matches!(qualified_name.segments(), ["pytest", "raises"]))
&& arguments.find_keyword("match").is_none()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,15 @@ pub(crate) fn unreliable_callable_check(
if value != "__call__" {
return;
}
let Some(qualified_name) = checker.semantic().resolve_qualified_name(func) else {
let Some(builtins_function) = checker.semantic().resolve_builtin_symbol(func) else {
return;
};
let ["" | "builtins", function @ ("hasattr" | "getattr")] = qualified_name.segments() else {
if !matches!(builtins_function, "hasattr" | "getattr") {
return;
};
}

let mut diagnostic = Diagnostic::new(UnreliableCallableCheck, expr.range());
if *function == "hasattr" {
if builtins_function == "hasattr" {
diagnostic.try_set_fix(|| {
let (import_edit, binding) = checker.importer().get_or_import_builtin_symbol(
"callable",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,20 +96,20 @@ fn check_annotation(checker: &mut Checker, annotation: &Expr) {
let mut has_complex = false;
let mut has_int = false;

let mut func = |expr: &Expr, _parent: &Expr| {
let Some(qualified_name) = checker.semantic().resolve_qualified_name(expr) else {
let mut find_numeric_type = |expr: &Expr, _parent: &Expr| {
let Some(builtin_type) = checker.semantic().resolve_builtin_symbol(expr) else {
return;
};

match qualified_name.segments() {
["" | "builtins", "int"] => has_int = true,
["" | "builtins", "float"] => has_float = true,
["" | "builtins", "complex"] => has_complex = true,
_ => (),
match builtin_type {
"int" => has_int = true,
"float" => has_float = true,
"complex" => has_complex = true,
_ => {}
}
};

traverse_union(&mut func, checker.semantic(), annotation);
traverse_union(&mut find_numeric_type, checker.semantic(), annotation);

if has_complex {
if has_float {
Expand Down
7 changes: 3 additions & 4 deletions crates/ruff_linter/src/rules/pylint/rules/nested_min_max.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,9 @@ impl MinMax {
if !keywords.is_empty() {
return None;
}
let qualified_name = semantic.resolve_qualified_name(func)?;
match qualified_name.segments() {
["" | "builtins", "min"] => Some(MinMax::Min),
["" | "builtins", "max"] => Some(MinMax::Max),
match semantic.resolve_builtin_symbol(func)? {
"min" => Some(Self::Min),
"max" => Some(Self::Max),
_ => None,
}
}
Expand Down
14 changes: 5 additions & 9 deletions crates/ruff_linter/src/rules/refurb/rules/bit_count.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,22 +97,18 @@ pub(crate) fn bit_count(checker: &mut Checker, call: &ExprCall) {
return;
};

// Ensure that we're performing a `bin(...)`.
if !checker
.semantic()
.resolve_qualified_name(func)
.is_some_and(|qualified_name| matches!(qualified_name.segments(), ["" | "builtins", "bin"]))
{
return;
}

if !arguments.keywords.is_empty() {
return;
};
let [arg] = &*arguments.args else {
return;
};

// Ensure that we're performing a `bin(...)`.
if !checker.semantic().match_builtin_expr(func, "bin") {
return;
}

// Extract, e.g., `x` in `bin(x)`.
let literal_text = checker.locator().slice(arg);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,8 @@ pub(crate) fn raise_within_try(checker: &mut Checker, body: &[Stmt], handlers: &
|| handled_exceptions.iter().any(|expr| {
checker
.semantic()
.resolve_qualified_name(expr)
.is_some_and(|qualified_name| {
matches!(
qualified_name.segments(),
["" | "builtins", "Exception" | "BaseException"]
)
})
.resolve_builtin_symbol(expr)
.is_some_and(|builtin| matches!(builtin, "Exception" | "BaseException"))
})
{
checker
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,26 +74,20 @@ fn has_control_flow(stmt: &Stmt) -> bool {
}

/// Returns `true` if an [`Expr`] is a call to check types.
fn check_type_check_call(checker: &mut Checker, call: &Expr) -> bool {
checker
.semantic()
.resolve_qualified_name(call)
.is_some_and(|qualified_name| {
matches!(
qualified_name.segments(),
["" | "builtins", "isinstance" | "issubclass" | "callable"]
)
})
fn check_type_check_call(semantic: &SemanticModel, call: &Expr) -> bool {
semantic
.resolve_builtin_symbol(call)
.is_some_and(|builtin| matches!(builtin, "isinstance" | "issubclass" | "callable"))
}

/// Returns `true` if an [`Expr`] is a test to check types (e.g. via isinstance)
fn check_type_check_test(checker: &mut Checker, test: &Expr) -> bool {
fn check_type_check_test(semantic: &SemanticModel, test: &Expr) -> bool {
match test {
Expr::BoolOp(ast::ExprBoolOp { values, .. }) => values
.iter()
.all(|expr| check_type_check_test(checker, expr)),
Expr::UnaryOp(ast::ExprUnaryOp { operand, .. }) => check_type_check_test(checker, operand),
Expr::Call(ast::ExprCall { func, .. }) => check_type_check_call(checker, func),
.all(|expr| check_type_check_test(semantic, expr)),
Expr::UnaryOp(ast::ExprUnaryOp { operand, .. }) => check_type_check_test(semantic, operand),
Expr::Call(ast::ExprCall { func, .. }) => check_type_check_call(semantic, func),
_ => false,
}
}
Expand Down Expand Up @@ -161,22 +155,23 @@ pub(crate) fn type_check_without_type_error(
elif_else_clauses,
..
} = stmt_if;

if let Some(Stmt::If(ast::StmtIf { test, .. })) = parent {
if !check_type_check_test(checker, test) {
if !check_type_check_test(checker.semantic(), test) {
return;
}
}

// Only consider the body when the `if` condition is all type-related
if !check_type_check_test(checker, test) {
if !check_type_check_test(checker.semantic(), test) {
return;
}
check_body(checker, body);

for clause in elif_else_clauses {
if let Some(test) = &clause.test {
// If there are any `elif`, they must all also be type-related
if !check_type_check_test(checker, test) {
if !check_type_check_test(checker.semantic(), test) {
return;
}
}
Expand Down
10 changes: 8 additions & 2 deletions crates/ruff_python_ast/src/name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,15 @@ impl<'a> QualifiedName<'a> {
self.0.as_slice()
}

/// If the first segment is empty, the `CallPath` is that of a builtin.
/// If the first segment is empty, the `CallPath` represents a "builtin binding".
///
/// A builtin binding is the binding that a symbol has if it was part of Python's
/// global scope without any imports taking place. However, if builtin members are
/// accessed explicitly via the `builtins` module, they will not have a
/// "builtin binding", so this method will return `false`.
///
/// Ex) `["", "bool"]` -> `"bool"`
pub fn is_builtin(&self) -> bool {
fn is_builtin(&self) -> bool {
matches!(self.segments(), ["", ..])
}

Expand Down
46 changes: 35 additions & 11 deletions crates/ruff_python_semantic/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -251,27 +251,51 @@ impl<'a> SemanticModel<'a> {
}

/// Return `true` if `member` is bound as a builtin.
///
/// Note that a "builtin binding" does *not* include explicit lookups via the `builtins`
/// module, e.g. `import builtins; builtins.open`. It *only* includes the bindings
/// that are pre-populated in Python's global scope before any imports have taken place.
pub fn is_builtin(&self, member: &str) -> bool {
self.lookup_symbol(member)
.map(|binding_id| &self.bindings[binding_id])
.is_some_and(|binding| binding.kind.is_builtin())
}

/// Return `true` if `member` is a reference to `builtins.$target`,
/// If `expr` is a reference to a builtins symbol,
/// return the name of that symbol. Else, return `None`.
///
/// This method returns `true` both for "builtin bindings"
/// (present even without any imports, e.g. `open()`), and for explicit lookups
/// via the `builtins` module (e.g. `import builtins; builtins.open()`).
pub fn resolve_builtin_symbol<'expr>(&'a self, expr: &'expr Expr) -> Option<&'a str>
where
'expr: 'a,
{
// Fast path: we only need to worry about name expressions
if !self.seen_module(Modules::BUILTINS) {
let name = &expr.as_name_expr()?.id;
return if self.is_builtin(name) {
Some(name)
} else {
None
};
}

// Slow path: we have to consider names and attributes
let qualified_name = self.resolve_qualified_name(expr)?;
match qualified_name.segments() {
["" | "builtins", name] => Some(*name),
_ => None,
}
}

/// Return `true` if `expr` is a reference to `builtins.$target`,
/// i.e. either `object` (where `object` is not overridden in the global scope),
/// or `builtins.object` (where `builtins` is imported as a module at the top level)
pub fn match_builtin_expr(&self, expr: &Expr, symbol: &str) -> bool {
debug_assert!(!symbol.contains('.'));
if self.seen_module(Modules::BUILTINS) {
// slow path
self.resolve_qualified_name(expr)
.is_some_and(|qualified_name| {
matches!(qualified_name.segments(), ["builtins" | "", member] if *member == symbol)
})
} else {
// fast(er) path
matches!(expr, Expr::Name(ast::ExprName {id, ..}) if id == symbol && self.is_builtin(symbol))
}
self.resolve_builtin_symbol(expr)
.is_some_and(|name| name == symbol)
}

/// Return `true` if `member` is an "available" symbol, i.e., a symbol that has not been bound
Expand Down

0 comments on commit e441e77

Please sign in to comment.