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

[bug] RUF010 autofix bug #4927

Closed
smackesey opened this issue Jun 7, 2023 · 8 comments · Fixed by #4947
Closed

[bug] RUF010 autofix bug #4927

smackesey opened this issue Jun 7, 2023 · 8 comments · Fixed by #4947
Assignees
Labels
bug Something isn't working

Comments

@smackesey
Copy link

On upgrading to 0.0.271, running ruff --fix is giving a lot of errors on trying to fix f strings. Here's an example string that triggers the error:

f"Member of tuple mismatches type at index {i}. Expected {of_shape_i}. Got "
f"{repr(obj)} of type {type(obj)}.{additional_message}"

And here's the full output, which references many more examples

error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
error: Failed to create fix for ExplicitFStringTypeConversion: Failed to extract expression from source
python_modules/dagster/dagster/_check/__init__.py:1587:24: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_check/__init__.py:1709:18: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_check/__init__.py:1709:56: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_check/__init__.py:1709:77: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_check/__init__.py:1724:69: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_check/__init__.py:1729:68: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_check/__init__.py:1745:83: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_check/__init__.py:1771:54: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_check/__init__.py:1798:21: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_check/__init__.py:1820:77: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_check/__init__.py:1821:21: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_check/__init__.py:1827:35: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_core/definitions/assets.py:1154:20: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_core/definitions/assets.py:1269:40: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_core/definitions/multi_dimensional_partitions.py:342:49: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_core/definitions/multi_dimensional_partitions.py:343:49: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_core/definitions/reconstruct.py:605:45: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_core/definitions/resource_requirement.py:227:31: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_core/definitions/resource_requirement.py:229:21: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_core/execution/execute_in_process.py:135:45: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_core/execution/plan/compute.py:201:53: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_core/execution/plan/compute.py:202:24: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_core/execution/with_resources.py:90:24: RUF010 Use conversion in f-string
python_modules/dagster/dagster/_grpc/server.py:1041:84: RUF010 Use conversion in f-string
python_modules/libraries/dagstermill/dagstermill/factory.py:329:29: RUF010 Use conversion in f-string
@charliermarsh
Copy link
Member

Interesting, thanks. Will take a look at these today.

@charliermarsh charliermarsh added the bug Something isn't working label Jun 7, 2023
@charliermarsh
Copy link
Member

Anyone is welcome to beat me to it :)

@charliermarsh charliermarsh self-assigned this Jun 7, 2023
@qdegraaf
Copy link
Contributor

qdegraaf commented Jun 7, 2023

Having a quick look but can't replicate. Both running ruff 0.0.271 after installing it through pip and running:

cargo run -p ruff_cli -- check --fix test.py --select RUF010 --no-cache

on up to date main just fix the snippet.

@charliermarsh
Copy link
Member

I'm able to repro by running on:

(
    f"Member of tuple mismatches type at index {i}. Expected {of_shape_i}. Got "
    f"{repr(obj)} of type {type(obj)}.{additional_message}"
)

I'm assuming that we're not matching properly against implicit concatenations.

@charliermarsh
Copy link
Member

I think I have a fix for this.

@qdegraaf
Copy link
Contributor

qdegraaf commented Jun 7, 2023

I'm able to repro by running on:

(
    f"Member of tuple mismatches type at index {i}. Expected {of_shape_i}. Got "
    f"{repr(obj)} of type {type(obj)}.{additional_message}"
)

I'm assuming that we're not matching properly against implicit concatenations.

Yeah that triggers it here too. Let me know if you need an extra pair of hands on the fix.

@charliermarsh
Copy link
Member

I'd appreciate it if you have time. What I was planning to do was artificially parenthesize the expression when passing to the LibCST parser:

❯ git diff HEAD
diff --git a/crates/ruff/src/rules/ruff/rules/explicit_f_string_type_conversion.rs b/crates/ruff/src/rules/ruff/rules/explicit_f_string_type_conversion.rs
index 18e93ab3c..f276b837b 100644
--- a/crates/ruff/src/rules/ruff/rules/explicit_f_string_type_conversion.rs
+++ b/crates/ruff/src/rules/ruff/rules/explicit_f_string_type_conversion.rs
@@ -1,11 +1,11 @@
 use anyhow::{bail, Result};
 use rustpython_parser::ast::{self, Expr, Ranged};

-use crate::autofix::codemods::CodegenStylist;
 use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::source_code::{Locator, Stylist};

+use crate::autofix::codemods::CodegenStylist;
 use crate::checkers::ast::Checker;
 use crate::cst::matchers::{
     match_call_mut, match_expression, match_formatted_string, match_formatted_string_expression,
@@ -55,7 +55,8 @@ fn fix_explicit_f_string_type_conversion(
     // Replace the call node with its argument and a conversion flag.
     let range = expr.range();
     let content = locator.slice(range);
-    let mut expression = match_expression(content)?;
+    let parenthesized_content = format!("({})", content);
+    let mut expression = match_expression(&parenthesized_content)?;
     let formatted_string = match_formatted_string(&mut expression)?;

Right now, we're passing this literal, which isn't a valid expression (it has to be parenthesized):

f"Member of tuple mismatches type at index {i}. Expected {of_shape_i}. Got "
    f"{repr(obj)} of type {type(obj)}.{additional_message}"

We then need to do two things:

  1. Strip the parentheses that we added artificially.
  2. Handle the case of implicitly concatenated substrings, which LibCST represents with a different node (ConcatenatedString). We assume that LibCST gives us a FormattedString. We need to handle both cases.

@qdegraaf
Copy link
Contributor

qdegraaf commented Jun 7, 2023

I'll have a look, see how far I get

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants