Skip to content

SSR internal refactorings #5197

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

Merged
merged 4 commits into from
Jul 4, 2020
Merged

Conversation

davidlattimore
Copy link
Contributor

  • Extract error code out to a separate module
  • Improve error reporting when a test fails
  • Refactor matching code
  • Update tests so that all paths in search patterns can be resolved

@matklad
Copy link
Member

matklad commented Jul 3, 2020

bors d+

@bors
Copy link
Contributor

bors bot commented Jul 3, 2020

✌️ davidlattimore can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@matklad
Copy link
Member

matklad commented Jul 3, 2020

FYI, SSR just saved me a bunch of time (well, if I exlclude gif recording and twitter bragging :) )

https://twitter.com/rust_analyzer/status/1279045923373559813

@matklad
Copy link
Member

matklad commented Jul 3, 2020

btw, I've fond SSR to take non-trivial amount of time for rust-analyzer repository. i think we can make it faster by using the following tricks (see search.rs):

  • use approximate text-matching before checking syntactic/semantic match. One day, we'll build a trigram index, and text searches would be 🔥-fast
  • allow the user to specify the scope somehow. Today, I needed to ssr in a single file, for example.

This is to make reusing it outside of parsing easier in a subsequent
change.
Mutable state is now stored in the enum Phase.
MatchState, since it now has no mutable state is renamed Matcher.
MatchInputs is merged into Matcher
@davidlattimore
Copy link
Contributor Author

In terms of speed, I’ve been working on matching paths by resolving all paths in the pattern. Then when a path is encountered in the code we'd resolve that path and check if it resolved to the same thing as the corresponding path in the pattern. This means that we need a scope in which to resolve paths in the pattern. Initially I was thinking the “root”, but since there can be multiple roots, I eventually concluded that it might make sense to instead use the scope that the user was in when they invoked SSR. i.e. pass through a FilePosition. This means that the pattern should be able to refer to private paths in the current scope, or even local variables in the current scope.

Once we’ve resolved each path in the pattern, I was thinking I’d try to leverage the existing code in Definition::find_usages to find references. We can then restrict the search to just those nodes that correspond to those references.
This might look something like the following:

  • Suppose we’ve got two paths in our search pattern, foo and Some. foo is private to the current module and Some is public.
  • Start with the path that has the narrowest search scope, in this case foo and find references to it.
  • If foo is at depth 2 in the search pattern, then find parent().parent() of each reference to foo.
  • We could optionally, especially if we found lots of references to foo, look for references to Some within the intersection of foos search scope and Somes search scope, then intersect the resulting node sets.
  • Finally, attempt matching each of the nodes in the remaining node set.

This should I think make things pretty fast and would reuse the find_usages code without writing some very similar logic. It'd also mean that any future improvements to find_usages would be obtained by SSR for free. Does this approach sound reasonable?

One thing I've been thinking about is how to handle situations like use a::b::Foo as Bar. If our pattern then contains Foo, can we find code that references the alias Bar. I think using find_usages might help here provided we're not too aggressive with intersecting search scopes. It looks like currently find_usages on Foo will find the use-statement, however find-usages on the Bar in the use-statement doesn't find usages of Bar. But that's probably fixable. Effectively we'd need to follow chains of use-statements until we get to actual references.

I think we probably in addition to leveraging paths, we need some way to be able to restrict the scope of the search, even when it only contains public paths (or contains no paths). I guess common scopes might be the current file, the current crate or everything. Not sure of the best way. We could have separate "SSR in current file", "SSR in crate" options. That doesn't feel like it scales very well. Another option would be to allow some marker in the rule. e.g. ${:in(module)} or ${:in(crate)}. This format would be effectively an unnamed placeholder that wouldn't match any nodes, but could place constraints on where the match was permitted. The in constraint would be evaluated relative to the location where SSR was invoked from.

@davidlattimore
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 4, 2020

@bors bors bot merged commit 212fa29 into rust-lang:master Jul 4, 2020
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.

2 participants