Fix attr-defined check for werkzeug.wrappers.BaseRequest (fixes #3913)

[LarsMichelsen]

The PR #2740 added setattr and getattr to the
werkzeug.wrappers.BaseRequest definition to silence mypy when
monkeypatching this class.

The change disabled the errors for monkeypatching, but disabled disabled the
important check attr-defined.

[srittau]

Cc @ilevkivskyi @pcorpet who added the stubs originally.

[ilevkivskyi]

I don't think this is a good idea TBH. This will cause many false positives with the benefit of catching trivial bugs that would be caught at runtime easily.

[LarsMichelsen]

I had assumed that typing systems and static checkers are all about finding bugs before execution.

In fact, in practice, such behaviour contributes to a loss of confidence in such tools. Or at least the tools don't seem to be that useful anymore.

In the concrete situation one can ideally work according to the motto: mypy will tell me if I use a non-existent attribute. But as soon as basic checks in elementary (or unpredictable) classes of an application (or popular library) no longer work and I as a developer always have to keep in mind that the problem exists, this reduces the value of the tool for me, because I can no longer blindly rely on it.

But to be more specific: Isn't it cleaner to subclass BaseRequest and define its own attributes there? In that case you would be able to check attributes cleanly.

I am not deep in the Werkzeug universe. Is it really more common to monkeypatch wild attributes?

[ilevkivskyi]

Is it really more common to monkeypatch wild attributes?

I am not a big expert either, but IIRC last time we tried this it actually caused issues. Also if you so into purity, why don't you just fix this locally for yourself? You can use --custom-typeshed-dir or something similar.

[LarsMichelsen]

Also if you so into purity, why don't you just fix this locally for yourself? You can use --custom-typeshed-dir or something similar.

Sure. Don't worry about my PR. I just wanted to offer my help. The PR can be closed if it's not useful.

We have already adapted the stubs locally (Checkmk/checkmk@e5064fa). Unfortunately I didn't manage to make the adjustment with a partial stub according to PEP 561. So I had to copy all the stubs from tool and make the adjustment. Now it works as expected.

[srittau]

I am going to merge this PR. The original change to add the monkey patching behavior was added in #2740 (which was in a PR about adding the flask package) without much discussion. (There was a bit more discussion about the Flask class being monkey patched.)

Nothing in the documentation of werkzeug.BaseRequest stands out to me to suggest that monkey patching it is encouraged. The documentation even suggests that to add additional functionality, you should use custom subclasses.

Projects that decide to monkey patch this class can always use # type: ignore, which marks the monkey patched attributes as not typed checked explicitly. If it turns out that this is indeed a common pattern in many code bases, we can always revert this decision.