diff --git a/README.md b/README.md index 4aa8f4901..7aa8a7c2e 100644 --- a/README.md +++ b/README.md @@ -66,6 +66,16 @@ We rely on different `django` and `mypy` versions: | 0.12.x | old semantic analyzer (<0.711), dmypy support | 2.1.x | ^3.6 +## Plugin settings + +An optional strictness flag is available for stronger checks: + +- `warn_manager_type_created_in_model_body` + + If enabled, disallow manager creation via the `.from_queryset` method inside of a model class body. + Read through [My QuerySet methods are returning Any rather than my Model](#my-queryset-methods-are-returning-any-rather-than-my-model) + to get an idea of how to structure manager creation for a model, that'll silence the check. + ## FAQ ### Is this an official Django project? diff --git a/mypy_django_plugin/config.py b/mypy_django_plugin/config.py index b972f7cea..b756d025f 100644 --- a/mypy_django_plugin/config.py +++ b/mypy_django_plugin/config.py @@ -44,8 +44,9 @@ def exit_with_error(msg: str, is_toml: bool = False) -> NoReturn: class DjangoPluginConfig: - __slots__ = ("django_settings_module",) + __slots__ = ("django_settings_module", "warn_manager_type_created_in_model_body") django_settings_module: str + warn_manager_type_created_in_model_body: bool def __init__(self, config_file: Optional[str]) -> None: if not config_file: @@ -80,6 +81,12 @@ def parse_toml_file(self, filepath: Path) -> None: if not isinstance(self.django_settings_module, str): toml_exit("invalid 'django_settings_module': the setting must be a string") + for key in self.__slots__[1:]: + setting = config.get(key, False) + if not isinstance(setting, bool): + toml_exit(INVALID_SETTING(key=key)) + setattr(self, key, setting) + def parse_ini_file(self, filepath: Path) -> None: parser = configparser.ConfigParser() try: @@ -96,3 +103,9 @@ def parse_ini_file(self, filepath: Path) -> None: exit_with_error(MISSING_DJANGO_SETTINGS) self.django_settings_module = parser.get(section, "django_settings_module").strip("'\"") + for key in self.__slots__[1:]: + try: + setting = parser.getboolean(section, key, fallback=False) + except ValueError: + exit_with_error(INVALID_SETTING(key=key)) + setattr(self, key, setting) diff --git a/mypy_django_plugin/errorcodes.py b/mypy_django_plugin/errorcodes.py new file mode 100644 index 000000000..475144a34 --- /dev/null +++ b/mypy_django_plugin/errorcodes.py @@ -0,0 +1,3 @@ +from mypy.errorcodes import ErrorCode + +MANAGER_UNTYPED = ErrorCode("django-manager", "Untyped manager disallowed", "Django") diff --git a/mypy_django_plugin/main.py b/mypy_django_plugin/main.py index 5a14877a3..8f9c156af 100644 --- a/mypy_django_plugin/main.py +++ b/mypy_django_plugin/main.py @@ -24,6 +24,7 @@ from mypy_django_plugin.transformers import fields, forms, init_create, meta, querysets, request, settings from mypy_django_plugin.transformers.managers import ( create_new_manager_class_from_from_queryset_method, + fail_if_manager_type_created_in_model_body, resolve_manager_method, ) from mypy_django_plugin.transformers.models import ( @@ -225,6 +226,11 @@ def get_method_hook(self, fullname: str) -> Optional[Callable[[MethodContext], M django_context=self.django_context, ) + if method_name == "from_queryset" and self.plugin_config.warn_manager_type_created_in_model_body: + info = self._get_typeinfo_or_none(class_fullname) + if info and info.has_base(fullnames.BASE_MANAGER_CLASS_FULLNAME): + return fail_if_manager_type_created_in_model_body + return None def get_base_class_hook(self, fullname: str) -> Optional[Callable[[ClassDefContext], None]]: diff --git a/mypy_django_plugin/transformers/managers.py b/mypy_django_plugin/transformers/managers.py index ef7ff4cc7..c86886ee1 100644 --- a/mypy_django_plugin/transformers/managers.py +++ b/mypy_django_plugin/transformers/managers.py @@ -15,11 +15,12 @@ TypeInfo, Var, ) -from mypy.plugin import AttributeContext, ClassDefContext, DynamicClassDefContext +from mypy.plugin import AttributeContext, ClassDefContext, DynamicClassDefContext, MethodContext from mypy.types import AnyType, CallableType, Instance, ProperType from mypy.types import Type as MypyType from mypy.types import TypeOfAny, TypeVarType, UnboundType, get_proper_type +from mypy_django_plugin import errorcodes from mypy_django_plugin.lib import fullnames, helpers @@ -278,3 +279,20 @@ def create_new_manager_class_from_from_queryset_method(ctx: DynamicClassDefConte # Insert the new manager (dynamic) class assert semanal_api.add_symbol_table_node(ctx.name, SymbolTableNode(GDEF, new_manager_info, plugin_generated=True)) + + +def fail_if_manager_type_created_in_model_body(ctx: MethodContext) -> MypyType: + """ + Method hook that checks if method `.from_queryset` is called inside a model class body. + + Doing so won't, for instance, trigger the dynamic class hook(`create_new_manager_class_from_from_queryset_method`) + for managers. + """ + api = helpers.get_typechecker_api(ctx) + outer_model_info = api.scope.active_class() + if not outer_model_info or not outer_model_info.has_base(fullnames.MODEL_CLASS_FULLNAME): + # Not inside a model class definition + return ctx.default_return_type + + api.fail("`.from_queryset` called from inside model class body", ctx.context, code=errorcodes.MANAGER_UNTYPED) + return ctx.default_return_type diff --git a/tests/test_error_handling.py b/tests/test_error_handling.py index f667978b0..6bf5245cd 100644 --- a/tests/test_error_handling.py +++ b/tests/test_error_handling.py @@ -52,6 +52,15 @@ def write_to_file(file_contents: str, suffix: typing.Optional[str] = None) -> ty "missing required 'django_settings_module' config", id="no-settings-given", ), + pytest.param( + [ + "[mypy.plugins.django-stubs]", + "\tdjango_settings_module = valid", + "\twarn_manager_type_created_in_model_body = abc", + ], + "invalid 'warn_manager_type_created_in_model_body': the setting must be a boolean", + id="warn_manager_type_created_in_model_body is not bool", + ), ], ) def test_misconfiguration_handling(capsys, config_file_contents, message_part): @@ -114,6 +123,15 @@ def test_handles_filename(capsys, filename: str): "could not load configuration file", id="invalid toml", ), + pytest.param( + """ + [tool.django-stubs] + django_settings_module = "badbadmodule" + warn_manager_type_created_in_model_body = "abc" + """, + "invalid 'warn_manager_type_created_in_model_body': the setting must be a boolean", + id="warn_manager_type_created_in_model_body is not bool", + ), ], ) def test_toml_misconfiguration_handling(capsys, config_file_contents, message_part): @@ -138,6 +156,41 @@ def test_correct_toml_configuration() -> None: assert config.django_settings_module == "my.module" +@pytest.mark.parametrize( + ("optional_settings_value", "key", "expected"), + [ + pytest.param( + "warn_manager_type_created_in_model_body = true", + "warn_manager_type_created_in_model_body", + True, + id="warn_manager_type_created_in_model_body as true", + ), + pytest.param( + "warn_manager_type_created_in_model_body = false", + "warn_manager_type_created_in_model_body", + False, + id="warn_manager_type_created_in_model_body as false", + ), + pytest.param( + "", + "warn_manager_type_created_in_model_body", + False, + id="warn_manager_type_created_in_model_body as false when not set", + ), + ], +) +def test_toml_can_parse_optional_setting(optional_settings_value: str, key: str, expected: bool): + file_contents = f""" + [tool.django-stubs] + django_settings_module = "my.module" + {optional_settings_value} + """ + with write_to_file(file_contents, suffix=".toml") as filename: + config = DjangoPluginConfig(filename) + + assert getattr(config, key) is expected + + def test_correct_configuration() -> None: """Django settings module gets extracted given valid configuration.""" config_file_contents = "\n".join( @@ -151,3 +204,40 @@ def test_correct_configuration() -> None: config = DjangoPluginConfig(filename) assert config.django_settings_module == "my.module" + + +@pytest.mark.parametrize( + ("optional_settings_value", "key", "expected"), + [ + pytest.param( + "warn_manager_type_created_in_model_body = true", + "warn_manager_type_created_in_model_body", + True, + id="warn_manager_type_created_in_model_body as true", + ), + pytest.param( + "warn_manager_type_created_in_model_body = false", + "warn_manager_type_created_in_model_body", + False, + id="warn_manager_type_created_in_model_body as false", + ), + pytest.param( + "", + "warn_manager_type_created_in_model_body", + False, + id="warn_manager_type_created_in_model_body as false when not set", + ), + ], +) +def test_can_parse_optional_setting(optional_settings_value: str, key: str, expected: bool): + config_file_contents = "\n".join( + [ + "[mypy.plugins.django-stubs]", + "\tdjango_settings_module = my.module", + f"\t{optional_settings_value}", + ] + ).expandtabs(4) + with write_to_file(config_file_contents) as filename: + config = DjangoPluginConfig(filename) + + assert getattr(config, key) is expected diff --git a/tests/typecheck/managers/querysets/test_from_queryset.yml b/tests/typecheck/managers/querysets/test_from_queryset.yml index 724d5db19..bff5184de 100644 --- a/tests/typecheck/managers/querysets/test_from_queryset.yml +++ b/tests/typecheck/managers/querysets/test_from_queryset.yml @@ -309,3 +309,75 @@ NewManager = MyManager.from_queryset(ModelQuerySet) class MyModel(models.Model): objects = NewManager() + +- case: enabled_from_queryset_in_model_class_body_warning_yields_message + main: | + from myapp.models import MyModel + reveal_type(MyModel.base_manager) # N: Revealed type is "myapp.models.BaseManagerFromMyQuerySet[myapp.models.MyModel]" + reveal_type(MyModel.manager) # N: Revealed type is "myapp.models.ManagerFromMyQuerySet[myapp.models.MyModel]" + reveal_type(MyModel.custom_manager) # N: Revealed type is "myapp.models.MyManagerFromMyQuerySet[myapp.models.MyModel]" + mypy_config: | + [mypy.plugins.django-stubs] + django_settings_module = scripts.django_tests_settings + warn_manager_type_created_in_model_body = true + installed_apps: + - myapp + files: + - path: myapp/__init__.py + - path: myapp/models.py + content: | + from django.db import models + from django.db.models.manager import BaseManager + + class MyQuerySet(models.QuerySet["MyModel"]): + def queryset_method(self) -> int: + return 1 + + class MyManager(BaseManager): + ... + + BaseManagerFromMyQuerySet = BaseManager.from_queryset(MyQuerySet) + ManagerFromMyQuerySet = models.Manager.from_queryset(MyQuerySet) + MyManagerFromMyQuerySet = MyManager.from_queryset(MyQuerySet) + class MyModel(models.Model): + objects1 = BaseManager.from_queryset(MyQuerySet)() # E: `.from_queryset` called from inside model class body + objects2 = BaseManager.from_queryset(MyQuerySet) # E: `.from_queryset` called from inside model class body + objects3 = models.Manager.from_queryset(MyQuerySet)() # E: `.from_queryset` called from inside model class body + objects4 = models.Manager.from_queryset(MyQuerySet) # E: `.from_queryset` called from inside model class body + objects5 = MyManager.from_queryset(MyQuerySet) # E: `.from_queryset` called from inside model class body + objects6 = MyManager.from_queryset(MyQuerySet)() # E: `.from_queryset` called from inside model class body + # Initiating the manager type is fine + base_manager = BaseManagerFromMyQuerySet() + manager = ManagerFromMyQuerySet() + custom_manager = MyManagerFromMyQuerySet() + +- case: disabled_from_queryset_in_model_class_body_warning_is_silent + main: | + from myapp.models import MyModel + reveal_type(MyModel.base_manager) # N: Revealed type is "Any" + reveal_type(MyModel.manager) # N: Revealed type is "Any" + reveal_type(MyModel.custom_manager) # N: Revealed type is "Any" + mypy_config: | + [mypy.plugins.django-stubs] + django_settings_module = scripts.django_tests_settings + warn_manager_type_created_in_model_body = false + installed_apps: + - myapp + files: + - path: myapp/__init__.py + - path: myapp/models.py + content: | + from django.db import models + from django.db.models.manager import BaseManager + + class MyQuerySet(models.QuerySet["MyModel"]): + def queryset_method(self) -> int: + return 1 + + class MyManager(BaseManager): + ... + + class MyModel(models.Model): + base_manager = BaseManager.from_queryset(MyQuerySet)() + manager = models.Manager.from_queryset(MyQuerySet)() + custom_manager = MyManager.from_queryset(MyQuerySet)()