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

false positive for PERF203 with try-except-continue-else #11602

Closed
trim21 opened this issue May 29, 2024 · 8 comments
Closed

false positive for PERF203 with try-except-continue-else #11602

trim21 opened this issue May 29, 2024 · 8 comments
Labels
rule Implementing or modifying a lint rule

Comments

@trim21
Copy link
Contributor

trim21 commented May 29, 2024

ruff 0.6.5 raise false positive for try-except in a look with else

import io
import unicodedata


def __clean_rss_invalid_char(s: str):
    with io.StringIO() as f:
        for c in s:
            try:
                unicodedata.name(c)
            except ValueError:
                continue
            else:
                f.write(c)

        return f.getvalue()

previous issue: #6535

this looks like #6535 but it still exists then targeting py38

https://play.ruff.rs/eadb6e43-5f78-4bcd-93f3-2d409bdbf4c8

@trim21 trim21 changed the title false positive for PERF203 false positive for PERF203 with try-except-else May 29, 2024
@trim21

This comment was marked as outdated.

@trim21 trim21 closed this as completed May 29, 2024
@trim21 trim21 reopened this Sep 16, 2024
@trim21 trim21 changed the title false positive for PERF203 with try-except-else false positive for PERF203 with try-except-continue-else Sep 16, 2024
@MichaReiser
Copy link
Member

MichaReiser commented Sep 16, 2024

Looking at the code, the reason why the above is flagged and wasn't addressed by #6535 is that the rule only checks for continue and break statements inside the try body but not in the except handlers

if has_break_or_continue(body) {
return;
}

The rule only applies to Python 310 or older because

/// This rule is only enforced for Python versions prior to 3.11, which
/// introduced "zero-cost" exception handling. However, note that even on
/// Python 3.11 and newer, refactoring your code to avoid exception handling in
/// tight loops can provide a significant speedup in some cases, as zero-cost
/// exception handling is only zero-cost in the "happy path" where no exception
/// is raised in the `try`-`except` block.

@MichaReiser
Copy link
Member

MichaReiser commented Sep 16, 2024

I still think the rule should flag the above pattern because whether there's a better way to write the above loop depends on the unicodedata API. For example, I think the following could work:

for c in s:
      name = unicodedata.name("a", None)

      if name is None:
          continue
      else:
          f.write(c)

which eliminates the need for try

@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Sep 16, 2024
@trim21
Copy link
Contributor Author

trim21 commented Sep 16, 2024

I still think the rule should flag the above pattern because whether there's a better way to write the above loop depends on the unicodedata API. For example, I think the following could work:

for c in s:
      name = unicodedata.name("a", None)

      if name is None:
          continue
      else:
          f.write(c)

which eliminates the need for try

I don't think ruff understand unicodedata.name, it flag this didn't base on "whether there's a better way".

because ruff still flag this, and I don't think there is a better way to write this:

import requests


def check_url_live(urls: str):
    for url in urls:
        try:
            requests.get(url, timeout=5)
        except ConnectionRefusedError:
            continue
        else:
            print(url)

@trim21
Copy link
Contributor Author

trim21 commented Sep 16, 2024

if you think it should flag this, then there is another problem: it doesn't flag this:

import unicodedata


def check_url_live(s: str):
    for c in s:
        try:
            unicodedata.name(c)
        except ValueError:
            continue

        print(c)

@MichaReiser
Copy link
Member

because ruff still flag this, and I don't think there is a better way to write this:

Yeah, there's probably not a better way to write this and using a noqa comment here seems appropriate.

if you think it should flag this, then there is another problem: it doesn't flag this:

I would have to double check the upstream rule to understand if this is a bug but the rule currently only flags cases where the try statement is the only statement in the loop body.

@MichaReiser
Copy link
Member

Going through the history. Ignoring multi-statement loops is intentional, see #6145

Using noqa comment seems the right solution to deal with cases where using try...except is the only solution because the API doesn't provide an alternative way of testing the input. Making this decision isn't something that ruff can do (even with better inference).

@MichaReiser MichaReiser closed this as not planned Won't fix, can't repro, duplicate, stale Sep 16, 2024
@trim21
Copy link
Contributor Author

trim21 commented Sep 16, 2024

Going through the history. Ignoring multi-statement loops is intentional, see #6145

Using noqa comment seems the right solution to deal with cases where using try...except is the only solution because the API doesn't provide an alternative way of testing the input. Making this decision isn't something that ruff can do (even with better inference).

I think we are actully talking about 2 topics here...

Ignoring multi-statement loops is fine to me, and this issue is not about it should or should not ignore multi-statement loops. the "false positive" is, it didn't ignore single try-except(continue)-else statments like multi-statement loops

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

No branches or pull requests

2 participants