Avoid parsing other parts of a format specification if replacements are present #6858

zanieb · 2023-08-24T19:31:35Z

Closes #6767
Replaces #6773 (this cherry-picks some parts from there)
Alternative to the approach introduced in #6616 which added support for placeholders in format specifications while retaining parsing of other format specification parts.

The idea is that if there are placeholders in a format specification we will not attempt to glean semantic meaning from the other parts of the format specification we'll just extract all of the placeholders ignoring other characters. The dynamic content of placeholders can drastically change the meaning of the format specification in ways unknowable by static analysis. This change prevents false analysis and will ensure safety if we build other rules on top of this at the cost of missing detection of some bad specifications.

Minor note: I've use "replacements" and "placeholders" interchangeably but am trying to go with "placeholder" as I think it's a better term for the static analysis concept here

…re present

zanieb · 2023-08-24T19:42:08Z

crates/ruff_python_literal/src/format.rs

-            Err(FormatSpecError::InvalidFormatType)
+            // Note this should be an `InvalidFormatType` but we give up
+            // on all other parsing validation when we see a placeholder
+            Ok(FormatSpec::Dynamic(DynamicFormatSpec {
+                placeholders: vec![FormatPart::Field {
+                    field_name: String::new(),
+                    conversion_spec: None,
+                    format_spec: String::new()
+                }]
+            }))


Here's an example case where we no longer detect invalid format type specifications because there was a replacement earlier — this is a trade-off for generally safer analysis.

zanieb · 2023-08-24T19:42:57Z

crates/ruff_python_literal/src/format.rs

    #[test]
-    fn test_format_part_with_others() {
-        let expected = Ok(FormatSpec {
-            conversion: None,
-            fill: None,
-            align: Some(FormatAlign::Left),
-            sign: None,
-            alternate_form: false,
-            width: Some(20),
-            grouping_option: None,
-            precision: None,
-            format_type: Some(FormatType::Binary),
-            replacements: vec![FormatPart::Field {
+    fn test_dynamic_format_spec_with_others() {
+        let expected = Ok(FormatSpec::Dynamic(DynamicFormatSpec {
+            placeholders: vec![FormatPart::Field {
                field_name: "x".to_string(),
                conversion_spec: None,
                format_spec: String::new(),
            }],
-        });
+        }));
        assert_eq!(FormatSpec::parse("<{x}20b"), expected);
    }


Here we lose all these additional fields which were correct in this test but could be wrong depending on the dynamic content of the placeholder.

github-actions · 2023-08-24T19:55:41Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.04      5.3±0.09ms     7.6 MB/sec    1.00      5.2±0.06ms     7.9 MB/sec
formatter/numpy/ctypeslib.py               1.01   1055.0±3.85µs    15.8 MB/sec    1.00  1041.2±14.72µs    16.0 MB/sec
formatter/numpy/globals.py                 1.08    107.0±6.56µs    27.6 MB/sec    1.00     99.2±1.34µs    29.7 MB/sec
formatter/pydantic/types.py                1.09      2.1±0.14ms    11.9 MB/sec    1.00  1972.2±25.70µs    12.9 MB/sec
linter/all-rules/large/dataset.py          1.00     11.6±0.20ms     3.5 MB/sec    1.08     12.5±0.64ms     3.3 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.1±0.04ms     5.4 MB/sec    1.08      3.3±0.11ms     5.0 MB/sec
linter/all-rules/numpy/globals.py          1.00    448.1±4.94µs     6.6 MB/sec    1.02    458.4±1.65µs     6.4 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.1±0.09ms     4.2 MB/sec    1.05      6.4±0.10ms     4.0 MB/sec
linter/default-rules/large/dataset.py      1.00      6.1±0.07ms     6.7 MB/sec    1.05      6.4±0.11ms     6.4 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1356.8±15.71µs    12.3 MB/sec    1.03   1395.8±7.68µs    11.9 MB/sec
linter/default-rules/numpy/globals.py      1.01    163.6±0.59µs    18.0 MB/sec    1.00    161.8±2.24µs    18.2 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.9±0.02ms     8.9 MB/sec    1.02      2.9±0.08ms     8.8 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      5.3±0.11ms     7.7 MB/sec    1.00      5.3±0.17ms     7.7 MB/sec
formatter/numpy/ctypeslib.py               1.01  1029.8±32.42µs    16.2 MB/sec    1.00  1024.3±27.21µs    16.3 MB/sec
formatter/numpy/globals.py                 1.00     97.8±3.29µs    30.2 MB/sec    1.01     98.8±5.43µs    29.9 MB/sec
formatter/pydantic/types.py                1.00  1983.0±45.52µs    12.9 MB/sec    1.00  1982.5±57.70µs    12.9 MB/sec
linter/all-rules/large/dataset.py          1.00     12.6±0.16ms     3.2 MB/sec    1.01     12.7±0.20ms     3.2 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.5±0.05ms     4.8 MB/sec    1.01      3.5±0.09ms     4.8 MB/sec
linter/all-rules/numpy/globals.py          1.00   431.6±10.74µs     6.8 MB/sec    1.01    435.3±9.36µs     6.8 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.5±0.14ms     3.9 MB/sec    1.01      6.6±0.13ms     3.9 MB/sec
linter/default-rules/large/dataset.py      1.00      7.1±0.13ms     5.8 MB/sec    1.00      7.0±0.12ms     5.8 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1503.2±36.14µs    11.1 MB/sec    1.01  1517.2±43.22µs    11.0 MB/sec
linter/default-rules/numpy/globals.py      1.01    177.2±6.23µs    16.6 MB/sec    1.00    176.0±4.86µs    16.8 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.1±0.06ms     8.1 MB/sec    1.00      3.1±0.06ms     8.1 MB/sec

charliermarsh

Looks great!

crates/ruff_python_literal/src/format.rs

Thanks Charlie :)

zanieb · 2023-08-25T16:28:11Z

#6877 fixes CI

zanieb added 3 commits August 24, 2023 14:31

Avoid parsing other parts of a format specification if replacements a…

fdc15b1

…re present

Clean up implementation

094e51e

Add failing test case for bad-string-format-character

35d2440

zanieb requested a review from charliermarsh August 24, 2023 19:41

zanieb commented Aug 24, 2023

View reviewed changes

Remove debug prints

53de2d3

zanieb added the bug Something isn't working label Aug 24, 2023

charliermarsh approved these changes Aug 25, 2023

View reviewed changes

crates/ruff_python_literal/src/format.rs Show resolved Hide resolved

crates/ruff_python_literal/src/format.rs Show resolved Hide resolved

crates/ruff_python_literal/src/format.rs Outdated Show resolved Hide resolved

Review feedback

78a965a

Thanks Charlie :)

zanieb enabled auto-merge (squash) August 25, 2023 16:10

Merge branch 'main' into fix/format-spec

04754c5

Merge branch 'main' into fix/format-spec

e89519f

zanieb merged commit 100904a into main Aug 25, 2023

zanieb deleted the fix/format-spec branch August 25, 2023 17:42

chenrui333 mentioned this pull request Aug 26, 2023

ruff 0.0.286 Homebrew/homebrew-core#140518

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid parsing other parts of a format specification if replacements are present #6858

Avoid parsing other parts of a format specification if replacements are present #6858

zanieb commented Aug 24, 2023 •

edited

Loading

zanieb Aug 24, 2023

zanieb Aug 24, 2023

github-actions bot commented Aug 24, 2023 •

edited

Loading

charliermarsh left a comment

zanieb commented Aug 25, 2023

Avoid parsing other parts of a format specification if replacements are present #6858

Avoid parsing other parts of a format specification if replacements are present #6858

Conversation

zanieb commented Aug 24, 2023 • edited Loading

zanieb Aug 24, 2023

Choose a reason for hiding this comment

zanieb Aug 24, 2023

Choose a reason for hiding this comment

github-actions bot commented Aug 24, 2023 • edited Loading

PR Check Results

Ecosystem

Benchmark

Linux

Windows

charliermarsh left a comment

Choose a reason for hiding this comment

zanieb commented Aug 25, 2023

zanieb commented Aug 24, 2023 •

edited

Loading

github-actions bot commented Aug 24, 2023 •

edited

Loading