[Bug]: FURB143 suggesting change that isn't compatible with the context of code

[owenlamont]

Has your issue already been fixed?

• [ ] 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:

abbreviation: str | None = None
word = "blah"
abbreviation = (abbreviation or "") + word

Emits the following error:

$ refurb file.py
# example.py:31:25 [FURB143]: Replace `x or ""` with `x`

But it should not be emitting an error instance because...

The recommended code change will break the code when the input string is None as None type and string can't be concatenated.

Version Info

Refurb: v1.20.0

Python Version

3.11.4

Config File

[tool.refurb]
ignore = [
    "FURB124", # Ignore rule to recommend chaining comparisons over anding pairs
    "FURB140" # Ignore rule to prefer starmap when iterating and constructing
]
python_version = "3.9"

Extra Info

I hacked the actual example line to be more minimal but this is my example of rule FURB143 being incorrectly applied where coercing a None to a string makes sense and it isn't simply mapping one falsey type to another as the recommended change would break the code.

[dosisod]

@owenlamont thank you for opening this. Refurb already checks for None types, so you shouldn't be seeing an error here. Also, I tested your code snippet and it isn't giving me an error. What version of Mypy are you using? Running either refurb --version or mypy --version should tell you.

Do you have a more complete MVP of this issue? It could be that Mypy/Refurb isn't deducing the type correctly in your full example, but can in the small MVP you provided.

[owenlamont]

Sorry, I should've checked my simplified version before posting. I'll try again and look up the mypy details.

[owenlamont]

So I went back and found a minimal example that actually reproduced the issue with:

Refurb: v1.20.0
Mypy: v1.5.1

Here's a minimal example:

def _concat_abbreviations(words: list[str]):
    abbreviation = None

    for word in words:
        abbreviation = (abbreviation or "") + word

When I typehinted abbreviation as str | None then I stop getting the warning. So I guess that resolves my issue, whether you consider it a bug without the typehint I'm not sure. In the full function abbreviation was later used and assumed to be a string (with checks for being None first).

[dosisod]

Thank you for the updated code, and sorry for the late response! I've ran into a (slightly different) version of this a while ago: Mypy does not seem to update the type of abbreviation after it's instantiation, which is why manually setting the type fixes it.

For reference, this is what Mypy thinks the types of abbreviation are:

def _concat_abbreviations(words: list[str]):
    abbreviation = None
    reveal_type(abbreviation)  # Revealed type is "None"

    for word in words:
        abbreviation = (abbreviation or "") + word
        reveal_type(abbreviation)  # Revealed type is "builtins.str"

So I would say this is an issue with Mypy, but since Refurb relies on accurate type information, Refurb should give some sort of warning or indication when it might guess the wrong type. I'll see if there is a way to handle this on Refurb's side of things, otherwise I'll open an issue on Mypy.