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

unsafe ANN autofix seems to ignore return in for loop #9269

Closed
twoertwein opened this issue Dec 25, 2023 · 4 comments · Fixed by #9310
Closed

unsafe ANN autofix seems to ignore return in for loop #9269

twoertwein opened this issue Dec 25, 2023 · 4 comments · Fixed by #9310
Assignees
Labels
bug Something isn't working

Comments

@twoertwein
Copy link

ruff --select "ANN001,ANN2" --fix-only (version 0.1.9) on the following code snippet seems to ignore the return statement in the for loop

https://github.com/pandas-dev/pandas/blob/b0c2e45997e8f164a181ce8e896dfb414e3eb60c/pandas/_version.py#L120

def versions_from_parentdir(parentdir_prefix, root, verbose): ###   ruff adds  -> NoReturn
    """Try to determine the version from the parent directory name.

    Source tarballs conventionally unpack into a directory that includes both
    the project name and a version string. We will also support searching up
    two directory levels for an appropriately named parent directory
    """
    rootdirs = []
    
    for _ in range(3):
        dirname = os.path.basename(root)
        if dirname.startswith(parentdir_prefix):
            return { ###   return statement!
                "version": dirname[len(parentdir_prefix) :],
                "full-revisionid": None,
                "dirty": False,
                "error": None,
                "date": None,
            }
        rootdirs.append(root)
        root = os.path.dirname(root)  # up a level

    if verbose:
        print(
            f"Tried directories {str(rootdirs)} \
            but none started with prefix {parentdir_prefix}"
        )
    raise NotThisMethod("rootdir doesn't start with parentdir_prefix")
@charliermarsh charliermarsh added the bug Something isn't working label Dec 25, 2023
@charliermarsh charliermarsh self-assigned this Dec 25, 2023
@charliermarsh
Copy link
Member

Thanks! Will take a look.

@charliermarsh
Copy link
Member

Will be fixed in the next release.

@twoertwein
Copy link
Author

The same issue happens here with if/else: https://github.com/pandas-dev/pandas/blob/b0c2e45997e8f164a181ce8e896dfb414e3eb60c/pandas/util/__init__.py#L1 ruff thinks __getattr__ should 'return' NoReturn

@charliermarsh
Copy link
Member

Yeah all the same bug. I overlooked an interaction when I implemented the NoReturn.

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.
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
Development

Successfully merging a pull request may close this issue.

2 participants