Default type for Field.errors of tuple() is immediately overwritten

[miketheman]

The initial data type for Field.errors is set to a tuple:

wtforms/src/wtforms/fields/core.py

Line 19 in 9cc223a

errors = tuple()

which is reassigned to a list if there's any errors:

wtforms/src/wtforms/fields/core.py

Line 215 in 9cc223a

self.errors = list(self.process_errors)

which causes type checkers like mypy to be sad when you use the field in your own code directly.

Actual Behavior

in repro.py:

import wtforms

class MyForm(wtforms.Form):
    name = wtforms.StringField()

    def validate(self, extra_validators=None, *args, **kwargs):
        super().validate(extra_validators, *args, **kwargs)

        if self.name.data == 'repro':
            # note the use of `append()`, unavailable on a tuple.
            self.name.errors.append('Name cannot be "repro"')

$ mypy --check-untyped-defs repro.py
repro.py:10: error: "Sequence[str]" has no attribute "append"  [attr-defined]
Found 1 error in 1 file (checked 1 source file)

Expected Behavior

$ mypy --check-untyped-defs repro.py
Success: no issues found in 1 source file

Environment

• Python version: 3.12
• wtforms version: 3.2.1
• mypy version: 1.12.1

Yes, this specific example could be solved another way with validate_name, but that's not the point. The point is that the error's initial datatype is incorrect.
Everywhere it's used, it's treated as a list.

I'd be happy to send a PR changing to an empty list, but I didn't know if there was some other reason to have the data type as a tuple initially.

[Daverball]

I generally use assert isinstance(field.errors, list) in my code when I need to append errors inside a validator.

I think there is some value to having the default be immutable, since it indicates that validate has not yet been called and there may be process_errors that haven't yet been copied over to errors.

Generally the data model is a bit dangerous here. since it relies on people calling things in the proper order, so although it can be a little bit annoying to use in practice, it seems preferable to me over letting people do things that are unsafe.

We could potentially be more generous by detecting incorrect use in some other way and either emit an Exception or Warning.

Although looking at the code there probably wasn't a big safety concern behind the default value, an empty tuple just happened to be the only safe class-scoped default value they could use.

Personally I'm +0 on changing process_errors and errors to always be a list. It gets rid of some annoying false positives, but on the other hand, the false positives are also a good hint that what you're doing is not always totally safe.

[miketheman]

I generally use assert isinstance(field.errors, list) in my code when I need to append errors inside a validator.

I'm finding the same in my code, type narrowing to tell mypy what's what.

Personally I'm +0 on changing process_errors and errors to always be a list. It gets rid of some annoying false positives, but on the other hand, the false positives are also a good hint that what you're doing is not always totally safe.

Maybe the correct thing to do here is to modify the stubs instead?

https://github.com/python/typeshed/blob/6029bf18192fba492a9d82d88238a4e1929e1c43/stubs/WTForms/wtforms/fields/core.pyi#L26-L27

Instead of a Sequence, would a MutableSequence would be appropriate here, and thus no changes to the actual codebase?
Does that create a further disconnect between the default behavior vs the one folks actively use?

[Daverball]

I did consider writing the stubs to be more permissive initially, but I ultimately decided against it, since FormField and FieldList violate LSP on errors returning a dict and a list of lists or list of dicts respectively, so taking some additional care and clueing people into the fact that getting a list is not guaranteed, seemed warranted. I would feel more comfortable with the change if errors were always a list at runtime. I might even consider changing it to list[str] | Any at that point to signal that some fields may yield errors with a different shape.

Changing process_errors in the stub however seems a lot more safe, since processing always happens (or at least it should, can't force subclasses to behave).