Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts Lunary’s module function-call scoping so that module-defined identifiers take precedence over caller-local identifiers, and adds a regression test to ensure module functions can share names with local variables.
Changes:
- Fix scope merge order for module function access so module scope wins on key collisions.
- Add a unit test covering module function name collision with a local identifier.
- Remove a leftover commented
@tag :skipline in a chain test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/lunary.ex |
Changes how scopes are merged during module function access to prioritize module scope. |
test/unit/module_test.exs |
Adds regression coverage for calling a module function when a local variable has the same name. |
test/unit/chain_test.exs |
Removes a stray commented skip tag. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| defp evaluate({:func_access, index, {:fn, {:identifier, fn_line, fn_id}, fn_args}}, scope, opts) do | ||
| {func_scope, _} = evaluate(index, scope, opts) | ||
| merged_scope = Map.merge(func_scope, scope) | ||
| merged_scope = Map.merge(scope, func_scope) # function scope takes priority | ||
| evaluate({:fn, {:identifier, fn_line, fn_id}, fn_args}, merged_scope, opts) |
There was a problem hiding this comment.
The scope precedence change here fixes direct module calls, but there is still a second code path for chained module calls ({:chain, ..., {:func_access, ...}} around lib/lunary.ex:396-403) that merges scopes in the old order (Map.merge(func_scope, scope)). That means val |> @mod.fn() can still resolve identifiers using the caller scope ahead of the module scope, making scoping behavior inconsistent and likely leaving the original bug unfixed for pipelines. Update that merge order as well (and consider adding a test that pipes into a module function while a conflicting local identifier exists).
| defp evaluate({:func_access, index, {:fn, {:identifier, fn_line, fn_id}, fn_args}}, scope, opts) do | ||
| {func_scope, _} = evaluate(index, scope, opts) | ||
| merged_scope = Map.merge(func_scope, scope) | ||
| merged_scope = Map.merge(scope, func_scope) # function scope takes priority |
There was a problem hiding this comment.
Minor: the inline comment says "function scope takes priority", but func_scope here is actually the evaluated module scope coming from index (e.g., @example). Consider rewording to avoid confusion when revisiting scoping rules later.
| merged_scope = Map.merge(scope, func_scope) # function scope takes priority | |
| merged_scope = Map.merge(scope, func_scope) # evaluated scope from `index` takes priority |
No description provided.