Repository: gleam-lang/gleam (rust)

Issue:
Rename Types incorrectly replace in named variants of types in other modules
if I have:

main.gleam
```gleam

pub type MyType {
    MyType
}
fn main() {
  let my_type = MyType
}
```
other_module.gleam
```gleam

pub type OtherType {
    OtherType(t: main.MyType)
}
```

When I try to rename MyType in main.gleam to MyTypeRenamed, other_module duplicates the diff, so it would be `OtherType(main.MyTypeRenamedRenamed)`.

If I rename it back to `MyType`, it removes the parenthesis from other module: `OtherType(main.MyType`

Pull request:
Fix renaming of types in variant constructor definitions
Fixes #4372 
The issue occurred because in the `custom_type_accessors` function, we traverse the `TypeAst` twice after we've already done so in `register_values_from_custom_type`. This caused 3 references to be registered for any types in the arguments, rather than just 1.
To fix this, I've changed it so that we use the type information we already have instead of rebuilding it continuously from the AST.

Patch:
diff --git a/compiler-core/src/analyse.rs b/compiler-core/src/analyse.rs
index 4eada4c4d..0d8358b98 100644
--- a/compiler-core/src/analyse.rs
+++ b/compiler-core/src/analyse.rs
@@ -993,25 +993,6 @@ impl<'a, A> ModuleAnalyzer<'a, A> {
             .type_
             .clone();
 
-        let Accessors {
-            shared_accessors,
-            variant_specific_accessors,
-        } = custom_type_accessors(constructors, &mut hydrator, environment, &mut self.problems)?;
-
-        let map = AccessorsMap {
-            publicity: if *opaque {
-                Publicity::Private
-            } else {
-                *publicity
-            },
-            shared_accessors,
-            // TODO: improve the ownership here so that we can use the
-            // `return_type_constructor` below rather than looking it up twice.
-            type_: type_.clone(),
-            variant_specific_accessors,
-        };
-        environment.insert_accessors(name.clone(), map);
-
         let mut constructors_data = vec![];
 
         let mut index = 0;
@@ -1151,6 +1132,25 @@ impl<'a, A> ModuleAnalyzer<'a, A> {
             );
         }
 
+        let Accessors {
+            shared_accessors,
+            variant_specific_accessors,
+        } = custom_type_accessors(&constructors_data)?;
+
+        let map = AccessorsMap {
+            publicity: if *opaque {
+                Publicity::Private
+            } else {
+                *publicity
+            },
+            shared_accessors,
+            // TODO: improve the ownership here so that we can use the
+            // `return_type_constructor` below rather than looking it up twice.
+            type_: type_.clone(),
+            variant_specific_accessors,
+        };
+        environment.insert_accessors(name.clone(), map);
+
         let opaque = if *opaque {
             Opaque::Opaque
         } else {
@@ -1807,46 +1807,39 @@ struct Accessors {
     variant_specific_accessors: Vec<HashMap<EcoString, RecordAccessor>>,
 }
 
-fn custom_type_accessors<A: std::fmt::Debug>(
-    constructors: &[RecordConstructor<A>],
-    hydrator: &mut Hydrator,
-    environment: &mut Environment<'_>,
-    problems: &mut Problems,
-) -> Result<Accessors, Error> {
+fn custom_type_accessors(constructors: &[TypeValueConstructor]) -> Result<Accessors, Error> {
     let args = get_compatible_record_fields(constructors);
 
     let mut shared_accessors = HashMap::with_capacity(args.len());
 
-    hydrator.disallow_new_type_variables();
-    for (index, label, ast) in args {
-        let type_ = hydrator.type_from_ast(ast, environment, problems)?;
+    for (index, label, type_) in args {
         let _ = shared_accessors.insert(
             label.clone(),
             RecordAccessor {
                 index: index as u64,
                 label: label.clone(),
-                type_,
+                type_: type_.clone(),
             },
         );
     }
 
-    let mut variant_specific_accessors = Vec::with_capacity(constructors.len());
+    let mut variant_specific_accessors: Vec<HashMap<EcoString, RecordAccessor>> =
+        Vec::with_capacity(constructors.len());
 
     for constructor in constructors {
-        let mut fields = HashMap::with_capacity(constructor.arguments.len());
+        let mut fields = HashMap::with_capacity(constructor.parameters.len());
 
-        for (index, argument) in constructor.arguments.iter().enumerate() {
-            let Some((_location, label)) = &argument.label else {
+        for (index, parameter) in constructor.parameters.iter().enumerate() {
+            let Some(label) = &parameter.label else {
                 continue;
             };
 
-            let type_ = hydrator.type_from_ast(&argument.ast, environment, problems)?;
             let _ = fields.insert(
                 label.clone(),
                 RecordAccessor {
                     index: index as u64,
                     label: label.clone(),
-                    type_,
+                    type_: parameter.type_.clone(),
                 },
             );
         }
@@ -1861,9 +1854,9 @@ fn custom_type_accessors<A: std::fmt::Debug>(
 
 /// Returns the fields that have the same label and type across all variants of
 /// the given type.
-fn get_compatible_record_fields<A: std::fmt::Debug>(
-    constructors: &[RecordConstructor<A>],
-) -> Vec<(usize, &EcoString, &TypeAst)> {
+fn get_compatible_record_fields(
+    constructors: &[TypeValueConstructor],
+) -> Vec<(usize, &EcoString, &Arc<Type>)> {
     let mut compatible = vec![];
 
     let first = match constructors.first() {
@@ -1871,10 +1864,10 @@ fn get_compatible_record_fields<A: std::fmt::Debug>(
         None => return compatible,
     };
 
-    'next_argument: for (index, first_argument) in first.arguments.iter().enumerate() {
+    'next_argument: for (index, first_parameter) in first.parameters.iter().enumerate() {
         // Fields without labels do not have accessors
-        let first_label = match first_argument.label.as_ref() {
-            Some((_, label)) => label,
+        let first_label = match first_parameter.label.as_ref() {
+            Some(label) => label,
             None => continue 'next_argument,
         };
 
@@ -1882,22 +1875,22 @@ fn get_compatible_record_fields<A: std::fmt::Debug>(
         // with the same label and the same type
         for constructor in constructors.iter().skip(1) {
             // The field must exist in all variants
-            let argument = match constructor.arguments.get(index) {
+            let parameter = match constructor.parameters.get(index) {
                 Some(argument) => argument,
                 None => continue 'next_argument,
             };
 
             // The labels must be the same
-            if argument
+            if parameter
                 .label
                 .as_ref()
-                .is_none_or(|(_, arg_label)| arg_label != first_label)
+                .is_none_or(|arg_label| arg_label != first_label)
             {
                 continue 'next_argument;
             }
 
             // The types must be the same
-            if !argument.ast.is_logically_equal(&first_argument.ast) {
+            if !parameter.type_.same_as(&first_parameter.type_) {
                 continue 'next_argument;
             }
         }
@@ -1905,7 +1898,7 @@ fn get_compatible_record_fields<A: std::fmt::Debug>(
         // The previous loop did not find any incompatible fields in the other
         // variants so this field is compatible across variants and we should
         // generate an accessor for it.
-        compatible.push((index, first_label, &first_argument.ast))
+        compatible.push((index, first_label, &first_parameter.type_))
     }
 
     compatible
diff --git a/compiler-core/src/language_server/reference.rs b/compiler-core/src/language_server/reference.rs
index c55c0b32b..d53b3a801 100644
--- a/compiler-core/src/language_server/reference.rs
+++ b/compiler-core/src/language_server/reference.rs
@@ -213,23 +213,24 @@ pub fn reference_for_ast_node(
         }),
         Located::Annotation { ast, type_ } => match type_.named_type_name() {
             Some((module, name)) => {
-                let target_kind = match ast {
+                let (target_kind, location) = match ast {
                     ast::TypeAst::Constructor(constructor) => {
-                        if constructor.module.is_some() {
+                        let kind = if constructor.module.is_some() {
                             RenameTarget::Qualified
                         } else {
                             RenameTarget::Unqualified
-                        }
+                        };
+                        (kind, constructor.name_location)
                     }
                     ast::TypeAst::Fn(_)
                     | ast::TypeAst::Var(_)
                     | ast::TypeAst::Tuple(_)
-                    | ast::TypeAst::Hole(_) => RenameTarget::Unqualified,
+                    | ast::TypeAst::Hole(_) => (RenameTarget::Unqualified, ast.location()),
                 };
                 Some(Referenced::ModuleType {
                     module,
                     name,
-                    location: ast.location(),
+                    location,
                     target_kind,
                 })
             }


Test patch:
diff --git a/compiler-core/src/language_server/tests/rename.rs b/compiler-core/src/language_server/tests/rename.rs
index ba02ea3a7..d7087f190 100644
--- a/compiler-core/src/language_server/tests/rename.rs
+++ b/compiler-core/src/language_server/tests/rename.rs
@@ -1275,3 +1275,59 @@ pub fn main() -> wibble.Wibble { todo }
         find_position_of("Wibble")
     );
 }
+
+// https://github.com/gleam-lang/gleam/issues/4372
+#[test]
+fn rename_type_referenced_in_variant_constructor_argument() {
+    assert_rename!(
+        (
+            "mod",
+            "
+import app
+
+pub type Wobble {
+  Wobble(w: app.Wibble)
+}
+"
+        ),
+        "
+pub type Wibble {
+  Wibble
+}
+
+pub fn main() {
+  let wibble = Wibble
+}
+",
+        "SomeType",
+        find_position_of("Wibble")
+    );
+}
+
+// https://github.com/gleam-lang/gleam/issues/4372
+#[test]
+fn rename_type_from_variant_constructor_argument() {
+    assert_rename!(
+        (
+            "mod",
+            "
+pub type Wibble {
+  Wibble
+}
+
+pub fn main() {
+  let wibble = Wibble
+}
+"
+        ),
+        "
+import mod
+
+pub type Wobble {
+  Wobble(w: mod.Wibble)
+}
+",
+        "SomeType",
+        find_position_of("Wibble")
+    );
+}
diff --git a/compiler-core/src/language_server/tests/snapshots/gleam_core__language_server__tests__rename__rename_type_from_qualified_reference.snap b/compiler-core/src/language_server/tests/snapshots/gleam_core__language_server__tests__rename__rename_type_from_qualified_reference.snap
index d116905cf..3edc22dd2 100644
--- a/compiler-core/src/language_server/tests/snapshots/gleam_core__language_server__tests__rename__rename_type_from_qualified_reference.snap
+++ b/compiler-core/src/language_server/tests/snapshots/gleam_core__language_server__tests__rename__rename_type_from_qualified_reference.snap
@@ -15,7 +15,7 @@ fn wibble(w: Wibble) -> Wibble { todo }
 import mod
 
 pub fn main(w: mod.Wibble) -> mod.Wibble { todo }
-               ↑▔▔▔▔▔▔▔▔▔                        
+                   ↑▔▔▔▔▔                        
 
 
 ----- AFTER RENAME
diff --git a/compiler-core/src/language_server/tests/snapshots/gleam_core__language_server__tests__rename__rename_type_from_variant_constructor_argument.snap b/compiler-core/src/language_server/tests/snapshots/gleam_core__language_server__tests__rename__rename_type_from_variant_constructor_argument.snap
new file mode 100644
index 000000000..88490f212
--- /dev/null
+++ b/compiler-core/src/language_server/tests/snapshots/gleam_core__language_server__tests__rename__rename_type_from_variant_constructor_argument.snap
@@ -0,0 +1,45 @@
+---
+source: compiler-core/src/language_server/tests/rename.rs
+expression: "\nimport mod\n\npub type Wobble {\n  Wobble(w: mod.Wibble)\n}\n"
+---
+----- BEFORE RENAME
+-- mod.gleam
+
+pub type Wibble {
+  Wibble
+}
+
+pub fn main() {
+  let wibble = Wibble
+}
+
+
+-- app.gleam
+
+import mod
+
+pub type Wobble {
+  Wobble(w: mod.Wibble)
+                ↑▔▔▔▔▔ 
+}
+
+
+----- AFTER RENAME
+-- mod.gleam
+
+pub type SomeType {
+  Wibble
+}
+
+pub fn main() {
+  let wibble = Wibble
+}
+
+
+-- app.gleam
+
+import mod
+
+pub type Wobble {
+  Wobble(w: mod.SomeType)
+}
diff --git a/compiler-core/src/language_server/tests/snapshots/gleam_core__language_server__tests__rename__rename_type_referenced_in_variant_constructor_argument.snap b/compiler-core/src/language_server/tests/snapshots/gleam_core__language_server__tests__rename__rename_type_referenced_in_variant_constructor_argument.snap
new file mode 100644
index 000000000..69db6ef74
--- /dev/null
+++ b/compiler-core/src/language_server/tests/snapshots/gleam_core__language_server__tests__rename__rename_type_referenced_in_variant_constructor_argument.snap
@@ -0,0 +1,45 @@
+---
+source: compiler-core/src/language_server/tests/rename.rs
+expression: "\npub type Wibble {\n  Wibble\n}\n\npub fn main() {\n  let wibble = Wibble\n}\n"
+---
+----- BEFORE RENAME
+-- mod.gleam
+
+import app
+
+pub type Wobble {
+  Wobble(w: app.Wibble)
+}
+
+
+-- app.gleam
+
+pub type Wibble {
+         ↑▔▔▔▔▔  
+  Wibble
+}
+
+pub fn main() {
+  let wibble = Wibble
+}
+
+
+----- AFTER RENAME
+-- mod.gleam
+
+import app
+
+pub type Wobble {
+  Wobble(w: app.SomeType)
+}
+
+
+-- app.gleam
+
+pub type SomeType {
+  Wibble
+}
+
+pub fn main() {
+  let wibble = Wibble
+}