Skip to content

Remove some unnecessary defer rounds#2252

Open
flaeppe wants to merge 2 commits into
typeddjango:masterfrom
flaeppe:fix/pointless-defers
Open

Remove some unnecessary defer rounds#2252
flaeppe wants to merge 2 commits into
typeddjango:masterfrom
flaeppe:fix/pointless-defers

Conversation

@flaeppe

@flaeppe flaeppe commented Jul 5, 2024

Copy link
Copy Markdown
Member

Change so that we don't trigger unnecessary defer rounds during manager creation via the from_queryset method call.

Refs:

Comment on lines 544 to 562

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm going to argue that the changes here improves correctness. Since the manager in the test comes from a function that specifies a manager as return type:

def LocalManager() -> models.Manager:
    """
    Returns a manager instance of an inlined manager type that can't
    be resolved.
    """
    class InnerManager(models.Manager): ...
    return InnerManager()

This (now) assigns a type models.Manager[Any], the return type of the function. I would say that the previous behaviour did too much, since it tried to express that assigned an invalid manager. But to know that it had to draw conclusions from the function body and thus "override" the return type.

The plugin should now only consider calls of the format: objects = <Manager>.from_queryset(<QuerySet>)() or objects = <QuerySet>.as_manager() in this case

Change so that we don't trigger unnecessary defer rounds during manager
creation via the `from_queryset` method call.
@flaeppe flaeppe force-pushed the fix/pointless-defers branch from 8ed6834 to 07503e6 Compare July 13, 2024 10:00
@flaeppe

flaeppe commented Jul 13, 2024

Copy link
Copy Markdown
Member Author

To clarify a little bit more; previously the plugin could tell mypy to defer on e.g. a case like this:

def not_a_manager() -> Any: ...

class SomeModel(models.Model):
    objects = not_a_manager()

Deferring isn't gonna make it a more of a manager. The changed code looks for a more narrow/specific case that we want and can handle that comes from from_queryset. For any other situation we now just continue without deferring.

@flaeppe

flaeppe commented Jul 15, 2024

Copy link
Copy Markdown
Member Author

@sobolevn, @intgr would any of you guys mind checking this out? I'm suspecting it could improve a couple of errors in #1023 (though I have no proof that it will)

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.

1 participant