-
Notifications
You must be signed in to change notification settings - Fork 35
Include decorators in Class
and FunctionDef
range
#49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Include decorators in Class
and FunctionDef
range
#49
Conversation
Oh.. this is a Python spec omg. Python 3.11.2 (main, Feb 16 2023, 02:55:59) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import ast
>>> ast.parse("""
... @decorator
... def x(): pass
... """)
<ast.Module object at 0x100738be0>
>>> a = _
>>> f = a.body[0]
>>> f.lineno
3
>>> f.decorator_list[0].lineno
2 We will really need a feature flag for ruff. |
I'd better to understand it better if this is also related to symbol table or SyntaxError. |
@MichaReiser Do you want to make it has different value only for |
In my view, this stretches feature flags to the limit, or even over the limit. We would need to rename it at least. I also plan to make other range changes: def a():
pass
# comment The range of the function should include the I think maintaining this change in a fork is straightforward enough and probably easier than dealing with even more conditional code that has very limited test coverage. |
it seems going really different.
Yes, that sounds like truth. |
Reusing ast crate but making a fork of parser crate makes sense to me. |
We can make a new crate like |
I don't think that this is necessary. I still plan to pull in upstream changes. I don't expect to many conflicts since the grammar shouldn't change frequently |
This PR changes the
ClassDef
andFunctionDef
definitions to include the range of their decorators.Example
@test
decorator)Motivation
This change is because these are the only nodes for where the property
parent.range.includes(child.range)
does not hold. The fact that this property is not respected means it is e.g. impossible to use a binary search over the AST to find a node by its range.Downsides
The main downside that I'm aware of is that diagnostics using
range.start
to compute the line index for functions and classes may now incorrectly point to the line of the function's decorator instead of the function definition. I don't know if this is a problem for RustPython. Ruff already uses custom infrastructure to extract the range of the function name only to limit the diagnostic range (and e.g. avoid highlighting the whole function or class including the body)Ruff
astral-sh/ruff#4467