Form.clean method return annotation

[sondrelg]

Bug report

What's wrong

The clean method for Django forms is currently defined as Optional[Dict[str, Any]].

How is that should be

Since the clean method just returns cleaned_data, it seems like it should match the type of cleaned_data (django source):

    def clean(self):
        """
        Hook for doing any extra form-wide cleaning after Field.clean() has been
        called on every field. Any ValidationError raised by this method will
        not be associated with a particular field; it will have a special-case
        association with the field named '__all__'.
        """
        return self.cleaned_data

I also think cleaned_data seems correctly typed (as Dict[str, Any]), though I haven't looked into it too in-depth.

Additional context

The type for clean seems to have been changed in #862 on the basis that the clean method is described as "permitted to return None" wrt. the frameworks constraints.

I can see why the change was made, but all calls to super().clean() now require (unnecessary in almost all cases) checking for optionality:

def clean_some_field():
    cleaned_data = super().clean()  # type is Optional[dict], so this method will create a mypy error
    raw_some_field = cleaned_data['some_field']
    return some_operation(raw_some_field)

This code will generate error: Value of type "Optional[Mapping[str, Any]]" is not indexable [index] which seems wrong.

It seems wrong because the type of super().clean() will always be dict, unless the form inherits from another form instance that has changed the type signature (AFACIT).

---

There might be a third option, but if our choices are:

1. Keep the type hint Optional[dict] to prevent mypy errors when changing the return type of clean in a form inheriting from Form
2. Revert to dict to prevent mypy errors when calling super().clean()

I personally think nr. 2 is the correct choice.

Does that make sense?

@adamchainz, do you see things differently?

[adamchainz]

I do see things differently... This is just how inheritance and types work.

If you don't put None in the base Form type hints, Mypy complains for forms that do use return None in their clean() methods.

Moreover, if you write a form class, it could always (later) be combined via multiple inheritance with another one that returns None. So Mypy's strictness around the types is catching a potential error.

Yes one has to check for Nones but that's just the general nuisance of things being optional...

[sondrelg]

To put it more plainly, Django will never return an optional dict, and my forms will also never return optional dicts, so I don't see why this should be the return signature for the code.

I'm open to being convinced otherwise though. Could you elaborate on why you think type hinting all potential returns types of inherited forms makes more sense than specifying precisely what the library does return? Is there a good reason why you would change the signature for your own forms' clean methods (other than for the fact that it's possible)? I'm not really sold on the fact that this is a valid return signature to begin with.

If there's a documented standard/convention for this, I guess that would settle this pretty quickly.

[ngnpope]

I don't think this is a bug and Optional[Dict[str, Any]] is correct.

If you look at the documentation it states the following:

The call to super().clean() in the example code ensures that any validation logic in parent classes is maintained. If your form inherits another that doesn’t return a cleaned_data dictionary in its clean() method (doing so is optional), then don’t assign cleaned_data to the result of the super() call and use self.cleaned_data instead:

This is also backed up in where Django calls .clean() internally:

https://github.com/django/django/blob/9a22d1769b042a88741f0ff3087f10d94f325d86/django/forms/forms.py#L463-L464

The problem is that there is no way to determine whether super().clean() is going to return None or Dict[str, Any].

Instead, we have to do the following if we want to access the cleaned data:

class Form(forms.Form):
    ...
    
    def clean(self) -> Optional[Dict[str, Any]]:
        cleaned_data = super().clean()
        if cleaned_data is None:
            cleaned_data = self.cleaned_data

        ...
        
        return cleaned_data

This narrows the type so that we can always expect cleaned_data to be Dict[str, Any].

It seems that it would be prudent to do this always given that None can be returned and some refactoring, e.g. adding some mixin with a .clean() that only validates and doesn't want to return modifications, could cause bugs to surface:

class FormMixin:
    def clean(self):
        cleaned_data = super().clean()
        if cleaned_data.get("field") != "value":
            raise ValidationError("Invalid value.")

class Form(FormMixin, forms.Form):
    def clean(self):
        cleaned_data = super.clean()
        cleaned_data.get("field")    # AttributeError

[adamchainz]

Indeed. Your mixin example shows the kind of pattern that type hints help us catch, which would not be possible without the correct hint.