Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

override http.HTTPMessage methods to only use str for the header type #11114

Merged
merged 5 commits into from
Dec 19, 2023

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Dec 6, 2023

Assuming HTTPMessage should only work in str headers, which I'm not certain about. But either way would close #2333 .

Signature of the overridden methods based on #11095 .
An alternative to overriding is to make email.message.Message generic. Which would cause close to 100 changes in stdlib. Unless we defer until PEP 696 support.

This comment has been minimized.

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the PR looks good overall. Limiting HTTPMessage to just str seems ok, especially given the primer output.

stdlib/http/client.pyi Outdated Show resolved Hide resolved
@@ -99,6 +99,23 @@ responses: dict[int, str]

class HTTPMessage(email.message.Message):
def getallmatchingheaders(self, name: str) -> list[str]: ... # undocumented
# override below all of Message's methods that use `_HeaderType` / `_HeaderTypeParam` with `str`
# avoiding having to make Message generic
def __getitem__(self, name: str) -> str: ... # AnyOf[str, None]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

str | None should be correct here. A header is not guaranteed to exist, so not being prepared for a None value is a strong indication of a bug.

Copy link
Collaborator Author

@Avasam Avasam Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's a copy, the reasoning is the same as in https://github.com/python/typeshed/pull/11095/files#diff-ced02b01b42b244e9b7bc2e12ef7a215bde89a29072c99ade903f58fb83a3bc7R66-R67

Same as get with failobj=None, but with the expectation that it won't return None in most scenarios
This is important for protocols using __getitem__, like SupportsKeysAndGetItem

And #11095 (comment)

[...] I don't think the false-positives a None union here would introduce is something we want. Especially when .get exists for that purpose.

Copy link
Collaborator Author

@Avasam Avasam Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That being said, the Any trick could be usable here (and in the referenced PR). I should try a primer run with it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I agree with that reasoning. While get() exists, __getattr__ returns it as well. This is arguably a misfeature in the implementation (as it should just raise), but still that's something that a user should check against, because it absolutely can return None, even in unexpected scenarios.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#11095 (comment) At the very least using the Any trick should avoid the bad false-positive with __getitem__ based protocols like SupportsKeysAndGetItem.
Ref #11094

Suggested change
def __getitem__(self, name: str) -> str: ... # AnyOf[str, None]
def __getitem__(self, name: str) -> str | Any: ... # See comment on `Message.__getitem__`

Copy link
Collaborator

@srittau srittau Dec 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not happy about this. This should be an error in my opinion:

def parse_headers(headers: SupportsGetItem[str]):
    header_value = headers["Content-Type"]
    full_header = f"Content-Type: {header_value}"

parse_headers(HTTPMessage(...))

Users of HTTPMessage (and email.message.Message) must be equipped to get None values from __getitem__.

Comment on lines +114 to +115
@overload
def get_all(self, name: str, failobj: _T) -> list[str] | _T: ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we'd be able to have an overload like this, but I don't think it's possible:

@overload
def get_all(self, name: str, failobj: list[<empty>]) -> list[str]: ...

Copy link
Collaborator Author

@Avasam Avasam Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what such an overload would be trying to do. Consumer code is the one passing the failobj and immediately receiving the return type. get_all does nothing with failobj.
If it only supports a list[_HeaderType] it would be its responsibility to pass a value of the appropriate type to failobj, which a typechecker checks for.

def foo(message: HttpMessage) -> list[str] | SentinelType:
	return message.get_all("foo", sentinel_value)  # Ok
	
def foo(message: HttpMessage) -> list[str]:
	return message.get_all("foo", [])  # Fails pyright `reportUnknownArgumentType` but by no fault of the definition
	
def foo(message: HttpMessage) -> list[str]:
	return message.get_all("foo", list[str]())  # ok
	
def foo(message: HttpMessage) -> list[str]:
	return message.get_all("foo", sentinel_value)  # Fails type-checking

If we still really wanna restrict it, we can bind _T to list[_HeaderType]

This comment has been minimized.

This comment has been minimized.

@Avasam Avasam requested a review from srittau December 14, 2023 22:54
stdlib/http/client.pyi Outdated Show resolved Hide resolved
Copy link
Contributor

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

spack (https://github.com/spack/spack)
+ lib/spack/spack/oci/oci.py:142: error: Argument 1 to "endpoint" of "ImageReference" has incompatible type "Optional[str]"; expected "str"  [arg-type]

@srittau srittau merged commit cf6bafb into python:main Dec 19, 2023
60 checks passed
@Avasam Avasam deleted the HTTPMessage-str-override branch December 19, 2023 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should HTTPMessage return Optional[str] for __getitem__?
2 participants