Skip to content

Type mismatch in email.charset.Charset.get_body_encoding #10430

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

Closed
cushionbadak opened this issue Jul 10, 2023 · 4 comments · Fixed by #10437
Closed

Type mismatch in email.charset.Charset.get_body_encoding #10430

cushionbadak opened this issue Jul 10, 2023 · 4 comments · Fixed by #10437
Labels
stubs: false negative Type checkers do not report an error, but should

Comments

@cushionbadak
Copy link
Contributor

cushionbadak commented Jul 10, 2023

Summary

Modification Target: Charset.get_body_encoding in stdlib/email/charset.pyi
Current type hint: (self) -> str
Proposed type hint: (self) -> str | Callable[[Message], None]

See the existing implementation here: charset.pyi#L17

Description

The method Charset.get_body_encoding can return a function encode_7or8bit if it reaches final else-branch case.

    def get_body_encoding(self):
        assert self.body_encoding != SHORTEST
        if self.body_encoding == QP:
            return 'quoted-printable'
        elif self.body_encoding == BASE64:
            return 'base64'
        else:
            return encode_7or8bit

Using the type hint for encode_7or8bit, the return type hint for get_body_encoding should be str | Callable[[Message], None.

def encode_7or8bit(msg: Message) -> None: ...

I found this mismatch in the CPython email unit test, TestMiscellaneous.test_get_body_encoding_with_uppercase_charset, found in test/test_email/test_email.py. The minimized example below shows type mismatch at runtime, where the call charset.get_body_encoding() at the last statement returns encode_7or8bit function:

    def test_get_body_encoding_with_uppercase_charset(self):
        eq = self.assertEqual
        # Try another one
        msg = Message()
        msg['Content-Type'] = 'text/plain; charset="US-ASCII"'
        charsets = msg.get_charsets()
        charset = Charset(charsets[0])
        eq(charset.get_body_encoding(), encoders.encode_7or8bit)
        ...

P.S. All CPython links refer to commit hash 0481b8... from the current CPython 3.12 branch.

@AlexWaygood

This comment was marked as outdated.

@AlexWaygood AlexWaygood marked this as a duplicate of #10429 Jul 10, 2023
@AlexWaygood AlexWaygood closed this as not planned Won't fix, can't repro, duplicate, stale Jul 10, 2023
@cushionbadak
Copy link
Contributor Author

@AlexWaygood

Duplicate of #10429

Thanks for your contribution, but I disagree with the classification of these as duplicated issues. While #10429 and #10430 might seem similar in structure, they target different methods and the reasons for the type mismatches are not identical. Therefore, in my view, these should be regarded as distinct issues. much like how issues 8615 and 8616 were jointly addressed in pull request 8617.

If the preference is still to collect similar proposals in one issue, may I suggest moving the issue report from #10430 to #10429?

@AlexWaygood AlexWaygood reopened this Jul 10, 2023
@AlexWaygood
Copy link
Member

AlexWaygood commented Jul 10, 2023

My apologies, that serves me right for trying to triage issues before having my coffee :(

@srittau srittau added the stubs: false negative Type checkers do not report an error, but should label Jul 10, 2023
@srittau
Copy link
Collaborator

srittau commented Jul 10, 2023

Thanks! Normally we try to avoid union return types and prefer to use Any, but in this case I think it's sensible to have one, since a caller needs to check the return type anyway before being able to use it. The current annotation is incomplete in any case. PR welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stubs: false negative Type checkers do not report an error, but should
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants