Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement any_over_expr for type alias and type params #5866

Merged
merged 5 commits into from
Jul 19, 2023
Merged
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 150 additions & 4 deletions crates/ruff_python_ast/src/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use rustc_hash::FxHashMap;
use rustpython_ast::CmpOp;
use rustpython_parser::ast::{
self, Arguments, Constant, ExceptHandler, Expr, Keyword, MatchCase, Pattern, Ranged, Stmt,
TypeParam,
};
use rustpython_parser::{lexer, Mode, Tok};
use smallvec::SmallVec;
Expand Down Expand Up @@ -265,6 +266,19 @@ where
}
}

pub fn any_over_type_param<F>(type_param: &TypeParam, func: &F) -> bool
where
F: Fn(&Expr) -> bool,
{
match type_param {
TypeParam::TypeVar(ast::TypeParamTypeVar { bound, .. }) => bound
.as_ref()
.map_or(false, |value| any_over_expr(value, func)),
TypeParam::TypeVarTuple(ast::TypeParamTypeVarTuple { .. }) => false,
TypeParam::ParamSpec(ast::TypeParamParamSpec { .. }) => false,
}
}

pub fn any_over_pattern<F>(pattern: &Pattern, func: &F) -> bool
where
F: Fn(&Expr) -> bool,
Expand Down Expand Up @@ -391,6 +405,18 @@ where
targets,
range: _range,
}) => targets.iter().any(|expr| any_over_expr(expr, func)),
Stmt::TypeAlias(ast::StmtTypeAlias {
name,
type_params,
value,
..
}) => {
any_over_expr(name, func)
|| type_params
.iter()
.any(|type_param| any_over_type_param(type_param, func))
|| any_over_expr(value, func)
}
Stmt::Assign(ast::StmtAssign { targets, value, .. }) => {
targets.iter().any(|expr| any_over_expr(expr, func)) || any_over_expr(value, func)
}
Expand Down Expand Up @@ -539,7 +565,6 @@ where
range: _range,
}) => any_over_expr(value, func),
Stmt::Pass(_) | Stmt::Break(_) | Stmt::Continue(_) => false,
Stmt::TypeAlias(_) => todo!(),
}
}

Expand Down Expand Up @@ -1564,15 +1589,22 @@ pub fn locate_cmp_ops(expr: &Expr, locator: &Locator) -> Vec<LocatedCmpOp> {
mod tests {
use std::borrow::Cow;

use std::cell::RefCell;

use std::vec;

use anyhow::Result;
use ruff_text_size::{TextLen, TextRange, TextSize};
use rustpython_ast::{CmpOp, Expr, Ranged};
use rustpython_ast::{
self, CmpOp, Constant, Expr, ExprConstant, ExprContext, ExprName, Identifier, Ranged, Stmt,
StmtTypeAlias, TypeParam, TypeParamParamSpec, TypeParamTypeVar, TypeParamTypeVarTuple,
};
use rustpython_parser::ast::Suite;
use rustpython_parser::Parse;

use crate::helpers::{
first_colon_range, has_trailing_content, locate_cmp_ops, resolve_imported_module_path,
LocatedCmpOp,
any_over_stmt, any_over_type_param, first_colon_range, has_trailing_content,
locate_cmp_ops, resolve_imported_module_path, LocatedCmpOp,
};
use crate::source_code::Locator;

Expand Down Expand Up @@ -1746,4 +1778,118 @@ y = 2

Ok(())
}

#[test]
fn any_over_stmt_type_alias() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is a bit contrived but I'd appreciate any general Rust feedback as I'm using it for practice!

Copy link
Member

@MichaReiser MichaReiser Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine. Or let's say, it's what the API allows you to do. You could have considered changing the Fn to FnMut so that you don't need to use RefCell (I guess it was a good learning opportunity ;)).

I'm normally too lazy to bother with building the tree manually and instead parse the source code. Now, using the source code makes asserting a bit annoying because you don't have access to the node instances. That's where you could collect NodeKinds instead (you can get the kind by calling AnyNodeRef::from(expr).kind()). However, NodeKind only works if you never have to distinguish between two nodes of the same kind. Here some tests that I wrote that use NodeKind.

#[test]
fn function_arguments() {
let source = r#"def a(b, c,/, d, e = 20, *args, named=5, other=20, **kwargs): pass"#;
let trace = trace_preorder_visitation(source);
assert_snapshot!(trace);
}
#[test]
fn function_positional_only_with_default() {
let source = r#"def a(b, c = 34,/, e = 20, *args): pass"#;
let trace = trace_preorder_visitation(source);
assert_snapshot!(trace);
}
#[test]
fn compare() {
let source = r#"4 < x < 5"#;
let trace = trace_preorder_visitation(source);
assert_snapshot!(trace);
}
#[test]
fn list_comprehension() {
let source = "[x for x in numbers]";
let trace = trace_preorder_visitation(source);
assert_snapshot!(trace);
}
#[test]
fn dict_comprehension() {
let source = "{x: x**2 for x in numbers}";
let trace = trace_preorder_visitation(source);
assert_snapshot!(trace);
}
#[test]
fn set_comprehension() {
let source = "{x for x in numbers}";
let trace = trace_preorder_visitation(source);
assert_snapshot!(trace);
}
#[test]
fn match_class_pattern() {
let source = r#"
match x:
case Point2D(0, 0):
...
case Point3D(x=0, y=0, z=0):
...
"#;
let trace = trace_preorder_visitation(source);
assert_snapshot!(trace);
}
#[test]
fn decorators() {
let source = r#"
@decorator
def a():
pass
@test
class A:
pass
"#;
let trace = trace_preorder_visitation(source);
assert_snapshot!(trace);
}

With an example snapshot

---
source: crates/ruff_python_ast/src/visitor/preorder.rs
expression: trace
---
- ModModule
- StmtExpr
- ExprCompare
- ExprConstant
- Int(4)
- Lt
- ExprName
- Lt
- ExprConstant
- Int(5)

That said. I think what you have is good. The only thing I'm not a 100% sure of is if I would bother refactoring this test if I ever end up breaking it (thinking about added value / maintenance cost)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look!

You could have considered changing the Fn to FnMut

It seems wrong to change the API just for testing.

I'm not a 100% sure of is if I would bother refactoring this test if I ever end up breaking it

Definitely. That's kind of what I was getting at with it being a bit contrived.

I'm normally too lazy to bother with building the tree manually and instead parse the source code.

Ah that's very wise. I feel like that's much more maintainable. I'll look into doing that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh this is kind of hard to do. I'd rather address it in a follow-up or next time someone wants to add a test here.

let seen = RefCell::new(Vec::new());
let name = Expr::Name(ExprName {
id: "x".to_string(),
range: TextRange::default(),
ctx: ExprContext::Load,
});
let constant_one = Expr::Constant(ExprConstant {
value: Constant::Int(1.into()),
kind: Some("x".to_string()),
range: TextRange::default(),
});
let constant_two = Expr::Constant(ExprConstant {
value: Constant::Int(2.into()),
kind: Some("y".to_string()),
range: TextRange::default(),
});
let constant_three = Expr::Constant(ExprConstant {
value: Constant::Int(3.into()),
kind: Some("z".to_string()),
range: TextRange::default(),
});
let type_var_one = TypeParam::TypeVar(TypeParamTypeVar {
range: TextRange::default(),
bound: Some(Box::new(constant_one.clone())),
name: Identifier::new("x", TextRange::default()),
});
let type_var_two = TypeParam::TypeVar(TypeParamTypeVar {
range: TextRange::default(),
bound: Some(Box::new(constant_two.clone())),
name: Identifier::new("x", TextRange::default()),
});
let type_alias = Stmt::TypeAlias(StmtTypeAlias {
name: Box::new(name.clone()),
type_params: vec![type_var_one, type_var_two],
value: Box::new(constant_three.clone()),
range: TextRange::default(),
});
assert_eq!(
any_over_stmt(&type_alias, &|expr| {
seen.borrow_mut().push(expr.clone());
false
}),
false
);
assert_eq!(
seen.take(),
vec![name, constant_one, constant_two, constant_three]
)
}

#[test]
fn any_over_type_param_type_var() {
let type_var_no_bound = TypeParam::TypeVar(TypeParamTypeVar {
range: TextRange::default(),
bound: None,
name: Identifier::new("x", TextRange::default()),
});
assert_eq!(
zanieb marked this conversation as resolved.
Show resolved Hide resolved
any_over_type_param(&type_var_no_bound, &|_expr| true),
false
);

let bound = Expr::Constant(ExprConstant {
value: Constant::Int(1.into()),
kind: Some("x".to_string()),
range: TextRange::default(),
});

let type_var_with_bound = TypeParam::TypeVar(TypeParamTypeVar {
range: TextRange::default(),
bound: Some(Box::new(bound.clone())),
name: Identifier::new("x", TextRange::default()),
});
assert_eq!(
any_over_type_param(&type_var_with_bound, &|expr| {
assert_eq!(
*expr, bound,
"the received expression should be the unwrapped bound"
);
true
}),
true,
"if true is returned from `func` it should be respected"
);
}

#[test]
fn any_over_type_param_type_var_tuple() {
let type_var_tuple = TypeParam::TypeVarTuple(TypeParamTypeVarTuple {
range: TextRange::default(),
name: Identifier::new("x", TextRange::default()),
});
assert_eq!(
any_over_type_param(&type_var_tuple, &|_expr| true),
false,
"type var tuples have no expressions to visit"
);
}

#[test]
fn any_over_type_param_param_spec() {
let type_param_spec = TypeParam::ParamSpec(TypeParamParamSpec {
range: TextRange::default(),
name: Identifier::new("x", TextRange::default()),
});
assert_eq!(
any_over_type_param(&type_param_spec, &|_expr| true),
false,
"param specs have no expressions to visit"
);
}
}