jakebailey
commented
3 years ago Any chance you could try out the insiders build which has indexing enabled to see if that changes the import that's presented? #623
Closed tiangolo closed 3 years ago
jakebailey
commented
3 years ago Any chance you could try out the insiders build which has indexing enabled to see if that changes the import that's presented? #623
tiangolo
commented
3 years ago Thanks for the response @jakebailey !
I just enabled insiders, it was downloaded and I was asked to reload the editor, which I did.
Now the completion system doesn't suggest anything for Query, not even from fastapi.param_functions import Query.
I get suggestions for other symbols with a name that starts with Query, but not the one I'm interested in.
With the same snippet, I get:
jakebailey
commented
3 years ago If you enable trace logging, do you see the indexer running and completing by the time you perform this completion? Curious that it doesn't show. @heejaechang can likely take a look when he's back from his break.
tiangolo
commented
3 years ago I think I found how to do it and tried it, it seems the indexing is running and completing before I trigger completion.
Here's the full log for the Python Language Server output:
heejaechang
commented
3 years ago it is happening due to FastApi/ init .py doesn't define all . to reduce number of items to suggest, for regular py file, we only suggest symbols defined in all for now.
@savannahostrowski @judej we might want to add import alias defined as "import * as alias" in index if all doesn't exist in init .py file?
tiangolo
commented
3 years ago Thanks for replying @heejaechang !
I don't declare __all__ in FastAPI as it already uses import x as x, following the guidelines from Pyright: https://github.com/microsoft/pyright/blob/master/docs/typed-libraries.md#library-interface, the discussion with @erictraut (on Pyright issues and email), and the guidelines from mypy using the stricter flags: https://mypy.readthedocs.io/en/stable/command_line.html?highlight=import%20as#cmdoption-mypy-no-implicit-reexport
I prefer to declare explicit import x as x than to declare __all__. As __all__ is based on plain strings (and black magic) and not on "normal" Python syntax, so those wouldn't benefit as easily from static analysis.
jakebailey
commented
3 years ago For reference, this is not a recommendation to use __all__, just an explanation of the limitations we put on the indexer to attempt to not offer a world's worth of suggestions (and have bad perf while indexing). Plain Python files take a bit more effort to process than stubs, but we still want indexing to be fast and not impede other features.
We won't be able to use only "X as X" to determine what to offer, as that'll miss anything actually declared in the file, I think, so we'll have to take a look to see how to handle this.
erictraut
commented
3 years ago FastAPI is a "py.typed" package. I think the indexer should change its behavior for such packages and follow the rules layed out in the typed library guidance. This guidance indicates specifically which symbols are considered public and which are not. For non-"py.typed" packages, it makes sense for the indexer to use more conservative heuristics to avoid adding lots of garbage to the completion suggestion list.
jakebailey
commented
3 years ago We've identified a bug in one of the performance optimizations for py.typed that affected __all__ processing; it'll be included in the next release and we can see if that helps.
jakebailey
commented
3 years ago This issue has been fixed in version 2021.4.0, which we've just released. You can find the changelog here: https://github.com/microsoft/pylance-release/blob/main/CHANGELOG.md#202140-7-april-2021
justindujardin
commented
3 years ago @jakebailey I tried out the latest versions and the original issue still persists. The only way I've been able to resolve it is to move all the symbols directly into __init__.py. 😢
Can you elaborate on the fix and what (if any) changes should be made to libraries in order to have the auto-import functionality generate imports from __init__.py and not the source files the symbols come from?
jakebailey
commented
3 years ago Are you testing with fastapi or another project? This bug seemed to be specific to fastapi's layout, so your issue may be worth a new issue with a reproducer we can test and fix.
justindujardin
commented
3 years ago I tested with FastAPI specifically (the snippet in the OP). The behavior is slightly different than before and it generates a shorter import path (
params instead of param_functions), but it still misses the one coming from __init__.py:
jakebailey
commented
3 years ago Just to double check, do you have indexing explicitly enabled?
justindujardin
commented
3 years ago Hmm, no, I didn't have it explicitly turned on. 😅
When I enable it, the suggestions include the shorter path as expected:
jakebailey
commented
3 years ago Right, thanks. This bug is fixed with indexing, but we're not yet ready to get that enabled for everyone yet. Insiders sometimes has it enabled, but we are currently working on making sure it won't cause a big perf regression (so it isn't enabled right now to help us diff versions).
Environment data
Expected behaviour
I would expect the auto-import feature to pick the shortest import path for a symbol, or at least offer it as one of the alternatives, rather than only the source file where that symbol is defined.
For example, take this snippet:
After triggering auto-completion for
Query, I would want it to suggest to import it as:Or at least include that in the possible locations to import.
Actual behaviour
The auto-import feature is only suggesting the import path that points to the source file where the symbol is created.
So it suggests importing:
Logs
I think logs are not relevant here, but let me know.
Code Snippet / Additional information
Code snippet and screenshot provided above in context.
I recently released FastAPI
0.63.0with support formypy --strict, including re-exports in the source files for the symbols I want re-exported.This is a continuation of this comment: https://github.com/microsoft/pylance-release/issues/222#issuecomment-751269904 in a previous issue.