Auto merge of rust-lang#16277 - roife:fix-issue16276, r=Veykril

Resolve panic in `generate_delegate_methods`

Fixes rust-lang#16276

This PR addresses two issues:

1. When using `PathTransform`, it searches for the node corresponding to the `path` in the `source_scope` during `make::fn_`. Therefore, we need to perform the transform before `make::fn_` (similar to the problem in issue rust-lang#15804). Otherwise, even though the tokens are the same, their offsets (i.e., `span`) differ, resulting in the error "Can't find CONST_ARG@xxx."

2. As mentioned in the first point, `PathTransform` searches for the node corresponding to the `path` in the `source_scope`. Thus, when transforming paths, we should update nodes from right to left (i.e., use **reverse of preorder** (right -> left -> root) instead of **postorder** (left -> right -> root)). Reasons are as follows:

    In the red-green tree (rowan), we do not store absolute ranges but instead store the length of each node and dynamically calculate offsets (spans). Therefore, when modifying the left-side node (such as nodes are inserted or deleted), it causes all right-side nodes' spans to change. This, in turn, leads to PathTransform being unable to find nodes with the same paths (due to different spans), resulting in errors.

Author: bors

crates/ide-assists/src/handlers/generate_delegate_methods.rs

@@ -107,31 +107,48 @@ pub(crate) fn generate_delegate_methods(acc: &mut Assists, ctx: &AssistContext<'
             |edit| {
                 // Create the function
                 let method_source = match ctx.sema.source(method) {
-                    Some(source) => source.value,
+                    Some(source) => {
+                        let v = source.value.clone_for_update();
+                        let source_scope = ctx.sema.scope(v.syntax());
+                        let target_scope = ctx.sema.scope(strukt.syntax());
+                        if let (Some(s), Some(t)) = (source_scope, target_scope) {
+                            PathTransform::generic_transformation(&t, &s).apply(v.syntax());
+                        }
+                        v
+                    }
                     None => return,
                 };
+
                 let vis = method_source.visibility();
+                let is_async = method_source.async_token().is_some();
+                let is_const = method_source.const_token().is_some();
+                let is_unsafe = method_source.unsafe_token().is_some();
+
                 let fn_name = make::name(&name);
+
+                let type_params = method_source.generic_param_list();
+                let where_clause = method_source.where_clause();
                 let params =
                     method_source.param_list().unwrap_or_else(|| make::param_list(None, []));
-                let type_params = method_source.generic_param_list();
-                let arg_list = match method_source.param_list() {
-                    Some(list) => convert_param_list_to_arg_list(list),
-                    None => make::arg_list([]),
-                };
+
+                // compute the `body`
+                let arg_list = method_source
+                    .param_list()
+                    .map(|list| convert_param_list_to_arg_list(list))
+                    .unwrap_or_else(|| make::arg_list([]));
+
                 let tail_expr = make::expr_method_call(field, make::name_ref(&name), arg_list);
-                let ret_type = method_source.ret_type();
-                let is_async = method_source.async_token().is_some();
-                let is_const = method_source.const_token().is_some();
-                let is_unsafe = method_source.unsafe_token().is_some();
                 let tail_expr_finished =
                     if is_async { make::expr_await(tail_expr) } else { tail_expr };
                 let body = make::block_expr([], Some(tail_expr_finished));
+
+                let ret_type = method_source.ret_type();
+
                 let f = make::fn_(
                     vis,
                     fn_name,
                     type_params,
-                    method_source.where_clause(),
+                    where_clause,
                     params,
                     body,
                     ret_type,
@@ -184,12 +201,6 @@ pub(crate) fn generate_delegate_methods(acc: &mut Assists, ctx: &AssistContext<'
                 let assoc_items = impl_def.get_or_create_assoc_item_list();
                 assoc_items.add_item(f.clone().into());
 
-                if let Some((target, source)) =
-                    ctx.sema.scope(strukt.syntax()).zip(ctx.sema.scope(method_source.syntax()))
-                {
-                    PathTransform::generic_transformation(&target, &source).apply(f.syntax());
-                }
-
                 if let Some(cap) = ctx.config.snippet_cap {
                     edit.add_tabstop_before(cap, f)
                 }

crates/ide-db/src/path_transform.rs

@@ -3,6 +3,7 @@
 use crate::helpers::mod_path_to_ast;
 use either::Either;
 use hir::{AsAssocItem, HirDisplay, ModuleDef, SemanticsScope};
+use itertools::Itertools;
 use rustc_hash::FxHashMap;
 use syntax::{
     ast::{self, make, AstNode},
@@ -227,24 +228,28 @@ struct Ctx<'a> {
     same_self_type: bool,
 }
 
-fn postorder(item: &SyntaxNode) -> impl Iterator<Item = SyntaxNode> {
-    item.preorder().filter_map(|event| match event {
-        syntax::WalkEvent::Enter(_) => None,
-        syntax::WalkEvent::Leave(node) => Some(node),
-    })
+fn preorder_rev(item: &SyntaxNode) -> impl Iterator<Item = SyntaxNode> {
+    let x = item
+        .preorder()
+        .filter_map(|event| match event {
+            syntax::WalkEvent::Enter(node) => Some(node),
+            syntax::WalkEvent::Leave(_) => None,
+        })
+        .collect_vec();
+    x.into_iter().rev()
 }
 
 impl Ctx<'_> {
     fn apply(&self, item: &SyntaxNode) {
         // `transform_path` may update a node's parent and that would break the
         // tree traversal. Thus all paths in the tree are collected into a vec
         // so that such operation is safe.
-        let paths = postorder(item).filter_map(ast::Path::cast).collect::<Vec<_>>();
+        let paths = preorder_rev(item).filter_map(ast::Path::cast).collect::<Vec<_>>();
         for path in paths {
             self.transform_path(path);
         }
 
-        postorder(item).filter_map(ast::Lifetime::cast).for_each(|lifetime| {
+        preorder_rev(item).filter_map(ast::Lifetime::cast).for_each(|lifetime| {
             if let Some(subst) = self.lifetime_substs.get(&lifetime.syntax().text().to_string()) {
                 ted::replace(lifetime.syntax(), subst.clone_subtree().clone_for_update().syntax());
             }
@@ -263,7 +268,7 @@ impl Ctx<'_> {
             // `transform_path` may update a node's parent and that would break the
             // tree traversal. Thus all paths in the tree are collected into a vec
             // so that such operation is safe.
-            let paths = postorder(value).filter_map(ast::Path::cast).collect::<Vec<_>>();
+            let paths = preorder_rev(value).filter_map(ast::Path::cast).collect::<Vec<_>>();
             for path in paths {
                 self.transform_path(path);
             }