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

Proposal: Allow narrowing ORM fields #511

Open
antonagestam opened this issue Oct 30, 2020 · 14 comments · Fixed by #573
Open

Proposal: Allow narrowing ORM fields #511

antonagestam opened this issue Oct 30, 2020 · 14 comments · Fixed by #573
Labels
bug Something isn't working mypy-plugin Issues specific to mypy_django_plugin

Comments

@antonagestam
Copy link
Contributor

I am currently working on implementing phantom types for Python, which, when implemented throughout an application, would allow statically proving that a value saved on model is validated.

To be able to use this with django-stubs there would need to be some way of narrowing the types that ORM fields hold.

One example would be a field for country codes:

from phantom.ext.iso3166 import CountryCode


class Member(models.Model):
    name = CharField()
    country: CountryCode = CharField(max_length=2)

This makes mypy error if I try to assign a string value to the field:

m = Member()
m.country = "SE"  # E: Incompatible types in assignment (expression has type "str", variable has type "CountryCode")

Unless I first prove that the string is a valid country code:

country_code = "SE"
assert isinstance(country_code, CountryCode)
m.country = country_code
# ... or
m.country = CountryCode.parse(country_code)

Unfortunately mypy raises an error when annotating ORM fields:

Incompatible types in assignment (expression has type "CharField[Union[str, int, Combinable, None], Optional[str]]", variable has type "CountryCode") [assignment]

Although my usecase is phantom types, I believe a feature that enables this would also work equally well with typing.NewType types.

This can be worked around with using the usual dirty tricks of ignore comments or using e.g. typing.cast(CountryCode, CharField()). But it would be very nice to see builtin support for this. I think it would be optimal if this builtin support also verifies that the annotated type is a subtype of the type that the field holds, so that the type annotated to a CharField must be a subtype of str and so on.

What do you think?

PS.: This is a proposal to get a discussion started, I'm not asking anyone to implement this for me.

@antonagestam antonagestam added the bug Something isn't working label Oct 30, 2020
@sobolevn
Copy link
Member

sobolevn commented Oct 30, 2020

Hi, @antonagestam!

You can use this notation:

score: 'models.Field[int, int]' = models.IntegerField(default=0)

Will it work for you?

The problem is that you currently can specify wrong generic params:

score: 'models.Field[str, str]' = models.IntegerField(default=0)  # wrong, but typechecks!

@antonagestam
Copy link
Contributor Author

antonagestam commented Oct 31, 2020

@sobolevn Cool, I did not know about that! I think I'm running into a variance issue though.

I setup a minimal Django project to test with, and testing with this definition just to have less moving parts to think about (also tested with NewType with same results):

from __future__ import annotations

from django.db import models


class CountryCode(str):
    pass


class Member(models.Model):
    name = models.CharField(max_length=200)
    country: models.Field[CountryCode, CountryCode] = models.CharField(max_length=2)

I get:

phango/models.py:12: error: Incompatible types in assignment (expression has type "CharField[Union[str, int, Combinable], str]", variable has type "Field[CountryCode, CountryCode]") [assignment]
        country: models.Field[CountryCode, CountryCode] = models.CharField(max_length=2)
                                                          ^

If I change the definitions of _ST and _GT that Field is generic with regards to, to be contravariant I seem to get the expected behavior without errors. I can open a PR introducing this change.

One further issue though, that might be unrelated to this at all is that I don't get type errors passing field values when instantiating the model. I'm assuming this is unrelated because it passes even with this example:

from __future__ import annotations

from django.db import models


class Member(models.Model):
    value: models.Field[int, int] = models.IntegerField()


m = Member(value="test")  # Expected error here

@leamingrad
Copy link
Contributor

@antonagestam I've been trying to get to the bottom of some similar issues and I don't think that _ST and _GT should both be contravariant. I think that _GT should be covariant instead. This would mean that in practice the Field type is type invariant, but you should still be able to get some better typing when getting and setting field values.

Say we have a phantom NegativeInteger type, which is a subtype of int, and we have the models:

from __future__ import annotations

from django.db import models

class IntMember(models.Model):
    value: models.Field[int, int] = models.IntegerField()

class NegativeIntMember(models.Model):
    value: models.Field[NegativeInt, NegativeInt] = models.IntegerField()

The field should be contravariant when you set it, as IntMember is effectively a subclass of NegativeIntMember for the purposes of setting value. This would result in:

def set_value_to_minus_ten(instance: NegativeIntMember):
    instance.value = NegativeInt(-10)

def set_value_to_fifteen(instance: IntMember):
    instance.value = 15

set_value_to_minus_ten(IntMember(value=7))  # This is fine
set_value_to_minus_ten(NegativeIntMember(value=NegativeInt(-20))  # This is also fine

set_value_to_fifteen(IntMember(value=7))  # This is fine
set_value_to_fifteen(NegativeIntMember(value=NegativeInt(-20))  # This should raise an error

but it should be covariant when you get it, as NegativeIntMember is effectively a subclass of IntMember when you are getting value:

int_member = IntMember(value=7)
negative_member = NegativeIntMember(value=NegativeInt(-20))

int_value: int = int_member.value  # This is fine
negative_value: int = negative_member.value  # Also fine

negative_value: NegativeInt = negative_member.value  # Also fine
int_value: NegativeInt = int_member.value  # This should raise an error

@sobolevn
Copy link
Member

sobolevn commented Mar 5, 2021

@leamingrad this looks like a valid proposal! Do you have the time / will to submit an experiment PR?

@antonagestam
Copy link
Contributor Author

@leamingrad Ah, I didn't realize they should be reversed to each other. Very nicely explained! 👍

(Sorry for off topic): Feel free to jump in on phantom-types/discussions if you'd feel like sharing how you use phantom types! We've slowly started to use them in a production project at work now, would be nice with feedback from other users too :)

@antonagestam
Copy link
Contributor Author

antonagestam commented Mar 6, 2021

I created a new PR: #573

It does make it possible to narrow a field's type, but I found no other way to do that than to use cast(). I think that's related to the plugin's method of picking it up from _pyi_private_{set,get}_type. It should probably only do that if the arguments aren't explicitly overridden?

@sobolevn
Copy link
Member

sobolevn commented Mar 6, 2021

@antonagestam I guess so, but it should still check that types are defined corretly. Maybe we can incorporate this change into your PR as well?

@antonagestam
Copy link
Contributor Author

@sobolevn Yes, fixing that would be awesome. I started looking into it a bit but I somehow always find it extremely hard to understand what's going on in mypy plugins. I assume changes have to be made in https://github.com/typeddjango/django-stubs/blob/master/mypy_django_plugin/transformers/fields.py, but I would need some help to figure out how get types from explicit arguments when provided.

@antonagestam
Copy link
Contributor Author

These are the results I get when not using cast():

Both these give the error below, the first one probably makes sense, but the second one is because the explicit arguments aren't picked up.

published: models.Field[Year, Year] = models.IntegerField()
published: models.Field[Year, Year] = models.IntegerField[Year, Year]()
main:6: error: Incompatible types in assignment (expression has type "IntegerField[Union[float, int, str, Combinable], int]", variable has type "Field[Year, Year]") (diff)

Also tried these:

published: models.IntegerField[Year, Year] = models.IntegerField()
published: models.IntegerField[Year, Year] = models.IntegerField[Year, Year]()
main:7: error: Incompatible types in assignment (expression has type "IntegerField[Union[float, int, str, Combinable], int]", variable has type "IntegerField[Year, Year]") (diff)

This doesn't give an error at assignment, but reveal_type() doesn't give [Year, Year]:

published = models.IntegerField[Year, Year]()
Revealed type is 'django.db.models.fields.IntegerField[Union[builtins.float, builtins.str, django.db.models.expressions.Combinable], builtins.int]' (diff)

@sobolevn
Copy link
Member

sobolevn commented Mar 7, 2021

Yes, looks like we need to fix this in our plugin.

I assume changes have to be made in https://github.com/typeddjango/django-stubs/blob/master/mypy_django_plugin/transformers/fields.py

Yes, I would start from here:

if info:
if info.has_base(fullnames.FIELD_FULLNAME):
return partial(fields.transform_into_proper_return_type, django_context=self.django_context)

but I would need some help to figure out how get types from explicit arguments when provided

Sure, please feel free to ask!

@sobolevn sobolevn reopened this Mar 7, 2021
@sobolevn
Copy link
Member

sobolevn commented Mar 7, 2021

I am going to leave this open, since we can also change how our plugin works.

@leamingrad
Copy link
Contributor

@antonagestam Thanks for getting a PR merged so quickly! To be honest, I don't actually use phantom types, but it seemed like a clearer way to give examples (most of the issues I have been working with are to do with relations between abstract models which this is a precursor of). That said, it seems like an interesting idea so I will definitely check it out.

@sobolevn
Copy link
Member

I don't actually use phantom types

This is pure gold, @antonagestam I am marketing it a lot 🙂

@last-partizan
Copy link

Hi, I'm trying to add types to django-phonenumber-field package, and it turned out trickier than expected.

I've got it working locally with django-types and pyright, by using following stubs for django-phonenumber

class PhoneNumberField(models.CharField[_T]):  # type: ignore
    @overload
    def __new__(
        cls, *args, null: Literal[False] = ..., **kwargs
    ) -> PhoneNumberField[PhoneNumber]: ...
    @overload
    def __new__(
        cls, *args, null: Literal[True] = ..., **kwargs
    ) -> PhoneNumberField[PhoneNumber | None]: ...
    @overload
    def __new__(cls, *args, **kwargs) -> PhoneNumberField[PhoneNumber]: ...
    def __new__(cls, *args, **kwargs) -> PhoneNumberField[PhoneNumber | None]: ...

And now i want to make proper PR for the project, but we decided to use mypy and django-stubs, and i cannot figure out how to properly assign types.

My first try was to use class PhoneNumberField(CharField[PhoneNumber | str, PhoneNumber]), but it doesn't work.

Then i tried using _pyi_private_set_type:, and it not working either.

tests/models.py:3: note: In module imported here:
phonenumber_field/modelfields.py: note: In class "PhoneNumberField":
phonenumber_field/modelfields.py:53: error: Incompatible types in assignment (expression has type
"Union[str, PhoneNumber, Combinable]", base class "CharField" defined the type as "Union[str, int, Combinable]")
[assignment]
        _pyi_private_set_type: str | PhoneNumber | Combinable
        ^
phonenumber_field/modelfields.py:54: error: Incompatible types in assignment (expression has type "PhoneNumber", base
class "CharField" defined the type as "str")  [assignment]
        _pyi_private_get_type: PhoneNumber
        ^
tests/tests.py: note: In member "test_typing" of class "PhoneNumberFieldAppTest":
tests/tests.py:298: note: Revealed type is "Any"
Found 2 errors in 1 file (checked 9 source files)

Any ideas how to do this?

@flaeppe flaeppe added the mypy-plugin Issues specific to mypy_django_plugin label Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mypy-plugin Issues specific to mypy_django_plugin
Development

Successfully merging a pull request may close this issue.

5 participants