Skip to content

Type from settings not correctly loaded in a model #312

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

Closed
lfrodrigues opened this issue Jan 31, 2020 · 22 comments
Closed

Type from settings not correctly loaded in a model #312

lfrodrigues opened this issue Jan 31, 2020 · 22 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@lfrodrigues
Copy link

lfrodrigues commented Jan 31, 2020

Bug report

I have this in settings:

BANK_ENTITIES = [('a', 'A Bank'), ('b', 'B Bank'),]

and this in my model:

entity = models.CharField(max_length=20, choices=settings.BANK_ENTITIES, blank=True)

What's wrong

mypy fails with:

models.py:247: error: Argument "choices" to "CharField" has incompatible type "tuple"; expected "Optional[Iterable[Union[Tuple[Any, Any], Tuple[str, Iterable[Tuple[Any, Any]]]]]]"

If I place a reveal in settings and another in models.py I get

settingsbase.py:90: note: Revealed type is 'builtins.list[Tuple[builtins.str, builtins.str]]'
models.py:214: note: Revealed type is 'builtins.list'

How is that should be

mypy should be able to load the full type from settings, not only that it's a list

System information

  • OS: Ubuntu 18.04
  • python version: 3.6
  • django version: 2.2
  • mypy version: 0.761
  • django-stubs version: 1.4.0
@lfrodrigues lfrodrigues added the bug Something isn't working label Jan 31, 2020
@sobolevn
Copy link
Member

Can you please post the result of reveal_type(BANK_ENTITIES) inside your settings and reveal_type(settings.BANK_ENTITIES) inside your models?

@lfrodrigues
Copy link
Author

lfrodrigues commented Jan 31, 2020

I posted it in the initial report:

settingsbase.py:90: note: Revealed type is 'builtins.list[Tuple[builtins.str, builtins.str]]'
models.py:214: note: Revealed type is 'builtins.list'

@sobolevn
Copy link
Member

Oh, sorry, I have missed it. Yes, looks like a bug!
Thanks for the report.

@AdrienLemaire

This comment has been minimized.

@sobolevn sobolevn added the help wanted Extra attention is needed label Jun 16, 2020
@kszmigiel
Copy link
Member

@lfrodrigues could you verify if this bug still occurs? I wasn't able to reproduce it at all. I guess that someone fixed this in the meantime, or maybe your mypy.ini file wasn't properly configured.

@lfrodrigues
Copy link
Author

I just tested it again. If I just have a simple settings.py file like the one created when django starts a project there is no problem.

The problem is that my settings import different sub modules for settings depending on the country that was configured.

Here's a demo repository:
https://github.com/lfrodrigues/testdjangotypes

from django.conf import settings

def view():
    a = settings.MY_SETTINGS['item']['subitem']
~/tmp/demo_error/src$ mypy app1/
app1/views.py:9: error: Value of type "_VT" is not indexable

@kszmigiel
Copy link
Member

@lfrodrigues Thank you for the tips, I'll try to fix this as soon as I have enough time.

@anentropic
Copy link

anentropic commented Sep 16, 2020

I think I have a related issue

# settings.py
SOME_DICT = {
    'a': 'whatever',
    'b': 'whatever',
    'c': 'whatever',
}
# myproject/mymodule.py
from django.conf import settings

whatever = settings.SOME_DICT["b"]

and mypy gives the following error:

myproject/mymodule.py:3: error: Invalid index type "str" for "dict"; expected type "_KT"

The _KT TypeVar appears to be defined in mypy typeshed in relation to Mapping container, i.e. for dicts

Adding an annotation to the settings like this does not help:

# settings.py
from typing import Dict

SOME_DICT: Dict[str, str] = {
    'a': 'whatever',
    'b': 'whatever',
    'c': 'whatever',
}

The problem is that my settings import different sub modules for settings depending on the country that was configured.

we also have similar situation with conditional import of sub-module settings, maybe is relevant

(my example code above is simplified so might not reproduce the issue by itself)

I am able to make mypy happy with the following workaround:

# myproject/mymodule.py
from typing import Dict
from django.conf import settings

_some_dict: Dict[str, str] = settings.SOME_DICT

whatever = _some_dict["b"]

(and no annotation in settings.py)

@anentropic
Copy link

this looks the same #352

@BeWe11
Copy link

BeWe11 commented Apr 15, 2021

I am experiencing the same bug with django-stubs 1.8.0 and django-stubs-ext 0.2.0. Defining dictionaries in a settings file and than importing them via django.conf.settings loses the type annotation, e.g.

# in settings.py
mydict = {"a": ["b", "c"]}
reveal_type(mydict) -> builtins.dict[builtins.str*, typing.Sequence*[builtins.str]]

becomes

# in other.py
from django.conf import settings
reveal_type(settings.mydict) -> builtings.dict

@RJPercival
Copy link
Contributor

@kszmigiel, are you still intending to fix this or should it be unassigned?

@sobolevn
Copy link
Member

@RJPercival feel free to take this over! 👍

@bogdan-zs
Copy link

bogdan-zs commented May 30, 2021

Hello, I have the same issue, I tried to understand why it happens. In this function, plugin cant extract type from sym variable, because it's dict variable? after that, the plugin tries to understand what`s type, based on the variable value.

def get_type_of_settings_attribute(ctx: AttributeContext, django_context: DjangoContext) -> MypyType:
assert isinstance(ctx.context, MemberExpr)
setting_name = ctx.context.name
if not hasattr(django_context.settings, setting_name):
ctx.api.fail(f"'Settings' object has no attribute {setting_name!r}", ctx.context)
return ctx.default_attr_type
typechecker_api = helpers.get_typechecker_api(ctx)
# first look for the setting in the project settings file, then global settings
settings_module = typechecker_api.modules.get(django_context.django_settings_module)
global_settings_module = typechecker_api.modules.get("django.conf.global_settings")
for module in [settings_module, global_settings_module]:
if module is not None:
sym = module.names.get(setting_name)
if sym is not None and sym.type is not None:
return sym.type
# if by any reason it isn't present there, get type from django settings
value = getattr(django_context.settings, setting_name)
value_fullname = helpers.get_class_fullname(value.__class__)
value_info = helpers.lookup_fully_qualified_typeinfo(typechecker_api, value_fullname)
if value_info is None:
return ctx.default_attr_type
return Instance(value_info, [])

In the case with dict value, the plugin gets type from builtins module.
https://github.com/python/mypy/blob/master/mypy/typeshed/stdlib/builtins.pyi#L788

class dict(MutableMapping[_KT, _VT], Generic[_KT, _VT]):

I think, the plugin needs to check if its dict and return dict type from mypy module, like

Dict[Any, Any]

If it's the good solution I will make PR.

@GergelyKalmar
Copy link

Any chance to move this issue forward? Configuration keeps producing quite a lot of warnings and it is a bit difficult to work around it.

@andersk
Copy link
Contributor

andersk commented Sep 20, 2022

Here’s a complete minimal reproducible test case in ~20 lines. Note that bad.py and good.py are identical, but they show different types from reveal_type. All that distinguishes them is that bad happens to be imported from my_app.models.

bad.py:4: note: Revealed type is "builtins.list"
good.py:4: note: Revealed type is "builtins.list[builtins.int]"

bad.py

from django.conf import settings

def test() -> None:
    reveal_type(settings.MY_LIST)

good.py

from django.conf import settings

def test() -> None:
    reveal_type(settings.MY_LIST)

my_app/__init__.py

# empty

my_app/models.py

from django.db import models
import bad

class MyUser(models.Model):
    pass

my_settings.py

from other import other

AUTH_USER_MODEL = "my_app.MyUser"
INSTALLED_APPS = ["my_app"]
MY_LIST = [1]

other.py

from django_stubs_ext import ValuesQuerySet

other = ()

pyproject.toml

[tool.django-stubs]
django_settings_module = "my_settings"

[tool.mypy]
plugins = ["mypy_django_plugin.main"]

Reproduction:

$ pip install Django \
    'git+https://github.com/typeddjango/django-stubs.git@6f9afd3c#egg=django-stubs[compatible-mypy]' \
    'git+https://github.com/typeddjango/django-stubs.git@6f9afd3c#subdirectory=django_stubs_ext'

$ pip freeze
asgiref==3.5.2
Django==4.1.1
django-stubs @ git+https://github.com/typeddjango/django-stubs.git@6f9afd3cc9202a3d1147f78e53ef31ced2f03452
django-stubs-ext==0.6.0
mypy==0.971
mypy-extensions==0.4.3
sqlparse==0.4.2
tomli==2.0.1
types-pytz==2022.2.1.0
types-PyYAML==6.0.11
typing_extensions==4.3.0

$ mypy .
bad.py:4: note: Revealed type is "builtins.list"
good.py:4: note: Revealed type is "builtins.list[builtins.int]"
Success: no issues found in 6 source files

Notes:

  • The Git version of django-stubs seems to be necessary for this test case. A git bisect finds that the problem reproduces with 8b5509b and not with its parent. Of course, we’ve seen reports from well before then, so that’s probably a fluke.
  • I’m not sure what role other.py plays, but it seems to be necessary for reproduction.

@andersk
Copy link
Contributor

andersk commented Sep 20, 2022

In my test case, during the first run of get_type_of_settings_attribute (for bad.py), module.names is

SymbolTable(
  AUTH_USER_MODEL : Gdef/Var (my_settings.AUTH_USER_MODEL) : builtins.str
  INSTALLED_APPS : Gdef/Var (my_settings.INSTALLED_APPS)
  MY_LIST : Gdef/Var (my_settings.MY_LIST)
  other : Gdef/Var (other.other) : Tuple[])

so sym.type is None and it falls back to inspecting the runtime value of value.__class__, resulting in builtins.list. But during the second run (for good.py), module.names is now

SymbolTable(
  AUTH_USER_MODEL : Gdef/Var (my_settings.AUTH_USER_MODEL) : builtins.str
  INSTALLED_APPS : Gdef/Var (my_settings.INSTALLED_APPS) : builtins.list[builtins.str]
  MY_LIST : Gdef/Var (my_settings.MY_LIST) : builtins.list[builtins.int]
  other : Gdef/Var (other.other) : Tuple[])

so sym.type is correctly builtins.list[builtins.int].

It seems to me that falling back to the runtime type isn’t how a static type checker should ever behave. I don’t know much about the mypy API, but if the static type hasn’t been inferred yet, surely there must be a way to trigger mypy to finish inferring the type and/or delay until it has done so.

@PIG208
Copy link
Contributor

PIG208 commented Sep 21, 2022

The reproduction seems plausible, but I'm not yet able to reproduce it in the pytest environment. I guess this is very specific to how the modules are set up and how mypy is invoked. There is a defer_node method available on the type checker, but I will need to verify if it suits this use case.

andersk added a commit to andersk/django-stubs that referenced this issue Sep 22, 2022
@andersk
Copy link
Contributor

andersk commented Sep 22, 2022

@PIG208 Here’s my example as a failing pytest case: #1159.

@PIG208
Copy link
Contributor

PIG208 commented Sep 22, 2022

Deferring type analysis for an attribute seems to be tricky, as I can't find direct support for that. The only relevant plugin API for deferring is only supported for get_dynamic_class_hook and get_base_class_hook. Attempted to fix this by returning PlaceholderType or UnboundType, but that didn't work.

@PIG208
Copy link
Contributor

PIG208 commented Sep 23, 2022

Actually, this might have more to do with the get_additional_deps hook. Any modules accessing settings should depend on the settings module.

@PIG208
Copy link
Contributor

PIG208 commented Sep 24, 2022

Tracking the debugging process in #1162. We need to come up with a proper fix for this issue.

andersk added a commit to andersk/django-stubs that referenced this issue Sep 24, 2022
This fallback to value.__class__ seems to be doing more harm than
good; see typeddjango#312 and typeddjango#1162.  Replace it with a clear error message that
suggests a way to fix the problem rather than papering over it.

Signed-off-by: Anders Kaseorg <[email protected]>
andersk added a commit to andersk/django-stubs that referenced this issue Sep 24, 2022
This fallback to value.__class__ seems to be doing more harm than
good; see typeddjango#312 and typeddjango#1162.  Replace it with a clear error message that
suggests a way to fix the problem rather than incompletely papering
over it.

Signed-off-by: Anders Kaseorg <[email protected]>
andersk added a commit to andersk/django-stubs that referenced this issue Sep 24, 2022
This fallback to value.__class__ seems to be doing more harm than
good; see typeddjango#312 and typeddjango#1162.  Replace it with a clear error message that
suggests a way to fix the problem rather than incompletely papering
over it.

Signed-off-by: Anders Kaseorg <[email protected]>
andersk added a commit to andersk/django-stubs that referenced this issue Sep 24, 2022
This fallback to value.__class__ seems to be doing more harm than
good; see typeddjango#312 and typeddjango#1162.  Replace it with a clear error message that
suggests a way to fix the problem rather than incompletely papering
over it.

Signed-off-by: Anders Kaseorg <[email protected]>
sobolevn pushed a commit that referenced this issue Sep 24, 2022
This fallback to value.__class__ seems to be doing more harm than
good; see #312 and #1162.  Replace it with a clear error message that
suggests a way to fix the problem rather than incompletely papering
over it.

Signed-off-by: Anders Kaseorg <[email protected]>

Signed-off-by: Anders Kaseorg <[email protected]>
@flaeppe
Copy link
Member

flaeppe commented Sep 24, 2023

Since #1163 was merged in favour of #1162, I'm going to close this off.

@flaeppe flaeppe closed this as not planned Won't fix, can't repro, duplicate, stale Sep 24, 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 help wanted Extra attention is needed
Development

Successfully merging a pull request may close this issue.