Init implementation of structural search replace #3099

mikhail-m1 · 2020-02-10T23:20:23Z

next steps:

ignore space and other minor difference
add support to ra_cli
call rust parser to check pattern
documentation

original issue #2267

matklad · 2020-02-11T16:16:51Z

Gave it a shoot, it seems to work perfectly for small cases. Spend at least couple of minutes mindlessly marveling at flipping $a <-> $b :)

However I've found at least one case where it doesn't quite work:

After SSR

Haven't looked into the code closely yet, so can't tell what's the issue

matklad · 2020-02-11T16:17:48Z

That's for this bit of code: https://github.com/rust-analyzer/rust-analyzer/blob/305d9219823f5c0a6ffe526351b225b410702dfd/xtask/src/cmd.rs#L21-L27

matklad · 2020-02-11T16:40:41Z

crates/ra_ide/src/lib.rs

@@ -464,6 +467,30 @@ impl Analysis {
        self.with_db(|db| references::rename(db, position, new_name))
    }

+    pub fn structural_search_replace(
+        &self,
+        template: &str,


Not sure what's the best API around passing the template. I sort of wish that this was more structured, query: SsrQuery, like we do for workspace_symbol, but the query is not really structured yet. Still, I think I'd prefer this API

struct SsrQuery { ... } impl FromStr for SsrQuery { ... } pub fn structural_search_replace(&self, query: SsrQuery) { ... }

I imagine we might want to add quite a few knobs to the SsrQuey in the future, like this:

let query = str.parse::<SsrQuery>() .ignore_macros(true) .strict_whitespace(true) .crates(vec![foo, bar, baz])

Passing in a struct would be more convenient for adding those kinds of tweakables later.

This also nicely separates fast, synchronous and fail able operation of parsing the template from slow, async and infailable op of doing actual search&replace

matklad · 2020-02-11T16:47:49Z

crates/ra_ide/src/lib.rs

+                        });
+                    }
+                }
+            }


Let's move this to the ssr module.

matklad · 2020-02-11T16:51:19Z

crates/ra_ide/src/ssr.rs

+#[derive(Debug, PartialEq)]
+pub enum SsrError {
+    ParseError(String),
+}


I would probably do just struct SsrError(String). In general, I've found that making error opaque struct (as opposed to open enums) usually works better, as you almost never actually want to match on the result anyway.

I agree on that, but only under the condition that we are the only users of that error (i.e. we are not shipping a library for anyone else where we cannot be sure that the users will want to match on particular error types).
As a reference, we move to opaque error in ra_syntax crate right now

matklad · 2020-02-11T16:57:32Z

crates/ra_ide/src/ssr.rs

+        code: &SyntaxElement,
+        placeholders: &[String],
+        m: &mut Match,
+    ) -> bool {


A better signature might be

fn check( pattern: &SyntaxElement, code: &SyntaxElement, placeholders: &[String], ) -> Option<Match> {

that they, there's no implicit invariant of "only use Match if the function returned true"

matklad · 2020-02-11T17:01:45Z

crates/ra_ide/src/ssr.rs

+#[derive(Debug, PartialEq)]
+pub struct SsrPattern {
+    pattern: SyntaxNode,
+    names: Vec<String>,


Not sure if it's worth it, but we might want add some newtype wrappers here

/// represents an `$var` in an SSR query struct Var(String);

matklad · 2020-02-11T17:05:39Z

crates/ra_ide/src/ssr.rs

+
+fn render(binding: &Binding, template: &SsrTemplate) -> String {
+    fn replace(p: &SyntaxNode, binding: &Binding, builder: &mut TextEditBuilder) {
+        if let Some(name) = binding.keys().find(|&n| n.as_str() == p.text()) {


I think it would be clearer and perhaps more efficient to pre-process SsrTemplate like this:

struct SsrTemplates { node: SyntaxNode, placeholders: FxHashMap<SyntaxElement, Var>, }

we can then do roughly the following:

for element in template.node.descendats_with_tokens() { if let Some(var) = template.placeholders.get(element) { builder.replace(element.text_range(), binding[var]) } }

matklad · 2020-02-11T17:11:54Z

editors/code/src/commands/ssr.ts

+        if (!client) return;
+
+        const options: InputBoxOptions = {
+            placeHolder: "foo($a:expr, $b:expr) ==>> foo($a: expr, bar($b:expr))",


Suggested change

placeHolder: "foo($a:expr, $b:expr) ==>> foo($a: expr, bar($b:expr))",

placeHolder: "foo($a:expr, $b:expr) ==>> foo($a, bar($b))",

Veetaha · 2020-02-11T23:52:11Z

editors/code/src/commands/ssr.ts

@@ -0,0 +1,36 @@
+import { Ctx, Cmd } from '../ctx';
+import { applySourceChange, SourceChange } from '../source_change';
+import { window, InputBoxOptions } from 'vscode';


import * as vscode from 'vscode';

I'd suggest to keep everything under vscode namespace. I'd like to achieve consistency across the vscode extension on that...

Veetaha · 2020-02-12T00:02:26Z

editors/code/src/commands/ssr.ts

+            placeHolder: "foo($a:expr, $b:expr) ==>> foo($a: expr, bar($b:expr))",
+            prompt: "Enter request",
+            validateInput: (x: string) => {
+                if (x.indexOf('==>>') >= 0) {


Suggested change

if (x.indexOf('==>>') >= 0) {

if (x.includes('==>>')) {

Veetaha · 2020-02-12T00:11:01Z

crates/ra_ide/src/lib.rs

+    ) -> Cancelable<Result<SourceChange, SsrError>> {
+        self.with_db(|db| {
+            let mut edits = vec![];
+            let (pattern, template) = ssr::parse(template)?;


Hmm, pattern and template are total synonyms for me (i.e. I don't take them apart). Can we use some other names? E.g. ssr::SearchPattern and ssr::ReplacementPattern or something like that?

Hmm, pattern and template are total synonyms

Pattern/template are specific terms in this case, which precisely capture the difference between the two. We use this terminology for macro rules as well.

Pattern is something you can compare with. Ie, you have an existing thing and can say if it matches the pattern or not.

Template is something you use to make new things.

This is similar to how (actual) argument and (formal) parameter are different.

It's curious that both "design patterns" and "C++ templates" are translated as Шаблоны into Russian. This is the correct semantics for templates, but is totally backwards for design patterns, as the book is very specific about the fact that it describes and names existing similar shapes of code and does not give you a template which you can/should apply to new code.

@matklad thank you for the clarification! The actual and formal parameter is the core difference berween these words, though it somehow wasn't obvious to me... Though I still can't think of them in the same terms. Though if this is an already settled naming concept in rust-analyzer I wouldn't like to bring new rules in foreign church ;).

Veetaha · 2020-02-12T00:24:05Z

crates/ra_ide/src/ssr.rs

+#[derive(Debug, PartialEq)]
+pub enum SsrError {
+    ParseError(String),
+}


I agree on that, but only under the condition that we are the only users of that error (i.e. we are not shipping a library for anyone else where we cannot be sure that the users will want to match on particular error types).
As a reference, we move to opaque error in ra_syntax crate right now

Veetaha · 2020-02-12T00:36:36Z

crates/ra_ide/src/ssr.rs

+}
+
+fn is_name(s: &str) -> Result<(), SsrError> {
+    if s.chars().all(|c| char::is_ascii_alphanumeric(&c) || c == '_') {


Suggested change

if s.chars().all(|c| char::is_ascii_alphanumeric(&c) || c == '_') {

if s.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') {

And don't we need to check that the name is not begun with a number? I guess we already have something for this validation somewhere around the project. Maybe @matklad knows?

I will leave names from numbers for now, I don't see any harm in $0, to be honest I want to remove this code and reuse parser for macros.

Veetaha · 2020-02-12T00:40:02Z

crates/ra_ide/src/ssr.rs

+    let name = &s[0..end_of_name];
+    is_name(name)?;
+    let type_begin = end_of_name + 1;
+    let type_length = s[type_begin..].find(|c| !char::is_ascii_alphanumeric(&c)).unwrap_or(s.len());


Suggested change

let type_length = s[type_begin..].find(|c| !char::is_ascii_alphanumeric(&c)).unwrap_or(s.len());

let type_length = s[type_begin..].find(|c| !c.is_ascii_alphanumeric()).unwrap_or(s.len());

it doesn't work, comptroller cannot deduce type for c

Veetaha · 2020-02-12T00:47:03Z

crates/ra_ide/src/ssr.rs

+        m: &mut Match,
+    ) -> bool {
+        match (pattern, code) {
+            (SyntaxElement::Token(ref pattern), SyntaxElement::Token(ref code)) => {


I guess if we change to match (&pattern, &code) we can avoid writing ref here and there, can't we?

Veetaha · 2020-02-12T00:50:55Z

crates/ra_ide/src/ssr.rs

+        pattern: &SyntaxElement,
+        code: &SyntaxElement,
+        placeholders: &[String],
+        m: &mut Match,


Just a nit: I'd propose to avoid single-char names as much as we can. While writing the code it is much simpler, but when reading ...

match is a keyword

Veetaha · 2020-02-12T00:56:39Z

crates/ra_ide/src/ssr.rs

+    use ra_syntax::SourceFile;
+    #[test]
+    fn parser_happy_case() {
+        let result = parse("foo($a:expr, $b:expr) ==>> bar($b, $a)").unwrap();


I'd propose to destructure tuples into named variables. Access to tuple members by numbers is why we don't like them, but simple in-place destructuring is why we do.

Veetaha · 2020-02-12T01:07:48Z

Left small nits, and nothing more. Looks great, big thanks for the implementation!

matklad · 2020-02-17T10:00:55Z

I think there's still a lot to do here, but this seems reasonable to merge now!

I'll open a follow-up issue

bors r+

3099: Init implementation of structural search replace r=matklad a=mikhail-m1 next steps: * ignore space and other minor difference * add support to ra_cli * call rust parser to check pattern * documentation original issue #2267 Co-authored-by: Mikhail Modin <[email protected]>

bors · 2020-02-17T10:10:28Z

Build succeeded

Rust (macos-latest)
Rust (ubuntu-latest)
Rust (windows-latest)
TypeScript

matklad reviewed Feb 11, 2020

View reviewed changes

Veetaha suggested changes Feb 12, 2020

View reviewed changes

Init implementation of structural search replace

f8f454a

mikhail-m1 force-pushed the i2267 branch from 700cd21 to f8f454a Compare February 14, 2020 21:46

mikhail-m1 requested a review from matklad February 15, 2020 12:13

matklad mentioned this pull request Feb 17, 2020

StructuralSearchReplace: next steps #3186

Open

4 tasks

bors bot merged commit f8f454a into rust-lang:master Feb 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Init implementation of structural search replace #3099

Init implementation of structural search replace #3099

mikhail-m1 commented Feb 10, 2020 •

edited

Loading

matklad commented Feb 11, 2020

matklad commented Feb 11, 2020

matklad Feb 11, 2020

matklad Feb 11, 2020

matklad Feb 11, 2020

Veetaha Feb 12, 2020 •

edited

Loading

matklad Feb 11, 2020

matklad Feb 11, 2020

matklad Feb 11, 2020

matklad Feb 11, 2020

Veetaha Feb 11, 2020

Veetaha Feb 12, 2020

Veetaha Feb 12, 2020

matklad Feb 13, 2020 •

edited

Loading

Veetaha Feb 13, 2020

Veetaha Feb 12, 2020 •

edited

Loading

Veetaha Feb 12, 2020

Veetaha Feb 12, 2020

mikhail-m1 Feb 12, 2020

Veetaha Feb 12, 2020

mikhail-m1 Feb 12, 2020 •

edited

Loading

Veetaha Feb 12, 2020

Veetaha Feb 12, 2020

mikhail-m1 Feb 12, 2020

Veetaha Feb 12, 2020 •

edited

Loading

Veetaha commented Feb 12, 2020

matklad commented Feb 17, 2020

bors bot commented Feb 17, 2020

	placeHolder: "foo($a:expr, $b:expr) ==>> foo($a: expr, bar($b:expr))",
	placeHolder: "foo($a:expr, $b:expr) ==>> foo($a, bar($b))",

	if s.chars().all(\|c\| char::is_ascii_alphanumeric(&c) \|\| c == '_') {
	if s.chars().all(\|c\| c.is_ascii_alphanumeric() \|\| c == '_') {

	let type_length = s[type_begin..].find(\|c\| !char::is_ascii_alphanumeric(&c)).unwrap_or(s.len());
	let type_length = s[type_begin..].find(\|c\| !c.is_ascii_alphanumeric()).unwrap_or(s.len());

Init implementation of structural search replace #3099

Init implementation of structural search replace #3099

Conversation

mikhail-m1 commented Feb 10, 2020 • edited Loading

matklad commented Feb 11, 2020

matklad commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Veetaha Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Veetaha Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikhail-m1 Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Veetaha Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

Veetaha commented Feb 12, 2020

matklad commented Feb 17, 2020

bors bot commented Feb 17, 2020

Build succeeded

mikhail-m1 commented Feb 10, 2020 •

edited

Loading

Veetaha Feb 12, 2020 •

edited

Loading

matklad Feb 13, 2020 •

edited

Loading

Veetaha Feb 12, 2020 •

edited

Loading

mikhail-m1 Feb 12, 2020 •

edited

Loading

Veetaha Feb 12, 2020 •

edited

Loading