Use sequence instead of list

[bartv]

This patch uses immutable sequence to ensure that List[Rules] is a valid type for _RuleList

With the current typing it is not possible to correctly type code that uses List[Rule]. This is caused by the type annotation using List[Union[Rule]] which is not a superclass of List[Rule]. This is solved by using Sequence.

This is also illustrated by:

from typing import List, Sequence, Union, Any

class Rule: pass

RuleList = List[
    Union[
        "Rule",
        List[Any],  # Can't do detailed typechecking of lists.
    ]
]

RuleSequence = Sequence[
    Union[
        "Rule",
        Sequence[Any],  # Can't do detailed typechecking of lists.
    ]
]


my_rules = [Rule(), Rule()]

var1: RuleList = my_rules
var2: RuleSequence = my_rules

When you run mypy on it, it will produce the following output:

❯ mypy demo.py
demo.py:22: error: Incompatible types in assignment (expression has type "List[Rule]", variable has type "List[Union[Rule, List[Any]]]")
demo.py:22: note: "List" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
demo.py:22: note: Consider using "Sequence" instead, which is covariant
Found 1 error in 1 file (checked 1 source file)

[bdarnell]

Thanks. This makes sense, but it looks like it's a little more complicated than that. There's some code further down in this file that need changing but I'm not sure how exactly to fix it.

[bartv]

The problem is that the isinstance in add_rules only checked for tuple and list and so there are possibly other Sequence types that can still get through.

It might even be that only the isinstance with Sequence would be enough. Or a more defensive variant that checks if the rule is not an instance of Rule.

I will leave that up to you, but I am happy to assist in any way!

[bdarnell]

What happens if we only change the outer List to Sequence? I don't think we need covariance on the inner List[Any], and then we wouldn't need to change the (tuple, list) typecheck below. (Logically, this inner sequence is only supposed to be 2-, 3-, or 4-tuples. For historical reasons we also allow lists of the same lengths, but I don't see any reason to extend this to arbitrary sequences).

[ivorpeles]

Changing the List type to a Sequence here seems incorrect, it implies the _RuleList is a subtype of the abstract base class Sequence and therefore immutable. The sample code @bartv provided doesn't raise any mypy errors, but if you added the line var2.append(Rule()) mypy would complain that var2 doesn't have an append method. We know it does because it's obviously a list but the type assertion var2: RuleSequence directly misleads mypy.

I think the real issue here is a minor confusion about typing. Users do something like this:

ROUTES = [Rule()]

app = Application(handlers=ROUTES)

and mypy complains that ROUTES is a list of Rules. The solution is to annotate ROUTES with _RuleList, i.e:

ROUTES: _RuleList = [Rule()]

app = Application(handlers=ROUTES)

This makes sense because semantically, ROUTES is a _RuleList. It may only contain Rules in a given implementation but the type annotation is essentially an assertion that, if we added some code like ROUTES.append(("/some/route", SomeHandler)), ROUTES would still have the intended type.

I think a better solution would be to make _RuleList public so users can annotate their code properly. @bdarnell I'd be happy to take a crack at this.

An additional note: the reason Sequence[T] is covariant in T is because Sequence instances are immutable. The suggestion emitted by mypy isn't that the type should be changed to sequence, it's that an immutable container type like Sequence should be used to facilitate covariance. I don't think we want this since Application.__init__ already contains code like line 2081 of web.py:

                handlers.insert(0, (pattern, static_handler_class, static_handler_args))