Skip to content
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

get_dynamic_class_hook does not handle defer properly #17402

Open
asottile-sentry opened this issue Jun 18, 2024 · 3 comments
Open

get_dynamic_class_hook does not handle defer properly #17402

asottile-sentry opened this issue Jun 18, 2024 · 3 comments
Labels
bug mypy got something wrong topic-plugins The plugin API and ideas for new plugins

Comments

@asottile-sentry
Copy link

Bug Report

typically, ctx.api.defer() can be called in a plugin when there is insufficient information to defer calling to a future pass. this "works" for get_dynamic_class_hook but causes the dynamic class assignment to be converted into a Var rendering it useless as a base class later.

I've adapted a test plugin from the mypy codebase to demonstrate this problem

To Reproduce

the plugin I've adapted responds to an EAGER=1 environment variable -- where it will not defer the class creation. EAGER=0 simulates when insufficient information is available so it must defer the class to a separate pass

# mypy.ini
[mypy]
plugins = _plugin
# _plugin.py
from __future__ import annotations

import os
from typing import Callable

from mypy.nodes import GDEF, Block, ClassDef, SymbolTable, SymbolTableNode, TypeInfo, Var
from mypy.plugin import ClassDefContext, DynamicClassDefContext, Plugin
from mypy.types import Instance, get_proper_type


_DEFERRED = os.environ.get('EAGER') == '1'


class DynPlugin(Plugin):
    def get_dynamic_class_hook(
        self, fullname: str
    ) -> Callable[[DynamicClassDefContext], None] | None:
        if fullname == "mod.declarative_base":
            return add_info_hook
        return None


def add_info_hook(ctx: DynamicClassDefContext) -> None:
    global _DEFERRED
    if not _DEFERRED:
        _DEFERRED = True
        ctx.api.defer()
        print('defering!')
        return

    class_def = ClassDef(ctx.name, Block([]))
    class_def.fullname = ctx.api.qualified_name(ctx.name)

    info = TypeInfo(SymbolTable(), class_def, ctx.api.cur_mod_id)
    class_def.info = info
    obj = ctx.api.named_type("builtins.object")
    info.mro = [info, obj.type]
    info.bases = [obj]
    ctx.api.add_symbol_table_node(ctx.name, SymbolTableNode(GDEF, info))
    print('creating!')


def plugin(version: str) -> type[DynPlugin]:
    return DynPlugin
# mod.py
def declarative_base(name: str) -> object:
    raise NotImplementedError

cls = declarative_base("wat")

class C(cls): pass

Expected Behavior

both of these should pass type checking

rm -rf .mypy_cache && EAGER=1 python -m mypy mod.py
rm -rf .mypy_cache && EAGER=0 python -m mypy mod.py

Actual Behavior

$ rm -rf .mypy_cache && EAGER=1 python -m mypy mod.py
creating!
Success: no issues found in 1 source file
$ rm -rf .mypy_cache && EAGER=0 python -m mypy mod.py
defering!
creating!
mod.py:6: error: Variable "mod.cls" is not valid as a type  [valid-type]
mod.py:6: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
mod.py:6: error: Invalid base class "cls"  [misc]
Found 2 errors in 1 file (checked 1 source file)

Your Environment

  • Mypy version used: current HEAD 10f18a8
  • Mypy command-line flags: see above
  • Mypy configuration options from mypy.ini (and other config files): see above
  • Python version used: 3.12.2 (though it doesn't seem to matter)
@asottile-sentry
Copy link
Author

this is almost certainly wrong but this does make the example pass:

diff --git a/mypy/semanal.py b/mypy/semanal.py
index 03e6172bb..3cfd6a6b0 100644
--- a/mypy/semanal.py
+++ b/mypy/semanal.py
@@ -3126,7 +3126,16 @@ class SemanticAnalyzer(
         self.store_final_status(s)
         self.check_classvar(s)
         self.process_type_annotation(s)
+
+        orig, self.deferred = self.deferred, False
         self.apply_dynamic_class_hook(s)
+        new, self.deferred = self.deferred, orig
+        if new is True:
+            for expr in names_modified_by_assignment(s):
+                self.current_symbol_table().pop(expr.name)
+                self.mark_incomplete(expr.name, expr)
+            return
+
         if not s.type:
             self.process_module_assignment(s.lvalues, s.rvalue, s)
         self.process__all__(s)

may also give me enough information to make a bad workaround in the meantime 🤔

@asottile-sentry
Copy link
Author

scratch that, I think that only "worked" because subclassing Any is allowed

@AlexWaygood AlexWaygood added the topic-plugins The plugin API and ideas for new plugins label Jun 18, 2024
@asottile-sentry
Copy link
Author

this also makes the example pass and seems to properly preserve the dynamic class and may give me enough for a workaround:

         ctx.api.defer()
+        ph = PlaceholderNode(
+            ctx.api.qualified_name(ctx.name),
+            ctx.call,
+            ctx.call.line,
+            becomes_typeinfo=True
+        )
+        ctx.api.add_symbol_table_node(ctx.name, SymbolTableNode(GDEF, ph))
         print('defering!')

unclear how to generalize this to mypy though. I think the api of the get_dynamic_class_hook itself doesn't lend itself well to solving this "generically"

asottile-sentry added a commit to getsentry/sentry that referenced this issue Jul 29, 2024
an absolute ton of work went into enabling this -- but after this PR
model types / create / update / etc. should mostly be typechecked
whereas they were entirely unchecked (everything `Any`) prior to this.

most of the core problem was that mypy and django-stubs did not
understand our custom `BaseManager` and `BaseQuerySet` since there's a
significant amount of django stuff that goes into making those classes
"real"

fix that involved these issues:

- python/mypy#17402 (worked around)
- typeddjango/django-stubs#2228 (the workaround)

with that fixed, it exposed a handful issues in django-stubs itself:

- typeddjango/django-stubs#2240
- typeddjango/django-stubs#2241
- typeddjango/django-stubs#2244
- typeddjango/django-stubs#2248
- typeddjango/django-stubs#2276
- typeddjango/django-stubs#2281

fixing all of those together exposed around 1k issues in sentry code
itself which was fixed over a number of PRs:
- #75186
- #75108
- #75149
- #75162
- #75150
- #75154
- #75158
- #75148
- #75157
- #75156
- #75152
- #75153
- #75151
- #75113
- #75112
- #75111
- #75110
- #75109
- #75013
- #74641
- #74640
- #73783
- #73787
- #73788
- #73793
- #73791
- #73786
- #73761
- #73742
- #73744
- #73602
- #73596
- #73595
- #72892
- #73589
- #73587
- #73581
- #73580
- #73213
- #73207
- #73206
- #73205
- #73202
- #73198
- #73121
- #73109
- #73073
- #73061
- getsentry/getsentry#14370
- #72965
- #72963
- #72967
- #72877 (eventually was able to remove this when upgrading to mypy
1.11)
- #72948
- #72799
- #72898
- #72899
- #72896
- #72895
- #72903
- #72894
- #72897
- #72893
- #72889
- #72887
- #72888
- #72811
- #72872
- #72813
- #72815
- #72812
- #72808
- #72802
- #72807
- #72809
- #72810
- #72814
- #72816
- #72818
- #72806
- #72801
- #72797
- #72800
- #72798
- #72771
- #72718
- #72719
- #72710
- #72709
- #72706
- #72693
- #72641
- #72591
- #72635


mypy 1.11 also included some important improvements with typechecking
`for model_cls in (M1, M2, M2): ...` which also exposed some problems in
our code. unfortunately upgrading mypy involved fixing a lot of things
as well:
- #75020
- #74982
- #74976
- #74974
- #74972
- #74967
- #74954
- getsentry/getsentry#14739
- getsentry/getsentry#14734
- #74959
- #74958
- #74956
- #74953
- #74955
- #74952
- #74895
- #74892
- #74897
- #74896
- #74893
- #74880
- #74900
- #74902
- #74903
- #74904
- #74899
- #74894
- #74882
- #74881
- #74871
- #74870
- #74855
- #74856
- #74853
- #74857
- #74858
- #74807
- #74805
- #74803
- #74806
- #74798
- #74809
- #74801
- #74804
- #74800
- #74799
- #74725
- #74682
- #74677
- #74680
- #74683
- #74681


needless to say this is probably the largest stacked PR I've ever done
-- and I'm pretty happy with how I was able to split this up into
digestable chunks

<!-- Describe your PR here. -->
roaga pushed a commit to getsentry/sentry that referenced this issue Jul 31, 2024
an absolute ton of work went into enabling this -- but after this PR
model types / create / update / etc. should mostly be typechecked
whereas they were entirely unchecked (everything `Any`) prior to this.

most of the core problem was that mypy and django-stubs did not
understand our custom `BaseManager` and `BaseQuerySet` since there's a
significant amount of django stuff that goes into making those classes
"real"

fix that involved these issues:

- python/mypy#17402 (worked around)
- typeddjango/django-stubs#2228 (the workaround)

with that fixed, it exposed a handful issues in django-stubs itself:

- typeddjango/django-stubs#2240
- typeddjango/django-stubs#2241
- typeddjango/django-stubs#2244
- typeddjango/django-stubs#2248
- typeddjango/django-stubs#2276
- typeddjango/django-stubs#2281

fixing all of those together exposed around 1k issues in sentry code
itself which was fixed over a number of PRs:
- #75186
- #75108
- #75149
- #75162
- #75150
- #75154
- #75158
- #75148
- #75157
- #75156
- #75152
- #75153
- #75151
- #75113
- #75112
- #75111
- #75110
- #75109
- #75013
- #74641
- #74640
- #73783
- #73787
- #73788
- #73793
- #73791
- #73786
- #73761
- #73742
- #73744
- #73602
- #73596
- #73595
- #72892
- #73589
- #73587
- #73581
- #73580
- #73213
- #73207
- #73206
- #73205
- #73202
- #73198
- #73121
- #73109
- #73073
- #73061
- getsentry/getsentry#14370
- #72965
- #72963
- #72967
- #72877 (eventually was able to remove this when upgrading to mypy
1.11)
- #72948
- #72799
- #72898
- #72899
- #72896
- #72895
- #72903
- #72894
- #72897
- #72893
- #72889
- #72887
- #72888
- #72811
- #72872
- #72813
- #72815
- #72812
- #72808
- #72802
- #72807
- #72809
- #72810
- #72814
- #72816
- #72818
- #72806
- #72801
- #72797
- #72800
- #72798
- #72771
- #72718
- #72719
- #72710
- #72709
- #72706
- #72693
- #72641
- #72591
- #72635


mypy 1.11 also included some important improvements with typechecking
`for model_cls in (M1, M2, M2): ...` which also exposed some problems in
our code. unfortunately upgrading mypy involved fixing a lot of things
as well:
- #75020
- #74982
- #74976
- #74974
- #74972
- #74967
- #74954
- getsentry/getsentry#14739
- getsentry/getsentry#14734
- #74959
- #74958
- #74956
- #74953
- #74955
- #74952
- #74895
- #74892
- #74897
- #74896
- #74893
- #74880
- #74900
- #74902
- #74903
- #74904
- #74899
- #74894
- #74882
- #74881
- #74871
- #74870
- #74855
- #74856
- #74853
- #74857
- #74858
- #74807
- #74805
- #74803
- #74806
- #74798
- #74809
- #74801
- #74804
- #74800
- #74799
- #74725
- #74682
- #74677
- #74680
- #74683
- #74681


needless to say this is probably the largest stacked PR I've ever done
-- and I'm pretty happy with how I was able to split this up into
digestable chunks

<!-- Describe your PR here. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong topic-plugins The plugin API and ideas for new plugins
Projects
None yet
Development

No branches or pull requests

2 participants