Skip to content

feat(code-action): add flatten nested attribute set refactor action#757

Merged
takeokunn merged 7 commits into
mainfrom
feature/add-code-action-2
Jan 4, 2026
Merged

feat(code-action): add flatten nested attribute set refactor action#757
takeokunn merged 7 commits into
mainfrom
feature/add-code-action-2

Conversation

@takeokunn

Copy link
Copy Markdown
Collaborator

Summary

Adds a new "Flatten nested attribute set" code action that transforms nested attribute sets into dot notation:

# Before
{ foo = { bar = 1; }; }

# After
{ foo.bar = 1; }

This follows the existing pattern established in #756 for code actions.

Changes

File Description
nixd/lib/Controller/CodeAction.cpp Add getFlattenableBinds() helper and addFlattenAttrsAction() for the flatten refactoring
nixd/tools/nixd/test/code-action/flatten-attrs.md Basic test for simple nested attribute flattening
nixd/tools/nixd/test/code-action/flatten-attrs-multi.md Test for nested sets with multiple bindings
nixd/tools/nixd/test/code-action/flatten-attrs-deep.md Test for deeply nested attribute paths
nixd/tools/nixd/test/code-action/flatten-attrs-rec.md Test that rec sets are excluded
nixd/tools/nixd/test/code-action/flatten-attrs-inherit.md Test that sets with inherit are excluded
nixd/tools/nixd/test/code-action/flatten-attrs-dynamic.md Test that dynamic ${} names are excluded
nixd/tools/nixd/test/code-action/flatten-attrs-empty.md Test that empty nested sets are excluded

Behavior

Flatten action offered when:

  • Cursor is on a binding whose value is an attribute set
  • All attribute names (outer and inner) are static identifiers
  • The nested set contains at least one binding

Explicitly excluded:

  • Recursive attribute sets (rec { ... }): Flattening would change semantics
  • Inherited bindings (inherit x;): Cannot be represented in dot notation
  • Dynamic attribute names (${expr}): Cannot be statically flattened
  • Empty nested sets ({ foo = {}; }): No meaningful transformation

Test Plan

  • Added 7 lit tests covering positive and negative cases
  • Tests verify the action is offered only when appropriate
  • Tests verify correct transformation output for various input shapes
  • All existing tests pass
CleanShot 2026-01-04 at 20 12 35@2x CleanShot 2026-01-04 at 20 14 09@2x

Related

- Introduce addFlattenAttrsAction to provide a code action that flattens
  nested attribute sets into dot-notation assignments
- Integrate flatten action into code action controller for eligible nodes
- Ensure flattening only applies to static, non-recursive attribute sets
- Add tests for flattening simple, deep, and multi-level attrsets
- Add negative tests for dynamic, empty, inherit, and rec attrsets
- Verify correct code action offering and ordering in each scenario
@takeokunn takeokunn requested a review from inclyc as a code owner January 4, 2026 11:24

@inclyc inclyc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation LGTM; I’ve left some comments regarding the LLVM FileCheck directives.

In general, we should try to avoid testing redundant content or logic from other PRs within the .md regression tests (though this is sometimes unavoidable). You can use CHECK: patterns to skip over certain sections instead of using CHECK-NEXT:, and you can use CHECK-NOT: to verify the absence of specific structures.

Comment thread nixd/tools/nixd/test/code-action/flatten-attrs-dynamic.md Outdated
Comment thread nixd/tools/nixd/test/code-action/flatten-attrs-empty.md Outdated
Comment thread nixd/tools/nixd/test/code-action/flatten-attrs-inherit.md Outdated
Comment thread nixd/tools/nixd/test/code-action/flatten-attrs-multi.md Outdated
Comment thread nixd/tools/nixd/test/code-action/flatten-attrs-multi.md Outdated
Verify the absence of flatten action using CHECK-NOT directive
instead of testing quote action from another PR.
Verify the absence of flatten action using CHECK-NOT directive
instead of testing quote action from another PR.
Verify the absence of flatten action using CHECK-NOT directive
instead of testing quote action from another PR.
- Remove quote action tests (belongs to different PR)
- Use CHECK: instead of CHECK-NEXT: to skip intermediate sections
- Focus only on testing flatten action functionality
@takeokunn

Copy link
Copy Markdown
Collaborator Author

@inclyc
Thanks for the review!
I've fixed the part you mentioned in the comments.
I didn't really understand how to write tests, so this was a good learning experience!

@takeokunn takeokunn requested a review from inclyc January 4, 2026 14:01
@takeokunn takeokunn merged commit 30d15dc into main Jan 4, 2026
19 checks passed
@takeokunn takeokunn deleted the feature/add-code-action-2 branch January 4, 2026 15:07
takeokunn added a commit that referenced this pull request Jan 5, 2026
Split pack-attrs-siblings-positive.md into two separate test files to fix
the CI failure in the nix-build-develop-gcc (undefined, release, false)
configuration.

The original test contained 2 sequential code action requests followed by
an exit notification. Under UBSan, the sanitizer overhead caused a race
condition where the second response wasn't fully flushed to stdout before
process termination.

Following the pattern from PR #757, each test file now contains only ONE
code action request, eliminating the race condition.

New test files:
- pack-attrs-sibling-inherit.md: Tests pack action with inherit sibling
- pack-attrs-sibling-different-prefix.md: Tests pack with different prefix
takeokunn added a commit that referenced this pull request Jan 5, 2026
…#758)

## Summary

Add a new code action that converts dotted attribute paths to nested
attribute sets - the inverse of the "Flatten nested attribute set"
action from #757.

**Single Pack:**
```nix
{ foo.bar = 1; }
→ { foo = { bar = 1; }; }
```

**Bulk Pack** (multiple sibling bindings with same prefix):
```nix
{ foo.bar = 1; foo.baz = 2; }
→ { foo = { bar = 1; baz = 2; }; }
```

## Changes

| File | Description |
|------|-------------|
| nixd/lib/Controller/CodeAction.cpp | Add packAttrs function
implementing the pack transformation with single and bulk modes |
| **Single Pack Tests** | |
| pack-attrs.md | Basic test for dotted path packing |
| pack-attrs-deep.md | Test for multi-level dotted paths (packs one
level at a time) |
| pack-attrs-quoted.md | Test for preserving quoted attribute names |
| pack-attrs-multiline.md | Test for multiline attribute set formatting
|
| pack-attrs-values.md | Test for various value types (empty sets, let
expressions, nested sets) |
| **Bulk Pack Tests** | |
| pack-attrs-bulk.md | Test for bulk pack with sibling bindings |
| pack-attrs-bulk-quoted.md | Test for bulk pack with quoted keys
requiring proper quoting |
| pack-attrs-escape.md | Test for escaping special characters (", \\, $)
in quoted keys |
| pack-attrs-sibling-inherit.md | Test for pack with inherit sibling
(non-conflicting) |
| pack-attrs-sibling-different-prefix.md | Test for pack with different
prefix siblings |
| **Negative Tests** | |
| pack-attrs-single.md | Negative test for single-segment paths |
| pack-attrs-rec.md | Negative test for recursive attribute sets |
| pack-attrs-dynamic.md | Negative test for dynamic interpolation in
paths |
| pack-attrs-conflict.md | Negative test for non-path binding conflicts
(e.g., `{ foo = 1; foo.bar = 2; }`) |
| pack-attrs-sibling-dynamic.md | Negative test for dynamic attributes
in sibling bindings |
| flatten-attrs-deep.md | Updated to verify both Pack and Flatten
actions offered together |

## Behavior

### Single Pack Action ("Pack dotted path to nested set")

Action is offered when:
- Cursor is on a dotted attribute path with 2+ segments (e.g.,
`foo.bar`)
- The attribute is inside a non-recursive attribute set
- No sibling bindings share the same first segment prefix

### Bulk Pack Action ("Pack all 'X' bindings to nested set")

Action is offered when:
- Multiple sibling bindings share the same first segment prefix (e.g.,
`foo.bar` and `foo.baz`)
- All sibling binding paths are static (no dynamic interpolation)
- No conflicting non-path binding exists for the prefix

Transformation combines all matching siblings:
```nix
{ foo.bar = 1; foo.baz = 2; foo.qux = 3; }
→ { foo = { bar = 1; baz = 2; qux = 3; }; }
```

### Explicitly Excluded

- **Single-segment paths** (e.g., `foo = 1`): Nothing to pack
- **Recursive attribute sets** (`rec { ... }`): Packing would change
semantics
- **Dynamic attribute paths** (e.g., `foo.${bar}`): Cannot safely
transform interpolated paths
- **Conflicting non-path bindings** (e.g., `{ foo = 1; foo.bar = 2; }`):
Would create invalid Nix code
- **Dynamic sibling attributes**: When any sibling has dynamic
interpolation, bulk pack is disabled

### Deep Path Handling

Deep paths are packed one level at a time:
- `a.b.c = 1` → `a = { b.c = 1; }` (first application)
- `a = { b.c = 1; }` → `a = { b = { c = 1; }; }` (second application)

### Key Quoting and Escaping

The action correctly handles:
- **Quoted keys**: Keys requiring quotes (e.g., `"1foo"`) are properly
quoted in output
- **Special character escaping**: Characters like `"`, `\`, and `$` are
correctly escaped

## Test Plan

- ✅ **All CI checks passing**
- **16 test files** covering positive and negative cases
- **Positive tests** verify correct transformation output for:
  - Basic single pack
  - Deep paths (multi-level)
  - Quoted attribute names
  - Multiline formatting
  - Various value types (empty sets, let expressions, nested sets)
  - Bulk pack with multiple siblings
  - Bulk pack with special characters in keys
  - Non-conflicting sibling scenarios (inherit, different prefixes)
- **Negative tests** verify action is NOT offered in excluded scenarios:
  - Single-segment paths
  - Recursive attribute sets
  - Dynamic interpolation
  - Conflicting non-path bindings
  - Dynamic sibling attributes

<img width="876" height="404" alt="CleanShot 2026-01-05 at 22 59 36@2x"
src="https://github.com/user-attachments/assets/b32ba748-9ff6-4d40-95d1-8a51e7e82bd3"
/>

<img width="984" height="412" alt="CleanShot 2026-01-06 at 03 05 10@2x"
src="https://github.com/user-attachments/assets/bb69bdc7-9d1f-40b1-8b21-a3ad125c91f2"
/>

## Related

- Part of #466
- Inverse of #757 (flatten nested attribute set)
- Draft PR: #755 (this implements Split 3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request nixd:controller

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants