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

[WIP] Add range-based scopes for captured items #8828

Closed
wants to merge 3 commits into from

Conversation

auduchinok
Copy link
Member

@auduchinok auduchinok commented Mar 28, 2020

The compiler uses ranges when capturing various info during a file type check. These ranges and/or end positions are then used to filter/lookup the infos at particular locations. An example feature using ranges is code completion where FCS looks for expressions types before a . and filters the previously captured expression types based on the expression end position.

This PR introduces range-based scopes that group items in a range and allow to skip the whole scopes when they don't contain the requested range. The scopes are currently used for captured expression types only and it would be great to extend it to other captured items in future PRs.

The scopes can also be used for working with scoped items in parallel which can be an interesting way for future optimizations.

At the moment of PR creation not many scopes are created, namely there are scopes for top level module member declarations in implementation files. Ideally there should be separate scopes for each type, type member, and top level binding which is probably going to be coarse- and even-enough for most features.

This is a prototype and I expect things to change. I'm opening the PR since I'm looking for feedback and/or suggestions of better names or places for the things. I'm also interested to see if there are any failing tests with the change.

@abelbraaksma
Copy link
Contributor

I'm curious, does this improve the experience for tooling writing, and /or does it speed up the process of finding scoped items of a given type? Is there an impact on memory consumption (less, because it skips parts, more?).

@brettfo brettfo changed the base branch from master to main August 19, 2020 20:03
@cartermp
Copy link
Contributor

cartermp commented Nov 5, 2020

This would be cool!

@abelbraaksma I'll let @auduchinok speak more towards this, but a current problem today when doing operations on sets of symbols is that you may often want to do things like filter based on a range.

For example, in #10295, VS gives us a range representing a slice of the active document that's a little bit larger than what is in view. What I need to do is find all the symbols in a document that fall within that range, filter them down based on some criteria, and ultimately annotate the editor in areas that correspond to those symbols.

The way that happens today is:

  1. Compiler service gets a big array of "captured items" that roughly correspond to every symbol in a document
  2. Tooling asks for all symbols in that document
  3. A routine runs through every captured item and constructs an FSharpSymbol subtype based on the kind of item, and then an FSharpSymbolUse to capture additional information about that symbol (like the range)
  4. That list of all symbols is filtered, mapped, etc. as needed

However, it's inefficient. Often times, you want to filter the list of symbols down by range (like in my example), but every item in the document ends up getting constructed as an FSharpSymbol subtype only to get discarded because I wanted to filter down based on its location.

One solution to this is to have all tooling no longer use the FSharpSymbol abstraction. However, that's a horrible solution because dealing with captured items is complicated too low-level for most editor tooling. This PR instead looks to add range information to these captured items so that we can filter things by range more intelligently at the FCS level and no longer create all of these FSharpSymbol subtypes only to discard them.

@KevinRansom
Copy link
Member

@auduchinok ,

Thank you for this Contribution, I am going to close it due to inactivity, please reactivate when you get the time to develop it further.

Thanks

Kevin

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.

4 participants