[behavior-change] RUF010 delete unnecessary str()

[smackesey]

IIUC, these two expressions are exactly equivalent:

f"{str(x)}"  # (1)
f"{x}"  # (2)

The str is superfluous in (1). But instead of RUF010 removing the str call, instead it autofixes to f"{x!s}"-- but the !s here is still superfluous-- the only time it makes sense to use !s is if there is an additional format specifier, e.g. f"{x!s:20}" (in this case str is applied before the format specifier, whereas without !s it is applied after the format specifier).

So IMO RUF010 should remove str() unless there is a format specifier, in which case it converts to !s.

[dhruvmanila]

@charliermarsh I can take this up unless you've already started working on it :)

[charliermarsh]

It’s ok, I’ve already started on it :)

[mishamsk]

@charliermarsh @smackesey I believe this is wrong, and thus #4971 should be reverted. See below:

class Test:
    def __str__(self) -> str:
        return "str"
    
    def __format__(self, __format_spec: str) -> str:
        return "format"
    
print(f"{Test()} {str(Test())} {Test()!s} {Test()!s:6} {Test():6}")
# gives: format       str         str         str         format

so, i.e. even the stdlib's StrEnum will not work correctly after such an auto "fix"

[charliermarsh]

(Taking a look.)

[charliermarsh]

Can you include an example to illustrate the behavior you're describing with StrEnum? In my testing, including or omitting !s yields the same result in that case.

[mishamsk]

Can you include an example to illustrate the behavior you're describing with StrEnum? In my testing, including or omitting !s yields the same result in that case.

ah, sorry, my bad. I was using my "polyfill" in 3.10 which apparently doesn't match 3.11 stdlib exactly. Anyways, the {value} calls value.format(), so the check and fix that was implemented here is not correct.

Here is another example with enums in 3.11:

from enum import StrEnum, Enum


class Test(StrEnum):
    VALUE = "value"


class NonStrEnum(str, Enum):
    VALUE = "value"

    def __str__(self) -> str:
        return str(self.value)

    def __format__(self, format_spec: str) -> str:
        return repr(self)


print(f"{Test.VALUE} {Test.VALUE!s}")  # prints "value value"
print(f"{NonStrEnum.VALUE} {NonStrEnum.VALUE!s}")  # prints "<NonStrEnum.VALUE: 'value'> value"

[charliermarsh]

All good. Just to be totally clear, I'm not trying to "disprove" the claim here, I think you're right that these aren't equivalent (it looks like f-strings by default are calling __format__, and we're assuming they're calling __str__), I'm just trying to assess the blast radius :)

[mishamsk]

sure, no problem. Sorry for confusion with StrEnum, it just happened to be something I was (re)implementing in one of my commercial projects in 3.10 that is impacted. Can't think of an easy way to estimate the impact here, probably not a lot of custom format functions out there. But my gut tells me that introducing silent non-100%-compatible changes is not what users of Ruff expect. Since Ruff (at least originally) was supposed to lint, not "optimize" code, at least I expect it to retain everything (just like black), without any possible side-effects.

Since Ruff is so fast and I bet 80%+ just have auto-fix for most of the rules, most won't even notice such a small "benign" change, which may hit at some point if custom formatting is applied... but ultimately it's your call.

I personally would have to make a mental notice to either wait for a release that reverts this or remember to disable auto-fix for this rule...

[charliermarsh]

A couple quick comments (a little tight on time but want to keep this convo going, sorry for brevity):

• Python is so dynamic that it's hard for us to implement almost any fixes with complete, 100% certainty. Even reordering imports is not a completely "safe" operation, but I think most users would expect Ruff to support import sorting. So we have to draw various lines around what we're comfortable fixing and what we're not.
• One way we're trying to improve this is by introducing autofix levels: automatic, suggested, and manual. Automatic would always be fixed, suggested would require --fix-unsafe (instead of --fix), and manual would never be autofixed. We've actually added these annotations to almost all of the fixes, but they're not yet respected on the CLI. (So, at the very least, using --fix would "guarantee" that we make no behavioral changes, or at least are very confident in the fixes.)
• In practice this seems really rare...
• But I actually agree that we should revert it, mostly because it will be hard to detect/notice when this does do the wrong thing (unlike, e.g., removing an unused import that's actually used in some capacity). Here, it's pretty likely that nothing would break at parse-time or at run-time or even in tests, which feels dangerous to me.

[mishamsk]

no worries, that’s actually a lot of great details, appreciate you taking the time.

• you’re totally right rgd dynamicity. Haven’t even thought about import order, but now as you’ve said it, I even know specific examples where order matters (basically with any packages that mingle with import machinery).
• classifying fixes is an awesome approach. Hopefully it gets into a release sooner rather then later
• In terms of impact and/or how to classify this a fix: something that may go unnoticed and at the same time is not a lot of value (like this issue) makes more sense under —fix-unsafe. Things like import sort that (a) not enabled by default in Ruff (b) probably will create an immediate failure in tests or downstream (c) add a lot of value (e.g. stable git diffs) - may be considered as regular —fix.
• For this specific case - another justification here. I suspect if someone did add !s or str() inside f-expression, it may have been done because of unexpected __format__ behavior. Most devs probably do not even think/know about !s\r\a modifiers, so wouldn’t use them unless they stumbled upon an issue.

[smackesey]

Nice catch @mishamsk, I wasn't even aware __format__ existed.

[mishamsk]

@smackesey thanks!