Repository: intellij-rust/intellij-rust
Issue: "Extract variable" refactoring renames usages of an accidentally name-conflicting variable

<!--
Hello and thank you for the issue!
If you would like to report a bug, we have added some points below that you can fill out.
Feel free to remove all the irrelevant text to request a new feature.
-->

## Environment

* **Intellij-Rust plugin version:**
Latest git (b1ce136d7332a2b031688e00443625ce076ed904)
* **Rust toolchain version:**
rustc 1.31.0-nightly (2bd5993ca 2018-10-02)
* **IDE name and version:**
Idea Community 2018.2.4 (IC-182.4505.22)
* **Operating system:**
Linux x86-64

## Problem description
When selecting an expression and executing "Extract variable" refactoring, a new variable with the generic name, e.g. "x" is introduced and its name is highlighted in such way, that editing it also changes its name at the point of usage, where there has been the extracted expression. 

But if a variable with such name have already existed, any typing attempt changes all its usages too, not just at the place of extracted expression.  

The refactoring apparently tries to avoid that by choosing "i" or "x", whichever is currently unoccupied, but if both are present, it just gives up and uses "x", with ensuing incorrect behavior.

## Steps to reproduce

1. Take this simple program.
1. Select "3".
1. Execute the refactoring.
1. Type to rename the new variable to "blah".

```rust
fn main() {
    let i = 1; 
    let x = 2;

    let z = 3 + 4;
    let w = x + 5;
}
```
### Expected result
```rust
fn main() {
    let i = 1; 
    let x = 2;
    let blah = 3;

    let z = blah + 4;
    let w = x + 5;
}
```
### Actual result
```rust
fn main() {
    let i = 1; 
    let x = 2;
    let blah = 3;

    let z = blah + 4;
    let w = blah + 5;
}
```

Note how `let w = x + 5` became `let w = blah + 5`, even though it had nothing to do with any of it.

Changed files:
src/main/kotlin/org/rust/lang/refactoring/introduceVariable/RsNameSuggestions.kt
src/test/kotlin/org/rust/ide/template/postfix/LetPostfixTemplateTest.kt
src/test/kotlin/org/rust/lang/refactoring/implementMembers/RsNameSuggestionsKtTest.kt
src/test/kotlin/org/rust/lang/refactoring/RsIntroduceVariableHandlerTest.kt

diff --git a/src/main/kotlin/org/rust/lang/refactoring/introduceVariable/RsNameSuggestions.kt b/src/main/kotlin/org/rust/lang/refactoring/introduceVariable/RsNameSuggestions.kt
index ef24dcfba..cd0f29e77 100644
--- a/src/main/kotlin/org/rust/lang/refactoring/introduceVariable/RsNameSuggestions.kt
+++ b/src/main/kotlin/org/rust/lang/refactoring/introduceVariable/RsNameSuggestions.kt
@@ -7,19 +7,20 @@ package org.rust.lang.refactoring.introduceVariable
 
 import com.intellij.psi.PsiElement
 import com.intellij.psi.codeStyle.NameUtil
+import com.intellij.psi.util.PsiTreeUtil
 import org.rust.ide.inspections.toSnakeCase
 import org.rust.ide.utils.CallInfo
 import org.rust.lang.core.psi.*
 import org.rust.lang.core.psi.ext.ancestorOrSelf
 import org.rust.lang.core.psi.ext.ancestorStrict
-import org.rust.lang.core.psi.ext.descendantsOfType
-import org.rust.lang.core.types.ty.TyInteger
 import org.rust.lang.core.types.ty.TyAdt
+import org.rust.lang.core.types.ty.TyInteger
 import org.rust.lang.core.types.ty.TyTraitObject
 import org.rust.lang.core.types.ty.TyTypeParameter
 import org.rust.lang.core.types.type
 import org.rust.lang.refactoring.isValidRustVariableIdentifier
 import org.rust.openapiext.hitOnFalse
+import org.rust.stdext.mapNotNullToSet
 
 
 class SuggestedNames(
@@ -27,6 +28,8 @@ class SuggestedNames(
     val all: LinkedHashSet<String>
 )
 
+const val FRESHEN_LIMIT = 1000
+
 /**
  * This suggests names for an expression about to be bound to a local variable
  *
@@ -37,6 +40,16 @@ class SuggestedNames(
  * If a name is already bound in the local scope do not suggest it.
  */
 fun RsExpr.suggestedNames(): SuggestedNames {
+    fun freshenName(name: String, usedNames: Set<String>): String {
+        var newName = name
+        var i = 1
+        while (i < FRESHEN_LIMIT && usedNames.contains(newName)) {
+            newName = "$name$i"
+            ++i
+        }
+        return newName
+    }
+
     val names = LinkedHashSet<String>()
     val type = type
     when (type) {
@@ -65,10 +78,13 @@ fun RsExpr.suggestedNames(): SuggestedNames {
         names.addName(parent.identifier.text)
     }
 
+    val topName = names.firstOrNull() ?: "x"
     val usedNames = findNamesInLocalScope(this)
+
+    val name = freshenName(topName, usedNames)
     names.removeAll(usedNames)
 
-    return SuggestedNames(names.firstOrNull() ?: "x", names)
+    return SuggestedNames(name, names)
 }
 
 private val uselessNames = listOf("new", "default")
@@ -91,9 +107,11 @@ private fun nameForCall(expr: RsCallExpr): List<String> {
     return listOf(pathElement.text)
 }
 
-private fun findNamesInLocalScope(expr: PsiElement): List<String> {
-    val blockScope = expr.ancestorOrSelf<RsBlock>()
-    val letDecls = blockScope?.descendantsOfType<RsLetDecl>().orEmpty()
+private fun findNamesInLocalScope(expr: PsiElement): Set<String> {
+    val functionScope = expr.ancestorOrSelf<RsFunction>()
 
-    return letDecls.mapNotNull { it.pat?.text }
+    // Existing names should not be shadowed.
+    // For example, see https://github.com/intellij-rust/intellij-rust/issues/2919
+    return PsiTreeUtil.findChildrenOfAnyType(functionScope, RsPatBinding::class.java, RsPath::class.java)
+        .mapNotNullToSet { (it as? RsPath)?.referenceName ?: (it as RsPatBinding).name }
 }
diff --git a/src/test/kotlin/org/rust/ide/template/postfix/LetPostfixTemplateTest.kt b/src/test/kotlin/org/rust/ide/template/postfix/LetPostfixTemplateTest.kt
index 4f12b023c..b1e175582 100644
--- a/src/test/kotlin/org/rust/ide/template/postfix/LetPostfixTemplateTest.kt
+++ b/src/test/kotlin/org/rust/ide/template/postfix/LetPostfixTemplateTest.kt
@@ -42,7 +42,7 @@ class LetPostfixTemplateTest : RsPostfixTemplateTest(LetPostfixTemplate(RsPostfi
         fn foo() { }
 
         fn main() {
-            let /*caret*/foo = foo();
+            let /*caret*/foo1 = foo();
         }
     """)
 
diff --git a/src/test/kotlin/org/rust/lang/refactoring/RsIntroduceVariableHandlerTest.kt b/src/test/kotlin/org/rust/lang/refactoring/RsIntroduceVariableHandlerTest.kt
index 1135af54b..5f682548a 100644
--- a/src/test/kotlin/org/rust/lang/refactoring/RsIntroduceVariableHandlerTest.kt
+++ b/src/test/kotlin/org/rust/lang/refactoring/RsIntroduceVariableHandlerTest.kt
@@ -96,7 +96,7 @@ class RsIntroduceVariableHandlerTest : RsTestBase() {
         }
     """, listOf("10", "5 + 10", "foo(5 + 10)"), 2, """
         fn hello() {
-            let foo = foo(5 + 10);
+            let foo1 = foo(5 + 10);
         }
     """)
 
@@ -155,8 +155,8 @@ class RsIntroduceVariableHandlerTest : RsTestBase() {
             (1, 2)
         }
         fn main() {
-            let bar = bar();
-            foo(bar);
+            let bar1 = bar();
+            foo(bar1);
         }
     """)
 
@@ -183,6 +183,97 @@ class RsIntroduceVariableHandlerTest : RsTestBase() {
         }
     """, mark = IntroduceVariableTestmarks.invalidNamePart)
 
+    // https://github.com/intellij-rust/intellij-rust/issues/2919
+    fun `test issue2919`() = doTest("""
+        fn main() {
+            let i1 = 1;
+            let i2 = 1;
+            let i3 = 1;
+            let (i, x) = (1, 2);
+
+            let z = /*caret*/3 + 4;
+            let w = x + 5;
+        }
+    """, listOf("3", "3 + 4"), 0, """
+        fn main() {
+            let i1 = 1;
+            let i2 = 1;
+            let i3 = 1;
+            let (i, x) = (1, 2);
+            let i4 = 3;
+
+            let z = i4 + 4;
+            let w = x + 5;
+        }
+    """)
+
+    fun `test patterns 1`() = doTest("""
+        fn main() {
+            let (i, x) = (1, 2);
+            let z = /*caret*/3 + 4;
+        }
+    """, listOf("3", "3 + 4"), 0, """
+        fn main() {
+            let (i, x) = (1, 2);
+            let i1 = 3;
+            let z = i1 + 4;
+        }
+    """)
+
+    fun `test patterns 2`() = doTest("""
+        struct S { a: i32 }
+
+        fn main() {
+            let s = S { a: 0 };
+            match s {
+                S { i } => {
+                    let x = /*caret*/5 + i;
+                }
+            }
+        }
+    """, listOf("5", "5 + i"), 0, """
+        struct S { a: i32 }
+
+        fn main() {
+            let s = S { a: 0 };
+            match s {
+                S { i } => {
+                    let i1 = 5;
+                    let x = i1 + i;
+                }
+            }
+        }
+    """)
+
+    fun `test nested scopes`() = doTest("""
+        fn main() {
+            { { let i = 1; } }
+            let i1 = /*caret*/3 + 5;
+        }
+    """, listOf("3", "3 + 5"), 0, """
+        fn main() {
+            { { let i = 1; } }
+            let i2 = 3;
+            let i1 = i2 + 5;
+        }
+    """)
+
+    fun `test functions and static`() = doTest("""
+        fn i() -> i32 { 42 }
+        static i1: i32 = 1;
+        fn main() {
+            let x = i1 + i() + /*caret*/3 + 5;
+        }
+    """, listOf("3", "i1 + i() + 3", "i1 + i() + 3 + 5"), 0, """
+        fn i() -> i32 { 42 }
+        static i1: i32 = 1;
+        fn main() {
+            let i2 = 3;
+            let x = i1 + i() + i2 + 5;
+        }
+    """)
+
+
     private fun doTest(
         @Language("Rust") before: String,
         expressions: List<String>,
diff --git a/src/test/kotlin/org/rust/lang/refactoring/implementMembers/RsNameSuggestionsKtTest.kt b/src/test/kotlin/org/rust/lang/refactoring/implementMembers/RsNameSuggestionsKtTest.kt
index 496d3a829..a2c54f24d 100644
--- a/src/test/kotlin/org/rust/lang/refactoring/implementMembers/RsNameSuggestionsKtTest.kt
+++ b/src/test/kotlin/org/rust/lang/refactoring/implementMembers/RsNameSuggestionsKtTest.kt
@@ -41,12 +41,12 @@ class RsNameSuggestionsKtTest : RsTestBase() {
 
 
     fun `test function names`() = doTest("""
-        fn foo(a: i32, veryCoolVariableName: i32) -> i32 {
+        fn get_foo(a: i32, veryCoolVariableName: i32) -> i32 {
             a + b
         }
 
         fn bar() {
-            f/*caret*/oo(4, 10 + 2)
+            get_f/*caret*/oo(4, 10 + 2)
         }
     """,
         listOf("i", "foo")