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

Possible error in PLE0605 #7672

Closed
nstarman opened this issue Sep 26, 2023 · 2 comments · Fixed by #7673
Closed

Possible error in PLE0605 #7672

nstarman opened this issue Sep 26, 2023 · 2 comments · Fixed by #7673
Assignees
Labels
bug Something isn't working

Comments

@nstarman
Copy link

nstarman commented Sep 26, 2023

In astropy/astropy#15385 we are trying to fix some __all__ statements and ran into the following

__all__ = [...]
logging_levels = ["NOTSET", ...]
...
__all__ += logging_levels

My suggested fix was

__all__ = [...]
__all__ += (logging_levels := ["NOTSET", ...])

However PLE0605 is still complaining. I don't think this is correct since the above should be, from the "perspective" of __all__ equivalent to

__all__ = [...]
__all__ += ["NOTSET", ...]

which passes the check.

The inclusion of the walrus operator for assigning to another variable makes the AST more complicated but should be equivalent statically to excluding the walrus.

@zanieb
Copy link
Member

zanieb commented Sep 26, 2023

Thanks for the report!

Hm... in the example:

from other import a, b

__all__ = ["a"]
__all__ += (foo := ["b"])

pyright reports the symbol b as unused and displays the warning Operation on "__all__" is not supported, so exported symbol list may be incorrect (reportUnsupportedDunderAll).

Although I agree this is syntactically equivalent, if it's not supported by type checkers I'm not sure we should treat it as valid.

@charliermarsh
Copy link
Member

I think it's reasonable to support if the code changes aren't too tricky :)

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

This PR adds support for named expressions when analyzing `__all__`
assignments, as per #7672. It
also loosens the enforcement around assignments like: `__all__ =
list(some_other_expression)`. We shouldn't flag these as invalid, even
though we can't analyze the members, since we _know_ they evaluate to a
`list`.

Closes #7672.

## Test Plan

`cargo test`
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.

3 participants