From 900c89ed880a589382c22a33c36ed923250e69f7 Mon Sep 17 00:00:00 2001 From: Jesse Herrick Date: Wed, 10 Jun 2026 22:57:18 -0400 Subject: [PATCH] Fix bare script rename scoping and alias references Treat top-level scripts, module bodies, and definition bodies as separate variable scopes during rename. Resolve plain alias-backed require/import/use references so related rename edits stay consistent. --- internal/lsp/rename_test.go | 43 +++++++ internal/parser/parser_tokenized.go | 16 +-- internal/treesitter/variables.go | 93 +++++++++++++- internal/treesitter/variables_test.go | 177 ++++++++++++++++++++++++++ internal/version/version.go | 2 +- 5 files changed, 315 insertions(+), 16 deletions(-) diff --git a/internal/lsp/rename_test.go b/internal/lsp/rename_test.go index cc45d26..9c22faa 100644 --- a/internal/lsp/rename_test.go +++ b/internal/lsp/rename_test.go @@ -1267,6 +1267,49 @@ end } } +func TestRename_Module_PlainAliasUpdatesRequireInSameFile(t *testing.T) { + server, cleanup := setupTestServer(t) + defer cleanup() + + defContent := `defmodule SharedLib.Values.ServiceEndpointConfig do + defmacro __using__(_), do: :ok +end +` + // A config script that aliases the module by its plain (last-segment) name + // and then refers to it by that short name in a `require`. + callerContent := `import Config + +alias SharedLib.Values.ServiceEndpointConfig + +require ServiceEndpointConfig +` + callerPath := filepath.Join(server.projectRoot, "config", "runtime.exs") + indexFile(t, server.store, server.projectRoot, "lib/service_endpoint_config.ex", defContent) + indexFile(t, server.store, server.projectRoot, "config/runtime.exs", callerContent) + + callerURI := "file://" + callerPath + server.docs.Set(callerURI, callerContent) + + // Rename the alias on the alias line (line 2). Column 20 is the start of + // "ServiceEndpointConfig" in "alias SharedLib.Values.ServiceEndpointConfig". + edit := renameAt(t, server, callerURI, 2, uint32(len("alias SharedLib.Values.")), "PublicEndpointConfig") + if edit == nil { + t.Fatal("expected non-nil edit") + } + + edits := collectEdits(edit, callerPath) + + // The alias declaration's full module reference must be updated. + if !hasEdit(edits, "SharedLib.Values.PublicEndpointConfig") { + t.Errorf("expected alias line full-module edit to SharedLib.Values.PublicEndpointConfig; edits=%+v", edits) + } + // The `require` referencing the short alias name must also be updated, and + // only its short-name segment (line 4) — not turned into the full module. + if !editsContainLine(edits, 4) || !hasEdit(edits, "PublicEndpointConfig") { + t.Errorf("expected `require` short-name on line 4 to be renamed to PublicEndpointConfig; edits=%+v", edits) + } +} + func TestRename_Module_AliasAsPreserved(t *testing.T) { server, cleanup := setupTestServer(t) defer cleanup() diff --git a/internal/parser/parser_tokenized.go b/internal/parser/parser_tokenized.go index f18b61e..f554577 100644 --- a/internal/parser/parser_tokenized.go +++ b/internal/parser/parser_tokenized.go @@ -427,8 +427,8 @@ func parseTextFromTokens(path string, source []byte, tokens []Token) ([]Definiti j := nextSig(i) modName, k := collectModuleName(j) if modName != "" { - resolved := resolveModule(modName, currentModule()) - if !strings.Contains(resolved, "__MODULE__") { + resolved := ResolveModuleRef(modName, aliases, currentModule()) + if resolved != "" { refs = append(refs, Reference{Module: resolved, Line: importLine, FilePath: path, Kind: "import"}) injectors[resolved] = true } @@ -442,8 +442,8 @@ func parseTextFromTokens(path string, source []byte, tokens []Token) ([]Definiti j := nextSig(i) modName, k := collectModuleName(j) if modName != "" { - resolved := resolveModule(modName, currentModule()) - if !strings.Contains(resolved, "__MODULE__") { + resolved := ResolveModuleRef(modName, aliases, currentModule()) + if resolved != "" { refs = append(refs, Reference{Module: resolved, Line: useLine, FilePath: path, Kind: "use"}) injectors[resolved] = true } @@ -464,8 +464,8 @@ func parseTextFromTokens(path string, source []byte, tokens []Token) ([]Definiti // Check for require Module, as: Name if asName, nextPos, ok := ScanKeywordOptionValue(source, tokens, n, k, "as"); ok { - resolved := resolveModule(modName, cm) - if !strings.Contains(resolved, "__MODULE__") { + resolved := ResolveModuleRef(modName, aliases, cm) + if resolved != "" { aliases[asName] = resolved refs = append(refs, Reference{Module: resolved, Line: requireLine, FilePath: path, Kind: "require"}) } @@ -474,8 +474,8 @@ func parseTextFromTokens(path string, source []byte, tokens []Token) ([]Definiti } // Simple require (no as:) — still emit reference but no alias - resolved := resolveModule(modName, cm) - if !strings.Contains(resolved, "__MODULE__") { + resolved := ResolveModuleRef(modName, aliases, cm) + if resolved != "" { refs = append(refs, Reference{Module: resolved, Line: requireLine, FilePath: path, Kind: "require"}) } i = k diff --git a/internal/treesitter/variables.go b/internal/treesitter/variables.go index e948d2b..282ee81 100644 --- a/internal/treesitter/variables.go +++ b/internal/treesitter/variables.go @@ -78,7 +78,10 @@ func FindVariableOccurrencesWithTree(root *tree_sitter.Node, src []byte, line, c return occurrences } - skipRoot := resolved.scope.Kind() == "stab_clause" + // A def-family call (scope.Kind() == "call" here, since with/for calls are + // handled above) is the scope root, so skip the def-boundary check on it — + // otherwise collection would bail immediately on the very scope it chose. + skipRoot := resolved.scope.Kind() == "stab_clause" || resolved.scope.Kind() == "call" collectVariableOccurrences(resolved.scope, src, resolved.varName, &occurrences, skipRoot) return occurrences } @@ -114,16 +117,27 @@ func NameExistsInScopeOf(root *tree_sitter.Node, src []byte, line, col uint, new } // findFirstNonCallIdentifier returns the first identifier node in the subtree -// matching name that is not a function name in a call expression. +// matching name that is not a function name in a call expression. Nested +// function definitions (def/defp/etc.) are independent scopes, so they are not +// descended into — a same-named binding inside one is not a collision in the +// scope rooted at node. (The root itself may be such a def call when renaming a +// function-local; that is the chosen scope and is always searched.) func findFirstNonCallIdentifier(node *tree_sitter.Node, src []byte, name string) *tree_sitter.Node { + return findFirstNonCallIdentifierInScope(node, src, name, true) +} + +func findFirstNonCallIdentifierInScope(node *tree_sitter.Node, src []byte, name string, isRoot bool) *tree_sitter.Node { if node == nil { return nil } + if !isRoot && definesNestedScope(node, src) { + return nil + } if node.Kind() == "identifier" && node.Utf8Text(src) == name && !isFunctionNameInCall(node, src) { return node } for i := uint(0); i < uint(node.ChildCount()); i++ { - if found := findFirstNonCallIdentifier(node.Child(i), src, name); found != nil { + if found := findFirstNonCallIdentifierInScope(node.Child(i), src, name, false); found != nil { return found } } @@ -289,6 +303,44 @@ func isDefinitionKeyword(name string) bool { return defKeywords[name] } +// moduleKeywords are the keywords that open a module body — an independent +// variable scope. Variables bound directly in a module body belong only to +// that module, not to sibling modules or the surrounding script. +var moduleKeywords = map[string]bool{ + "defmodule": true, "defprotocol": true, "defimpl": true, +} + +// isFunctionDefinitionCall reports whether node is a def/defp/defmacro/etc. +// call — the boundary of an independent variable scope. Variables inside a +// function definition do not leak to (and cannot reference) an enclosing +// module/script scope, so traversals rooted at an outer scope must not descend +// into these. +func isFunctionDefinitionCall(node *tree_sitter.Node, src []byte) bool { + if node.Kind() != "call" || node.ChildCount() == 0 { + return false + } + first := node.Child(0) + return first.Kind() == "identifier" && functionKeywords[first.Utf8Text(src)] +} + +// isModuleDefinitionCall reports whether node is a defmodule/defprotocol/defimpl +// call, which opens a module-body scope. +func isModuleDefinitionCall(node *tree_sitter.Node, src []byte) bool { + if node.Kind() != "call" || node.ChildCount() == 0 { + return false + } + first := node.Child(0) + return first.Kind() == "identifier" && moduleKeywords[first.Utf8Text(src)] +} + +// definesNestedScope reports whether node is a call that introduces its own +// variable scope — a function or module definition. A traversal rooted at an +// outer scope (a module body, or the whole file) must not descend into these, +// or a rename/collision check would wrongly reach into an unrelated scope. +func definesNestedScope(node *tree_sitter.Node, src []byte) bool { + return isFunctionDefinitionCall(node, src) || isModuleDefinitionCall(node, src) +} + // isAssignmentTarget returns true if node is on the left-hand side of a `=` // binary operator, meaning it is unambiguously a variable binding. func isAssignmentTarget(node *tree_sitter.Node, src []byte) bool { @@ -309,16 +361,23 @@ func isAssignmentTarget(node *tree_sitter.Node, src []byte) bool { // at the cursor position is ambiguous (could be a zero-arity function call) // and should not be treated as a variable. func variableDefinedInScope(scope *tree_sitter.Node, src []byte, varName string, cursorLine, cursorCol uint) bool { - return identifierExistsElsewhere(scope, src, varName, cursorLine, cursorCol) + return identifierExistsElsewhere(scope, src, varName, cursorLine, cursorCol, true) } // identifierExistsElsewhere returns true if an identifier matching name // exists anywhere in the subtree at a position different from (line, col). -// It skips function names in calls and definition keywords. -func identifierExistsElsewhere(node *tree_sitter.Node, src []byte, name string, line, col uint) bool { +// It skips function names in calls and definition keywords. Nested function +// definitions are independent scopes and are not descended into (isRoot guards +// the chosen scope itself, which may be such a def call) — otherwise a bare +// top-level call sharing a name with a function-local would be misread as a +// variable. +func identifierExistsElsewhere(node *tree_sitter.Node, src []byte, name string, line, col uint, isRoot bool) bool { if node == nil { return false } + if !isRoot && definesNestedScope(node, src) { + return false + } if node.Kind() == "identifier" && node.Utf8Text(src) == name && !isFunctionNameInCall(node, src) { pos := node.StartPosition() if uint(pos.Row) != line || uint(pos.Column) != col { @@ -326,7 +385,7 @@ func identifierExistsElsewhere(node *tree_sitter.Node, src []byte, name string, } } for i := uint(0); i < uint(node.ChildCount()); i++ { - if identifierExistsElsewhere(node.Child(i), src, name, line, col) { + if identifierExistsElsewhere(node.Child(i), src, name, line, col, false) { return true } } @@ -363,6 +422,12 @@ func findEnclosingScope(node *tree_sitter.Node, src []byte, varName string) *tre if firstChild.Kind() == "identifier" && functionKeywords[firstChild.Utf8Text(src)] { return current } + // A module body (defmodule/defprotocol/defimpl) is its own scope: + // module-level bindings belong to this module, not to sibling + // modules or the surrounding script. + if firstChild.Kind() == "identifier" && moduleKeywords[firstChild.Utf8Text(src)] { + return current + } // with/for/etc.: scope boundary unless cursor is on clause 0's rhs (outer scope). if callHasDoBlock(current) && callArgumentPatternsBindVariable(current, src, varName) { if cursorNeedsWithScope(current, prev, node, src, varName) { @@ -370,6 +435,11 @@ func findEnclosingScope(node *tree_sitter.Node, src []byte, varName string) *tre } } } + // Reached the file root without an inner scope: top-level script + // bindings (e.g. config/runtime.exs) are scoped to the whole file. + if current.Kind() == "source" { + return current + } prev = current current = current.Parent() } @@ -463,6 +533,15 @@ func collectVariableOccurrences(node *tree_sitter.Node, src []byte, varName stri collectPatternExpressionOccurrences(node, src, varName, out) return } + + // Function and module definitions introduce their own variable scope. + // When collecting from an outer scope — e.g. the whole file for a + // top-level script binding, or a module body — do not descend into a + // nested definition, or a rename would wrongly touch same-named bindings + // that live in that separate scope. + if definesNestedScope(node, src) { + return + } } for i := uint(0); i < uint(node.ChildCount()); i++ { diff --git a/internal/treesitter/variables_test.go b/internal/treesitter/variables_test.go index 083d965..4400e29 100644 --- a/internal/treesitter/variables_test.go +++ b/internal/treesitter/variables_test.go @@ -1380,3 +1380,180 @@ end`) } } } + +func TestFindVariableOccurrences_TopLevelScript(t *testing.T) { + // Config scripts (e.g. config/runtime.exs) bind variables at the file's + // top level, with no enclosing def/defmodule. These must still be + // renameable — the whole file is the variable's scope. + src := []byte(`import Config + +environment = System.get_env("ENVIRONMENT", "dev") +config :app, env: environment +`) + + // Cursor on the binding site "environment" at line 2, col 0. + occs := FindVariableOccurrences(src, 2, 0) + if len(occs) != 2 { + t.Fatalf("expected 2 occurrences of top-level 'environment', got %d: %+v", len(occs), occs) + } + if occs[0].Line != 2 { + t.Errorf("occ[0] line: expected 2 (binding), got %d", occs[0].Line) + } + if occs[1].Line != 3 { + t.Errorf("occ[1] line: expected 3 (reference), got %d", occs[1].Line) + } + + // Cursor on the reference "environment" at line 3 resolves to the same set. + refOccs := FindVariableOccurrences(src, 3, uint(len("config :app, env: "))) + if len(refOccs) != 2 { + t.Fatalf("expected 2 occurrences from reference site, got %d: %+v", len(refOccs), refOccs) + } +} + +func TestFindVariableOccurrences_TopLevelDoesNotCrossDefBoundary(t *testing.T) { + // A top-level script binding is scoped to the whole file, but def/defp + // bodies are independent scopes. Renaming a top-level variable must not + // touch a same-named local inside a nested function. + src := []byte(`config = load_config() + +defmodule App do + def start do + config = build() + use(config) + end +end + +apply(config) +`) + + // Cursor on the top-level binding "config" at line 0, col 0. + occs := FindVariableOccurrences(src, 0, 0) + if len(occs) != 2 { + t.Fatalf("expected 2 occurrences of top-level 'config' (line 0 + line 9), got %d: %+v", len(occs), occs) + } + for _, occ := range occs { + if occ.Line == 4 || occ.Line == 5 { + t.Errorf("top-level rename leaked into def body at line %d: %+v", occ.Line, occs) + } + } + if occs[0].Line != 0 { + t.Errorf("occ[0] line: expected 0 (binding), got %d", occs[0].Line) + } + if occs[1].Line != 9 { + t.Errorf("occ[1] line: expected 9 (reference), got %d", occs[1].Line) + } +} + +func TestNameExistsInScopeOf_TopLevelDoesNotCrossDefBoundary(t *testing.T) { + // Collision detection for a top-level rename must use the same scope rules + // as collection: a same-named binding inside a def body is a separate scope + // and must not be reported as a collision. + src := []byte(`config = load_config() + +defmodule App do + def start do + other = build() + use(other) + end +end + +apply(config) +`) + root, cleanup := parseElixir(src) + defer cleanup() + + // Renaming top-level "config" to "other" is safe: "other" only exists as a + // def-local, which is a different scope. + if NameExistsInScopeOf(root, src, 0, 0, "other") { + t.Error("false-positive collision: 'other' is a def-local, not in the top-level scope") + } +} + +func TestFindVariableOccurrences_ModuleBodyVarsAreSeparateScopes(t *testing.T) { + // A variable bound directly in a module body is scoped to that module. + // Renaming it must not touch a same-named module-body binding in a sibling + // module. + src := []byte(`defmodule A do + port = 4000 + IO.puts(port) +end + +defmodule B do + port = 5000 + IO.puts(port) +end +`) + + // Cursor on the "port" binding in module A (line 1). + occs := FindVariableOccurrences(src, 1, 2) + if len(occs) != 2 { + t.Fatalf("expected 2 occurrences within module A, got %d: %+v", len(occs), occs) + } + for _, occ := range occs { + if occ.Line >= 5 { + t.Errorf("module A 'port' rename leaked into module B at line %d: %+v", occ.Line, occs) + } + } +} + +func TestFindVariableOccurrences_TopLevelVarDoesNotCrossModuleBoundary(t *testing.T) { + // A top-level script binding must not leak into a same-named binding inside + // a module body — module bodies are independent scopes. + src := []byte(`config = load() + +defmodule A do + config = 1 + IO.puts(config) +end + +use_it(config) +`) + + // Cursor on the top-level "config" binding (line 0). + occs := FindVariableOccurrences(src, 0, 0) + if len(occs) != 2 { + t.Fatalf("expected 2 occurrences (line 0 binding + line 7 reference), got %d: %+v", len(occs), occs) + } + for _, occ := range occs { + if occ.Line == 3 || occ.Line == 4 { + t.Errorf("top-level 'config' rename leaked into module body at line %d: %+v", occ.Line, occs) + } + } +} + +func TestFindVariableOccurrences_TopLevelBareCallSharedWithDefLocal(t *testing.T) { + // A parenless bare zero-arity call at the top level (e.g. a config DSL + // reference) whose name coincides with a local inside a def body must not be + // treated as a variable — the def-local is a separate scope and must not + // make the top-level reference look "bound". + src := []byte(`config :app, value: helper + +defmodule App do + def start do + helper = 1 + use(helper) + end +end +`) + + // Cursor on top-level "helper" at line 0. + occs := FindVariableOccurrences(src, 0, uint(len("config :app, value: "))) + if occs != nil { + t.Errorf("top-level bare call 'helper' misclassified as variable: %+v", occs) + } +} + +func TestFindVariableOccurrences_TopLevelBareCallNotVariable(t *testing.T) { + // A bare zero-arity call at the top level (not bound anywhere) must not be + // mistaken for a variable, even though the file root is now a valid scope. + src := []byte(`import Config + +config :app, value: some_helper() +`) + + // Cursor on "some_helper" at line 2. + occs := FindVariableOccurrences(src, 2, uint(len("config :app, value: "))) + if occs != nil { + t.Errorf("expected nil for bare top-level call, got %d occurrences: %+v", len(occs), occs) + } +} diff --git a/internal/version/version.go b/internal/version/version.go index 527692d..2e2c620 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -5,4 +5,4 @@ const Version = "0.7.0" // IndexVersion is incremented whenever the index schema or parser changes in a // way that requires a full rebuild. Bump this alongside Version when releasing // a change that makes existing indexes stale. -const IndexVersion = 11 +const IndexVersion = 12