Skip to content

Commit

Permalink
Fix pd detection
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Jan 6, 2023
1 parent d34e6c0 commit 7781961
Show file tree
Hide file tree
Showing 4 changed files with 272 additions and 76 deletions.
17 changes: 0 additions & 17 deletions src/ast/helpers.rs
Expand Up @@ -213,23 +213,6 @@ pub fn is_constant_non_singleton(expr: &Expr) -> bool {
is_constant(expr) && !is_singleton(expr)
}

/// Return `true` if an `Expr` is not a reference to a variable (or something
/// that could resolve to a variable, like a function call).
pub fn is_non_variable(expr: &Expr) -> bool {
matches!(
expr.node,
ExprKind::Constant { .. }
| ExprKind::Tuple { .. }
| ExprKind::List { .. }
| ExprKind::Set { .. }
| ExprKind::Dict { .. }
| ExprKind::SetComp { .. }
| ExprKind::ListComp { .. }
| ExprKind::DictComp { .. }
| ExprKind::GeneratorExp { .. }
)
}

/// Return the `Keyword` with the given name, if it's present in the list of
/// `Keyword` arguments.
pub fn find_keyword<'a>(keywords: &'a [Keyword], keyword_name: &str) -> Option<&'a Keyword> {
Expand Down
77 changes: 67 additions & 10 deletions src/checkers/ast.rs
Expand Up @@ -191,13 +191,18 @@ impl<'a> Checker<'a> {
&& match_call_path(call_path, "typing_extensions", target, &self.from_imports))
}

/// Return `true` if `member` is bound as a builtin.
pub fn is_builtin(&self, member: &str) -> bool {
/// Return the current `Binding` for a given `name`.
pub fn find_binding(&self, member: &str) -> Option<&Binding> {
self.current_scopes()
.find_map(|scope| scope.values.get(member))
.map_or(false, |index| {
matches!(self.bindings[*index].kind, BindingKind::Builtin)
})
.map(|index| &self.bindings[*index])
}

/// Return `true` if `member` is bound as a builtin.
pub fn is_builtin(&self, member: &str) -> bool {
self.find_binding(member).map_or(false, |binding| {
matches!(binding.kind, BindingKind::Builtin)
})
}

/// Return `true` if a `CheckCode` is disabled by a `noqa` directive.
Expand Down Expand Up @@ -1727,10 +1732,30 @@ where
}
}
// Avoid flagging on non-DataFrames (e.g., `{"a": 1}.values`).
if helpers::is_non_variable(value) {
continue;
if pandas_vet::helpers::is_dataframe_candidate(value) {
// If the target is a named variable, avoid triggering on
// irrelevant bindings (like imports).
if let ExprKind::Name { id, .. } = &value.node {
if self.find_binding(id).map_or(true, |binding| {
matches!(
binding.kind,
BindingKind::Builtin
| BindingKind::ClassDefinition
| BindingKind::FunctionDefinition
| BindingKind::Export(..)
| BindingKind::FutureImportation
| BindingKind::StarImportation(..)
| BindingKind::Importation(..)
| BindingKind::FromImportation(..)
| BindingKind::SubmoduleImportation(..)
)
}) {
continue;
}
}

self.add_check(Check::new(code.kind(), Range::from_located(expr)));
}
self.add_check(Check::new(code.kind(), Range::from_located(expr)));
};
}
}
Expand Down Expand Up @@ -2158,9 +2183,41 @@ where
(CheckCode::PD013, "stack"),
] {
if self.settings.enabled.contains(&code) {
if let ExprKind::Attribute { attr, .. } = &func.node {
if let ExprKind::Attribute { value, attr, .. } = &func.node {
if attr == name {
self.add_check(Check::new(code.kind(), Range::from_located(func)));
if pandas_vet::helpers::is_dataframe_candidate(value) {
// If the target is a named variable, avoid triggering on
// irrelevant bindings (like non-Pandas imports).
if let ExprKind::Name { id, .. } = &value.node {
if self.find_binding(id).map_or(true, |binding| {
if let BindingKind::Importation(.., module) =
&binding.kind
{
module != "pandas"
} else {
matches!(
binding.kind,
BindingKind::Builtin
| BindingKind::ClassDefinition
| BindingKind::FunctionDefinition
| BindingKind::Export(..)
| BindingKind::FutureImportation
| BindingKind::StarImportation(..)
| BindingKind::Importation(..)
| BindingKind::FromImportation(..)
| BindingKind::SubmoduleImportation(..)
)
}
}) {
continue;
}
}

self.add_check(Check::new(
code.kind(),
Range::from_located(func),
));
}
};
}
}
Expand Down
18 changes: 18 additions & 0 deletions src/pandas_vet/helpers.rs
@@ -0,0 +1,18 @@
use rustpython_ast::{Expr, ExprKind};

/// Return `true` if an `Expr` _could_ be a `DataFrame`. This rules out
/// obviously-wrong cases, like constants and literals.
pub fn is_dataframe_candidate(expr: &Expr) -> bool {
!matches!(
expr.node,
ExprKind::Constant { .. }
| ExprKind::Tuple { .. }
| ExprKind::List { .. }
| ExprKind::Set { .. }
| ExprKind::Dict { .. }
| ExprKind::SetComp { .. }
| ExprKind::ListComp { .. }
| ExprKind::DictComp { .. }
| ExprKind::GeneratorExp { .. }
)
}

0 comments on commit 7781961

Please sign in to comment.