Merged
Conversation
dhruvmanila
force-pushed
the
dhruv/dunder-all
branch
3 times, most recently
from
5035a99 to
01c1c41
Compare
MichaReiser
reviewed
May 6, 2025
MichaReiser
reviewed
May 6, 2025
MichaReiser
reviewed
May 6, 2025
dhruvmanila
force-pushed
the
dhruv/dunder-all
branch
2 times, most recently
from
191a065 to
e9c3012
Compare
Contributor
|
Contributor
|
dhruvmanila
force-pushed
the
dhruv/dunder-all
branch
from
1433259 to
6ed9029
Compare
These invalid overload diagnostic snapshots are so flaky
This reverts commit f65e2b4.
dhruvmanila
force-pushed
the
dhruv/dunder-all
branch
from
6ed9029 to
298f72a
Compare
dhruvmanila
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
MichaReiser
reviewed
May 7, 2025
dhruvmanila
commented
May 7, 2025
Comment on lines
+64
to
+66
| /// A flag indicating whether the module uses unrecognized `__all__` idioms or there are any | ||
| /// invalid elements in `__all__`. | ||
| invalid: bool, |
Member
Author
There was a problem hiding this comment.
This is mainly to reduce false positives because there are many different idioms that are being used in the ecosystem that dynamically generates the values of __all__ in the module.
dhruvmanila
commented
May 7, 2025
Comment on lines
127
to
133
| let Some(module_dunder_all_names) = | ||
| dunder_all_names(self.db, module_literal.module(self.db).file()) | ||
| else { | ||
| // The module either does not have a `__all__` variable or it is invalid. | ||
| // TODO: Should we return `false` here? | ||
| return true; | ||
| }; |
Member
Author
There was a problem hiding this comment.
I'm resolving this TODO by returning false. Ideally, I think it might be useful to differentiate between "__all__ not present" vs "invalid __all__" in the return type for dunder_all_names query but for now I've used a simplified approach here.
dhruvmanila
commented
May 7, 2025
Comment on lines
251
to
258
| // Here, we need to use the `dunder_all_names` query instead of the | ||
| // `exported_names` query because a `*`-import does not import the | ||
| // `__all__` attribute unless it is explicitly included in the `__all__` of | ||
| // the module. | ||
| let Some(all_names) = self.dunder_all_names_for_import_from(import_from) | ||
| else { | ||
| continue; | ||
| }; |
Member
Author
There was a problem hiding this comment.
I think I'll need to mark here as invalid (self.invalid = true) as well if dunder_all_names_for_import_from returns None as otherwise it adds new false positives.
Contributor
There was a problem hiding this comment.
Is there a reason you didn't do this yet? Should we at least have a TODO comment for it, or do you plan to add it in this PR?
Member
Author
There was a problem hiding this comment.
I'm planning to do it in this PR itself.
carljm
approved these changes
May 7, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Awesome work, this looks great!
I've reviewed everything except haven't done a thorough reading of the new visitor code yet. Have to jump into some other meetings for a bit, so wanted to submit what I have. Given that the tested behaviors look great, I think you can feel free to go ahead and land this; if there are any issues in the details of the visitor code, I'm sure they can be fixed as follow-up.
Comment on lines
251
to
258
| // Here, we need to use the `dunder_all_names` query instead of the | ||
| // `exported_names` query because a `*`-import does not import the | ||
| // `__all__` attribute unless it is explicitly included in the `__all__` of | ||
| // the module. | ||
| let Some(all_names) = self.dunder_all_names_for_import_from(import_from) | ||
| else { | ||
| continue; | ||
| }; |
Contributor
There was a problem hiding this comment.
Is there a reason you didn't do this yet? Should we at least have a TODO comment for it, or do you plan to add it in this PR?
Member
Author
|
I'm going to merge this, I'm still looking into a couple of false positives that I've noted down but don't want it to block this from merging this. I'll take this and any reviews that this PR receives as follow-up. |
dcreager
added a commit
that referenced
this pull request
May 7, 2025
…lass * origin/main: [`pylint`] add fix safety section (`PLC2801`) (#17825) Add instructions on how to upgrade to a newer Rust version (#17928) [parser] Flag single unparenthesized generator expr with trailing comma in arguments. (#17893) [ty] Ensure that `T` is disjoint from `~T` even when `T` is a TypeVar (#17922) [ty] Sort collected diagnostics before snapshotting them in mdtest (#17926) [ty] Add basic file watching to server (#17912) Make completions an opt-in LSP feature (#17921) Add link to `ty` issue tracker (#17924) [ty] Add support for `__all__` (#17856) [ty] fix assigning a typevar to a union with itself (#17910) [ty] Improve UX for `[duplicate-base]` diagnostics (#17914) Clean up some Ruff references in the ty server (#17920)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds support for the
__all__module variable.Reference spec: https://typing.python.org/en/latest/spec/distributing.html#library-interface-public-and-private-symbols
This PR adds a new
dunder_all_namesquery that returns a set ofNames defined in the__all__variable of the givenFile. The query works by implementing theStatementVisitorand collects all the names by recognizing the supported idioms as mentioned in the spec. Any idiom that's not recognized are ignored.The current implementation is minimum to what's required for us to remove all the false positives that this is causing. Refer to the "Follow-ups" section below to see what we can do next. I'll a open separate issue to keep track of them.
Closes: astral-sh/ty#106
Closes: astral-sh/ty#199
Follow-ups
__all__idioms,__all__containing non-string element__all__but not defined in the module. This could lead to runtime error<type>instead ofUnknown | <type>formodule.__all__. For example: https://playknot.ruff.rs/2a6fe5d7-4e16-45b1-8ec3-d79f2d4ca894__all__as used otherwise it could raise (possibly in the future) "unused-name" diagnosticSupporting diagnostics will require that we update the return type of the query to be something other than
Option<FxHashSet<Name>>, something that behaves like a result and provides a way to check whether a name exists in__all__, loop over elements in__all__, loop over the invalid elements, etc.Ecosystem analysis
The following are the maximum amount of diagnostics removed in the ecosystem:
collections.abc- 14numpy- 35534numpy.ma- 296numpy.char- 37numpy.testing- 175hashlib- 311scipy.fft- 2scipy.stats- 38collections.abc- 85numpy- 508numpy.testing- 741hashlib- 36scipy.stats- 68scipy.interpolate- 7scipy.signal- 5The following modules have dynamic
__all__definition, sotyassumes that__all__doesn't exists in that module:scipy.stats(https://github.com/scipy/scipy/blob/95a5d6ea8b9f2e482f9a79aa40b719433f8e3c4d/scipy/stats/__init__.py#L665)scipy.interpolate(https://github.com/scipy/scipy/blob/95a5d6ea8b9f2e482f9a79aa40b719433f8e3c4d/scipy/interpolate/__init__.py#L221)scipy.signal(indirectly via https://github.com/scipy/scipy/blob/95a5d6ea8b9f2e482f9a79aa40b719433f8e3c4d/scipy/signal/_signal_api.py#L30)numpy.testing(https://github.com/numpy/numpy/blob/de784cd6ee53ffddf07e0b3f89bf22bda9fd92fb/numpy/testing/__init__.py#L16-L18)There's this one category of false positives that have been added:Fixed the false positives by also ignoring__all__from a module that uses unrecognized idioms.Details about the false postivie:
The
scipy.statsmodule has dynamic__all__and it imports a bunch of symbols via star imports. Some of those modules have a mix of valid and invalid__all__idioms. For example, in https://github.com/scipy/scipy/blob/95a5d6ea8b9f2e482f9a79aa40b719433f8e3c4d/scipy/stats/distributions.py#L18-L24, 2 out of 4__all__idioms are invalid but currentlytyrecognizes two of them and says that the module has a__all__with 5 values. This leads to around 2055 newly added false positives of the form:I think the fix here is to completely ignore
__all__, not only if there are invalid elements in it, but also if there are unrecognized idioms used in the module.Test Plan
Add a bunch of test cases using the new
ty_extensions.dunder_all_namesfunction to extract a module's__all__names.Update various test cases to remove false positives around
*imports and re-export convention.Add new test cases for named import behavior as
*imports covers all of it already (thanks Alex!).