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

Fix partially analyzed settings module #1162

Closed
wants to merge 2 commits into from

Conversation

PIG208
Copy link
Contributor

@PIG208 PIG208 commented Sep 24, 2022

I have made things!

For modules django.http and django.http.request, we manually add
dependency for the module containing AUTH_USER_MODEL for mypy to
resolve first.

This potentially adds cyclic dependency when the models module contains
a reference to django.conf, and the settings depend on any modules that
rely on django.http or django.http.request. At this point, the settings module is
only partially analyzed. The symbol table is populated (so top-level settings are
accessible by the name), but the types have not been inferred.

Removing this dependency of django.http and django.http.request works. But
if the settings module also depends on django.contrib.auth, this still breaks.

This can potentially break the refined type for request.user, which does depend
on AUTH_USER_MODEL being available beforehand. This implicit dependency
still needs to be somehow established.

So, until these issues are addressed, this fix is not final.

Related issues

andersk and others added 2 commits September 21, 2022 17:14
For modules "django.http" and "django.http.request", we manually add
dependency for the module containing "AUTH_USER_MODEL" for mypy to
resolve first.

This potentially adds cyclic dependency when the models module contains
reference to django.conf, and the settings depend on any modules that
rely on "django.http" or "django.http.request". At this point, the types
of the settings are only partially resolved.

Signed-off-by: Zixuan James Li <[email protected]>
@andersk
Copy link
Contributor

andersk commented Sep 24, 2022

But if the settings module also depends on django.contrib.auth, this still breaks.

That might not be a problem. My understanding is that a settings module is not supposed to have any dependencies on Django. That’s why AUTH_USER_MODEL is a string, for example.

@PIG208
Copy link
Contributor Author

PIG208 commented Sep 24, 2022

Yes. Essentially the problem we want to solve here is the cyclic dependency that is not supposed to emerge from the settings.

@andersk
Copy link
Contributor

andersk commented Sep 24, 2022

Hmm. Maybe my test case, and by extension the Zulip code it originated from, is just wrong for that reason.

@PIG208
Copy link
Contributor Author

PIG208 commented Sep 24, 2022

It is possible that this dependency is inevitable because it is from django_stubs_ext, which needs to be imported into the settings module. So probably we can fix that.

@andersk
Copy link
Contributor

andersk commented Sep 24, 2022

Maybe the application’s __init__.py is a better place for the django_stubs_ext.monkeypatch()? (If so, the documentation should be adjusted.)

@PIG208
Copy link
Contributor Author

PIG208 commented Sep 24, 2022

I ran this fix in Zulip. The error does not go away unless this get_user_model dependency as well as those for defined_model_classes (models referred via ForeignKeys) get removed completely. It appears that there are more ways to trigger this error through adding depdencies.

@andersk
Copy link
Contributor

andersk commented Sep 24, 2022

I got it working by breaking a bunch of import cycles in Zulip (zulip/zulip#23040), and moving django_stubs_ext.monkeypatch() to zerver/__init__.py.

To make this solution robust, ideally we’d replace this fallback to value.__class__ with an error, so that we’ll see the mistake if anyone tries to reintroduce such a cycle.

andersk added a commit to andersk/django-stubs that referenced this pull request 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 pull request 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 pull request 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 pull request 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 pull request 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]>
@sobolevn
Copy link
Member

Instead we merged #1163

Thanks for working on this!

@sobolevn sobolevn closed this Sep 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Type from settings not correctly loaded in a model
3 participants