mirror of
https://github.com/astral-sh/ruff.git
synced 2025-08-30 07:08:07 +00:00
New AST nodes for f-string elements (#8835)
Rebase of #6365 authored by @davidszotten. ## Summary This PR updates the AST structure for an f-string elements. The main **motivation** behind this change is to have a dedicated node for the string part of an f-string. Previously, the existing `ExprStringLiteral` node was used for this purpose which isn't exactly correct. The `ExprStringLiteral` node should include the quotes as well in the range but the f-string literal element doesn't include the quote as it's a specific part within an f-string. For example, ```python f"foo {x}" # ^^^^ # This is the literal part of an f-string ``` The introduction of `FStringElement` enum is helpful which represent either the literal part or the expression part of an f-string. ### Rule Updates This means that there'll be two nodes representing a string depending on the context. One for a normal string literal while the other is a string literal within an f-string. The AST checker is updated to accommodate this change. The rules which work on string literal are updated to check on the literal part of f-string as well. #### Notes 1. The `Expr::is_literal_expr` method would check for `ExprStringLiteral` and return true if so. But now that we don't represent the literal part of an f-string using that node, this improves the method's behavior and confines to the actual expression. We do have the `FStringElement::is_literal` method. 2. We avoid checking if we're in a f-string context before adding to `string_type_definitions` because the f-string literal is now a dedicated node and not part of `Expr`. 3. Annotations cannot use f-string so we avoid changing any rules which work on annotation and checks for `ExprStringLiteral`. ## Test Plan - All references of `Expr::StringLiteral` were checked to see if any of the rules require updating to account for the f-string literal element node. - New test cases are added for rules which check against the literal part of an f-string. - Check the ecosystem results and ensure it remains unchanged. ## Performance There's a performance penalty in the parser. The reason for this remains unknown as it seems that the generated assembly code is now different for the `__reduce154` function. The reduce function body is just popping the `ParenthesizedExpr` on top of the stack and pushing it with the new location. - The size of `FStringElement` enum is the same as `Expr` which is what it replaces in `FString::format_spec` - The size of `FStringExpressionElement` is the same as `ExprFormattedValue` which is what it replaces I tried reducing the `Expr` enum from 80 bytes to 72 bytes but it hardly resulted in any performance gain. The difference can be seen here: - Original profile: https://share.firefox.dev/3Taa7ES - Profile after boxing some node fields: https://share.firefox.dev/3GsNXpD ### Backtracking I tried backtracking the changes to see if any of the isolated change produced this regression. The problem here is that the overall change is so small that there's only a single checkpoint where I can backtrack and that checkpoint results in the same regression. This checkpoint is to revert using `Expr` to the `FString::format_spec` field. After this point, the change would revert back to the original implementation. ## Review process The review process is similar to #7927. The first set of commits update the node structure, parser, and related AST files. Then, further commits update the linter and formatter part to account for the AST change. --------- Co-authored-by: David Szotten <davidszotten@gmail.com>
This commit is contained in:
parent
fcc08894cf
commit
cdac90ef68
77 changed files with 1714 additions and 1925 deletions
|
@ -1,6 +1,6 @@
|
|||
use crate::{
|
||||
Alias, Arguments, BoolOp, BytesLiteral, CmpOp, Comprehension, Decorator, ElifElseClause,
|
||||
ExceptHandler, Expr, FString, Keyword, MatchCase, Mod, Operator, Parameter,
|
||||
ExceptHandler, Expr, FString, FStringElement, Keyword, MatchCase, Mod, Operator, Parameter,
|
||||
ParameterWithDefault, Parameters, Pattern, PatternArguments, PatternKeyword, Singleton, Stmt,
|
||||
StringLiteral, TypeParam, TypeParams, UnaryOp, WithItem,
|
||||
};
|
||||
|
@ -74,11 +74,6 @@ pub trait PreorderVisitor<'a> {
|
|||
walk_except_handler(self, except_handler);
|
||||
}
|
||||
|
||||
#[inline]
|
||||
fn visit_format_spec(&mut self, format_spec: &'a Expr) {
|
||||
walk_format_spec(self, format_spec);
|
||||
}
|
||||
|
||||
#[inline]
|
||||
fn visit_arguments(&mut self, arguments: &'a Arguments) {
|
||||
walk_arguments(self, arguments);
|
||||
|
@ -158,6 +153,11 @@ pub trait PreorderVisitor<'a> {
|
|||
walk_f_string(self, f_string);
|
||||
}
|
||||
|
||||
#[inline]
|
||||
fn visit_f_string_element(&mut self, f_string_element: &'a FStringElement) {
|
||||
walk_f_string_element(self, f_string_element);
|
||||
}
|
||||
|
||||
#[inline]
|
||||
fn visit_string_literal(&mut self, string_literal: &'a StringLiteral) {
|
||||
walk_string_literal(self, string_literal);
|
||||
|
@ -289,7 +289,6 @@ where
|
|||
Expr::YieldFrom(expr) => expr.visit_preorder(visitor),
|
||||
Expr::Compare(expr) => expr.visit_preorder(visitor),
|
||||
Expr::Call(expr) => expr.visit_preorder(visitor),
|
||||
Expr::FormattedValue(expr) => expr.visit_preorder(visitor),
|
||||
Expr::FString(expr) => expr.visit_preorder(visitor),
|
||||
Expr::StringLiteral(expr) => expr.visit_preorder(visitor),
|
||||
Expr::BytesLiteral(expr) => expr.visit_preorder(visitor),
|
||||
|
@ -518,6 +517,20 @@ where
|
|||
visitor.leave_node(node);
|
||||
}
|
||||
|
||||
pub fn walk_f_string_element<'a, V: PreorderVisitor<'a> + ?Sized>(
|
||||
visitor: &mut V,
|
||||
f_string_element: &'a FStringElement,
|
||||
) {
|
||||
let node = AnyNodeRef::from(f_string_element);
|
||||
if visitor.enter_node(node).is_traverse() {
|
||||
match f_string_element {
|
||||
FStringElement::Expression(element) => element.visit_preorder(visitor),
|
||||
FStringElement::Literal(element) => element.visit_preorder(visitor),
|
||||
}
|
||||
}
|
||||
visitor.leave_node(node);
|
||||
}
|
||||
|
||||
pub fn walk_bool_op<'a, V>(_visitor: &mut V, _bool_op: &'a BoolOp)
|
||||
where
|
||||
V: PreorderVisitor<'a> + ?Sized,
|
||||
|
|
|
@ -1,8 +1,8 @@
|
|||
use crate::{
|
||||
self as ast, Alias, Arguments, BoolOp, BytesLiteral, CmpOp, Comprehension, Decorator,
|
||||
ElifElseClause, ExceptHandler, Expr, ExprContext, FString, Keyword, MatchCase, Operator,
|
||||
Parameter, Parameters, Pattern, PatternArguments, PatternKeyword, Stmt, StringLiteral,
|
||||
TypeParam, TypeParamTypeVar, TypeParams, UnaryOp, WithItem,
|
||||
ElifElseClause, ExceptHandler, Expr, ExprContext, FString, FStringElement, Keyword, MatchCase,
|
||||
Operator, Parameter, Parameters, Pattern, PatternArguments, PatternKeyword, Stmt,
|
||||
StringLiteral, TypeParam, TypeParamTypeVar, TypeParams, UnaryOp, WithItem,
|
||||
};
|
||||
|
||||
/// A trait for transforming ASTs. Visits all nodes in the AST recursively in evaluation-order.
|
||||
|
@ -40,9 +40,6 @@ pub trait Transformer {
|
|||
fn visit_except_handler(&self, except_handler: &mut ExceptHandler) {
|
||||
walk_except_handler(self, except_handler);
|
||||
}
|
||||
fn visit_format_spec(&self, format_spec: &mut Expr) {
|
||||
walk_format_spec(self, format_spec);
|
||||
}
|
||||
fn visit_arguments(&self, arguments: &mut Arguments) {
|
||||
walk_arguments(self, arguments);
|
||||
}
|
||||
|
@ -88,6 +85,9 @@ pub trait Transformer {
|
|||
fn visit_f_string(&self, f_string: &mut FString) {
|
||||
walk_f_string(self, f_string);
|
||||
}
|
||||
fn visit_f_string_element(&self, f_string_element: &mut FStringElement) {
|
||||
walk_f_string_element(self, f_string_element);
|
||||
}
|
||||
fn visit_string_literal(&self, string_literal: &mut StringLiteral) {
|
||||
walk_string_literal(self, string_literal);
|
||||
}
|
||||
|
@ -463,14 +463,6 @@ pub fn walk_expr<V: Transformer + ?Sized>(visitor: &V, expr: &mut Expr) {
|
|||
visitor.visit_expr(func);
|
||||
visitor.visit_arguments(arguments);
|
||||
}
|
||||
Expr::FormattedValue(ast::ExprFormattedValue {
|
||||
value, format_spec, ..
|
||||
}) => {
|
||||
visitor.visit_expr(value);
|
||||
if let Some(expr) = format_spec {
|
||||
visitor.visit_format_spec(expr);
|
||||
}
|
||||
}
|
||||
Expr::FString(ast::ExprFString { value, .. }) => {
|
||||
for f_string_part in value.parts_mut() {
|
||||
match f_string_part {
|
||||
|
@ -584,16 +576,6 @@ pub fn walk_except_handler<V: Transformer + ?Sized>(
|
|||
}
|
||||
}
|
||||
|
||||
pub fn walk_f_string<V: Transformer + ?Sized>(visitor: &V, f_string: &mut FString) {
|
||||
for expr in &mut f_string.values {
|
||||
visitor.visit_expr(expr);
|
||||
}
|
||||
}
|
||||
|
||||
pub fn walk_format_spec<V: Transformer + ?Sized>(visitor: &V, format_spec: &mut Expr) {
|
||||
visitor.visit_expr(format_spec);
|
||||
}
|
||||
|
||||
pub fn walk_arguments<V: Transformer + ?Sized>(visitor: &V, arguments: &mut Arguments) {
|
||||
// Note that the there might be keywords before the last arg, e.g. in
|
||||
// f(*args, a=2, *args2, **kwargs)`, but we follow Python in evaluating first `args` and then
|
||||
|
@ -743,6 +725,31 @@ pub fn walk_pattern_keyword<V: Transformer + ?Sized>(
|
|||
visitor.visit_pattern(&mut pattern_keyword.pattern);
|
||||
}
|
||||
|
||||
pub fn walk_f_string<V: Transformer + ?Sized>(visitor: &V, f_string: &mut FString) {
|
||||
for element in &mut f_string.elements {
|
||||
visitor.visit_f_string_element(element);
|
||||
}
|
||||
}
|
||||
|
||||
pub fn walk_f_string_element<V: Transformer + ?Sized>(
|
||||
visitor: &V,
|
||||
f_string_element: &mut FStringElement,
|
||||
) {
|
||||
if let ast::FStringElement::Expression(ast::FStringExpressionElement {
|
||||
expression,
|
||||
format_spec,
|
||||
..
|
||||
}) = f_string_element
|
||||
{
|
||||
visitor.visit_expr(expression);
|
||||
if let Some(format_spec) = format_spec {
|
||||
for spec_element in &mut format_spec.elements {
|
||||
visitor.visit_f_string_element(spec_element);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
pub fn walk_expr_context<V: Transformer + ?Sized>(_visitor: &V, _expr_context: &mut ExprContext) {}
|
||||
|
||||
pub fn walk_bool_op<V: Transformer + ?Sized>(_visitor: &V, _bool_op: &mut BoolOp) {}
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue