StructuralSearchReplace: next steps

[matklad]

#3099 implemented initial version of SSR.

This issue collects things we need to do to make this really production-read

• add docs to https://github.com/rust-analyzer/rust-analyzer/blob/master/docs/user/features.md
• improve matching code to handle trivia
• improve matching code to handle semantically equivalent transformations (for example, the order of the fields)
• add support for matchers other than $expr

@mikhail-m1 any other things I've missed?

[matklad]

The most important feature here is documentation!

[adamrk]

What kinds of things are you thinking about for the second item?

[matklad]

White space differences should not matter for natching

[adamrk]

Made an initial change for the whitespace, but there's two points I'm not certain about:

1. The Rust reference says whitespace is defined by the Unicode property Pattern_White_Space, but the char method is_whitespace uses the Unicode property White_Space instead and they're not exactly the same. I didn't notice any code in rust-analyzer for checking Pattern_White_Space so I assume we don't really care about the difference and it's fine to use is_whitespace?
2. It seems like we might want to completely ignore whitespace in certain situations (e.g. match f(x) with f( x ), but I don't think we can do that in general. The current change only matches various forms of whitespace with each other, but doesn't ignore any whitespace. Any opinions on that?

[adamrk]

Actually I think for my second question it's fine to throw out all whitespace tokens. I can't actually think of any situations where it would be a problem.

[flodiebold]

Note that there's an is_trivia method on SyntaxKind, you don't need to implement this yourself. E.g. some_token.kind().is_trivia().

[matklad]

Yeah, the line that checks for Pattern_White_Space is in the rustc_lexer crate, which we share with the compiler

…

On Sat, 22 Feb 2020 at 23:18, Florian Diebold ***@***.***> wrote: Note that there's an is_trivia method on SyntaxKind, you don't need to implement this yourself. E.g. some_token.kind().is_trivia(). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3186>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M2QY4CRZWSFQLYQ6D3REGQDNANCNFSM4KWONEZA> .

[adamrk]

Cool, thanks.

[mikhail-m1]

@adamrk sorry, I missed that you started to work on it and created a PR for the second step.

[adamrk]

my mistake @mikhail-m1 - didn't realize you were working on it either.

[matklad]

Example of semanticly equivalent transformations -- trailing comma:

            // replace_children(self, $what:expr, $where:expr) ==>> self.replace_children($what, $where)
            return replace_children(
                self,
                single_node(old.syntax().clone()),
                iter::once(segment.syntax().clone().into()),
            );

[matklad]

I've actually been using ssr and it works surprisingly well.

One thing I've found is that typing ssr in comment and then copy-pasting it is the most productive workflow. I think we should probably make this the workflow.

Specifcally, I imaging something like this:

• If the cursor is on a comment with // ssr: tag, it initiates ssr session.
• The contents of the comment is interpreted as ssr query (we perhaps can replace ==>> with just splitting by lines?)
• The ssr can be applied via code action
• (stretch goal) the contents of ssr comment is syntax highlighted
• (stretch goal) the matching elements in the current file are highlighted

[mikhail-m1]

next steps:

• comment support: StructuralSearchReplace: next steps #3186 (comment). Should it be an assist?
• add support for matchers other than $expr, and I think we need to replace hand written naive parser.
• add SSR to CLI

[matklad]

@RReverser no, we currently run on pre-expansion code. Although we should probably run post-expansion eventually. A minimal test-case would be useful!

[RReverser]

no, we currently run on pre-expansion code. Although we should probably run post-expansion eventually

Hmm, my problem is exactly that it doesn't seem to match pre-expansion code.

I've tried something as simple as:

eprintln!($a:expr, $b:expr) ==>> $a + $b

and it didn't match anything.

[matklad]

Thanks for pointing out the possible slowness with using semantic info.

See also this nice blog post about how semantic code searches are handled in IntelliJ: https://habr.com/en/company/JetBrains/blog/451754/

I think for rust-analyzer we might be able to come up with something even more efficient (salsa allows us to lift the "single file index" constraint), but that is very much unexplored territory.

[flodiebold]

BTW, it might be nice to be able to apply SSR by running rust-analyzer on the command line as well 🤔

[davidlattimore]

Forgot to reference this issue, but #5120 added support for SSR from the rust-analyzer command line

[matklad]

I figred that it migbe be useful to support statements in SSR:

let $var = foo();
$var.bar();
==>>
foo().bar();

[davidlattimore]

I've added path resolution to SSR rules, the main user-visible changes are:

• SSR now matches paths based on whether they resolve to the same thing instead of whether they're written the same.
  • So foo() won't match foo() if it's a different function foo(), but will match bar::foo() if it's the same foo.
• Paths in the replacement will now be rendered with appropriate qualification for their context.
  • For example foo::Bar will render as just Bar inside the module foo, but might render as baz::foo::Bar from elsewhere.
• This means that all paths in the search pattern and replacement template must be able to be resolved.
• It now also matters where you invoke SSR from, since paths are resolved relative to wherever that is.
• Search now uses find-uses on paths to locate places to try matching. This means that when a path is present in the pattern, search will generally be pretty fast.
• Function calls can now match method calls again, but this time only if they resolve to the same function.

Regarding supporting statements, I agree, that would be good to support. It'll probably require some refactoring since currently we only match whole AST nodes, not sequences of nodes. Also, the example you gave would also require allowing a placeholder to be repeated, then checking the equivalence of each occurrence.

I just started looking at making an SSR assist, but I'm slightly unsure of how it should work if there's an SSR rule, but it fails validation. How should the error be presented? It looks like the assist API just does edits, but doesn't offer any scope for interaction or error reporting. Is that correct?

[bjorn3]

I just started looking at making an SSR assist, but I'm slightly unsure of how it should work if there's an SSR rule, but it fails validation. How should the error be presented? It looks like the assist API just does edits, but doesn't offer any scope for interaction or error reporting. Is that correct?

You could report an error for all invalid ssr comments instead of waiting for the assist to be invoked.

[davidlattimore]

That sounds like it would work.

Another thing that I've been thinking it would be good to support is a way to scope the rule. Sometimes you just want to automate a repetitive edit to the current file, or even within the current function. One way I was considering was by extending the placeholder constraint syntax. So ${:in(fn)} foo ==>> bar would replace foo with bar in the current function. Similarly in(file) for the current file. This has the advantage of not requiring any new syntax. However I'm open to suggestions if people have any ideas for a dedicated syntax for constraints that apply to the whole pattern.

A completely different option would be to just have different commands - e.g. "Structural Search Replace - current file". That's perhaps more discoverable, but less scalable (adding new scopes would be a pain).

A third option would be building a more sophisticated UI for doing SSR - one that could have drop downs, checkboxes etc. I know comparatively little about extending vscode, so I'm not sure how plausible that would be.

[RReverser]

Another thing that I've been thinking it would be good to support is a way to scope the rule. Sometimes you just want to automate a repetitive edit to the current file, or even within the current function. One way I was considering was by extending the placeholder constraint syntax.

I think a bit more natural option could be "search within selection" which is how regular Find & Replace already limits itself in VSCode. Then user can easily select a single function, or couple of them, or whole module, etc.

[crepererum]

Tried SSR today. One limitation seems to be the power of the matcher, e.g. foo(vec![$a]) ==>> foo(vec![Some($a)]) does not work.

[IndianBoy42]

Another thing that would make SSR a lot more approachable would be incremental search and replace, ie

• highlighting what is matched by a certain structural search
• previewing the changes that would be made if the replacement was run
• seeing the above two points update live while typing (without actually modifying the buffer)
  • the hard part here is probably making SSR run fast enough to keep up with typing (although debouncing is necessary anyways)

Also syntax highlighting in the SSR query+replacement would be nice

Some of this is more about the ide plugin side, but the server needs to have the capabilities. Is everything necessary for this available from the server already?