Hygienic mypy plugin: don't import Django into the mypy process

[intgr]

We have no immediate plans to implement this, but I decided to open this issue for discussions, to split it out from #1337.

Original post by @intgr #1337 (comment)

The mypy plugin should be "hygienic" with regards to Django code being checked. It should not directly import project code or Django into the mypy runtime, so it doesn't get entangled with the code it's checking. Instead it could for example invoke subprocesses to discover Django internals.

The current implementation is problematic for many reasons. For instance mypy shouldn't crash with traceback when there's a syntax error in Django settings. And when using mypy daemon dmypy, it should be capable of re-discovering changed models files, instead of using obsolete state or randomly crashing, as it does now.

[intgr]

Comment by @adamchainz #1337 (comment)

Probably a bit of a brain twister but would be nice to see. I think it would also make it a bit easier to separate out what needs discovering and open the door for other type checkers.

[intgr]

Original comment by @sobolevn #1337 (comment)

Since we discussing a lot of things in this issue, I will add my 2c about "Same with the hygienic plugin redesign".

I think it is a really bad idea. When we were originally designing the current state of the plugin, we already had static-analysis-only previous version of the plugin. It was a nightmare.

Since django has so many magic things happening in the model definition, it is almost impossible to get right.

And running django during type checking is not a bad thing. For example, django-check do this as well. So, this is rather standard.

So, I highly recommend not to change this.

[intgr]

Original comment by @flaeppe #1337 (comment)

I've touched the plugin code a bit and I'd would like to agree with both of your opinions regarding the hygienic plugin redesign. I've seen a bit of "shortcutting", in regards to choosing to use Django (runtime) internals to resolve types in some ends. What I mean is that it would/could be possible to use mypy's internals to figure out correct types, but Django's has been used instead, probably because it's more straight forward and simple. It could very well be possible what mypy offers is/was (currently) insufficient too.

With that said, I have a very hard time seeing that it will be possible to get rid of the dependency on Django's runtime completely, but I'm quite certain that some parts could (and perhaps should) stop using Django to compute types and start using some "mypy approach" instead(or custom, for that matter).

Two other "hygienic" parts on the top of my head are:

• Report coverage for the plugin's Python code (I'm suspecting quite a bit of dead code)
• Have test case support for cached mypy runs (there are a lot of bugs that has been resolved but we have no regression tests for, due to the limitation)

I think both of the above might be fixes for/in https://github.com/TypedDjango/pytest-mypy-plugins

[intgr]

I think it is a really bad idea. When we were originally designing the current state of the plugin, we already had static-analysis-only previous version of the plugin. It was a nightmare.

Since django has so many magic things happening in the model definition, it is almost impossible to get right.

I suspected this as well, reimplementing all of Django's logic in our plugin may be too big and messy to maintain. But the current situation is messy as well, does anyone really disagree?

My thinking was to implement something in between the two approaches: the plugin would work off a cache of model metadata. When our mypy plugin needs to update the cache of Django model metadata, it would invoke a subprocess that may import Django, discover all the internals it needs, and returns it in some format. When this subprocess crashes, we can emit some warnings and continue with the old cache, instead of failing entirely.

This would solve the main issues I see with the current design:

1. Slowness -- Django is a behemoth to boot up (e.g. #1341). I'm assuming we could instead use cached Django metadata during most invocations.
2. We don't support mypy daemon properly -- when models change, there's no way to discover new changes when the files were already imported.
3. Crashes -- it's really ugly that mypy crashes with a traceback when there's an error in project code that Django imports (e.g. models).

But I might just be underestimating how difficult this cache discovery or management would be.

[federicobond]

One approach that can be helpful here is to begin by restricting direct manipulation of Django objects to the DjangoContext class. All other users inside the plugin must deal with lightweight objects returned by DjangoContext methods that serve as representations of the underlying Django model metadata.

This way it's possible to map the extent of the information that we would have to obtain and cache from Django without committing to a particular implementation. Once the rest of the plugin is using these lightweight metadata objects it's then possible to construct them inside DjangoContext from information gathered by a separate isolated process, for example.

[ljodal]

I'd be very interested to work on this. We have multiple large Django projects at work and the startup times of our projects makes it quite painful to work with mypy, especially when you want like feedback like through an editor integration. It's also quite annoying that you'll loose all feedback while editing if there's an error anywhere in the codebase. Normally mypy would be happy processing the rest of the files.

Does anyone have an overview over what features currently rely on the runtime? I'm most familiar with the ORM parts of the plugin and know that it's used extensively there, but I also feel like it should be possible to extract the required information statically. Yes we end up implementing some Django logic, but I don't think it'd be that bad. The one thing I'm unsure about is wether it'd be possible to "backfill" previous models with related managers as we discover them or if we need some kind of global model state available before we can fully count the models as processed.

I like the idea of running a separate process to load the Django state, however that'd still have all the downsides of loading up a large and slow codebase. And if we want it to be performant it has to be cached, which means dealing with cache invalidation. I suspect the effort required to get a good caching strategy to work here would quite high.

[flaeppe]

[...] Does anyone have an overview over what features currently rely on the runtime? [...]

A lot of pieces relying on runtime related to models can partly be found here (triggered by mypy's early on get_base_class_hook):

django-stubs/mypy_django_plugin/transformers/models.py

Lines 590 to 601 in 0117348

	def process_model_class(ctx: ClassDefContext, django_context: DjangoContext) -> None:
	initializers = [
	InjectAnyAsBaseForNestedMeta,
	AddDefaultPrimaryKey,
	AddPrimaryKeyAlias,
	AddRelatedModelsId,
	AddManagers,
	AddDefaultManagerAttribute,
	AddRelatedManagers,
	AddExtraFieldMethods,
	AddMetaOptionsAttribute,
	]

Although I think the general case is that usage of the runtime is quite scattered over the whole plugin. I agree with others that there's probably a better abstraction layer to be had, to sort things out.

I like the idea of running a separate process to load the Django state, however that'd still have all the downsides of loading up a large and slow codebase. And if we want it to be performant it has to be cached, which means dealing with cache invalidation. I suspect the effort required to get a good caching strategy to work here would quite high.

What about starting to get better compatibility going with the Mypy daemon? That should probably speed things up, if possible.

[ljodal]

Hmm, okay, so looking around the code is seems like the django runtime is used for basically two things: 1) models and related metadata and 2) settings. I believe it should be possible to solve both of those cases without the runtime, but it would probably have some limitations related to things that are very dynamic (ie. managers defined with a dynamic super class and other things that are not easily resolved using using static analysis). For those cases it should be possible to add some manual type hints to help the plugin understand that a manager exists, similar to how you'd add hints if you're trying to type a django project without the plugin.

I like the idea of running a separate process to load the Django state, however that'd still have all the downsides of loading up a large and slow codebase. And if we want it to be performant it has to be cached, which means dealing with cache invalidation. I suspect the effort required to get a good caching strategy to work here would quite high.

What about starting to get better compatibility going with the Mypy daemon? That should probably speed things up, if possible.

Yeah, that would definitively be a nice improvement, though I'm completely unfamiliar with the daemon, so I wouldn't know where to start. I tried setting up dmypy though pylsp just the other day and couldn't get it to work. Not entirely sure what the problem was, but I suspect is was somehow related to the Django plugin

[intgr]

if we want it to be performant it has to be cached, which means dealing with cache invalidation. I suspect the effort required to get a good caching strategy to work here would quite high.

Mypy already caches types in parsed files and handles cache invalidation (.mypy_cache). It also provides some cache hooks for plugins. It might be possible to piggyback on that mechanism to know when we need to re-analyze models and/or settings.

What about starting to get better compatibility going with the Mypy daemon?

FWIW I've tried dmypy on multiple occasions to automatically typecheck files after saving, and found it to be quite unreliable. When invoking dmypy run in sequence to check various different files, it has caused crashes, hangs, false positive errors and false negative errors (compared to mypy without daemon). I haven't reported them as they are difficult to isolate, and they may be caused by mypy_django_plugin. But I get the impression they are upstream dmypy issues.

Proper invalidation for discovered Django models would be helpful for dmypy usage anyway. With our current architecture, the user would have to manually restart dmypy every time they change models, to get accurate results.

[ljodal]

I was curious about what it would take to set up the models without the runtime information, so I've spent a few hours today playing around with setting up a plugin from scratch. My progress so far can be found here: https://github.com/ljodal/django-stubs-poc (at the time of writing on this commit)

The readme describes some of the complexity I've had to work around to get the dependencies between models set up. It's not ideal, since there's no way of telling what related managers exist just by looking at a single model definition. It is however possible to make this work good enough I think.

My idea is basically to build up some state with the information needed to add all the dynamic runtime attributes. Another approach to getting this is also to manually resolve all the model files and parse the files using the ast library. I expect that to be significantly faster than loading the full runtime, especially for larger projects. If we can define this state entirely without relying on Django's classes, we could even have swappable "backends" here, so you'd be able to choose which approach you want based on the project.