Skip to content
This repository was archived by the owner on Jul 27, 2023. It is now read-only.

Include decorators in Class and FunctionDef range #2

Merged
merged 2 commits into from
May 22, 2023

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented May 19, 2023

This PR changes the ClassDef and FunctionDef definitions to include the range of their decorators.

Example

@test
def f(): 
	pass
  • Before: The functions' range was from line 2, column 0 to line 3, column 9
  • New: The functions' range spans from line 1, column 0 to line 3, column 9 (includes the @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)

Alternatives

An alternative to this would be to change our AST and introduce a new DecoratedDefinition node that includes the decorator and the Function or Class definition. This is what tree-sitter does.

Related

@MichaReiser MichaReiser requested a review from charliermarsh May 19, 2023 06:19
@MichaReiser
Copy link
Member Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@MichaReiser MichaReiser force-pushed the include-decorators-in-class-and-func-range branch from 89c63a6 to 41a0ef8 Compare May 19, 2023 08:14
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will require changes in Ruff.

@MichaReiser
Copy link
Member Author

This will require changes in Ruff.

Related PR: astral-sh/ruff#4467

@MichaReiser MichaReiser merged commit 335780a into main May 22, 2023
@MichaReiser MichaReiser deleted the include-decorators-in-class-and-func-range branch May 22, 2023 11:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants