Skip to content
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

Search all source trees for a given span #14436

Merged
merged 2 commits into from Feb 17, 2022
Merged

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Feb 8, 2022

When we create a synthetic companion object, for exmaple for enums, all the value definitions are put into it and the there are actually multiple trees that contain the position we are itnerested in.

I decided to check which path is the longest, which most likely is the most exact. I am open to all ideas as I am not sure this is the best solution. Actually, it dawned on me in the morning that there is already a logic for choosing the best tree and it already works fine.

Fixes #8738

@tgodzik tgodzik requested a review from smarter February 8, 2022 18:40
@tgodzik tgodzik changed the title Find the longest path when multiple trees contain position Search all source trees for a given span Feb 9, 2022
@tgodzik tgodzik requested a review from prolativ February 9, 2022 09:46
@ckipp01
Copy link
Member

ckipp01 commented Feb 9, 2022

@tgodzik did you mean #8738 or will this also fix #11683

@tgodzik
Copy link
Contributor Author

tgodzik commented Feb 9, 2022

@tgodzik did you mean #8738 or will this also fix #11683

I meant #8738 Thanks for noticing 😅

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

@@ -72,7 +72,7 @@ object NavigateAST {
* end point are the same, so this is useful when trying to reconcile
* nodes with source code.
*/
def pathTo(span: Span, from: Positioned, skipZeroExtent: Boolean = false)(using Context): List[Positioned] = {
Copy link
Member

Choose a reason for hiding this comment

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

The documentation comment above needs to be slightly tweaked: it says "from node from" but I guess now it would be "from any node in from"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. Good catch!

When we create a synthetic companion object, for exmaple for enums, all the value definitions are put into it and the there are actually multiple trees that contain the position we are itnerested in.

Now, NavigateAST.pathTo takes in multiple trees since the method is already able to find the best fit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Various inspections on enum positions Presentation Compiler
4 participants