Skip to content

[ty] support accessing __builtins__ global #18118

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

Merged
merged 3 commits into from
May 15, 2025

Conversation

felixscherz
Copy link
Contributor

Hi, this is in regards to astral-sh/ty#393 and intends to support __builtins__ access.

Summary

The PR adds an explicit check for "__builtins__" during name lookup, similar to how "__file__" is implemented. The inferred type is ModuleType.

I'm new to working on ty, so any feedback is welcome:)

Test Plan

Added a markdown test for __builtins__.

Copy link
Contributor

github-actions bot commented May 15, 2025

mypy_primer results

Changes were detected when running on open source projects
attrs (https://github.com/python-attrs/attrs)
- error[unresolved-reference] tests/test_make.py:1823:24: Name `__builtins__` used when not defined
- Found 618 diagnostics
+ Found 617 diagnostics

trio (https://github.com/python-trio/trio)
- error[unresolved-reference] src/trio/_tests/test_repl.py:47:29: Name `__builtins__` used when not defined
- Found 1097 diagnostics
+ Found 1096 diagnostics

scrapy (https://github.com/scrapy/scrapy)
- error[unresolved-reference] scrapy/spiderloader.py:37:52: Name `__builtins__` used when not defined
- Found 1423 diagnostics
+ Found 1422 diagnostics

dd-trace-py (https://github.com/DataDog/dd-trace-py)
+ warning[unused-ignore-comment] ddtrace/debugging/_expressions.py:79:61: Unused blanket `type: ignore` directive
+ warning[unused-ignore-comment] ddtrace/debugging/_expressions.py:199:38: Unused blanket `type: ignore` directive
- Found 8729 diagnostics
+ Found 8731 diagnostics

@sharkdp
Copy link
Contributor

sharkdp commented May 15, 2025

Thank you for your contribution! You found the right place to add the name == "__builtins__" check and the right place for the test. Looking at the ecosystem impact, it would be great if we could infer the actual Type::ModuleLiteral(…) type. Doing so would probably require adding something like a KnownModule::to_module_literal_type(db, …) function. You could then call KnownModule::Builtins.to_module_literal_type(db, …). The hardest part might be to get the right value for the importing_file when calling ModuleLiteralType::new.

Comment on lines 1016 to 1017
} else if name == "__builtins__" {
Symbol::bound(KnownClass::ModuleType.to_instance(db)).into()
Copy link
Contributor

Choose a reason for hiding this comment

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

To quote the docs:

As an implementation detail, most modules have the name __builtins__ made available as part of their globals. The value of __builtins__ is normally either this module or the value of this module’s __dict__ attribute. Since this is an implementation detail, it may not be used by alternate implementations of Python.

As such, the type of __builtins__ is, as David has pointed out, the builtins module, and the symbol's boundness should be PossiblyUnbound.

@@ -15,6 +15,7 @@ reveal_type(__package__) # revealed: str | None
reveal_type(__doc__) # revealed: str | None
reveal_type(__spec__) # revealed: ModuleSpec | None
reveal_type(__path__) # revealed: MutableSequence[str]
reveal_type(__builtins__) # revealed: ModuleType

class X:
reveal_type(__name__) # revealed: str
Copy link
Contributor

Choose a reason for hiding this comment

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

As per the previous review comment, these tests should be added as well:

import sys
reveal_type(sys.__builtins__)  # revealed: <module 'builtins'>

from builtins import __builtins__ as __bi__
reveal_type(__bi__)  # revealed: <module 'builtins'>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added tests for these cases.

@InSyncWithFoo
Copy link
Contributor

The hardest part might be to get the right value for the importing_file when calling ModuleLiteralType::new.

This is actually not that hard: all call sites of module_type_implicit_global_symbol() have access to Files as well.

  • global_symbol(): file parameter.
  • builtins_symbol(): module.file().
  • The two references in infer.rs: self.file().

As for some_module.__builtins__, a branch could probably be added to member_lookup_with_policy().

@carljm
Copy link
Contributor

carljm commented May 15, 2025

I notice that one of the ecosystem hits is using __builtins__ as if it were a dictionary, not a module instance. The docs quoted by @InSyncWithFoo call out that it can be either. And a simple test shows this to be true: with the code print(type(__builtins__)) in main.py, then python3 main.py prints <class 'module'>, but python3 -c "import main" prints <class 'dict'>.

So this suggests to me that we must either infer the type of __builtins__ as a union (and require users of it to check which it is) or just follow pyright's lead and infer it as Any. Given that this pattern is uncommon, and it would be quite complex to make the stricter version fully strict (it would require the dictionary variant of the union to be a TypedDict with known keys and values), I think we should just take the easier route and infer it as Any.

@AlexWaygood
Copy link
Member

(I agree with @carljm -- let's just use Any for now. We can always revisit it and try to improve the type at a later date if we feel it's important.)

Copy link
Contributor

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you for the updates!

@sharkdp
Copy link
Contributor

sharkdp commented May 15, 2025

The mypy_primer results also look as expected now.

@sharkdp sharkdp merged commit d3a7cb3 into astral-sh:main May 15, 2025
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ty Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants