Fix decorators on class __init__ methods #2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
October 17, 2018 19:15
commit 95cafcd Author: root <[email protected]> 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 <[email protected]> Date: Mon Oct 15 16:07:46 2018 -0700 Fix some test failures commit 32f05cb Author: Joel Croteau <[email protected]> Date: Mon Oct 15 11:52:52 2018 -0700 Fix false positive and add some type annotations commit cbaab5b Author: root <[email protected]> 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.
Softagram Impact Report for pull/2 (head commit: 6f9a449)⭐ Visual OverviewChanged elements and changed dependencies. ⭐ Change ImpactHow the changed files are used by the rest of the project 📄 Full report
Give feedback of this report to [email protected] |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a few issues around decorators, specifically resolves python#5398 and python#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.