[requests] Sync HttpError stub to implementation

[ubersan]

The implementation has no restrictions on providing a response. Is there a reason why the stub requires it?

[github-actions]

Diff from mypy_primer, showing the effect of this PR on open source code:

cloud-init (https://github.com/canonical/cloud-init)
+ cloudinit/url_helper.py:343: error: Item "None" of "Response | None" has no attribute "status_code"  [union-attr]
+ cloudinit/url_helper.py:344: error: Item "None" of "Response | None" has no attribute "headers"  [union-attr]
- tests/unittests/sources/azure/test_errors.py:169: error: Missing named argument "response" for "HTTPError"  [call-arg]

paasta (https://github.com/yelp/paasta)
- paasta_tools/mesos_maintenance.py:213: error: Missing named argument "response" for "HTTPError"  [call-arg]

poetry (https://github.com/python-poetry/poetry)
+ src/poetry/publishing/uploader.py:35: error: Item "None" of "Response | None" has no attribute "status_code"  [union-attr]
+ src/poetry/publishing/uploader.py:35: error: Item "None" of "Response | None" has no attribute "reason"  [union-attr]
+ src/poetry/publishing/uploader.py:35: error: Item "None" of "Response | None" has no attribute "content"  [union-attr]

twine (https://github.com/pypa/twine)
+ twine/__main__.py:36: error: Item "None" of "Response | None" has no attribute "status_code"  [union-attr]
+ twine/__main__.py:39: error: Item "None" of "Response | None" has no attribute "url"  [union-attr]
+ twine/__main__.py:39: error: Item "None" of "Response | None" has no attribute "reason"  [union-attr]

schemathesis (https://github.com/schemathesis/schemathesis)
+ src/schemathesis/cli/output/default.py: note: In function "display_service_error":
+ src/schemathesis/cli/output/default.py:368: error: Item "None" of "Optional[Response]" has no attribute "status_code"  [union-attr]
+ src/schemathesis/cli/__init__.py: note: In function "handle_service_error":
+ src/schemathesis/cli/__init__.py:1233: error: Item "None" of "Optional[Response]" has no attribute "status_code"  [union-attr]
+ src/schemathesis/cli/__init__.py:1234: error: Item "None" of "Optional[Response]" has no attribute "json"  [union-attr]
+ src/schemathesis/cli/__init__.py:1235: error: Item "None" of "Optional[Response]" has no attribute "status_code"  [union-attr]
+ src/schemathesis/cli/__init__.py: note: In function "login":
+ src/schemathesis/cli/__init__.py:1357: error: Item "None" of "Optional[Response]" has no attribute "json"  [union-attr]

apprise (https://github.com/caronc/apprise)
- test/helpers/rest.py:63: error: Missing named argument "response" for "HTTPError"  [call-arg]
- test/test_attach_http.py:59: error: Missing named argument "response" for "HTTPError"  [call-arg]
- test/test_config_http.py:54: error: Missing named argument "response" for "HTTPError"  [call-arg]

openlibrary (https://github.com/internetarchive/openlibrary)
+ openlibrary/catalog/get_ia.py: note: In function "urlopen_keep_trying":
+ openlibrary/catalog/get_ia.py:25: error: Item "None" of "Response | None" has no attribute "status_code"  [union-attr]

[srittau]

Cf #10764

[srittau]

This PR is problematic, as the primer output shows. A lot of code assumes that HTTPError.response is not None, which is true for the only place in requests that raises an HTTPError:

https://github.com/psf/requests/blob/b55bb15c35132f021fc0997c6a72730abe2216f5/src/requests/models.py#L1019

On the other hand, some code outside requests that constructs HTTPErrors assumes that the response argument is optional. Even requests has a test that asserts that:

https://github.com/psf/requests/blob/b55bb15c35132f021fc0997c6a72730abe2216f5/tests/test_requests.py#L1542

Especially considering the test in requests, I propose to accept this PR, even if it means that some code needs asserts or type ignore. It's also the safer alternative.

[srittau]

LGTM, see my last comment, but I would like another maintainer to chime in.

[AlexWaygood]

There's no way of making everybody happy here, but this approach does have the advantage that it is, well, correct.

[ubersan]

thanks for the comments, I agree it's not super nice to break quite some apps, but at least we're starting to being aligned again with the implementation going forward

[intgr]

FWIW I think this is a step backwards.

[srittau]

@intgr In the end, this is an issue to be taken up with the requests project. It would make sense to me that HTTPError requires the response to be set, but this isn't the case in the implementation, and in fact there is an explicit test in requests that HTTPError can be used without a response set, so this is clearly intended usage.

[bluetech]

This change has caused quite a lot of busy work for us as well. I think that in practice, it would have been better to leave response not optional. requests always passes a response to HTTPError, and "HTTP error" itself sort of implies that a response be present (otherwise it's not really an HTTP error...). I think that outweighs type unsafety from some 3rd party packages not passing a response. But I understand @srittau's rationale and I wouldn't change it back at this point.

[asottile-sentry]

is there any way to change this so that by default it has Response -- but when constructed directly it does not? this change is a big step backwards otherwise

[Akuli]

I think this should be reverted.

As already mentioned, the only place in requests that raises HTTPError is:

        if http_error_msg:
            raise HTTPError(http_error_msg, response=self)

So practically, when you do except HTTPError, the response will always be set. I think handling an exception with try/except is by far the most important use case for exception type stubs in a library. So making it "just work" should be our main goal.

IMO the unit test that initializes response to None doesn't mean anything. When you write unit tests, you tend to write unit tests for everything, including that dumb feature that seemed like a good idea at the time but was not needed after all. To me, the test would be more meaningful if it was more integration test style than unit test style (for example, if the test actually made requests throw HTTPError with no response set in some obscure corner case).

Is there really a practical problem that this PR solves, and is it really worth solving? If there is, and I'm just unaware of it, I'd suggest using the Any trick (now documented in CONTRIBUTING.md).

[srittau]

The practical problem is that people initialize HTTPError in their own code.

[Akuli]

is there any way to change this so that by default it has Response -- but when constructed directly it does not?

Not really, but we did the next best thing in #11207: you can now assume it has a Response or check whether it's missing without any type checking errors. A new version of types-requests with the fix will be released automatically in about 12 hours.