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

Incorrect ANN201 autofix #9304

Closed
hauntsaninja opened this issue Dec 29, 2023 · 4 comments · Fixed by #9310 or #9377
Closed

Incorrect ANN201 autofix #9304

hauntsaninja opened this issue Dec 29, 2023 · 4 comments · Fixed by #9310 or #9377
Assignees
Labels
bug Something isn't working

Comments

@hauntsaninja
Copy link
Contributor

Given:

from somewhere import get_cfg

def lookup_cfg(cfg_description):
    cfg = get_cfg(cfg_description)
    if cfg is not None:
        return cfg
    raise AttributeError(f"No cfg found matching {cfg_description}")

ANN201 suggests the following incorrect fix:

λ ruff check --select=ANN2 x.py --fix --unsafe-fixes --diff
--- x.py
+++ x.py
@@ -1,6 +1,7 @@
 from somewhere import get_cfg
+from typing import Never
 
-def lookup_cfg(cfg_description):
+def lookup_cfg(cfg_description) -> Never:
     cfg = get_cfg(cfg_description)
     if cfg is not None:
         return cfg

Would fix 1 error.
@hauntsaninja
Copy link
Contributor Author

Presumably relates to #9206

@charliermarsh charliermarsh self-assigned this Dec 29, 2023
@charliermarsh charliermarsh added the bug Something isn't working label Dec 29, 2023
charliermarsh added a commit that referenced this issue Dec 29, 2023
## Summary

Given:

```python
from somewhere import get_cfg

def lookup_cfg(cfg_description):
    cfg = get_cfg(cfg_description)
    if cfg is not None:
        return cfg
    raise AttributeError(f"No cfg found matching {cfg_description}")
```

We were analyzing the method from last-to-first statement. So we saw the
`raise`, then assumed the method _always_ raised. In reality, though, it
_might_ return. This PR improves the branch analysis to respect these
mixed cases.

Closes #9269.
Closes #9304.
@hauntsaninja
Copy link
Contributor Author

Thanks! :-)

@hauntsaninja
Copy link
Contributor Author

I think the behaviour still isn't quite right:

λ cat xx.py
def foo(a):
    if a > 5:
        raise ValueError("oops")
    else:
        print("nope")
λ ruff --version
ruff 0.1.11
λ ruff check xx.py --select=ANN2 --fix --unsafe-fixes --diff
--- xx.py
+++ xx.py
@@ -1,4 +1,5 @@
-def foo(a):
+from typing import Never
+def foo(a) -> Never:
     if a > 5:
         raise ValueError("oops")
     else:

Would fix 1 error.

@charliermarsh
Copy link
Member

🤦 One more try...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants