PEP 695 type parameter syntax for Python 3.12+#237
PEP 695 type parameter syntax for Python 3.12+#237davidhalter merged 1 commit intodavidhalter:masterfrom
Conversation
darki73
force-pushed
the
feat/pep695-type-params
branch
from
c202a83 to
463a86f
Compare
|
Did you run the tests in Jedi with this changed parso version? And did they all pass? Thanks a lot for trying to work on this! |
|
I am using jedi 0.19.2 as a direct dependency in my project, before submitting PR for parso, I "patched" the loading mechanism of parso in my projects code because it basically allowed me to submit the version I wanted to release without the need to wait for the PR to be merged. So far, on 62 cases without the patch, this patch provides 62/62 mappings (i am, like parso, also run my app on itself for checks) - https://github.com/darki73/sylvan/blob/899de480992eb79f1ba8f3c71ac8552b68e647fc/src/sylvan/analysis/structure/jedi_setup.py This is pretty much the same "fix", just monkey patched. I have not though ran any tests in jedi, i can try to do it later today / tomorrow when I have time to do so. Thank you for taking time to review the PR! |
darki73
force-pushed
the
feat/pep695-type-params
branch
from
463a86f to
fde9909
Compare
|
Ran the tests against the main of jedi, found some failing tests that required more changes on the parso side (which are already added to this PR as of now. As of now, 3743 tests in jedi have passed (had some issues with windows, but turns out just flaky tests on this specific platform). |
| else: | ||
| return self.children[3] | ||
| for i, child in enumerate(self.children): | ||
| if hasattr(child, 'value') and child.value == '(': |
There was a problem hiding this comment.
Can you use
try:
value = child.value
except AttributeError:
continue
if value == "(":
...
here and in the places below? hasattr should not be used like that because it uses multiple lookups and is harder to refactor.
There was a problem hiding this comment.
Actually, looked at the existing patterns in this file, we might even go with the direct string comparison.
(had to edit the next line, was confusing as hell to "unprepared" person)
Basically, the original code used the != operator to exit early, and since we are in the loop now, we are checking with == for a ( instead of its absence.
So just like before, instead of self.children[2] != '(' we can just do child == '('.
I will push the updated version for both places, but ultimately, it is for you to decide as you are the author and if you prefer the try/except - just let me know and i will update it.
With this, 1984 parso and 3743 jedi tests pass no issue.
darki73
force-pushed
the
feat/pep695-type-params
branch
from
fde9909 to
ea2a2b9
Compare
There was a problem hiding this comment.
Sorry for all the comments, I like the direction and probably prefer that to the AttributeError stuff, I just think that while it's hard to understand the code, it gets much easier if you know/look at the grammar. Does that help with understanding it for you? I don't think it's my goal that everybody understands it without looking at the grammar files.
| return None | ||
| for i, child in enumerate(self.children): | ||
| if child == '->': | ||
| if i + 1 < len(self.children): |
There was a problem hiding this comment.
I think this should always be true, since the grammar file specifies this. I think this line can just be removed.
| if not any(isinstance(child, Param) for child in parameters_children): | ||
| parameters.children[1:-1] = _create_params(parameters, parameters_children) | ||
| parameters = self._find_parameters() | ||
| if parameters is not None: |
There was a problem hiding this comment.
This should also never be reachable and can be removed. A function always has parameters, see grammar file
| for child in self.children: | ||
| if child.type == 'parameters': | ||
| return child | ||
| return None |
There was a problem hiding this comment.
This should just raise Exception("A function should always has parameters").
| return self.children[3] | ||
| for i, child in enumerate(self.children): | ||
| if child == '(': | ||
| if i + 1 >= len(self.children): |
There was a problem hiding this comment.
Same here, I guess this should always be true.
darki73
force-pushed
the
feat/pep695-type-params
branch
from
ea2a2b9 to
5b63e63
Compare
|
No problem! |
|
Thanks a lot again for working on this! I think it's especially meaningful, because it fixes things without causing too much work in other places. And feel free to checkout Zuban, I'm currently in the process of bringing some Jedi goodness to it and there should probably not be a lot of reasons anymore in a few months to use Jedi instead of Zuban. |
|
@darki73 @davidhalter excuse me for the noise, but thank you so much for fixing this!!!!!! |
3 participants
Closes #221
I ran into this while building a code intelligence tool that uses jedi for cross-module resolution. My ORM uses
class QueryBuilder[T: "Model"](Mixin1, Mixin2)and jedi couldn't resolve any of the inherited methods. Traced it back to parso parsing the class as an error node.Two things were broken:
The 3.12/3.13/3.14 grammars don't have
type_paramsrules, soclass Foo[T]anddef bar[T]()parse as error nodes.Class.get_super_arglist()hardcodeschildren[2]andchildren[3]to find the arglist. With type params present,(shifts to a later position and the method returns None. This means jedi can't find base classes at all, so the entire MRO is invisible.Fix: added
type_params,type_param,type_param_boundrules to the grammars. Addedtype_param_defaultfor 3.13+ (PEP 696). Replaced the hardcoded index inget_super_arglistwith a search for(.Worth noting - in the jedi issue (davidhalter/jedi#2025) there was concern that jedi would need major changes to support this. Turns out it doesn't. Once parso parses the class correctly and
get_super_arglistreturns the right node, jedi's existing MRO resolution handles everything. Completions, goto, inference on inherited methods - all work without touching jedi at all. Theget_super_arglistfix was the missing piece.The
typesoft keyword statement is not included here since that needs tokenizer changes.