Skip to content

Improve type signature for _RuleList #3455

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

Merged
merged 1 commit into from
Apr 29, 2025
Merged

Conversation

mhils
Copy link
Contributor

@mhils mhils commented Jan 17, 2025

This PR improves the type signature for _RuleList to be more forgiving. Minimal repro:

from tornado.web import Application, RequestHandler

class SampleHandler(RequestHandler):
    def get(self):
        self.write("Hello world")

handlers = [
    (r"/", SampleHandler),
]

application = Application(handlers)

Before Patch:

$ mypy repro.py
repro.py:11: error: Argument 1 to "Application" has incompatible type "list[tuple[str, type[SampleHandler]]]"; expected "list[Rule | list[Any] | tuple[str | Matcher, Any] | tuple[str | Matcher, Any, dict[str, Any]] | tuple[str | Matcher, Any, dict[str, Any], str]] | None"  [arg-type]
repro.py:11: note: "List" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
repro.py:11: note: Consider using "Sequence" instead, which is covariant
Found 1 error in 1 file (checked 1 source file)

After Patch:

$ mypy repro.py
Success: no issues found in 1 source file

Thank you for maintaining tornado, we (@mitmproxy) are super happy users for over a decade! 😃✨🍰

`List` is invariant, `Sequence` is covariant
@bdarnell
Copy link
Member

Thanks, but according to #3119 (comment) changing this List to Sequence is incorrect. I'm not sure if that's true, though - this change appears to pass our mypy tests. (the PR was submitted at a time that our CI wasn't working so we don't can't see the test results on the web but I just checked out the branch and verified it locally). What do you think about that?

@mhils
Copy link
Contributor Author

mhils commented Apr 28, 2025

Thanks, but according to #3119 (comment) changing this List to Sequence is incorrect.

I think that comment is a bit misguided. _RuleList is only used as an annotation when accepting arguments and it's not publicly exposed. _RuleList is never used to annotate attributes or return types. It's typically good to accept wide abstract types and return concrete ones.

It would be different if self.routes would be annotated with _RuleList, but that's already (correctly) annotated as List[Rule]. If anything, we could consider widening from Sequence to Iterable.

TL;DR: I think this is fine as-is, if anything we can make it an Iterable. :)

@bdarnell
Copy link
Member

That makes sense to me. Thanks!

@bdarnell bdarnell merged commit 1566286 into tornadoweb:master Apr 29, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants