Skip to content

Commit

Permalink
Split CallPath into QualifiedName and UnqualifiedName (astral-s…
Browse files Browse the repository at this point in the history
…h#10210)

## Summary

Charlie can probably explain this better than I but it turns out,
`CallPath` is used for two different things:

* To represent unqualified names like `version` where `version` can be a
local variable or imported (e.g. `from sys import version` where the
full qualified name is `sys.version`)
* To represent resolved, full qualified names

This PR splits `CallPath` into two types to make this destinction clear.

> Note: I haven't renamed all `call_path` variables to `qualified_name`
or `unqualified_name`. I can do that if that's welcomed but I first want
to get feedback on the approach and naming overall.

## Test Plan

`cargo test`
  • Loading branch information
MichaReiser authored and nkxxll committed Mar 10, 2024
1 parent acc37b3 commit b0cff02
Show file tree
Hide file tree
Showing 181 changed files with 1,692 additions and 1,412 deletions.
100 changes: 56 additions & 44 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -418,11 +418,11 @@ impl<'a> Visitor<'a> for Checker<'a> {
self.semantic.add_module(module);

if alias.asname.is_none() && alias.name.contains('.') {
let call_path: Box<[&str]> = alias.name.split('.').collect();
let qualified_name: Box<[&str]> = alias.name.split('.').collect();
self.add_binding(
module,
alias.identifier(),
BindingKind::SubmoduleImport(SubmoduleImport { call_path }),
BindingKind::SubmoduleImport(SubmoduleImport { qualified_name }),
BindingFlags::EXTERNAL,
);
} else {
Expand All @@ -439,11 +439,11 @@ impl<'a> Visitor<'a> for Checker<'a> {
}

let name = alias.asname.as_ref().unwrap_or(&alias.name);
let call_path: Box<[&str]> = alias.name.split('.').collect();
let qualified_name: Box<[&str]> = alias.name.split('.').collect();
self.add_binding(
name,
alias.identifier(),
BindingKind::Import(Import { call_path }),
BindingKind::Import(Import { qualified_name }),
flags,
);
}
Expand Down Expand Up @@ -503,12 +503,12 @@ impl<'a> Visitor<'a> for Checker<'a> {
// Attempt to resolve any relative imports; but if we don't know the current
// module path, or the relative import extends beyond the package root,
// fallback to a literal representation (e.g., `[".", "foo"]`).
let call_path = collect_import_from_member(level, module, &alias.name)
let qualified_name = collect_import_from_member(level, module, &alias.name)
.into_boxed_slice();
self.add_binding(
name,
alias.identifier(),
BindingKind::FromImport(FromImport { call_path }),
BindingKind::FromImport(FromImport { qualified_name }),
flags,
);
}
Expand Down Expand Up @@ -751,8 +751,8 @@ impl<'a> Visitor<'a> for Checker<'a> {
}) => {
let mut handled_exceptions = Exceptions::empty();
for type_ in extract_handled_exceptions(handlers) {
if let Some(call_path) = self.semantic.resolve_call_path(type_) {
match call_path.segments() {
if let Some(qualified_name) = self.semantic.resolve_qualified_name(type_) {
match qualified_name.segments() {
["", "NameError"] => {
handled_exceptions |= Exceptions::NAME_ERROR;
}
Expand Down Expand Up @@ -1065,42 +1065,54 @@ impl<'a> Visitor<'a> for Checker<'a> {
}) => {
self.visit_expr(func);

let callable = self.semantic.resolve_call_path(func).and_then(|call_path| {
if self.semantic.match_typing_call_path(&call_path, "cast") {
Some(typing::Callable::Cast)
} else if self.semantic.match_typing_call_path(&call_path, "NewType") {
Some(typing::Callable::NewType)
} else if self.semantic.match_typing_call_path(&call_path, "TypeVar") {
Some(typing::Callable::TypeVar)
} else if self
.semantic
.match_typing_call_path(&call_path, "NamedTuple")
{
Some(typing::Callable::NamedTuple)
} else if self
.semantic
.match_typing_call_path(&call_path, "TypedDict")
{
Some(typing::Callable::TypedDict)
} else if matches!(
call_path.segments(),
[
"mypy_extensions",
"Arg"
| "DefaultArg"
| "NamedArg"
| "DefaultNamedArg"
| "VarArg"
| "KwArg"
]
) {
Some(typing::Callable::MypyExtension)
} else if matches!(call_path.segments(), ["", "bool"]) {
Some(typing::Callable::Bool)
} else {
None
}
});
let callable =
self.semantic
.resolve_qualified_name(func)
.and_then(|qualified_name| {
if self
.semantic
.match_typing_qualified_name(&qualified_name, "cast")
{
Some(typing::Callable::Cast)
} else if self
.semantic
.match_typing_qualified_name(&qualified_name, "NewType")
{
Some(typing::Callable::NewType)
} else if self
.semantic
.match_typing_qualified_name(&qualified_name, "TypeVar")
{
Some(typing::Callable::TypeVar)
} else if self
.semantic
.match_typing_qualified_name(&qualified_name, "NamedTuple")
{
Some(typing::Callable::NamedTuple)
} else if self
.semantic
.match_typing_qualified_name(&qualified_name, "TypedDict")
{
Some(typing::Callable::TypedDict)
} else if matches!(
qualified_name.segments(),
[
"mypy_extensions",
"Arg"
| "DefaultArg"
| "NamedArg"
| "DefaultNamedArg"
| "VarArg"
| "KwArg"
]
) {
Some(typing::Callable::MypyExtension)
} else if matches!(qualified_name.segments(), ["", "bool"]) {
Some(typing::Callable::Bool)
} else {
None
}
});
match callable {
Some(typing::Callable::Bool) => {
let mut args = arguments.args.iter();
Expand Down
39 changes: 1 addition & 38 deletions crates/ruff_linter/src/cst/helpers.rs
Original file line number Diff line number Diff line change
@@ -1,44 +1,7 @@
use libcst_native::{
Expression, Name, NameOrAttribute, ParenthesizableWhitespace, SimpleWhitespace, UnaryOperation,
Expression, Name, ParenthesizableWhitespace, SimpleWhitespace, UnaryOperation,
};

fn compose_call_path_inner<'a>(expr: &'a Expression, parts: &mut Vec<&'a str>) {
match expr {
Expression::Call(expr) => {
compose_call_path_inner(&expr.func, parts);
}
Expression::Attribute(expr) => {
compose_call_path_inner(&expr.value, parts);
parts.push(expr.attr.value);
}
Expression::Name(expr) => {
parts.push(expr.value);
}
_ => {}
}
}

pub(crate) fn compose_call_path(expr: &Expression) -> Option<String> {
let mut segments = vec![];
compose_call_path_inner(expr, &mut segments);
if segments.is_empty() {
None
} else {
Some(segments.join("."))
}
}

pub(crate) fn compose_module_path(module: &NameOrAttribute) -> String {
match module {
NameOrAttribute::N(name) => name.value.to_string(),
NameOrAttribute::A(attr) => {
let name = attr.attr.value;
let prefix = compose_call_path(&attr.value);
prefix.map_or_else(|| name.to_string(), |prefix| format!("{prefix}.{name}"))
}
}
}

/// Return a [`ParenthesizableWhitespace`] containing a single space.
pub(crate) fn space() -> ParenthesizableWhitespace<'static> {
ParenthesizableWhitespace::SimpleWhitespace(SimpleWhitespace(" "))
Expand Down
47 changes: 43 additions & 4 deletions crates/ruff_linter/src/fix/codemods.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,16 @@
//! and return the modified code snippet as output.
use anyhow::{bail, Result};
use libcst_native::{
Codegen, CodegenState, ImportNames, ParenthesizableWhitespace, SmallStatement, Statement,
Codegen, CodegenState, Expression, ImportNames, NameOrAttribute, ParenthesizableWhitespace,
SmallStatement, Statement,
};
use ruff_python_ast::name::UnqualifiedName;
use smallvec::{smallvec, SmallVec};

use ruff_python_ast::Stmt;
use ruff_python_codegen::Stylist;
use ruff_source_file::Locator;

use crate::cst::helpers::compose_module_path;
use crate::cst::matchers::match_statement;

/// Glue code to make libcst codegen work with ruff's Stylist
Expand Down Expand Up @@ -78,7 +80,7 @@ pub(crate) fn remove_imports<'a>(
for member in member_names {
let alias_index = aliases
.iter()
.position(|alias| member == compose_module_path(&alias.name));
.position(|alias| member == qualified_name_from_name_or_attribute(&alias.name));
if let Some(index) = alias_index {
aliases.remove(index);
}
Expand Down Expand Up @@ -142,7 +144,7 @@ pub(crate) fn retain_imports(
aliases.retain(|alias| {
member_names
.iter()
.any(|member| *member == compose_module_path(&alias.name))
.any(|member| *member == qualified_name_from_name_or_attribute(&alias.name))
});

// But avoid destroying any trailing comments.
Expand All @@ -164,3 +166,40 @@ pub(crate) fn retain_imports(

Ok(tree.codegen_stylist(stylist))
}

fn collect_segments<'a>(expr: &'a Expression, parts: &mut SmallVec<[&'a str; 8]>) {
match expr {
Expression::Call(expr) => {
collect_segments(&expr.func, parts);
}
Expression::Attribute(expr) => {
collect_segments(&expr.value, parts);
parts.push(expr.attr.value);
}
Expression::Name(expr) => {
parts.push(expr.value);
}
_ => {}
}
}

fn unqualified_name_from_expression<'a>(expr: &'a Expression<'a>) -> Option<UnqualifiedName<'a>> {
let mut segments = smallvec![];
collect_segments(expr, &mut segments);
if segments.is_empty() {
None
} else {
Some(segments.into_iter().collect())
}
}

fn qualified_name_from_name_or_attribute(module: &NameOrAttribute) -> String {
match module {
NameOrAttribute::N(name) => name.value.to_string(),
NameOrAttribute::A(attr) => {
let name = attr.attr.value;
let prefix = unqualified_name_from_expression(&attr.value);
prefix.map_or_else(|| name.to_string(), |prefix| format!("{prefix}.{name}"))
}
}
}
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/renamer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ impl Renamer {
}
BindingKind::SubmoduleImport(import) => {
// Ex) Rename `import pandas.core` to `import pandas as pd`.
let module_name = import.call_path.first().unwrap();
let module_name = import.qualified_name.first().unwrap();
Some(Edit::range_replacement(
format!("{module_name} as {target}"),
binding.range(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ pub(crate) fn variable_name_task_id(
// If the function doesn't come from Airflow, we can't do anything.
if !checker
.semantic()
.resolve_call_path(func)
.is_some_and(|call_path| matches!(call_path.segments()[0], "airflow"))
.resolve_qualified_name(func)
.is_some_and(|qualified_name| matches!(qualified_name.segments()[0], "airflow"))
{
return None;
}
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/rules/flake8_2020/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@ use ruff_python_semantic::SemanticModel;

pub(super) fn is_sys(expr: &Expr, target: &str, semantic: &SemanticModel) -> bool {
semantic
.resolve_call_path(expr)
.is_some_and(|call_path| call_path.segments() == ["sys", target])
.resolve_qualified_name(expr)
.is_some_and(|qualified_name| qualified_name.segments() == ["sys", target])
}
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ pub(crate) fn name_or_attribute(checker: &mut Checker, expr: &Expr) {

if checker
.semantic()
.resolve_call_path(expr)
.is_some_and(|call_path| matches!(call_path.segments(), ["six", "PY3"]))
.resolve_qualified_name(expr)
.is_some_and(|qualified_name| matches!(qualified_name.segments(), ["six", "PY3"]))
{
checker
.diagnostics
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use ruff_python_ast::ExprCall;

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::call_path::CallPath;
use ruff_python_ast::name::QualifiedName;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
Expand Down Expand Up @@ -41,9 +41,9 @@ impl Violation for BlockingHttpCallInAsyncFunction {
}
}

fn is_blocking_http_call(call_path: &CallPath) -> bool {
fn is_blocking_http_call(qualified_name: &QualifiedName) -> bool {
matches!(
call_path.segments(),
qualified_name.segments(),
["urllib", "request", "urlopen"]
| [
"httpx" | "requests",
Expand All @@ -65,7 +65,7 @@ pub(crate) fn blocking_http_call(checker: &mut Checker, call: &ExprCall) {
if checker.semantic().in_async_context() {
if checker
.semantic()
.resolve_call_path(call.func.as_ref())
.resolve_qualified_name(call.func.as_ref())
.as_ref()
.is_some_and(is_blocking_http_call)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use ruff_python_ast::ExprCall;

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::call_path::CallPath;
use ruff_python_ast::name::QualifiedName;
use ruff_python_semantic::Modules;
use ruff_text_size::Ranged;

Expand Down Expand Up @@ -47,7 +47,7 @@ pub(crate) fn blocking_os_call(checker: &mut Checker, call: &ExprCall) {
if checker.semantic().in_async_context() {
if checker
.semantic()
.resolve_call_path(call.func.as_ref())
.resolve_qualified_name(call.func.as_ref())
.as_ref()
.is_some_and(is_unsafe_os_method)
{
Expand All @@ -60,9 +60,9 @@ pub(crate) fn blocking_os_call(checker: &mut Checker, call: &ExprCall) {
}
}

fn is_unsafe_os_method(call_path: &CallPath) -> bool {
fn is_unsafe_os_method(qualified_name: &QualifiedName) -> bool {
matches!(
call_path.segments(),
qualified_name.segments(),
[
"os",
"popen"
Expand Down
Loading

0 comments on commit b0cff02

Please sign in to comment.