Skip to content

Commit

Permalink
Parenthesize expressions prior to LibCST parsing (#6742)
Browse files Browse the repository at this point in the history
<!--
Thank you for contributing to Ruff! To help us out with reviewing,
please consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->

## Summary

This PR adds a utility for transforming expressions via LibCST that
automatically wraps the expression in parentheses, applies a
user-provided transformation, then strips the parentheses from the
generated code. LibCST can't parse arbitrary expression ranges, since
some expressions may require parenthesization in order to be parsed
properly. For example:

```python
option = (
    '{name}={value}'
    .format(nam=name, value=value)
)
```

In this case, the expression range is:

```python
'{name}={value}'
    .format(nam=name, value=value)
```

Which isn't valid on its own. So, instead, we add "fake" parentheses
around the expression.

We were already doing this in a few places, so this is mostly
formalizing and DRYing up that pattern.

Closes #6720.
  • Loading branch information
charliermarsh authored Aug 22, 2023
1 parent 5c1f7fd commit 214eb70
Show file tree
Hide file tree
Showing 12 changed files with 317 additions and 279 deletions.
3 changes: 1 addition & 2 deletions crates/ruff/resources/test/fixtures/pyflakes/F522.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,5 @@
"{bar}{}".format(1, bar=2, spam=3) # F522
"{bar:{spam}}".format(bar=2, spam=3) # No issues
"{bar:{spam}}".format(bar=2, spam=3, eggs=4, ham=5) # F522
# Not fixable
(''
.format(x=2))
.format(x=2)) # F522
2 changes: 1 addition & 1 deletion crates/ruff/resources/test/fixtures/pyflakes/F523.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,6 @@
"{1}{3}".format(1, 2, 3, 4) # F523, # F524
"{1} {8}".format(0, 1) # F523, # F524

# Not fixable
# Multiline
(''
.format(2))
12 changes: 6 additions & 6 deletions crates/ruff/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -381,34 +381,34 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
Ok(summary) => {
if checker.enabled(Rule::StringDotFormatExtraNamedArguments) {
pyflakes::rules::string_dot_format_extra_named_arguments(
checker, &summary, keywords, location,
checker, call, &summary, keywords,
);
}
if checker
.enabled(Rule::StringDotFormatExtraPositionalArguments)
{
pyflakes::rules::string_dot_format_extra_positional_arguments(
checker, &summary, args, location,
checker, call, &summary, args,
);
}
if checker.enabled(Rule::StringDotFormatMissingArguments) {
pyflakes::rules::string_dot_format_missing_argument(
checker, &summary, args, keywords, location,
checker, call, &summary, args, keywords,
);
}
if checker.enabled(Rule::StringDotFormatMixingAutomatic) {
pyflakes::rules::string_dot_format_mixing_automatic(
checker, &summary, location,
checker, call, &summary,
);
}
if checker.enabled(Rule::FormatLiterals) {
pyupgrade::rules::format_literals(checker, &summary, call);
pyupgrade::rules::format_literals(checker, call, &summary);
}
if checker.enabled(Rule::FString) {
pyupgrade::rules::f_strings(
checker,
call,
&summary,
expr,
value,
checker.settings.line_length,
);
Expand Down
65 changes: 58 additions & 7 deletions crates/ruff/src/cst/matchers.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
use crate::autofix::codemods::CodegenStylist;
use anyhow::{bail, Result};
use libcst_native::{
Arg, Attribute, Call, Comparison, CompoundStatement, Dict, Expression, FunctionDef,
GeneratorExp, If, Import, ImportAlias, ImportFrom, ImportNames, IndentedBlock, Lambda,
ListComp, Module, Name, SmallStatement, Statement, Suite, Tuple, With,
};
use ruff_python_codegen::Stylist;

pub(crate) fn match_module(module_text: &str) -> Result<Module> {
match libcst_native::parse_module(module_text, None) {
Expand All @@ -12,13 +14,6 @@ pub(crate) fn match_module(module_text: &str) -> Result<Module> {
}
}

pub(crate) fn match_expression(expression_text: &str) -> Result<Expression> {
match libcst_native::parse_expression(expression_text) {
Ok(expression) => Ok(expression),
Err(_) => bail!("Failed to extract expression from source"),
}
}

pub(crate) fn match_statement(statement_text: &str) -> Result<Statement> {
match libcst_native::parse_statement(statement_text) {
Ok(statement) => Ok(statement),
Expand Down Expand Up @@ -205,3 +200,59 @@ pub(crate) fn match_if<'a, 'b>(statement: &'a mut Statement<'b>) -> Result<&'a m
bail!("Expected Statement::Compound")
}
}

/// Given the source code for an expression, return the parsed [`Expression`].
///
/// If the expression is not guaranteed to be valid as a standalone expression (e.g., if it may
/// span multiple lines and/or require parentheses), use [`transform_expression`] instead.
pub(crate) fn match_expression(expression_text: &str) -> Result<Expression> {
match libcst_native::parse_expression(expression_text) {
Ok(expression) => Ok(expression),
Err(_) => bail!("Failed to extract expression from source"),
}
}

/// Run a transformation function over an expression.
///
/// Passing an expression to [`match_expression`] directly can lead to parse errors if the
/// expression is not a valid standalone expression (e.g., it was parenthesized in the original
/// source). This method instead wraps the expression in "fake" parentheses, runs the
/// transformation, then removes the "fake" parentheses.
pub(crate) fn transform_expression(
source_code: &str,
stylist: &Stylist,
func: impl FnOnce(Expression) -> Result<Expression>,
) -> Result<String> {
// Wrap the expression in parentheses.
let source_code = format!("({source_code})");
let expression = match_expression(&source_code)?;

// Run the function on the expression.
let expression = func(expression)?;

// Codegen the expression.
let mut source_code = expression.codegen_stylist(stylist);

// Drop the outer parentheses.
source_code.drain(0..1);
source_code.drain(source_code.len() - 1..source_code.len());
Ok(source_code)
}

/// Like [`transform_expression`], but operates on the source code of the expression, rather than
/// the parsed [`Expression`]. This _shouldn't_ exist, but does to accommodate lifetime issues.
pub(crate) fn transform_expression_text(
source_code: &str,
func: impl FnOnce(String) -> Result<String>,
) -> Result<String> {
// Wrap the expression in parentheses.
let source_code = format!("({source_code})");

// Run the function on the expression.
let mut transformed = func(source_code)?;

// Drop the outer parentheses.
transformed.drain(0..1);
transformed.drain(transformed.len() - 1..transformed.len());
Ok(transformed)
}
126 changes: 63 additions & 63 deletions crates/ruff/src/rules/flake8_simplify/rules/yoda_conditions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,9 @@ use ruff_python_codegen::Stylist;
use ruff_python_stdlib::str::{self};
use ruff_source_file::Locator;

use crate::autofix::codemods::CodegenStylist;
use crate::autofix::snippet::SourceCodeSnippet;
use crate::checkers::ast::Checker;
use crate::cst::matchers::{match_comparison, match_expression};
use crate::cst::matchers::{match_comparison, transform_expression};
use crate::registry::AsRule;

/// ## What it does
Expand Down Expand Up @@ -96,68 +95,69 @@ fn is_constant_like(expr: &Expr) -> bool {
/// Generate a fix to reverse a comparison.
fn reverse_comparison(expr: &Expr, locator: &Locator, stylist: &Stylist) -> Result<String> {
let range = expr.range();
let contents = locator.slice(range);

let mut expression = match_expression(contents)?;
let comparison = match_comparison(&mut expression)?;

let left = (*comparison.left).clone();

// Copy the right side to the left side.
comparison.left = Box::new(comparison.comparisons[0].comparator.clone());

// Copy the left side to the right side.
comparison.comparisons[0].comparator = left;

// Reverse the operator.
let op = comparison.comparisons[0].operator.clone();
comparison.comparisons[0].operator = match op {
CompOp::LessThan {
whitespace_before,
whitespace_after,
} => CompOp::GreaterThan {
whitespace_before,
whitespace_after,
},
CompOp::GreaterThan {
whitespace_before,
whitespace_after,
} => CompOp::LessThan {
whitespace_before,
whitespace_after,
},
CompOp::LessThanEqual {
whitespace_before,
whitespace_after,
} => CompOp::GreaterThanEqual {
whitespace_before,
whitespace_after,
},
CompOp::GreaterThanEqual {
whitespace_before,
whitespace_after,
} => CompOp::LessThanEqual {
whitespace_before,
whitespace_after,
},
CompOp::Equal {
whitespace_before,
whitespace_after,
} => CompOp::Equal {
whitespace_before,
whitespace_after,
},
CompOp::NotEqual {
whitespace_before,
whitespace_after,
} => CompOp::NotEqual {
whitespace_before,
whitespace_after,
},
_ => panic!("Expected comparison operator"),
};
let source_code = locator.slice(range);

transform_expression(source_code, stylist, |mut expression| {
let comparison = match_comparison(&mut expression)?;

let left = (*comparison.left).clone();

// Copy the right side to the left side.
comparison.left = Box::new(comparison.comparisons[0].comparator.clone());

// Copy the left side to the right side.
comparison.comparisons[0].comparator = left;

// Reverse the operator.
let op = comparison.comparisons[0].operator.clone();
comparison.comparisons[0].operator = match op {
CompOp::LessThan {
whitespace_before,
whitespace_after,
} => CompOp::GreaterThan {
whitespace_before,
whitespace_after,
},
CompOp::GreaterThan {
whitespace_before,
whitespace_after,
} => CompOp::LessThan {
whitespace_before,
whitespace_after,
},
CompOp::LessThanEqual {
whitespace_before,
whitespace_after,
} => CompOp::GreaterThanEqual {
whitespace_before,
whitespace_after,
},
CompOp::GreaterThanEqual {
whitespace_before,
whitespace_after,
} => CompOp::LessThanEqual {
whitespace_before,
whitespace_after,
},
CompOp::Equal {
whitespace_before,
whitespace_after,
} => CompOp::Equal {
whitespace_before,
whitespace_after,
},
CompOp::NotEqual {
whitespace_before,
whitespace_after,
} => CompOp::NotEqual {
whitespace_before,
whitespace_after,
},
_ => panic!("Expected comparison operator"),
};

Ok(expression.codegen_stylist(stylist))
Ok(expression)
})
}

/// SIM300
Expand Down
Loading

0 comments on commit 214eb70

Please sign in to comment.