Skip to content

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Jul 3, 2025

This implements filtering of private symbols from stub files based on type information as discussed in #19102. It extends the previous implementation to apply to all stub files, instead of just the builtins module, and uses type information to retain private names that are may be relevant at runtime.

@zanieb zanieb added server Related to the LSP server ty Multi-file analysis & type inference labels Jul 3, 2025
Copy link
Contributor

github-actions bot commented Jul 3, 2025

mypy_primer results

Changes were detected when running on open source projects
mypy_primer (https://github.com/hauntsaninja/mypy_primer)
-     memo fields = ~45MB
+     memo fields = ~41MB

Expression (https://github.com/cognitedata/Expression)
-     memo fields = ~54MB
+     memo fields = ~49MB

werkzeug (https://github.com/pallets/werkzeug)
-     memo fields = ~142MB
+     memo fields = ~129MB

trio (https://github.com/python-trio/trio)
-     memo fields = ~156MB
+     memo fields = ~142MB

hydra-zen (https://github.com/mit-ll-responsible-ai/hydra-zen)
- TOTAL MEMORY USAGE: ~80MB
+ TOTAL MEMORY USAGE: ~88MB

mongo-python-driver (https://github.com/mongodb/mongo-python-driver)
-     memo fields = ~171MB
+     memo fields = ~156MB

vision (https://github.com/pytorch/vision)
- TOTAL MEMORY USAGE: ~368MB
+ TOTAL MEMORY USAGE: ~334MB

mkdocs (https://github.com/mkdocs/mkdocs)
-     memo fields = ~106MB
+     memo fields = ~97MB

tornado (https://github.com/tornadoweb/tornado)
- TOTAL MEMORY USAGE: ~156MB
+ TOTAL MEMORY USAGE: ~171MB

cwltool (https://github.com/common-workflow-language/cwltool)
-     memo fields = ~228MB
+     memo fields = ~207MB

paasta (https://github.com/yelp/paasta)
-     memo fields = ~156MB
+     memo fields = ~171MB

@zanieb

This comment was marked as outdated.

Comment on lines +536 to +537
public_explicit_type_alias: TypeAlias = Literal[1]
_private_explicit_type_alias: TypeAlias = Literal[1]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexWaygood why "A private type alias that is explicitly annotated with typing.TypeAlias"? It looks like the behavior doesn't change either way.

Copy link
Member Author

@zanieb zanieb Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e., the naive alternative passes

diff --git a/crates/ty_ide/src/completion.rs b/crates/ty_ide/src/completion.rs
index 9a6190cb5..0ee759051 100644
--- a/crates/ty_ide/src/completion.rs
+++ b/crates/ty_ide/src/completion.rs
@@ -536,6 +536,9 @@ _private_type_var_tuple = TypeVarTuple("_private_type_var_tuple")
 public_explicit_type_alias: TypeAlias = Literal[1]
 _private_explicit_type_alias: TypeAlias = Literal[1]
 
+public_implicit_type_alias = Literal[1]
+_private_implicit_type_alias = Literal[1]
+
 class PublicProtocol(Protocol):
     def method(self) -> None: ...
 
@@ -558,6 +561,8 @@ class _PrivateProtocol(Protocol):
         test.assert_completions_do_not_include("_private_type_var_tuple");
         test.assert_completions_include("public_explicit_type_alias");
         test.assert_completions_include("_private_explicit_type_alias");
+        test.assert_completions_include("public_implicit_type_alias");
+        test.assert_completions_include("_private_implicit_type_alias");
         test.assert_completions_include("PublicProtocol");
         test.assert_completions_do_not_include("_PrivateProtocol");
     }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh whoops -- for some reason I thought that my patch excluded explicitly declared type aliases with private names. But you're quite right -- it doesn't do that. We'd probably need to add a new variant to the DynamicType enum to distinguish type-alias Todo types from other Todo types in order to get that working. (Shouldn't be too hard to do as a followup, I can pick it up.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave it to you then :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I picked this up in #19282)

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

Comment on lines +565 to +568
/// Unlike [`private_symbols_in_stub`], this test doesn't use a `.pyi` file so all of the names
/// are visible.
#[test]
fn private_symbols_in_module() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be nice to avoid the repetition here, but I don't love optimizing tests for that early. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also just test a few of the "do not include" cases here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For just two tests, I think this is fine to be honest. (And it seems like two cases is what we want.)

@zanieb zanieb force-pushed the zb/filter-completions branch from 74af9ee to 65f7338 Compare July 3, 2025 14:54
@zanieb zanieb marked this pull request as ready for review July 3, 2025 15:11
Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server Related to the LSP server ty Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants