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

add dynamic evaluation of __all__ as a list comprehension #1877

Closed
wants to merge 1 commit into from

Conversation

dyc3
Copy link

@dyc3 dyc3 commented May 18, 2021

This PR adds fairly minimal support for dynamically evaluated __all__ values, specifically when using list comprehensions.

Example:

__all__ = [x for x in dir() if not x.startswith("_")]

Justification

I am working on a project that pylance takes forever to update the type annotations, and frequently stops giving me code suggestions when working with sympy. I suspect its because it has a lot of * imports. However, it's not entirely feasible for me to manually add __all__ to all the modules.

This will allow me to keep using * imports, but also limit the import tree, without having to waste time updating __all__ manually, or setting up some tool to do it automatically. This approach also does not allow arbirary code execution, since the evaluation of the list comprehension is done analytically, not by actually executing any python code.

Why draft?

I'm looking for feedback before I do any more work on this. Do I have the right approach here? Does pyright already have an expression evaluator or something similar that I should be using instead? Should I move createFilterFunction out of the binder?

What should I do for tests? I added a couple, but I had to export a field that was private.

Additional Context

Fixes:

Related:

@dyc3 dyc3 force-pushed the dynamic-dunder-all branch from 39fba88 to 4afb034 Compare May 18, 2021 18:48
@erictraut
Copy link
Collaborator

The __all__ mechanism in Python is very problematic for type checkers because it needs to be evaluated in an early stage of analysis (in the "binder" in the case of pyright), whereas generalized expression evaluation requires the data structures created by these early stages of analysis.

For this reason, we have decided to support only a specific set of __all__ idioms. This set has been fixed (after discussing with maintainers of other type checkers), and we don't plan to support any more. You can see this list documented here.

If you want your code to play well with static type checkers, please modify it accordingly so it conforms to the supported list of __all__ idioms.

@dyc3
Copy link
Author

dyc3 commented May 18, 2021

I've seen that documentation, and I thought I'd take a stab at making it better. I realize that this could easily get out of hand, but this took way less code than I initially thought it would, and it's not even that nasty looking, IMO. Of course, its not feasible to support all possible ways to build __all__, but I think it would be hugely beneficial to at least support some dynamic behavior, especially because that is supported by Python. Nothing super insane, just something to improve accuracy in common scenarios.

Is this still open to discussion, or is this PR just dead on arrival then? That would be really unfortunate. :( If so, the related issues should probably be marked as wont-fix and closed, and the dunderAll1.py test does not cover list comprehensions to mark them with the warning.

@erictraut
Copy link
Collaborator

Sorry, this is dead on arrival. This is a slippery slope, and I'm not willing to entertain expanding this.

@erictraut erictraut closed this May 18, 2021
@erictraut
Copy link
Collaborator

Apologies, I didn't mean to come across as flippant in my last response, but there are good reasons why we standardize things like this, and it's not necessarily "better" to add support for non-standard behaviors.

We do appreciate contributions to the source base, but it's best to discuss new functionality in the discussions section or issues list before proceeding with a PR. We can then collectively work toward finding the best solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants