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 manager types scope #991

Merged
merged 4 commits into from
Jun 14, 2022
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
3 changes: 2 additions & 1 deletion mypy.ini
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
allow_redefinition = True
check_untyped_defs = True
ignore_missing_imports = True
incremental = True
# Incremental mode causes false-negatives
incremental = False
Copy link
Member

Choose a reason for hiding this comment

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

We had this flag to support mypy in incremental mode as the first-class-citizen. It is important for:

  • large projects that use --incremental to speed up their checks
  • IDEs that use mypy plugins

I am not sure that removing it is a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, okay. Reverted this and added explicit mention in CONTRIBUTING.md

Copy link
Member

Choose a reason for hiding this comment

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

Can you please post unexpected results here? What were they?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The unexpected result was quite simple: I started from this regression test and confirmed that it fails. Then I attempted first fix (added module_name) and rerun test suite - this reg.test passed as expected and two others failed. Then I reverted everything and... got fully passing test suite, including reg.test - which is obviously wrong. Removing mypy cache helped to make reg. test fail again, so incremental mode is harmful during development. Maybe we should have different configs for CI and local development?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should have different configs for CI and local development?

Yes, this is a good option!

strict_optional = True
show_traceback = True
warn_no_return = False
Expand Down
9 changes: 4 additions & 5 deletions mypy_django_plugin/lib/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -369,11 +369,10 @@ def bind_or_analyze_type(t: MypyType, api: SemanticAnalyzer, module_name: Option
That should hopefully give a bound type."""
if isinstance(t, UnboundType) and module_name is not None:
node = api.lookup_fully_qualified_or_none(module_name + "." + t.name)
if node is None:
return None
return node.type
else:
return api.anal_type(t)
if node is not None and node.type is not None:
return node.type

return api.anal_type(t)


def copy_method_to_another_class(
Expand Down
6 changes: 5 additions & 1 deletion mypy_django_plugin/transformers/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,11 @@ def create_new_model_parametrized_manager(self, name: str, base_manager_info: Ty
# replace self type with new class, if copying method
if isinstance(sym.node, FuncDef):
helpers.copy_method_to_another_class(
new_cls_def_context, self_type=custom_manager_type, new_method_name=name, method_node=sym.node
new_cls_def_context,
self_type=custom_manager_type,
new_method_name=name,
method_node=sym.node,
original_module_name=base_manager_info.module_name,
)
continue

Expand Down
19 changes: 19 additions & 0 deletions tests/typecheck/managers/test_managers.yml
Original file line number Diff line number Diff line change
Expand Up @@ -423,3 +423,22 @@
class MyModel(models.Model):

objects = MyModelManager()

- case: regression_manager_scope_foreign
main: |
from myapp.models import MyModel
reveal_type(MyModel.on_site) # N: Revealed type is "myapp.models.MyModel_CurrentSiteManager[myapp.models.MyModel]"
installed_apps:
- myapp
- django.contrib.sites
files:
- path: myapp/__init__.py
- path: myapp/models.py
content: |
from django.db import models
from django.contrib.sites.models import Site
from django.contrib.sites.managers import CurrentSiteManager

class MyModel(models.Model):
site = models.ForeignKey(Site, on_delete=models.CASCADE)
on_site = CurrentSiteManager()