Fix decorators on class __init__ methods #5798
Conversation
commit 95cafcd Author: root <jcroteau@liveramp.com> Date: Wed Oct 17 19:03:27 2018 -0700 Fix broken tests, add new tests, and remove things added for debugging commit c845501 Author: Joel Croteau <jcroteau@gmail.com> Date: Mon Oct 15 16:07:46 2018 -0700 Fix some test failures commit 32f05cb Author: Joel Croteau <jcroteau@gmail.com> Date: Mon Oct 15 11:52:52 2018 -0700 Fix false positive and add some type annotations commit cbaab5b Author: root <jcroteau@liveramp.com> Date: Thu Oct 11 17:31:33 2018 -0700 Here, have some code
@ilevkivskyi, I see your point about how overriding `__instancecheck__` would create problems with debugging and `mypyc`, so I have implemented an alternate solution. I have created a `get_callable` method to check whether a node is callable and, in the case of a decorator, return an instance of a separate `CallableDecorator` class that inherits from `FuncItem`. I had originally thought to just have an `is_callable` method that would test whether a `Node` was either an instance of `FuncBase` or a callable `Decorator`, but this created problems with type resolution. This solution is not exactly ideal either, but I think will create fewer problems. I agree that the best solution would be to remove the `Decorator` class altogether, but this would be a fairly major refactor. The problem is that we do need a `Decorator` class in early passes before we have type information, and we use the `Node.accept` method to add type information to an instance, we don't have a simple way to replace a node with another node of a different type. Let me know what you think of this approach. One other stray observation: * This fix requires that the decorator is explicitly annotated to return a `Callable`. It seems like this should be something we could determine through type inference, but I'm not sure how easy that would be to do.
There was a problem hiding this comment.
This looks better. I have a suggestion about possible strategy in a comment below. Here are two additional "large scale" suggestions/comments:
- Please update the PR description.
- It looks like you make many changes that are not necessary for the actual fix. If you do so, please add a test case for every change (a test that currently fails, but passes with your change).
| """A wrapper around a Decorator that allows it to be treated as a callable function""" | ||
| def __init__(self, decorator: Decorator) -> None: | ||
| super().__init__(decorator.func.arguments, decorator.func.body, | ||
| cast('mypy.types.CallableType', decorator.type)) |
There was a problem hiding this comment.
This is better but I think this can be made even simpler. I don't see why this class is necessary, moreover instead of one class that I don't like (Decorator), we will have two. Instead of trying to solve everything, why not start from a simple approach that fixes the original issues, and then generalize it to a reasonable extent?
There was a problem hiding this comment.
The main reason I do it this way is that FuncBase and FuncItem are both abstract, so I can't instantiate them directly, and I don't know if it makes sense to return a FuncDef, since that's not really what this is. I think it makes sense to have a callable decorator be represented as a FuncBase, so that code that is looking for something callable can treat it the same as it would any other callable. The alternative is to create a lot of special cases to handle Decorators, which doesn't seem like it serves the long-term interest of eliminating the Decorator class altogether.
| if second_type is None: | ||
| method = get_callable(second.node) | ||
| if method: | ||
| second_type = self.function_type(method) |
There was a problem hiding this comment.
I am not sure why you need this change, but please take into account that there are three places in checker.py that check Liskov: one here (for "horizontal" overriding in multiple inheritance), one for variables, and one for methods etc. (There is some code duplication, I know our code is not perfect.) So please double check if you need to update other two places.
There was a problem hiding this comment.
On closer inspection, I think you are correct that this change is unnecessary, so removing it.
|
@TV4Fun Sorry, but you will need to find another reviewer for this PR. |
|
(Didn't mean to close, hit a wrong button.) |
|
Superseded by #6645 |
This fixes a few issues around decorators, specifically resolves #5398 and #1706. Not sure if this is exactly the way you want to fix this. Probably better would be to get rid of the
Decoratorclass altogether, but this provides a workable fix. My reasoning on it is this:Nodeis callable by checking whether it is an instance ofFuncBase.get_callablemethod, which will return aFuncBaseif it makes sense to construct one for the passed inNode, otherwise returnNone.Nodeis aFuncBaseor a callableDecorator, but could conceivably be expanded to cover other cases with minimal additional work.Decoratorreturn an instance of theCallableDecoratorclass, which inherits fromFuncItemiff the decorated function is still callable.This works, but creates problems in a few places in the codebase that explicitly assume that an instance of
FuncBaseis not decorated. I have fixed the instances of this I could find, and all of the existing tests pass, but we probably want to do more testing on this to be sure.Stray observations:
propertyand similar descriptor-based decorators as special cases and could work with them more directly. Possibly by having their implementations in typeshed beGenerics with appropriate types. This would probably require more robust type annotation than we currently support though.Var.is_staticmethodandVar.is_classmethodtoVar.is_staticandVar.is_classor renameFuncBase.is_staticandFuncBase.is_classtoFuncBase.is_staticmethodandFuncBase.is_classmethod. They duplicate the same functionality, and this would help remove some unneeded type checks in a few places.testInferListInitializedToEmptyInClassBodyAndOverriden, but created problems in other places. This may be worth investigating further.Decorators that is no longer needed, but I am not familiar enough with the codebase to say that for sure.Callable. It seems like this should be something we could determine through type inference, but I'm not sure how easy that would be to do.I look forward to your criticisms.