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

Refactor support for annotate to utilise mypy.types.Instance.extra_attrs #2319

Merged
merged 2 commits into from
Aug 5, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 5 additions & 15 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -211,11 +211,11 @@ class MyManager(models.Manager["MyModel"]):

### How do I annotate cases where I called QuerySet.annotate?

Django-stubs provides a special type, `django_stubs_ext.WithAnnotations[Model]`, which indicates that the `Model` has
been annotated, meaning it allows getting/setting extra attributes on the model instance.
Django-stubs provides a special type, `django_stubs_ext.WithAnnotations[Model, <Annotations>]`, which indicates that
the `Model` has been annotated, meaning it requires extra attributes on the model instance.

Optionally, you can provide a `TypedDict` of these attributes,
e.g. `WithAnnotations[MyModel, MyTypedDict]`, to specify which annotated attributes are present.
You should provide a `TypedDict` of these attributes, e.g. `WithAnnotations[MyModel, MyTypedDict]`, to specify which
annotated attributes are present.

Currently, the mypy plugin can recognize that specific names were passed to `QuerySet.annotate` and
include them in the type, but does not record the types of these attributes.
Expand All @@ -235,21 +235,11 @@ class MyModel(models.Model):
username = models.CharField(max_length=100)


def func(m: WithAnnotations[MyModel]) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that we should remove this feature. Because I see that a lot of users discuss this and use this. Maybe we can treat WithAnnotations[MyModel] as WithAnnotations[MyModel, Any] implicitly? Via type var defaults or something?

This way we can continue to support this code (maybe with a deprecation warning, why not). But breaking it - does not seem right to me :(

Copy link
Member

Choose a reason for hiding this comment

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

But, I agree that we should not have done this in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I suppose, gonna have to see if I can get it working. The support for any/arbitrary attributes is a bit of work I think

Copy link
Member

Choose a reason for hiding this comment

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

We only need to support Any, no other arguments / types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the Any type argument is fine. But that doesn't add support for accessing an arbitrary attribute, like the previous implementation supported.

Essentially you'll still get something like:

def func(m: WithAnnotations[MyModel]) -> str:
    return m.asdf  # E: MyModel@AnnotatedWith[Any] has no attribute "asdf"

I don't have any solution that removes the "has no attribute" error.

Copy link
Member Author

@flaeppe flaeppe Aug 5, 2024

Choose a reason for hiding this comment

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

There's 1 problem that I see with that, unless I'm missing something, which is that Model@AnnotatedWith[Any] is an Instance and not a TypeInfo. But we need to add the __getattr__ method to the TypeInfo though if we do, all instances of Model@AnnotatedWith gets the __getattr__ method

Copy link
Member

Choose a reason for hiding this comment

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

Won't .extra_attrs do the job for single instance __getattr__? 🤔
I am not sure, just asking.

Copy link
Member Author

Choose a reason for hiding this comment

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

__getattr__ will work for the arbitrary attributes. But we still want/need to be able to match the model type on e.g. a function argument.

This whole thing is kind of twofold.

  1. Subclassing the model and the generic TypedDict argument for proper matching for WithAnnotations as a function argument
  2. extra_attrs for accessing declared annotated attributes e.g. inside a function body

__getattr__ would help out with attributes accessed inside the body. But I can't find a way to add it and keep 1. working and not affect all other instances. I've also tried adding the __getattr__ method to extra_attrs with no luck: ExtraAttrs(attrs={"__getattr__": CallableType(...)}, ...)

There's also the TypeInfo.fallback_to_any which is the same as subclassing Any or our AnyAttrAllowed. But same issue here, it affects the type and not instances of said type.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks a lot for your research! Not really what I hoped for :(

In this case, let's do the cleanest solution and break WithAnnotations[M] usage in a way that we restrict known attributes.

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 was also hoping to find a way that kept backwards compatibility. At least I'm hoping breaking this will make it a bit easier going forward.

Do you mean restrict known attributes like emitting "has no attribute" errors?

Then, calling wise, if we replace

WithAnnotations[MyModel] -> MyModel@AnnotatedWith[Any]

We can require that .annotate has been called, but we don't care with what. Just like before. I'm thinking this is the implementation we want.

Else if we replace like (this is what we do now)

WithAnnotations[MyModel] -> MyModel

This wouldn't distinguish between having called .annotate or not. Meaning that there's no requirement of having called .annotate but having done it is also accepted.

return m.asdf # OK, since the model is annotated as allowing any attribute


func(MyModel.objects.annotate(foo=Value("")).get(id=1)) # OK
func(
MyModel.objects.get(id=1)
) # Error, since this model will not allow access to any attribute


class MyTypedDict(TypedDict):
foo: str


def func2(m: WithAnnotations[MyModel, MyTypedDict]) -> str:
def func(m: WithAnnotations[MyModel, MyTypedDict]) -> str:
print(m.bar) # Error, since field "bar" is not in MyModel or MyTypedDict.
return m.foo # OK, since we said field "foo" was allowed

Expand Down
4 changes: 2 additions & 2 deletions ext/django_stubs_ext/annotations.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class Annotations(Generic[_Annotations]):

WithAnnotations = Annotated[_T, Annotations[_Annotations]]
"""Alias to make it easy to annotate the model `_T` as having annotations
`_Annotations` (a `TypedDict` or `Any` if not provided).
`_Annotations` (a `TypedDict`).

Use as `WithAnnotations[MyModel]` or `WithAnnotations[MyModel, MyTypedDict]`.
Use as `WithAnnotations[MyModel, MyTypedDict]`.
"""
10 changes: 0 additions & 10 deletions mypy_django_plugin/django/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@

from mypy_django_plugin.exceptions import UnregisteredModelError
from mypy_django_plugin.lib import fullnames, helpers
from mypy_django_plugin.lib.fullnames import WITH_ANNOTATIONS_FULLNAME

# This import fails when `psycopg2` is not installed, avoid crashing the plugin.
try:
Expand Down Expand Up @@ -142,15 +141,6 @@ def model_modules(self) -> Dict[str, Dict[str, Type[Model]]]:

def get_model_class_by_fullname(self, fullname: str) -> Optional[Type[Model]]:
"""Returns None if Model is abstract"""
annotated_prefix = WITH_ANNOTATIONS_FULLNAME + "["
if fullname.startswith(annotated_prefix):
# For our "annotated models", extract the original model fullname
fullname = fullname[len(annotated_prefix) :].rstrip("]")
if "," in fullname:
# Remove second type arg, which might be present
fullname = fullname[: fullname.index(",")]
fullname = fullname.replace("__", ".")

module, _, model_cls_name = fullname.rpartition(".")
return self.model_modules.get(module, {}).get(model_cls_name)

Expand Down
2 changes: 1 addition & 1 deletion mypy_django_plugin/lib/fullnames.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
MANAGER_CLASS_FULLNAME = "django.db.models.manager.Manager"
RELATED_MANAGER_CLASS = "django.db.models.fields.related_descriptors.RelatedManager"

WITH_ANNOTATIONS_FULLNAME = "django_stubs_ext.WithAnnotations"
WITH_ANNOTATIONS_FULLNAME = "django_stubs_ext.annotations.WithAnnotations"
ANNOTATIONS_FULLNAME = "django_stubs_ext.annotations.Annotations"

BASEFORM_CLASS_FULLNAME = "django.forms.forms.BaseForm"
Expand Down
21 changes: 14 additions & 7 deletions mypy_django_plugin/lib/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,14 @@
from typing_extensions import TypedDict

from mypy_django_plugin.lib import fullnames
from mypy_django_plugin.lib.fullnames import WITH_ANNOTATIONS_FULLNAME

if TYPE_CHECKING:
from mypy_django_plugin.django.context import DjangoContext


class DjangoTypeMetadata(TypedDict, total=False):
is_abstract_model: bool
is_annotated_model: bool
from_queryset_manager: str
reverse_managers: Dict[str, str]
baseform_bases: Dict[str, int]
Expand Down Expand Up @@ -104,6 +104,14 @@ def get_manager_to_model(manager: TypeInfo) -> Optional[str]:
return get_django_metadata(manager).get("manager_to_model")


def mark_as_annotated_model(model: TypeInfo) -> None:
get_django_metadata(model)["is_annotated_model"] = True


def is_annotated_model(model: TypeInfo) -> bool:
return get_django_metadata(model).get("is_annotated_model", False)


class IncompleteDefnException(Exception):
pass

Expand Down Expand Up @@ -285,10 +293,6 @@ def get_nested_meta_node_for_current_class(info: TypeInfo) -> Optional[TypeInfo]
return None


def is_annotated_model_fullname(model_cls_fullname: str) -> bool:
return model_cls_fullname.startswith(WITH_ANNOTATIONS_FULLNAME + "[")


def create_type_info(name: str, module: str, bases: List[Instance]) -> TypeInfo:
# make new class expression
classdef = ClassDef(name, Block([]))
Expand Down Expand Up @@ -382,9 +386,12 @@ def convert_any_to_type(typ: MypyType, referred_to_type: MypyType) -> MypyType:


def make_typeddict(
api: CheckerPluginInterface, fields: "OrderedDict[str, MypyType]", required_keys: Set[str]
api: Union[SemanticAnalyzer, CheckerPluginInterface], fields: Dict[str, MypyType], required_keys: Set[str]
) -> TypedDictType:
fallback_type = api.named_generic_type("typing._TypedDict", [])
if isinstance(api, CheckerPluginInterface):
fallback_type = api.named_generic_type("typing._TypedDict", [])
else:
fallback_type = api.named_type("typing._TypedDict", [])
typed_dict_type = TypedDictType(fields, required_keys=required_keys, fallback=fallback_type)
return typed_dict_type

Expand Down
2 changes: 1 addition & 1 deletion mypy_django_plugin/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ def get_type_analyze_hook(self, fullname: str) -> Optional[Callable[[AnalyzeType
"typing_extensions.Annotated",
"django_stubs_ext.annotations.WithAnnotations",
):
return partial(handle_annotated_type, django_context=self.django_context)
return partial(handle_annotated_type, fullname=fullname)
else:
return None

Expand Down
144 changes: 98 additions & 46 deletions mypy_django_plugin/transformers/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,29 @@
TypeInfo,
Var,
)
from mypy.plugin import AnalyzeTypeContext, AttributeContext, CheckerPluginInterface, ClassDefContext
from mypy.plugin import AnalyzeTypeContext, AttributeContext, ClassDefContext
from mypy.plugins import common
from mypy.semanal import SemanticAnalyzer
from mypy.typeanal import TypeAnalyser
from mypy.types import AnyType, Instance, ProperType, TypedDictType, TypeOfAny, TypeType, TypeVarType, get_proper_type
from mypy.types import (
AnyType,
ExtraAttrs,
Instance,
ProperType,
TypedDictType,
TypeOfAny,
TypeType,
TypeVarType,
get_proper_type,
)
from mypy.types import Type as MypyType
from mypy.typevars import fill_typevars
from mypy.typevars import fill_typevars, fill_typevars_with_any

from mypy_django_plugin.django.context import DjangoContext
from mypy_django_plugin.errorcodes import MANAGER_MISSING
from mypy_django_plugin.exceptions import UnregisteredModelError
from mypy_django_plugin.lib import fullnames, helpers
from mypy_django_plugin.lib.fullnames import ANNOTATIONS_FULLNAME, ANY_ATTR_ALLOWED_CLASS_FULLNAME, MODEL_CLASS_FULLNAME
from mypy_django_plugin.lib.fullnames import ANNOTATIONS_FULLNAME, MODEL_CLASS_FULLNAME
from mypy_django_plugin.transformers.fields import FieldDescriptorTypes, get_field_descriptor_types
from mypy_django_plugin.transformers.managers import (
MANAGER_METHODS_RETURNING_QUERYSET,
Expand Down Expand Up @@ -200,6 +210,50 @@ def run_with_model_cls(self, model_cls: Type[Model]) -> None:
raise NotImplementedError(f"Implement this in subclass {self.__class__.__name__}")


class AddAnnotateUtilities(ModelClassInitializer):
"""
Creates a model subclass that will be used when the model's manager/queryset calls
'annotate' to hold on to attributes that Django adds to a model instance.

Example:

class MyModel(models.Model):
...

class MyModel_AnnotatedWith(MyModel, django_stubs_ext.Annotations[_Annotations]):
...
"""

def run(self) -> None:
annotations = self.lookup_typeinfo_or_incomplete_defn_error("django_stubs_ext.Annotations")
object_does_not_exist = self.lookup_typeinfo_or_incomplete_defn_error(fullnames.OBJECT_DOES_NOT_EXIST)
multiple_objects_returned = self.lookup_typeinfo_or_incomplete_defn_error(fullnames.MULTIPLE_OBJECTS_RETURNED)
annotated_model_name = self.model_classdef.info.name + "_AnnotatedWith"
flaeppe marked this conversation as resolved.
Show resolved Hide resolved
annotated_model = self.lookup_typeinfo(self.model_classdef.info.module_name + "." + annotated_model_name)
if annotated_model is None:
model_type = fill_typevars_with_any(self.model_classdef.info)
assert isinstance(model_type, Instance)
annotations_type = fill_typevars(annotations)
assert isinstance(annotations_type, Instance)
annotated_model = self.add_new_class_for_current_module(
annotated_model_name, bases=[model_type, annotations_type]
)
annotated_model.defn.type_vars = annotations.defn.type_vars
annotated_model.add_type_vars()
helpers.mark_as_annotated_model(annotated_model)
if self.is_model_abstract:
# Below are abstract attributes, and in a stub file mypy requires
# explicit ABCMeta if not all abstract attributes are implemented i.e.
# class is kept abstract. So we add the attributes to get mypy off our
# back
helpers.add_new_sym_for_info(
annotated_model, "DoesNotExist", TypeType(Instance(object_does_not_exist, []))
)
helpers.add_new_sym_for_info(
annotated_model, "MultipleObjectsReturned", TypeType(Instance(multiple_objects_returned, []))
)


class InjectAnyAsBaseForNestedMeta(ModelClassInitializer):
"""
Replaces
Expand Down Expand Up @@ -1034,6 +1088,7 @@ def run(self) -> None:

def process_model_class(ctx: ClassDefContext, django_context: DjangoContext) -> None:
initializers = [
AddAnnotateUtilities,
InjectAnyAsBaseForNestedMeta,
AddDefaultPrimaryKey,
AddPrimaryKeyAlias,
Expand All @@ -1059,31 +1114,39 @@ def set_auth_user_model_boolean_fields(ctx: AttributeContext, django_context: Dj
return Instance(boolinfo, [])


def handle_annotated_type(ctx: AnalyzeTypeContext, django_context: DjangoContext) -> MypyType:
def handle_annotated_type(ctx: AnalyzeTypeContext, fullname: str) -> MypyType:
is_with_annotations = fullname == fullnames.WITH_ANNOTATIONS_FULLNAME
args = ctx.type.args
if not args:
return AnyType(TypeOfAny.from_omitted_generics) if is_with_annotations else ctx.type
type_arg = ctx.api.analyze_type(args[0])
if not isinstance(type_arg, Instance) or not type_arg.type.has_base(MODEL_CLASS_FULLNAME):
return type_arg

fields_dict = None
if len(args) > 1:
second_arg_type = get_proper_type(ctx.api.analyze_type(args[1]))
if isinstance(second_arg_type, TypedDictType):
if isinstance(second_arg_type, TypedDictType) and is_with_annotations:
fields_dict = second_arg_type
elif isinstance(second_arg_type, Instance) and second_arg_type.type.fullname == ANNOTATIONS_FULLNAME:
annotations_type_arg = get_proper_type(second_arg_type.args[0])
if isinstance(annotations_type_arg, TypedDictType):
fields_dict = annotations_type_arg
elif not isinstance(annotations_type_arg, AnyType):
ctx.api.fail("Only TypedDicts are supported as type arguments to Annotations", ctx.context)
elif annotations_type_arg.type_of_any == TypeOfAny.from_omitted_generics:
ctx.api.fail("Missing required TypedDict parameter for generic type Annotations", ctx.context)

if fields_dict is None:
return type_arg

assert isinstance(ctx.api, TypeAnalyser)
assert isinstance(ctx.api.api, SemanticAnalyzer)
return get_or_create_annotated_type(ctx.api.api, type_arg, fields_dict=fields_dict)
return get_annotated_type(ctx.api.api, type_arg, fields_dict=fields_dict)


def get_or_create_annotated_type(
api: Union[SemanticAnalyzer, CheckerPluginInterface], model_type: Instance, fields_dict: Optional[TypedDictType]
def get_annotated_type(
api: Union[SemanticAnalyzer, TypeChecker], model_type: Instance, fields_dict: TypedDictType
) -> ProperType:
"""

Expand All @@ -1094,42 +1157,31 @@ def get_or_create_annotated_type(
If the user wanted to annotate their code using this type, then this is the annotation they would use.
This is a bit of a hack to make a pretty type for error messages and which would make sense for users.
"""
model_module_name = "django_stubs_ext"

if helpers.is_annotated_model_fullname(model_type.type.fullname):
# If it's already a generated class, we want to use the original model as a base
model_type = model_type.type.bases[0]

if fields_dict is not None:
type_name = f"WithAnnotations[{model_type.type.fullname.replace('.', '__')}, {fields_dict}]"
if model_type.extra_attrs:
extra_attrs = ExtraAttrs(
attrs={**model_type.extra_attrs.attrs, **(fields_dict.items if fields_dict is not None else {})},
immutable=model_type.extra_attrs.immutable.copy(),
mod_name=None,
)
else:
type_name = f"WithAnnotations[{model_type.type.fullname.replace('.', '__')}]"

annotated_typeinfo = helpers.lookup_fully_qualified_typeinfo(
cast(TypeChecker, api), model_module_name + "." + type_name
)
if annotated_typeinfo is None:
model_module_file = api.modules.get(model_module_name) # type: ignore[union-attr]
if model_module_file is None:
return AnyType(TypeOfAny.from_error)

if isinstance(api, SemanticAnalyzer):
annotated_model_type = api.named_type_or_none(ANY_ATTR_ALLOWED_CLASS_FULLNAME, [])
assert annotated_model_type is not None
else:
annotated_model_type = api.named_generic_type(ANY_ATTR_ALLOWED_CLASS_FULLNAME, [])

annotated_typeinfo = helpers.add_new_class_for_module(
model_module_file,
type_name,
bases=[model_type] if fields_dict is not None else [model_type, annotated_model_type],
fields=fields_dict.items if fields_dict is not None else None,
no_serialize=True,
extra_attrs = ExtraAttrs(
attrs=fields_dict.items if fields_dict is not None else {},
immutable=None,
mod_name=None,
)
if fields_dict is not None:
# To allow structural subtyping, make it a Protocol
annotated_typeinfo.is_protocol = True
# Save for later to easily find which field types were annotated
annotated_typeinfo.metadata["annotated_field_types"] = fields_dict.items
annotated_type = Instance(annotated_typeinfo, [])
return annotated_type

annotated_model: Optional[TypeInfo]
if helpers.is_annotated_model(model_type.type):
annotated_model = model_type.type
if model_type.args and isinstance(model_type.args[0], TypedDictType):
fields_dict = helpers.make_typeddict(
api,
fields={**model_type.args[0].items, **fields_dict.items},
required_keys={*model_type.args[0].required_keys, *fields_dict.required_keys},
)
else:
annotated_model = helpers.lookup_fully_qualified_typeinfo(api, model_type.type.fullname + "_AnnotatedWith")

if annotated_model is None:
return model_type
return Instance(annotated_model, [fields_dict], extra_attrs=extra_attrs)
12 changes: 4 additions & 8 deletions mypy_django_plugin/transformers/orm_lookups.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
from mypy_django_plugin.django.context import DjangoContext
from mypy_django_plugin.exceptions import UnregisteredModelError
from mypy_django_plugin.lib import fullnames, helpers
from mypy_django_plugin.lib.helpers import is_annotated_model_fullname


def typecheck_queryset_filter(ctx: MethodContext, django_context: DjangoContext) -> MypyType:
Expand Down Expand Up @@ -33,13 +32,10 @@ def typecheck_queryset_filter(ctx: MethodContext, django_context: DjangoContext)
provided_type = resolve_combinable_type(provided_type, django_context)

lookup_type: MypyType
if is_annotated_model_fullname(model_cls_fullname):
lookup_type = AnyType(TypeOfAny.implementation_artifact)
else:
try:
lookup_type = django_context.resolve_lookup_expected_type(ctx, model_cls, lookup_kwarg)
except UnregisteredModelError:
lookup_type = AnyType(TypeOfAny.from_error)
try:
lookup_type = django_context.resolve_lookup_expected_type(ctx, model_cls, lookup_kwarg)
except UnregisteredModelError:
lookup_type = AnyType(TypeOfAny.from_error)
# Managers as provided_type is not supported yet
if isinstance(provided_type, Instance) and helpers.has_any_of_bases(
provided_type.type, (fullnames.MANAGER_CLASS_FULLNAME, fullnames.QUERYSET_CLASS_FULLNAME)
Expand Down
Loading
Loading