Allow renaming module from `ModuleSelect` #4750

llpaull · 2025-07-02T14:47:50Z

This PR addresses #4716

lpil

Brilliant, thank you. This will be really nice.

I've left some notes inline, thank you

compiler-core/src/language_server/reference.rs

compiler-core/src/language_server/tests/rename.rs

CHANGELOG.md

lpil · 2025-07-16T18:44:09Z

Hello! Just figuring out what is going to go into what release. Are you still working on this? :)

llpaull · 2025-07-16T21:24:47Z

Yes I am! As for a progress update, most tests are written but still need to write some for guard clause and record access with same name as module. From what I can tell and have tested, all references to the module (alias) can be found and are appropriately renamed but it can't yet be renamed from every reference.

lpil

Thank you! This is looking fab!

I've not finished reviewing this yet, the snapshots are quite a bit more verbose than we usually go for, so it is taking me longer than usual and I've run out of time, but my comments so far should be useful if not complete.

It would be fab if the tests could be slimmed down to make them more focused- I've left some comments inline on that theme.
I think there's some repetition with the test also, with multiple tests testing the use of the AST visitor reaching everywhere.

I've not yet found any test for nested modules, if there are none we want those too.

Thanks again! Looking really nice, it'll be very nice to have this functionality in the language server.

compiler-core/src/ast.rs

compiler-core/src/ast/constant.rs

compiler-core/src/ast/typed.rs

compiler-core/src/language_server/engine.rs

compiler-core/src/language_server/reference.rs

lpil · 2025-07-21T12:07:25Z

compiler-core/src/language_server/reference.rs

+                        .iter()
+                        .zip(elements_type.iter())
+                        .for_each(|(e, type_)| self.visit_type_ast(e, type_));
+                }


If the tuple AST must always have a tuple type then panic in the case that it does not instead of silently skipping over the malformed code. We want to know when there is a bug.

Could it be linked here? Do the links need to be collapsed?

lpil · 2025-07-21T12:07:46Z

compiler-core/src/language_server/reference.rs

+                    self.visit_type_ast(return_, ret_type);
+                }
+            }
+            _ => {}


Same as above- no catch alls, panic for impossible invariants, and what about links?

lpil · 2025-07-21T12:19:22Z

..._language_server__tests__rename__rename_module_from_local_var_annotation_fn_recursive-4.snap

+
+-- app.gleam
+import mod
+fn func() { let _: fn(mod.GenericType(mod.Type)) -> mod.GenericType(mod.Type) = fn(arg: mod.GenericType(mod.Type)) -> mod.GenericType(mod.Type) { arg } }


These snapshots with really long unformatted lines are hard to read and it's not immediately clear what bit is being focused. Please use more normal formatting.

lpil · 2025-07-21T12:20:01Z

...pshots/gleam_core__language_server__tests__rename__rename_module_from_import_with_alias.snap

+const c10 = alias.const3 <> alias.const3
+type Type1 { Var(alias.Type) }
+type Type2 = alias.GenericType(alias.Type)
+type Record { Record (bool: Bool) }


I can't tell what this test is testing, it's very large!

lpil · 2025-07-21T12:22:52Z

...m_core__language_server__tests__rename__rename_module_from_const_annotation_recursive-2.snap

+pub const const2 = <<0:8>>
+pub const const3 = "Hello"
+pub type Type { Variant1 Variant2(Int) }
+pub type GenericType(inner) { Node(inner, GenericType(inner)) Leaf }


It would be nicer if this repeated code wasn't so large, each test uses very little of it. Ideally tests don't include things that they don't use, to make it clearer what they test and the snapshots easier to read. Currently it's quite slow to review these snapshots as it takes a moment for each one to spot the actual test code in the snapshot.

That said, a lot of code has been written using this boilerplate now, so would be annoying to change that.

llpaull · 2025-08-18T22:53:22Z

Tests have been slimmed down quite extensively, if you want any specific or more tests let me know

lpil

Hello! This is looking fab! I've left a few small notes inline, and I think we may be missing tests to cover these situations:

Namespaced modules (e.g. one/two)
Imports with spaces (e.g. import one / two . { three , four } as five
Uses of modules with spaces (e.g. let x = thingy . Thingy)
Triggering rename of an aliased module from uses of it
Triggering rename of a namespaced module from uses of it

lpil · 2025-08-20T17:34:11Z

compiler-core/src/ast.rs

        }
    }

+    pub(crate) fn name_location(&self) -> SrcSpan {


This isn't the location of the module name, that would always be the value of the module_location field. Perhaps "used_name_location"? Please document what this function does also

compiler-core/src/ast.rs

compiler-core/src/language_server/engine.rs

lpil · 2025-08-20T17:40:34Z

compiler-core/src/language_server/reference.rs

+    ) {
+        if *module_name == self.module_name {
+            self.references.push(ModuleNameReference {
+                location: SrcSpan::new(location.start, field_start - 1),


Comments start with a capital letter 🙏

lpil · 2025-08-20T17:42:29Z

compiler-core/src/language_server/reference.rs

+
+                    for (arg, type_) in arguments.iter().zip(args_type) {
+                        self.visit_type_ast(arg, type_);
+                    }


Can't we call the original method to do the iteration?

The reason I was not using the original method was because I was using the full namespaced module name, and the TypeAstConstructor's module field contains the alias instead of the full name (as do other AST nodes). The type_ field however does contain the full name which is why I decided to use this custom method.

I have now been trying to use the alias instead of the full module name which has cleaned this Visit Impl quite a lot but causes the hover action to fail since they both use Located::ModuleName and the alias is not used as the key inside the ProjectCompiler's importable_modules hashmap.

So from what I can tell, either the AST nodes need to contain the full module name instead of/as well as the alias (see TypedExpr::ModuleSelect) or Located::ModuleName needs to contain both.

Which would you rather want or do you know of an alternative solution?

Sounds good to me.

lpil · 2025-08-20T17:43:25Z

compiler-core/src/language_server/rename.rs

+            }
+            // If new name is same as original, remove the alias
+            ModuleNameReferenceKind::AliasedImport if params.new_name == original => {
+                // subtract 1 from start to delete leading space added above


Capital letter please.

What leading space added above? What happens if there's multiple spaces?

there are two scenarios imports can fall into:

1: Import does not contain unqualified member import -> delete all of (module_location.end+1, import.location.end), this will delete all spaces between module and alias as well as the alias itself but not remove any spacing after alias

2: Import does contain unqualified member import -> import AST node does not explicitly contain end location for unqualified imports so the only safe space to remove is the one required to separate tokens

By only deleting the required space, even if there are multiple, I tried to introduce consistency and allow the user to keep multiple import aliases aligned when using the LSP to rename.

Is that okay with you?

compiler-core/src/parse.rs

lpil · 2025-08-25T14:57:34Z

compiler-core/src/language_server/reference.rs

+
+                    for (arg, type_) in arguments.iter().zip(args_type) {
+                        self.visit_type_ast(arg, type_);
+                    }


Sounds good to me.

lpil · 2025-08-25T14:59:27Z

compiler-core/src/language_server/reference.rs

+        match node {
+            TypeAst::Constructor(TypeAstConstructor { module, arguments, .. }) => {
+                let _ = type_.named_type_information().inspect(|(module_name, _, args_type)| {
+                    if let Some((_, location)) = module {


Do the module check first as it's cheaper than the call to named_type_information.

Write these clauses using pattern matching instead of the inspect please, as it's a bit cryptic and the end result isn't used, making inspect not the right combinator for this action also.

llpaull added 2 commits July 2, 2025 16:04

allow renaming module alias

d49dafd

changelog and rustfmt

f637a4f

lpil reviewed Jul 2, 2025

View reviewed changes

llpaull added 3 commits July 2, 2025 21:57

handle spaces and added documentation

9886509

more tests

c978d2b

more tests, rename reference in const value

dfe65cd

llpaull marked this pull request as draft July 4, 2025 01:11

llpaull added 5 commits July 5, 2025 15:26

references inside custom type, type alias, function args

5444e05

variable type annotations

fe07571

correct location for imports with unqualified members

9d9a9f4

tests for renaming from local var assignment

1d81b1b

tests for const, nested module, and custom types

851c79a

llpaull added 6 commits July 18, 2025 00:39

renaming from const value

832dd97

rename module alias from clause guard

3234c73

rename module alias from TypedExpr::Fn return annotation

34aae43

rename module from pattern

b55e390

clippy and rustfmt

0321878

all the snapshots

72324bc

llpaull marked this pull request as ready for review July 19, 2025 22:05

lpil reviewed Jul 21, 2025

View reviewed changes

lpil marked this pull request as draft July 21, 2025 12:29

llpaull added 3 commits August 1, 2025 22:35

logic improvements

1105c50

tests slimmed and cleaned

0710217

allow module rename from option in TypedExprBitArraySegment

4cc5960

llpaull marked this pull request as ready for review August 18, 2025 22:53

lpil reviewed Aug 20, 2025

View reviewed changes

lpil reviewed Aug 25, 2025

View reviewed changes

lpil marked this pull request as draft August 25, 2025 15:00

visit implementation cleanup

09402a8

Uh oh!

Allow renaming module from ModuleSelect #4750

Are you sure you want to change the base?

Allow renaming module from ModuleSelect #4750

Uh oh!

Conversation

llpaull commented Jul 2, 2025

Uh oh!

lpil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lpil commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llpaull commented Jul 16, 2025

Uh oh!

lpil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llpaull commented Aug 18, 2025

Uh oh!

lpil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Allow renaming module from `ModuleSelect` #4750

Allow renaming module from `ModuleSelect` #4750

lpil commented Jul 16, 2025 •

edited

Loading