[Bug]: False positive with FURB184

[shaolo1]

Has your issue already been fixed?

• [X] Have you checked to see if your issue still exists on the master branch? See the docs for instructions on how to setup a local build of Refurb.
• [X] Have you looked at the open/closed issues to see if anyone has already reported your issue?

The Bug

The following code:

class Foo:
    _path = '/'

    def get_path(self):
        return self._path

    def test(self):
        foo = self
        path = foo.get_path()

Emits the following error:

$ refurb refurb_chain.py
refurb_chain.py:9:9 [FURB184]: Assignment statement should be chained

But it should not be emitting an error instance because... There is nothing to chain here?

Version Info

Refurb: v1.26.0
Mypy: v1.8.0

Python Version

Python 3.11.3

Config File

# N/A

Extra Info

None

[dosisod]

@shaolo1 thank you for opening this issue!

I don't think this is a false positive, but perhaps the error message could be improved to better indicate what's going on. Because foo is assigned and then used on the next line to call .get_path() (and never again after that), FURB184 is suggesting you chain foo like so:

    def test(self):
        path = self.get_path()

This might just be a MVP to illustrate the problem, in which case I can imagine this error is a bit misleading/confusing if you're intending to use foo as a placeholder/temp variable. There should probably be a threshold needed to constitute as "chaining", ie, calling and re-assigning the same variable 2+ times instead of just 1.

[blablatdinov]

Hi! I like this rule, but sometimes it can lead to a false positive.

For example cases with a split definition of a template string and its formatting.

template = '{}'
template.format('User')

[dosisod]

cc @sbrugman what are your thoughts on this?

[shaolo1]

@shaolo1 thank you for opening this issue!

I don't think this is a false positive, but perhaps the error message could be improved to better indicate what's going on. Because foo is assigned and then used on the next line to call .get_path() (and never again after that), FURB184 is suggesting you chain foo like so:

    def test(self):
        path = self.get_path()

This might just be a MVP to illustrate the problem, in which case I can imagine this error is a bit misleading/confusing if you're intending to use foo as a placeholder/temp variable. There should probably be a threshold needed to constitute as "chaining", ie, calling and re-assigning the same variable 2+ times instead of just 1.

Fair enough. The original code that it was flagged in was a bit weird, and I just focused on the error message not making sense to me. In reality the original code really just needed changing, so perhaps a tweak to the message would be good enough.

[shaolo1]

@shaolo1 thank you for opening this issue!

I don't think this is a false positive, but perhaps the error message could be improved to better indicate what's going on. Because foo is assigned and then used on the next line to call .get_path() (and never again after that), FURB184 is suggesting you chain foo like so:

    def test(self):
        path = self.get_path()

This might just be a MVP to illustrate the problem, in which case I can imagine this error is a bit misleading/confusing if you're intending to use foo as a placeholder/temp variable. There should probably be a threshold needed to constitute as "chaining", ie, calling and re-assigning the same variable 2+ times instead of just 1.

Fair enough. The original code that it was flagged in was a bit weird, and I just focused on the error message not making sense to me. In reality the original code really just needed changing, so perhaps a tweak to the message would be good enough. The net effect was positive and in the spirit of refurb. It prompted me to make a change to the code for the better.

[gavindsouza]

I just got this with the following code block after upgrading from v1.22.1 to v1.28.0 :

m = chocolata.get_meta("Component")
+title_field = m.calculate_title()

data = chocolata.find_all(
    "Component",
    filters={
        "target_entity": doc.doctype,
        "is_identifier": True,
    },
+    fields=["name", title_field],
    order_by="creation asc",
)
# Note: order is important ^ to track matches

+notify({f"Component::{x['name']}": x[title_field] for x in data})

I've highlighted the relevant lines as to why this was a false positive