typeshed
typeshed copied to clipboard
python
Markdown: add fake base class to Pattern to fix overrides
Currently InlineProcessor and every subclass of it violates the override rule as per Liskov Substitution Principle.
Signature of "handleMatch" incompatible with supertype "Pattern" [override]
Superclass:
def handleMatch(self, m: Match[str]) -> Element | str
Subclass:
def handleMatch(self, m: Match[str], data: str) -> tuple[Element | str | None, int | None, int | None]
For all usage purposes these aren't actual subclasses to one another, they're handled totally separately inside __applyPattern.
However yes, this change totally disagrees with actual isinstance checks at run time (though even that is still for the best).
I further describe this here: https://github.com/Python-Markdown/markdown/pull/1402#discussion_r1381957216
There are plenty of cases of existing ignores due to this already https://github.com/search?q=%2FInlineProcessor%5C%29%2F+%2FhandleMatch.%2Bignore%2F+path%3A%2Fpy%24%2F+NOT+path%3Azerver&type=code
- Related issue: https://github.com/python/mypy/issues/10197
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
I'm not sure we need the fake class. From what I understand, the inheritance hierarchy at runtime is Pattern → InlineProcessor →SomeDerivedProcessor, where InlineProcessor has an incompatible handleMatch signature, compared to Pattern, while the classes derived from InlineProcessor have the same signature as InlineProcessor.
That mypy has a bug/misfeature, where the classes derived from InlineProcessor need a type ignore as well, is unfortunate, but I wouldn't want to regress our stubs for the benefit of a single type checker, even one as important as mypy. Especially not in a case like this, where a workaround (in the form of # type: ignore) is readily available.
I agree with @srittau here. Having to type: ignore[override] an incompatible override is pretty common in typeshed for one-off classes that don't respect polymorphism from the base class.
I wouldn't do such a change just to remove a type: ignore.
There is however, still an underlying issue that affects downstream users and is worth looking at:
If it's common enough, then the base class should account for all of its expected subtypes' signature to get "correct" polymorphism. But that will lead you to the Union return issue (https://github.com/python/typing/issues/566) But you already have a complex Union with incompatible types. So the user likely already has to typeguard the usage.
def returns_processor() -> InlineProcessor: ...
def accepts_pattern(pattern: Pattern):
foo = pattern.handleMatch(...)
is_instance(foo, tuple) # Always false, currently type-checkers don't see that foo can be a tuple
accepts_pattern(return_processor())
A potential solution (not saying it's the right one, just throwing ideas) could be to expand the Union of handleMatch (or use Any in wait of an AnyOf-like solution). And override handleMatch in all direct subclasses of Pattern to narrow down the return type.
@Avasam I can be fine with that position, but I think your comment does not really match real situations. All the problems really are just with writing subclasses (which is very common).
I'm sorry I just noticed the subclass' method has an extra parameter, it's not just about return types. Yeah that's gonna be more problematic :/
Oh and by the way, it's not just an extra parameter, these two work completely differently to the point of being unrelated. Other than isinstance checks, the actual project would also definitely be better off with them being unrelated. The "base" class just was the old idea of how the interface should work and the author thought it would be good to make a new idea of the interface be a subclass of the old one (which should never be used anymore)