Invert quote-style when generating code within f-strings #4487

charliermarsh · 2023-05-18T02:47:39Z

Summary

When we generate source code from an AST (via Generator), we have a preferred quote style, based on the inferred quote style in the file. Unfortunately, when we try to generate autofixes within f-strings, the Generator is unaware of the fact that we're "inside" an f-string, and so continues to use that preferred quote style, which often leads to syntax errors.

This PR modifies our logic to detect when we're inside a single-quoted f-string, and then pass in the "inverted" quote style based on whatever was used to quote that f-string. This in turn requires that we track all f-strings, and are able to map from the current expression to the surrounding f-strings.

Two considerations here:

The quote style that we provide to Generator is really a "preferred" quote style. The Generator will "ignore" that preference if, e.g., it lets us avoid escaping a quote within a string, similar to Black's preferences. This would be problematic within f-strings. However, f-strings can't contain backslashes (it's a syntax error?), so... it might be fine?
This doesn't really deal with nested f-strings. I'm not sure what we want to do in those cases. The grammar is so limiting and complicated. We may want to avoid fixing issues within nested f-strings altogether?

Closes #4322.

Closes #4324.

Closes #4323.

Closes #4321.

charliermarsh · 2023-05-18T02:48:08Z

crates/ruff/src/checkers/ast/mod.rs

@@ -134,6 +135,53 @@ impl<'a> Checker<'a> {
        }
        noqa::rule_is_ignored(code, offset, self.noqa_line_for, self.locator)
    }
+
+    /// Create a [`Generator`] to generate source code based on the current AST state.
+    pub fn generator(&self) -> Generator {


I don't know where this belongs -- it requires the Context, Indexer, Locator, and Stylist, so right now it lives on Checker...

charliermarsh · 2023-05-18T02:48:19Z

crates/ruff/src/rules/flake8_bandit/rules/hardcoded_sql_expression.rs

@@ -62,14 +62,14 @@ fn unparse_string_format_expression(checker: &mut Checker, expr: &Expr) -> Optio
        }) => {
            let Some(parent) = checker.ctx.expr_parent() else {
                if any_over_expr(expr, &has_string_literal) {
-                    return Some(unparse_expr(expr, checker.stylist));
+                    return Some(unparse_expr(expr, checker.generator()));


These are all mechanical.

charliermarsh · 2023-05-18T02:48:40Z

crates/ruff_python_ast/src/source_code/indexer.rs

+                        triple_quoted_string_ranges.push(*range);
+                    }
+                    if matches!(kind, StringKind::FString | StringKind::RawFString) {
+                        f_string_ranges.push(*range);


We now track f-string ranges, which lets us do a binary search to find the containing f-string whenever we need to fix within an f-string.

charliermarsh · 2023-05-18T02:49:56Z

As part of this effort... we may want to consider Generator to be deprecated, or an anti-pattern. In addition to this problem (it's context-independent), it also removes trivia, like comments. It is very useful, and we don't have a great replacement for it, but...

github-actions · 2023-05-18T02:58:22Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     14.2±0.13ms     2.9 MB/sec    1.00     14.2±0.05ms     2.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.4±0.01ms     4.9 MB/sec    1.00      3.4±0.02ms     4.8 MB/sec
linter/all-rules/numpy/globals.py          1.00    424.9±0.94µs     6.9 MB/sec    1.00    426.7±1.27µs     6.9 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.9±0.04ms     4.3 MB/sec    1.00      5.9±0.03ms     4.3 MB/sec
linter/default-rules/large/dataset.py      1.00      6.8±0.03ms     6.0 MB/sec    1.00      6.8±0.02ms     6.0 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1461.2±4.43µs    11.4 MB/sec    1.00   1460.5±3.69µs    11.4 MB/sec
linter/default-rules/numpy/globals.py      1.00    164.5±0.29µs    17.9 MB/sec    1.00    164.6±1.07µs    17.9 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.0±0.01ms     8.4 MB/sec    1.01      3.1±0.04ms     8.3 MB/sec
parser/large/dataset.py                    1.00      5.4±0.01ms     7.6 MB/sec    1.01      5.4±0.01ms     7.5 MB/sec
parser/numpy/ctypeslib.py                  1.00   1053.8±1.00µs    15.8 MB/sec    1.01   1062.5±7.26µs    15.7 MB/sec
parser/numpy/globals.py                    1.00    108.3±0.51µs    27.2 MB/sec    1.00    108.8±0.33µs    27.1 MB/sec
parser/pydantic/types.py                   1.00      2.3±0.00ms    11.2 MB/sec    1.01      2.3±0.00ms    11.1 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.01     16.5±0.10ms     2.5 MB/sec    1.00     16.3±0.14ms     2.5 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.2±0.05ms     4.0 MB/sec    1.00      4.2±0.04ms     4.0 MB/sec
linter/all-rules/numpy/globals.py          1.00    494.4±5.36µs     6.0 MB/sec    1.00    496.7±5.31µs     5.9 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.9±0.06ms     3.7 MB/sec    1.00      6.9±0.04ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.00      8.2±0.16ms     5.0 MB/sec    1.00      8.2±0.04ms     5.0 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1705.3±16.01µs     9.8 MB/sec    1.02  1732.8±13.39µs     9.6 MB/sec
linter/default-rules/numpy/globals.py      1.00    194.9±2.44µs    15.1 MB/sec    1.02    199.2±4.24µs    14.8 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.6±0.03ms     7.0 MB/sec    1.01      3.7±0.03ms     7.0 MB/sec
parser/large/dataset.py                    1.00      7.2±0.08ms     5.7 MB/sec    1.00      7.1±0.04ms     5.7 MB/sec
parser/numpy/ctypeslib.py                  1.00  1345.3±13.25µs    12.4 MB/sec    1.00  1341.8±16.59µs    12.4 MB/sec
parser/numpy/globals.py                    1.00    137.0±3.22µs    21.5 MB/sec    1.00    137.0±2.34µs    21.5 MB/sec
parser/pydantic/types.py                   1.00      3.0±0.03ms     8.5 MB/sec    1.00      3.0±0.03ms     8.5 MB/sec

MichaReiser

This PR modifies our logic by "inverting" the preferred quote style whenever we're inside a single-quoted f-string. This in turn requires that we track all f-strings, and are able to map from the current expression to the surrounding f-strings.

My understanding is that the preferred quote style only reflects the most used quote sign. This does not guarantee that the source code contains no fstrings with the not preferred fstring.

Edit: Your implementation handles this correctly, I think.

This doesn't really deal with nested f-strings. I'm not sure what we want to do in those cases. The grammar is so limiting and complicated. We may want to avoid fixing issues within nested f-strings altogether?

An alternative would be to make this a manual fix. This won't be very helpful now because our default text renderer doesn't show fixes (we should improve our diagnostics).

How difficult do you think would it be to add source map support to the Generator? We could then generate the code for the enclosing expression or statement and then extract the fstring range. They way this could work is that the generator writes a source map entry at the start and end of every node, mapping the node's start/end offset to the offset in the generated source (similar to the Printer in the formatter)

crates/ruff/src/checkers/ast/mod.rs

crates/ruff_python_ast/src/source_code/indexer.rs

MichaReiser · 2023-05-18T12:22:45Z

crates/ruff_python_ast/src/helpers.rs

 use crate::statement_visitor::{walk_body, walk_stmt, StatementVisitor};

 /// Generate source code from an [`Expr`].
-pub fn unparse_expr(expr: &Expr, stylist: &Stylist) -> String {
-    let mut generator: Generator = stylist.into();
+pub fn unparse_expr(expr: &Expr, mut generator: Generator) -> String {


For another PR: Should we move these methods to Generator instead, now that they always require a generator.

generator.unparse_expr(expr) // or generator.generate_expr(expr)

charliermarsh · 2023-05-18T13:03:28Z

Edit: Your implementation handles this correctly, I think.

👍 Yeah, sorry, the implementation does do this correctly, but I'll improve the summary.

charliermarsh · 2023-05-18T13:06:08Z

How difficult do you think would it be to add source map support to the Generator? We could then generate the code for the enclosing expression or statement and then extract the fstring range. They way this could work is that the generator writes a source map entry at the start and end of every node, mapping the node's start/end offset to the offset in the generated source (similar to the Printer in the formatter)

Can you elaborate on this a bit more? What does this buy us? (Genuinely asking, not objecting.)

konstin · 2023-05-19T07:25:13Z

crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP018.py.snap

+30 | f"{str()}"
+   |    ^^^^^ UP018
+   |
+   = help: Replace with `str`


that suggestion looks wrong

MichaReiser · 2023-05-19T07:26:50Z

Can you elaborate on this a bit more? What does this buy us? (Genuinely asking, not objecting.)

I wrote this before I understood that your fix solves the problem. The main benefit would be that it would make the Generator context aware. You won't need any special handling because you generate on a statement level.

charliermarsh requested review from MichaReiser and konstin May 18, 2023 02:47

charliermarsh commented May 18, 2023

View reviewed changes

charliermarsh force-pushed the charlie/f-string-quote branch from bbc239f to 867c7bd Compare May 18, 2023 02:50

MichaReiser reviewed May 18, 2023

View reviewed changes

MichaReiser approved these changes May 18, 2023

View reviewed changes

charliermarsh added 2 commits May 18, 2023 10:19

Invert quote-style when generating code within f-strings

01fc3f4

Move detection into Indexer

2d1b3a7

charliermarsh force-pushed the charlie/f-string-quote branch from 867c7bd to 2d1b3a7 Compare May 18, 2023 14:25

charliermarsh enabled auto-merge (squash) May 18, 2023 14:27

charliermarsh merged commit 73efbeb into main May 18, 2023

charliermarsh deleted the charlie/f-string-quote branch May 18, 2023 14:33

This was referenced May 18, 2023

Move triple-quoted string detection into Indexer method #4495

Merged

Move unparse utility methods onto Generator #4497

Merged

chenrui333 mentioned this pull request May 18, 2023

ruff 0.0.268 Homebrew/homebrew-core#131348

Merged

p-linnane mentioned this pull request May 18, 2023

ruff 0.0.269 Homebrew/homebrew-core#131367

Merged

konstin reviewed May 19, 2023

View reviewed changes

konstin mentioned this pull request May 19, 2023

Autofix errors from extended ecosystem checks #4523

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invert quote-style when generating code within f-strings #4487

Invert quote-style when generating code within f-strings #4487

charliermarsh commented May 18, 2023 •

edited

Loading

charliermarsh May 18, 2023

charliermarsh May 18, 2023

charliermarsh May 18, 2023

charliermarsh commented May 18, 2023

github-actions bot commented May 18, 2023 •

edited

Loading

MichaReiser left a comment •

edited

Loading

MichaReiser May 18, 2023

charliermarsh commented May 18, 2023

charliermarsh commented May 18, 2023

konstin May 19, 2023

MichaReiser commented May 19, 2023

Invert quote-style when generating code within f-strings #4487

Invert quote-style when generating code within f-strings #4487

Conversation

charliermarsh commented May 18, 2023 • edited Loading

Summary

charliermarsh May 18, 2023

Choose a reason for hiding this comment

charliermarsh May 18, 2023

Choose a reason for hiding this comment

charliermarsh May 18, 2023

Choose a reason for hiding this comment

charliermarsh commented May 18, 2023

github-actions bot commented May 18, 2023 • edited Loading

PR Check Results

Ecosystem

Benchmark

Linux

Windows

MichaReiser left a comment • edited Loading

Choose a reason for hiding this comment

MichaReiser May 18, 2023

Choose a reason for hiding this comment

charliermarsh commented May 18, 2023

charliermarsh commented May 18, 2023

konstin May 19, 2023

Choose a reason for hiding this comment

MichaReiser commented May 19, 2023

charliermarsh commented May 18, 2023 •

edited

Loading

github-actions bot commented May 18, 2023 •

edited

Loading

MichaReiser left a comment •

edited

Loading