Handle class decorators during typing checks. #3890
Conversation
There was a problem hiding this comment.
Confirmed that this resolves the bug on my projects under Python 3.9.0, and the code looks good overall, though I'm not a maintainer, so others might have more suggestions. This does need a changelog entry though describing the fixes, and you should add yourself to CONTRIBUTORS.txt if you'd like the recognition.
Take a look at the contributors' guide: http://pylint.pycqa.org/en/latest/development_guide/contribute.html
|
@jreese Thanks for the feedback! |
There was a problem hiding this comment.
@JulienPalard thanks a lot for this large step toward the support of python3.9! 👍
I have some questions/remarks. Can you also add an entry in doc/whatsnew/2.6.rst please?
pylint/checkers/typecheck.py
Outdated
| if inferred is None or inferred is astroid.Uninferable: | ||
| return | ||
|
|
||
| if inferred.decorators: |
There was a problem hiding this comment.
I think we should enter in these lines only if the protocol is not supported for the current inferred node.
There was a problem hiding this comment.
I initially did that way because a node could support the protocol, but its decorator remove it.
Dumb example:
def deco(f):
def wrapped():
return "Hello"
return wrappedSo I'd prefer to only trust the last decorator of the node than the node itself. But It's my first commit on pylint and first time I use astropy, so if you're sure, I'll gladly do as you feel :)
| if inferred is None or inferred is astroid.Uninferable: | ||
| return | ||
|
|
||
| if inferred.decorators: | ||
| first_decorator = helpers.safe_infer(inferred.decorators.nodes[0]) |
There was a problem hiding this comment.
IMHO we should not restrict to the first decorator but instead emit the message if none of the decorators enable the support of the protocol.
There was a problem hiding this comment.
The type of the object will be of the type of the outer decorator (the "first" one from top to bottom), as the result of other decorators being just passed as an argument to the last, so I though it was enough.
We have nothing prooving a decorator will return the object that it receive (most will not, most will just return a new callable).
So I think we should really just look at the first one (hope [0] is the top one, the outer one, I don't know how to call it) hope I did not get it wrong?
(When I say "outer" I think of decorators as applied manually:
@a
@b
@c
def foo(): ...
being foo = a(b(c(foo))), so when I say "outer decorator" I mean a in this case, I don't know if there's a better wording?)
There was a problem hiding this comment.
@JulienPalard i think you are right. Let's do as you think.
| return # It would be better to handle function | ||
| # decorators, but let's start slow. |
There was a problem hiding this comment.
No need to handle this for now. We are not sure it will be useful one day and if it become the case it will be easy to add lines here.
There was a problem hiding this comment.
That's why I added a comment, to guide future implementers it's here that it should be implemented. But I can reword this more lightly like "If we need to handle function decorators, do it here"? Or I can drop the comment if you prefer.
There was a problem hiding this comment.
Yes it could be reworded. Maybe something along the lines: If necessary handling function decorators should be here
| else: | ||
| return # It would be better to handle function | ||
| # decorators, but let's start slow. | ||
|
|
There was a problem hiding this comment.
The following lines take into account my remarks. Fill free to use it as it is or change everything. It is just a suggestion.
| if supported_protocol(inferred): | |
| return | |
| if inferred.decorators: | |
| # It there are decorators, maybe one of them enable the support of the protocol | |
| for deco_node in inferred.decorators.nodes: | |
| decorator = helpers.safe_infer(deco_node) | |
| if (isinstance(decorator, astroid.ClassDef) and | |
| supported_protocol(decorator.instantiate_class())): | |
| return | |
| # The protocol is not supported neither by the node nor through the decorators | |
| self.add_message(msg, args=node.value.as_string(), node=node.value) |
tests/checkers/unittest_typecheck.py
Outdated
| ) | ||
| ): | ||
| self.checker.visit_subscript(subscript) | ||
|
|
There was a problem hiding this comment.
Maybe we could add a test with a function decorated by more than one decorator and with one of them that enable the protocol?
What do you think about it?
There was a problem hiding this comment.
I think we should also test the case where a subscriptable class is decorated by an unsubscriptable decorator.
Something along the lines:
def test_subscriptable_decorated_by_an_unsubscriptable_class_issue3882(self):
module = astroid.parse(
"""
class Deco:
def __init__(self, f):
self.f = f
@Deco
class Decorated:
def __getitem__(self, item):
return item
test = Decorated()[None]
"""
)
subscript = module.body[-1].value
with self.assertNoMessages():
self.checker.visit_subscript(subscript)
There was a problem hiding this comment.
Maybe we could add a test with a function decorated by more than one decorator and with one of them that enable the protocol?
What do you think about it?
I'm not 100% sure I understand correctly when you say "one of them that enable the protocol", for me it depends too much on the next decorators that can easily "re-drop" the protocol by simply returning a function.
There was a problem hiding this comment.
I think your example should typically message an error, in this specific case it should warn than Decorated(), as being an instance of Deco is not callable.
If we then add a __call__ to Deco that returns something that does not implement the protocol, the Deco removed the protocol from Decorated, and we should say so (but it's probably a whole other level of complexity).
There was a problem hiding this comment.
Ok, forget the case with multiple decorators.
There was a problem hiding this comment.
Of forget also my suggestion. It was not pertinent.
|
Is there anything I can do to help get this merged? This is a blocker for Python 3.9 for us, so I'd like to do whatever I can to get this merged. |
|
@dalemyers Thanks for hopping in! Any help gladly appreciated as it's my very first contribution to this project, anything can go wrong.
|
|
I added some tests, I now test those cases: @Subscriptable
def ...@Unsubscriptable
def ...@Subscriptable
@Unsubscriptable
def ...@Unsubscriptable
@Subscriptable
def ... |
|
Also checked that the tests don't pass without my patch. Actually some of them pass without my patch, which make sense:
as they would "false positive" (checking the function was unsubscriptable) before and "true positive" (checking the top most decorator is unsubscriptable) now. |
|
For anyone who wants to test this, "pip install git+https://github.com/JulienPalard/pylint@issue3882". My results: this pull request solves the problem for me, looking forward to its inclusion! |
I always forgot this, it's so great!
\o/ Hope it breaks nothing (that would not be in the test suite :p). |
|
I have next trace (I can't show source code, sorry): Replace |
|
Thanks a lot for testing it, and for the stacktrace! Was able to reproduce it easily: import typing
print(typing[True])I'll add a test, and fix it. |
is it telling me I wrote too much tests? :) |
|
Hi folks! When can we expect the next release including this fix? |
|
Thanks and congrats for your first contribution! Hope to see many more! |
The next pylint version will be available as soon as |
Pylint does not yet support 3.9 <pylint-dev/pylint#3890 (comment)>.
Pylint does not yet support 3.9 <pylint-dev/pylint#3890 (comment)>.
Pylint does not yet support 3.9 <pylint-dev/pylint#3890 (comment)>.
Pylint does not yet support 3.9 <pylint-dev/pylint#3890 (comment)>.
Pylint does not yet support 3.9 <pylint-dev/pylint#3890 (comment)>.
Pylint does not yet support 3.9 <pylint-dev/pylint#3890 (comment)>.
Pylint does not yet support 3.9 <pylint-dev/pylint#3890 (comment)>.
* feat: Limit Python version to less than 3.9 Pylint does not yet support 3.9 <pylint-dev/pylint#3890 (comment)>. * feat: Set pyenv Python version to 3.8.6 This is the most recent working version until further notice. This file should ideally be kept in sync with the version in pyproject.toml.
Should only be necessary until pylint releases their next version See pylint-dev/pylint#3882 and pylint-dev/pylint#3890
Should only be necessary until pylint releases their next version See pylint-dev/pylint#3882 and pylint-dev/pylint#3890
The configuration change works around pylint-dev/pylint#3890 which is merged but not released yet.
The configuration change works around pylint-dev/pylint#3890 which is merged but not released yet.
Steps
doc/whatsnew/<current release.rst>.Description
I'm trying myself at fixing #3882, by handling the specific case where a decorator is a class. I bet this could be handled elsewere to be made more generic, but as a first contribution I'm starting slow.
Type of Changes
Related Issue
Closes #3882