-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ruff] Ignore str() when not used for simple conversion (RUF065)
#21330
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
Conversation
| if checker.semantic().match_builtin_expr(func.as_ref(), "str") | ||
| && str_call_args.args.len() == 1 | ||
| && !str_call_args.args[0].is_starred_expr() | ||
| && str_call_args.keywords.is_empty() => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logging.warning("%s", str(object="!")), with one keyword argument, should still be flagged.
|
I added a test case for |
ntBre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I had a suggestion for simplification, which will hopefully help with the test case too.
crates/ruff_linter/src/rules/ruff/rules/logging_eager_conversion.rs
Outdated
Show resolved
Hide resolved
|
Thanks! I pushed those changes, but it looks like we're running into the same issue as before. The fix works in isolation but not within the test. |
|
|
Oh, |
ntBre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
* origin/main: (38 commits) [ty] Make implicit submodule imports only occur in global scope (#21370) [ty] introduce local variables for `from` imports of submodules in `__init__.py(i)` (#21173) [`ruff`] Ignore `str()` when not used for simple conversion (`RUF065`) (#21330) [ty] implement `typing.NewType` by adding `Type::NewTypeInstance` [ty] supress inlay hints for `+1` and `-1` (#21368) [ty] Use type context for inference of generic constructors (#20933) [ty] Improve generic call expression inference (#21210) [ty] supress some trivial expr inlay hints (#21367) [`configuration`] Fix unclear error messages for line-length values exceeding `u16::MAX` (#21329) [ty] Fix incorrect inference of `enum.auto()` for enums with non-`int` mixins, and imprecise inference of `enum.auto()` for single-member enums (#20541) [`refurb`] Detect empty f-strings (`FURB105`) (#21348) [ty] provide `import` completion when in `from <name> <name>` statement (#21291) [ty] elide redundant inlay hints for function args (#21365) Fix syntax error false positive on alternative `match` patterns (#21362) Add a new "Opening a PR" section to the contribution guide (#21298) [`flake8-simplify`] Fix SIM222 false positive for `tuple(generator) or None` (`SIM222`) (#21187) Rebuild ruff binary instead of sharing it across jobs (#21361) [ty] Fix `--exclude` and `src.exclude` merging (#21341) [ty] Add support for properties that return `Self` (#21335) Add upstream linter URL to `ruff linter --output-format=json` (#21316) ...
Summary
Fixed RUF065 (
logging-eager-conversion) to only flagstr()calls when they perform a simple conversion that can be safely removed. The rule now ignoresstr()calls with no arguments, multiple arguments, starred arguments, or keyword unpacking, preventing false positives.Fixes #21315
Problem Analysis
The RUF065 rule was incorrectly flagging all
str()calls in logging statements, even whenstr()was performing actual conversion work beyond simple type coercion. Specifically, the rule flagged:str()with no arguments - which returns an empty stringstr(b"data", "utf-8")with multiple arguments - which performs encoding conversionstr(*args)with starred arguments - which unpacks argumentsstr(**kwargs)with keyword unpacking - which passes keyword argumentsThese cases cannot be safely removed because
str()is doing meaningful work (encoding conversion, argument unpacking, etc.), not just redundant type conversion.The root cause was that the rule only checked if the function was
str()without validating the call signature. It didn't distinguish between simplestr(value)conversions (which can be removed) and more complexstr()calls that perform actual work.Approach
The fix adds validation to the
str()detection logic inlogging_eager_conversion.rs:str()calls with exactly one positional argument (str_call_args.args.len() == 1)!str_call_args.args[0].is_starred_expr())str_call_args.keywords.is_empty())This ensures the rule only flags cases like
str(value)wherestr()is truly redundant and can be removed, while ignoring cases wherestr()performs actual conversion work.The fix maintains backward compatibility - all existing valid test cases continue to be flagged correctly, while the new edge cases are properly ignored.