[WIP/RFC] Revisit patching of mypy builtins (reveal_{type,locals}) (#615)

* Revisit patching of mypy builtins (reveal_{type,locals})

Fixes https://github.com/typeddjango/django-stubs/issues/609
Reverts ee58b18f15

* Create test_patching.yml

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>

README.md

@@ -82,7 +82,7 @@ But, it does not make any sense to use this project without `mypy`.
 
 ### mypy crashes when I run it with this plugin installed
 
-Current implementation uses Django runtime to extract models information, so it will crash, if your installed apps or `models.py` is not correct. For this same reason, you cannot use `reveal_type` inside global scope of any Python file that will be executed for `django.setup()`.
+The current implementation uses Django's runtime to extract information about models, so it might crash if your installed apps or `models.py` are broken.
 
 In other words, if your `manage.py runserver` crashes, mypy will crash too.
 You can also run `mypy` with [`--tb`](https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-show-traceback)

django_stubs_ext/django_stubs_ext/patch.py

@@ -1,4 +1,3 @@
-import builtins
 from typing import Any, Generic, List, Optional, Tuple, Type, TypeVar
 
 from django import VERSION as VERSION
@@ -62,9 +61,5 @@ def monkeypatch() -> None:
     for el in suited_for_this_version:
         el.cls.__class_getitem__ = classmethod(lambda cls, *args, **kwargs: cls)
 
-    # Define mypy builtins, to not cause NameError during setting up Django.
-    builtins.reveal_type = lambda _: None
-    builtins.reveal_locals = lambda: None
-
 
 __all__ = ["monkeypatch"]

django_stubs_ext/tests/test_monkeypatching.py

@@ -29,9 +29,6 @@ def make_generic_classes(
             with suppress(AttributeError):
                 delattr(el.cls, "__class_getitem__")
 
-        del builtins.reveal_type
-        del builtins.reveal_locals
-
     def factory(django_version: Optional[_VersionSpec] = None) -> None:
         if django_version is not None:
             monkeypatch.setattr(patch, "VERSION", django_version)
@@ -71,11 +68,15 @@ def test_patched_version_specific(
             assert el.cls[int] is el.cls
 
 
-def test_patched_mypy_builtins(
+def test_mypy_builtins_not_patched_globally(
     make_generic_classes: _MakeGenericClasses,
 ) -> None:
-    """Ensures that we properly patch builtins with `mypy` specific helpers."""
+    """Ensures that builtins are not patched with `mypy` specific helpers.
+
+    This should only happend during `django.setup()`
+    (https://github.com/typeddjango/django-stubs/issues/609).
+    """
     make_generic_classes()
 
-    assert builtins.reveal_type
+    assert not hasattr(builtins, "reveal_type")
-    assert builtins.reveal_locals
+    assert not hasattr(builtins, "reveal_locals")

django_stubs_ext/tests/typecheck/test_patching.yml

@@ -0,0 +1,15 @@
+-   case: patch_reveal_type_during_django_setup
+    main: |
+        # `main` is empty on purpose, because we test the `django.setup()` step.
+        # It executes the `myapp/models.py` file and should not produce anything,
+        # because `reveal_type` and `reveal_locals` are patched.
+    installed_apps:
+        - myapp
+    files:
+        -   path: myapp/__init__.py
+        -   path: myapp/models.py
+            content: |
+                # Will be executed during `django.setup()`:
+                a = 1
+                reveal_type(a)
+                reveal_locals()

mypy_django_plugin/django/context.py

@@ -1,3 +1,4 @@
+import builtins
 import os
 import sys
 from collections import defaultdict
@@ -60,6 +61,13 @@ def initialize_django(settings_module: str) -> Tuple["Apps", "LazySettings"]:
         models.QuerySet.__class_getitem__ = classmethod(noop_class_getitem)  # type: ignore
         models.Manager.__class_getitem__ = classmethod(noop_class_getitem)  # type: ignore
 
+        # Define mypy builtins, to not cause NameError during setting up Django.
+        # TODO: temporary/unpatch
+        assert not hasattr(builtins, "reveal_type")
+        builtins.reveal_type = lambda _: None
+        assert not hasattr(builtins, "reveal_locals")
+        builtins.reveal_locals = lambda: None
+
         from django.apps import apps
         from django.conf import settings