Skip to content

Commit

Permalink
Implement iter(), len() and is_empty() for all display-literal …
Browse files Browse the repository at this point in the history
…AST nodes (#12807)
  • Loading branch information
AlexWaygood authored Aug 12, 2024
1 parent a99a458 commit aa0db33
Show file tree
Hide file tree
Showing 56 changed files with 304 additions and 240 deletions.
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1290,8 +1290,8 @@ impl<'a> Visitor<'a> for Checker<'a> {
let Keyword { arg, value, .. } = keyword;
match (arg.as_ref(), value) {
// Ex) NamedTuple("a", **{"a": int})
(None, Expr::Dict(ast::ExprDict { items, .. })) => {
for ast::DictItem { key, value } in items {
(None, Expr::Dict(dict)) => {
for ast::DictItem { key, value } in dict {
if let Some(key) = key.as_ref() {
self.visit_non_type_definition(key);
self.visit_type_definition(value);
Expand Down
9 changes: 4 additions & 5 deletions crates/ruff_linter/src/fix/edits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,24 +151,23 @@ pub(crate) fn add_to_dunder_all<'a>(
stylist: &Stylist,
) -> Vec<Edit> {
let (insertion_point, export_prefix_length) = match expr {
Expr::List(ExprList { elts, range, .. }) => (
elts.last()
.map_or(range.end() - "]".text_len(), Ranged::end),
Expr::List(ExprList { elts, .. }) => (
elts.last().map_or(expr.end() - "]".text_len(), Ranged::end),
elts.len(),
),
Expr::Tuple(tup) if tup.parenthesized => (
tup.elts
.last()
.map_or(tup.end() - ")".text_len(), Ranged::end),
tup.elts.len(),
tup.len(),
),
Expr::Tuple(tup) if !tup.parenthesized => (
tup.elts
.last()
.expect("unparenthesized empty tuple is not possible")
.range()
.end(),
tup.elts.len(),
tup.len(),
),
_ => {
// we don't know how to insert into this expression
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,17 +122,15 @@ fn is_identical_types(
return_value: &Expr,
semantic: &SemanticModel,
) -> bool {
if let (Some(response_mode_name_expr), Some(return_value_name_expr)) = (
response_model_arg.as_name_expr(),
return_value.as_name_expr(),
) {
if let (Expr::Name(response_mode_name_expr), Expr::Name(return_value_name_expr)) =
(response_model_arg, return_value)
{
return semantic.resolve_name(response_mode_name_expr)
== semantic.resolve_name(return_value_name_expr);
}
if let (Some(response_mode_subscript), Some(return_value_subscript)) = (
response_model_arg.as_subscript_expr(),
return_value.as_subscript_expr(),
) {
if let (Expr::Subscript(response_mode_subscript), Expr::Subscript(return_value_subscript)) =
(response_model_arg, return_value)
{
return is_identical_types(
&response_mode_subscript.value,
&return_value_subscript.value,
Expand All @@ -143,15 +141,13 @@ fn is_identical_types(
semantic,
);
}
if let (Some(response_mode_tuple), Some(return_value_tuple)) = (
response_model_arg.as_tuple_expr(),
return_value.as_tuple_expr(),
) {
return response_mode_tuple.elts.len() == return_value_tuple.elts.len()
if let (Expr::Tuple(response_mode_tuple), Expr::Tuple(return_value_tuple)) =
(response_model_arg, return_value)
{
return response_mode_tuple.len() == return_value_tuple.len()
&& response_mode_tuple
.elts
.iter()
.zip(return_value_tuple.elts.iter())
.zip(return_value_tuple)
.all(|(x, y)| is_identical_types(x, y, semantic));
}
false
Expand Down
10 changes: 5 additions & 5 deletions crates/ruff_linter/src/rules/flake8_bandit/rules/django_extra.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr, ExprAttribute, ExprDict, ExprList};
use ruff_python_ast::{self as ast, Expr, ExprAttribute};
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
Expand Down Expand Up @@ -65,8 +65,8 @@ fn is_call_insecure(call: &ast::ExprCall) -> bool {
if let Some(argument) = call.arguments.find_argument(argument_name, position) {
match argument_name {
"select" => match argument {
Expr::Dict(ExprDict { items, .. }) => {
if items.iter().any(|ast::DictItem { key, value }| {
Expr::Dict(dict) => {
if dict.iter().any(|ast::DictItem { key, value }| {
key.as_ref()
.is_some_and(|key| !key.is_string_literal_expr())
|| !value.is_string_literal_expr()
Expand All @@ -77,8 +77,8 @@ fn is_call_insecure(call: &ast::ExprCall) -> bool {
_ => return true,
},
"where" | "tables" => match argument {
Expr::List(ExprList { elts, .. }) => {
if !elts.iter().all(Expr::is_string_literal_expr) {
Expr::List(list) => {
if !list.iter().all(Expr::is_string_literal_expr) {
return true;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -530,11 +530,11 @@ fn is_partial_path(expr: &Expr) -> bool {
/// subprocess.Popen(["/usr/local/bin/rsync", "*", "some_where:"], shell=True)
/// ```
fn is_wildcard_command(expr: &Expr) -> bool {
if let Expr::List(ast::ExprList { elts, .. }) = expr {
if let Expr::List(list) = expr {
let mut has_star = false;
let mut has_command = false;
for elt in elts {
if let Some(text) = string_literal(elt) {
for item in list {
if let Some(text) = string_literal(item) {
has_star |= text.contains('*');
has_command |= text.contains("chown")
|| text.contains("chmod")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,16 @@ impl Violation for DuplicateValue {
/// B033
pub(crate) fn duplicate_value(checker: &mut Checker, set: &ast::ExprSet) {
let mut seen_values: FxHashSet<ComparableExpr> = FxHashSet::default();
for (index, elt) in set.elts.iter().enumerate() {
if elt.is_literal_expr() {
let comparable_value: ComparableExpr = elt.into();
for (index, value) in set.iter().enumerate() {
if value.is_literal_expr() {
let comparable_value = ComparableExpr::from(value);

if !seen_values.insert(comparable_value) {
let mut diagnostic = Diagnostic::new(
DuplicateValue {
value: checker.generator().expr(elt),
value: checker.generator().expr(value),
},
elt.range(),
value.range(),
);

diagnostic.try_set_fix(|| {
Expand All @@ -73,7 +73,7 @@ pub(crate) fn duplicate_value(checker: &mut Checker, set: &ast::ExprSet) {

/// Remove the member at the given index from the [`ast::ExprSet`].
fn remove_member(set: &ast::ExprSet, index: usize, source: &str) -> Result<Edit> {
if index < set.elts.len() - 1 {
if index < set.len() - 1 {
// Case 1: the expression is _not_ the last node, so delete from the start of the
// expression to the end of the subsequent comma.
// Ex) Delete `"a"` in `{"a", "b", "c"}`.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -315,15 +315,15 @@ pub(crate) fn reuse_of_groupby_generator(
let Expr::Call(ast::ExprCall { func, .. }) = &iter else {
return;
};
let Expr::Tuple(ast::ExprTuple { elts, .. }) = target else {
let Expr::Tuple(tuple) = target else {
// Ignore any `groupby()` invocation that isn't unpacked
return;
};
if elts.len() != 2 {
if tuple.len() != 2 {
return;
}
// We have an invocation of groupby which is a simple unpacking
let Expr::Name(ast::ExprName { id: group_name, .. }) = &elts[1] else {
let Expr::Name(ast::ExprName { id: group_name, .. }) = &tuple.elts[1] else {
return;
};
// Check if the function call is `itertools.groupby`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ pub(crate) fn static_key_dict_comprehension(checker: &mut Checker, dict_comp: &a
/// comprehension.
fn is_constant(key: &Expr, names: &FxHashMap<&str, &ast::ExprName>) -> bool {
match key {
Expr::Tuple(ast::ExprTuple { elts, .. }) => elts.iter().all(|elt| is_constant(elt, names)),
Expr::Tuple(tuple) => tuple.iter().all(|elem| is_constant(elem, names)),
Expr::Name(ast::ExprName { id, .. }) => !names.contains_key(id.as_str()),
Expr::Attribute(ast::ExprAttribute { value, .. }) => is_constant(value, names),
Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,13 @@ pub(crate) fn unnecessary_generator_dict(
let Expr::Generator(ast::ExprGenerator { elt, .. }) = argument else {
return;
};
let Expr::Tuple(ast::ExprTuple { elts, .. }) = elt.as_ref() else {
let Expr::Tuple(tuple) = &**elt else {
return;
};
if elts.len() != 2 {
if tuple.len() != 2 {
return;
}
if elts.iter().any(Expr::is_starred_expr) {
if tuple.iter().any(Expr::is_starred_expr) {
return;
}
let mut diagnostic = Diagnostic::new(UnnecessaryGeneratorDict, expr.range());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,10 @@ pub(crate) fn unnecessary_list_comprehension_dict(
let Expr::ListComp(ast::ExprListComp { elt, .. }) = argument else {
return;
};
let Expr::Tuple(ast::ExprTuple { elts, .. }) = elt.as_ref() else {
let Expr::Tuple(tuple) = &**elt else {
return;
};
if elts.len() != 2 {
if tuple.len() != 2 {
return;
}
let mut diagnostic = Diagnostic::new(UnnecessaryListComprehensionDict, expr.range());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ pub(crate) fn unnecessary_literal_dict(
// Accept `dict((1, 2), ...))` `dict([(1, 2), ...])`.
if !elts
.iter()
.all(|elt| matches!(&elt, Expr::Tuple(ast::ExprTuple { elts, .. }) if elts.len() == 2))
.all(|elt| matches!(&elt, Expr::Tuple(tuple) if tuple.len() == 2))
{
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,8 @@ pub(crate) fn multiple_starts_ends_with(checker: &mut Checker, expr: &Expr) {
elts: words
.iter()
.flat_map(|value| {
if let Expr::Tuple(ast::ExprTuple { elts, .. }) = value {
Left(elts.iter())
if let Expr::Tuple(tuple) = value {
Left(tuple.iter())
} else {
Right(iter::once(*value))
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use ruff_python_ast::{self as ast, Expr, ExprLambda};
use ruff_python_ast::{Expr, ExprLambda};

use ruff_diagnostics::{Diagnostic, Edit, Fix};
use ruff_diagnostics::{FixAvailability, Violation};
Expand Down Expand Up @@ -70,8 +70,8 @@ pub(crate) fn reimplemented_container_builtin(checker: &mut Checker, expr: &Expr
}

let container = match &**body {
Expr::List(ast::ExprList { elts, .. }) if elts.is_empty() => Container::List,
Expr::Dict(ast::ExprDict { items, .. }) if items.is_empty() => Container::Dict,
Expr::List(list) if list.is_empty() => Container::List,
Expr::Dict(dict) if dict.is_empty() => Container::Dict,
_ => return,
};
let mut diagnostic = Diagnostic::new(ReimplementedContainerBuiltin { container }, expr.range());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,13 +87,13 @@ pub(crate) fn unnecessary_dict_kwargs(checker: &mut Checker, call: &ast::ExprCal
.iter_keys()
.filter_map(|key| key.and_then(as_kwarg))
.collect();
if kwargs.len() != dict.items.len() {
if kwargs.len() != dict.len() {
continue;
}

let mut diagnostic = Diagnostic::new(UnnecessaryDictKwargs, keyword.range());

if dict.items.is_empty() {
if dict.is_empty() {
diagnostic.try_set_fix(|| {
remove_argument(
keyword,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl Violation for UnnecessarySpread {
pub(crate) fn unnecessary_spread(checker: &mut Checker, dict: &ast::ExprDict) {
// The first "end" is the start of the dictionary, immediately following the open bracket.
let mut prev_end = dict.start() + TextSize::from(1);
for ast::DictItem { key, value } in &dict.items {
for ast::DictItem { key, value } in dict {
if key.is_none() {
// We only care about when the key is None which indicates a spread `**`
// inside a dict.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,11 @@ pub(crate) fn bad_generator_return_type(
// - if not, don't emit the diagnostic
let yield_type_info = match returns {
ast::Expr::Subscript(ast::ExprSubscript { slice, .. }) => match slice.as_ref() {
ast::Expr::Tuple(slice_tuple @ ast::ExprTuple { .. }) => {
ast::Expr::Tuple(slice_tuple) => {
if !slice_tuple
.elts
.iter()
.skip(1)
.all(|elt| is_any_or_none(elt, semantic))
.all(|element| is_any_or_none(element, semantic))
{
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ pub(crate) fn redundant_literal_union<'a>(checker: &mut Checker, union: &'a Expr
let mut func = |expr: &'a Expr, _parent: &'a Expr| {
if let Expr::Subscript(ast::ExprSubscript { value, slice, .. }) = expr {
if checker.semantic().match_typing_expr(value, "Literal") {
if let Expr::Tuple(ast::ExprTuple { elts, .. }) = slice.as_ref() {
typing_literal_exprs.extend(elts.iter());
if let Expr::Tuple(tuple) = &**slice {
typing_literal_exprs.extend(tuple);
} else {
typing_literal_exprs.push(slice);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -298,10 +298,10 @@ fn is_valid_default_value_with_annotation(
.iter()
.all(|e| is_valid_default_value_with_annotation(e, false, locator, semantic));
}
Expr::Dict(ast::ExprDict { items, range: _ }) => {
Expr::Dict(dict) => {
return allow_container
&& items.len() <= 10
&& items.iter().all(|ast::DictItem { key, value }| {
&& dict.len() <= 10
&& dict.iter().all(|ast::DictItem { key, value }| {
key.as_ref().is_some_and(|key| {
is_valid_default_value_with_annotation(key, false, locator, semantic)
}) && is_valid_default_value_with_annotation(value, false, locator, semantic)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,19 +70,15 @@ pub(crate) fn unnecessary_literal_union<'a>(checker: &mut Checker, expr: &'a Exp
literal_subscript = Some(value.as_ref());
}

let slice = &**slice;

// flatten already-unioned literals to later union again
if let Expr::Tuple(ast::ExprTuple {
elts,
range: _,
ctx: _,
parenthesized: _,
}) = slice.as_ref()
{
for expr in elts {
literal_exprs.push(expr);
if let Expr::Tuple(tuple) = slice {
for item in tuple {
literal_exprs.push(item);
}
} else {
literal_exprs.push(slice.as_ref());
literal_exprs.push(slice);
}
}
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ fn version_check(
}

// Tuple comparison, e.g., `sys.version_info == (3, 4)`.
let Expr::Tuple(ast::ExprTuple { elts, .. }) = comparator else {
let Expr::Tuple(tuple) = comparator else {
if checker.enabled(Rule::UnrecognizedVersionInfoCheck) {
checker
.diagnostics
Expand All @@ -190,15 +190,15 @@ fn version_check(
return;
};

if !elts.iter().all(is_int_constant) {
if !tuple.iter().all(is_int_constant) {
// All tuple elements must be integers, e.g., `sys.version_info == (3, 4)` instead of
// `sys.version_info == (3.0, 4)`.
if checker.enabled(Rule::UnrecognizedVersionInfoCheck) {
checker
.diagnostics
.push(Diagnostic::new(UnrecognizedVersionInfoCheck, test.range()));
}
} else if elts.len() > 2 {
} else if tuple.len() > 2 {
// Must compare against major and minor version only, e.g., `sys.version_info == (3, 4)`
// instead of `sys.version_info == (3, 4, 0)`.
if checker.enabled(Rule::PatchVersionComparison) {
Expand All @@ -216,7 +216,7 @@ fn version_check(
_ => return,
};

if elts.len() != expected_length {
if tuple.len() != expected_length {
checker.diagnostics.push(Diagnostic::new(
WrongTupleLengthVersionComparison { expected_length },
test.range(),
Expand Down
Loading

0 comments on commit aa0db33

Please sign in to comment.