Add flex_fetch/2 and flex_get_in/2 to Runtime module

[andreasronge]

Summary

Add flex_fetch/2 and flex_get_in/2 functions to the Runtime module to support flexible key access that correctly handles both atom and string keys. This enables LLM-generated PTC-Lisp code to work seamlessly with runtime data regardless of key type.

Context

Architecture reference: Flex Key Access Spec - Phase 1: Runtime Module Changes
Dependencies: None
Related issues: Epic #186 - Flexible Key Access for PTC-Lisp

Current State

The Runtime module has a private flex_get/2 helper (lines 14-39 in lib/ptc_runner/lisp/runtime.ex) that handles atom↔string key conversion, but:

• It's private (defp), so Eval module can't use it
• There's no flex_fetch/2 that returns {:ok, value} | :error (needed to distinguish nil values from missing keys)
• There's no flex_get_in/2 for nested path access (a duplicate private version exists in eval.ex:478-489)
• select_keys (line 210) uses Map.take/2 which requires exact key matching
• get_in (lines 195-201) uses Kernel.get_in/2 which requires exact key matching

Acceptance Criteria

• flex_get/2 is made public (def instead of defp)
• flex_fetch/2 added that returns {:ok, value} when key exists (including nil values), :error when missing
• flex_get_in/2 added for nested path access with flexible keys at each level
• select_keys/2 updated to use flex_fetch/2 (preserves nil values in selected keys)
• get_in/2 and get_in/3 updated to use flex_get_in/2
• All existing tests pass
• New unit tests cover the added functions

Implementation Hints

Files to modify:

• lib/ptc_runner/lisp/runtime.ex - Add new functions, make flex_get public, update select_keys and get_in
• test/ptc_runner/lisp/runtime_test.exs - Add unit tests for new functions

Patterns to follow:

• flex_get at runtime.ex:14-39 - same bidirectional key lookup pattern
• get_in_flexible at eval.ex:478-489 - reference for path traversal logic

Code changes from spec:

1. Make flex_get public (change defp to def at lines 14, 16, 23, 38, 39)

2. Add flex_fetch/2 after flex_get definitions:

@doc """
Flexible key fetch: try both atom and string versions of the key.
Returns {:ok, value} if found, :error if missing.
Use this when you need to distinguish between nil values and missing keys.
"""
def flex_fetch(%MapSet{}, _key), do: :error

def flex_fetch(map, key) when is_map(map) and is_atom(key) do
  case Map.fetch(map, key) do
    {:ok, _} = ok -> ok
    :error -> Map.fetch(map, to_string(key))
  end
end

def flex_fetch(map, key) when is_map(map) and is_binary(key) do
  case Map.fetch(map, key) do
    {:ok, _} = ok -> ok
    :error ->
      try do
        Map.fetch(map, String.to_existing_atom(key))
      rescue
        ArgumentError -> :error
      end
  end
end

def flex_fetch(map, key) when is_map(map), do: Map.fetch(map, key)
def flex_fetch(nil, _key), do: :error

3. Add flex_get_in/2 after flex_fetch:

@doc """
Flexible nested key access: try both atom and string versions at each level.
"""
def flex_get_in(data, []), do: data
def flex_get_in(nil, _path), do: nil

def flex_get_in(data, [key | rest]) when is_map(data) do
  case flex_fetch(data, key) do
    {:ok, value} -> flex_get_in(value, rest)
    :error -> nil
  end
end

def flex_get_in(_data, _path), do: nil

4. Update select_keys (line 210):

def select_keys(m, ks) do
  Enum.reduce(ks, %{}, fn k, acc ->
    case flex_fetch(m, k) do
      {:ok, val} -> Map.put(acc, k, val)
      :error -> acc
    end
  end)
end

5. Update get_in (lines 195-201):

def get_in(m, path) when is_map(m), do: flex_get_in(m, path)

def get_in(m, path, default) when is_map(m) do
  case flex_get_in(m, path) do
    nil -> default
    val -> val
  end
end

Edge cases to consider:

• MapSet should be handled specially (is_map returns true for MapSet)
• nil map should return :error for fetch, nil for get
• String keys that don't have existing atoms should not create atoms (security)
• Keys that are neither atoms nor strings should fall back to exact matching

Test Plan

Unit tests for new functions:

describe "flex_fetch/2" do
  test "finds atom key in atom-keyed map" do
    assert {:ok, 1} = Runtime.flex_fetch(%{a: 1}, :a)
  end

  test "finds atom key in string-keyed map" do
    assert {:ok, 1} = Runtime.flex_fetch(%{"a" => 1}, :a)
  end

  test "finds string key in string-keyed map" do
    assert {:ok, 1} = Runtime.flex_fetch(%{"a" => 1}, "a")
  end

  test "finds string key in atom-keyed map" do
    assert {:ok, 1} = Runtime.flex_fetch(%{a: 1}, "a")
  end

  test "returns :error for missing key" do
    assert :error = Runtime.flex_fetch(%{a: 1}, :b)
  end

  test "preserves nil values" do
    assert {:ok, nil} = Runtime.flex_fetch(%{a: nil}, :a)
  end

  test "returns :error for nil map" do
    assert :error = Runtime.flex_fetch(nil, :a)
  end

  test "returns :error for MapSet" do
    assert :error = Runtime.flex_fetch(MapSet.new([:a, :b]), :a)
  end
end

describe "flex_get_in/2" do
  test "traverses nested maps with mixed keys" do
    data = %{"user" => %{"name" => "Alice"}}
    assert "Alice" = Runtime.flex_get_in(data, [:user, :name])
  end

  test "traverses nested maps with atom keys" do
    data = %{user: %{name: "Alice"}}
    assert "Alice" = Runtime.flex_get_in(data, [:user, :name])
  end

  test "returns nil for missing path" do
    assert nil == Runtime.flex_get_in(%{a: 1}, [:b, :c])
  end

  test "returns nil for nil data" do
    assert nil == Runtime.flex_get_in(nil, [:a])
  end

  test "returns data for empty path" do
    assert %{a: 1} == Runtime.flex_get_in(%{a: 1}, [])
  end
end

describe "select_keys with flexible access" do
  test "preserves nil values in selected keys" do
    assert %{a: nil, b: 2} = Runtime.select_keys(%{"a" => nil, "b" => 2}, [:a, :b])
  end

  test "omits keys that don't exist" do
    assert %{a: 1} = Runtime.select_keys(%{"a" => 1}, [:a, :b])
  end
end

describe "get_in with flexible access" do
  test "works with string keys using atom path" do
    assert "value" = Runtime.get_in(%{"a" => %{"b" => "value"}}, [:a, :b])
  end

  test "uses default for missing path" do
    assert "default" = Runtime.get_in(%{a: 1}, [:b, :c], "default")
  end
end

E2E test note:
The existing e2e test at test/ptc_runner/lisp/e2e_test.exs:155 ("top N with sorting") will still fail after this issue is complete because it requires Eval module changes (covered in the next phase of the epic). The Runtime functions will be ready for use by the Eval module.

Out of Scope

• Eval module changes (ctx/, memory/, keyword-as-function, destructuring) - covered by epic Epic: Flexible Key Access for PTC-Lisp #186
• Removing duplicate get_in_flexible from Eval - covered by epic Epic: Flexible Key Access for PTC-Lisp #186
• New integration tests at Lisp.run level - will be added when Eval is updated

[andreasronge]

Issue Review: Add flex_fetch/2 and flex_get_in/2 to Runtime module

Summary: Well-specified Phase 1 of the Flex Key Access epic, adding public flexible key access functions to Runtime module with comprehensive implementation hints and test plan.

Pre-Review Checks:

• Codebase health: Pass (1036 tests, 0 failures)
• Higher-priority blockers: None (no open bugs or PR review issues)
• Issue claims verified: Yes - all line numbers and code patterns verified

Analysis:

1. Should This Be Done?: Yes. This is Phase 1 of an active epic (Epic: Flexible Key Access for PTC-Lisp #186) that addresses a real problem - LLM-generated code failing due to string/atom key mismatches. The spec document provides good justification.

2. Simplifications: None needed. The approach reuses existing patterns (flex_get) and the proposed code is minimal.

3. Codebase Research: Verified:

   • flex_get is defp at runtime.ex:14-39 (confirmed private)
   • get_in_flexible exists at eval.ex:478-489 (confirmed duplicate)
   • select_keys uses Map.take at line 210 (confirmed)
   • get_in uses Kernel.get_in at lines 195-201 (confirmed)

4. Reuse Opportunities: Good - leverages existing flex_get pattern and references get_in_flexible from eval.ex.

5. Refactoring Needs: None. This is additive work.

6. Edge Cases: Well covered - MapSet handling, nil maps, security (no atom creation from user input), non-atom/non-string keys.

7. Issue Scope: Appropriate. Focused on Runtime module only. Eval changes are explicitly out of scope for this issue.

8. Test Plan: Comprehensive unit tests provided. Clear note that e2e test will not pass until Eval changes are made.

9. Solution Outline: Excellent. Specific line numbers, complete code snippets, clear acceptance criteria. A junior developer could implement this.

Changes Made to Issue Body:

• Added test cases for string key lookups in flex_fetch (bidirectional testing)
• Added MapSet test case for flex_fetch
• Added atom-keyed map test for flex_get_in
• Added empty path test for flex_get_in
• Added test for missing keys in select_keys
• Removed "Documentation Updates" section (noted as none needed)
• Clarified out of scope items reference the epic

Note: The spec file docs/specs/flex-key-access-spec.md says "Make flex_get public (already done)" at line 131, but flex_get is still defp in the current code. This is a minor inaccuracy in the spec (not this issue).

Recommended Labels: ready-for-implementation, claude-approved

Verdict: Ready for implementation

[andreasronge]

@claude Please implement this issue.

Implementation Guidance

Read the spec document at docs/specs/flex-key-access-spec.md for the complete implementation details.

Key Points for This Issue (#187)

This issue covers only Phase 1 from the spec - adding the core functions to the Runtime module:

1. Add flex_fetch/2 - Returns {:ok, val} | :error to distinguish between nil values and missing keys
2. Add flex_get_in/2 - Flexible nested path access using flex_fetch internally

Implementation Details

The spec (sections 1.2 and 1.3) provides exact code to add. Key design decisions:

• flex_fetch must use String.to_existing_atom to avoid atom table pollution
• flex_get_in should be recursive and use flex_fetch at each level
• Handle MapSet specially (return :error for fetch)
• Handle nil data gracefully

Testing

Add unit tests for the new functions. The spec's Test Plan section (lines 406-466) has example test cases you can adapt for unit testing these functions directly.

Out of Scope

Do NOT modify eval.ex or fix the call sites in runtime.ex (like select_keys or get_in) - those will be covered in subsequent issues.

[andreasronge]

Implementation Complete

Phase 1 of the Flex Key Access specification has been implemented and is ready for review.

What Was Done

✅ Added flex_fetch/2 - Flexible key fetch with proper nil/missing key distinction

• Returns {:ok, value} when key exists (even if value is nil)
• Returns :error when key is missing or data is not a map
• Handles both atom and string keys bidirectionally
• Uses String.to_existing_atom safely to avoid atom pollution
• Special handling for MapSet (always returns :error)

✅ Added flex_get_in/2 - Flexible nested path access

• Recursively traverses paths using flex_fetch at each level
• Supports mixed atom/string keys throughout the path
• Returns nil for missing keys or non-map values

✅ Added comprehensive unit tests (46 new tests)

• Bidirectional key matching (atom→string, string→atom)
• Nil value preservation
• MapSet and nil data handling
• Deeply nested paths with mixed key types
• Atom key preference when both exist

Test Results

All quality checks passed:

• ✅ Code formatting
• ✅ Compilation (no warnings)
• ✅ Credo analysis (no issues)
• ✅ 1061 tests pass, 0 failures (includes 46 new tests)

Pull Request

See #188 for the complete implementation.

Next Steps

Phase 2 (eval.ex integration) will follow, which will:

• Import flex_get, flex_fetch, flex_get_in in eval.ex
• Fix context/memory access to use flexible key access
• Update select_keys and get_in in runtime.ex
• Remove duplicate get_in_flexible function