Skip to content

refactor(env): introduce VarName enum to unify nameref resolution API#1105

Open
lu-zero wants to merge 1 commit into
reubeno:nameref-expandedfrom
lu-zero:nameref-expanded
Open

refactor(env): introduce VarName enum to unify nameref resolution API#1105
lu-zero wants to merge 1 commit into
reubeno:nameref-expandedfrom
lu-zero:nameref-expanded

Conversation

@lu-zero

@lu-zero lu-zero commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

Summary

This is a stacked PR on top of the nameref-expanded branch. It refactors the environment API to use a VarName enum that encodes nameref resolution strategy in the type system, eliminating the dual-method pattern.

Problem

The nameref implementation added parallel method pairs to ShellEnvironment:

  • update_or_add() / update_or_add_bypassing_nameref()
  • unset() / unset_bypassing_nameref()
  • update_or_add_array_element() / update_or_add_array_element_raw()
  • Plus IntoVarLookup/IntoVarLookupMut traits + DirectVarLookup/DirectVarLookupMut builder types

Every caller had to pick the right variant, and getting it wrong produced subtle bash-incompatibility bugs.

Solution

Introduced a VarName enum with three variants:

enum VarName {
    Auto(String),                              // follow nameref chains (default for &str)
    Resolved { base: String, subscript: Option<String> },  // already resolved
    Direct(String),                            // bypass nameref resolution
}
  • &str / String convert to VarName::Auto by default (backward compatible)
  • ResolvedName converts to VarName::Resolved via From impl
  • VarName::direct("name") for the bypass case

Eliminated methods:

  • Dual update_or_add / update_or_add_bypassing_nameref → single update_or_add(impl Into<VarName>)
  • Dual unset / unset_bypassing_nameref → single unset(impl Into<VarName>)
  • Dual update_or_add_array_element / _raw → single method
  • 4 trait types + 4 builder types → 2 builder types (VarLookup / VarLookupMut)

Module split: env.rs (2128 lines) → env/mod.rs + env/names.rs

Lookup builder changes

Before After
env.lookup("name").get() Same (auto-resolve)
env.lookup("name").bypassing_nameref().get() env.lookup("name").get_direct()
env.lookup(&resolved).get() env.lookup(&resolved).get() (returns ResolvedVarRef) or .get_direct() for raw
env.lookup("name").bypassing_nameref().in_scope(p).get() env.lookup(VarName::direct("name")).in_scope(p).get_direct()

Test results

  • Before: 99 brush-core tests passing
  • After: 108 brush-core tests passing (+9 new VarName-specific tests)
  • All workspace tests pass, clippy clean, fmt clean

Files changed

  • brush-core/src/env/names.rs — new: VarName, ResolvedName, name validation functions
  • brush-core/src/env/mod.rs — refactored ShellEnvironment methods
  • brush-core/src/expansion.rs, interp.rs, extendedtests.rs, shell.rs — adapted callers
  • brush-builtins/src/{declare,export,unset,mapfile,read}.rs — adapted callers

Future work (Phase 2)

The VarName enum uses String fields now. When we add string interning (e.g., lasso), we can introduce a Name newtype and swap the fields to interned keys, making VarName Copy.

@lu-zero lu-zero force-pushed the nameref-expanded branch 4 times, most recently from d2d7e03 to 29508f6 Compare April 19, 2026 18:52
Replace dual method pairs (update_or_add/update_or_add_bypassing_nameref,
unset/unset_bypassing_nameref, etc.) with a single VarName-typed parameter
that encodes resolution strategy:

- VarName::Auto — follow nameref chains (default for &str)
- VarName::Resolved — already resolved, lookup directly
- VarName::Direct — bypass nameref resolution

Split env.rs into env/mod.rs + env/names.rs to separate name types
from environment logic.
@reubeno

reubeno commented Apr 20, 2026

Copy link
Copy Markdown
Owner

Thanks for helping with these changes! I've skimmed through and I'm going to start merging some of it into my working branch -- but there's a few pieces I'd like to think more about first (on the read-only lookup side).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants