Skip to content

feat(code-action): add pack dotted path to nested set refactor action#758

Merged
takeokunn merged 7 commits into
mainfrom
feature/code-actions-3
Jan 5, 2026
Merged

feat(code-action): add pack dotted path to nested set refactor action#758
takeokunn merged 7 commits into
mainfrom
feature/code-actions-3

Conversation

@takeokunn

@takeokunn takeokunn commented Jan 5, 2026

Copy link
Copy Markdown
Collaborator

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:

{ foo.bar = 1; }{ foo = { bar = 1; }; }

Bulk Pack (multiple sibling bindings with same prefix):

{ 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:

{ 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 = 1a = { 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
CleanShot 2026-01-05 at 22 59 36@2x CleanShot 2026-01-06 at 03 05 10@2x

Related

Add a new refactoring code action that transforms dotted attribute paths
into nested attribute sets (inverse of flatten action).

Transformation: { foo.bar = 1; } -> { foo = { bar = 1; }; }

The action is offered when:
- Path has at least 2 segments (e.g., foo.bar)
- All path segments are static identifiers
- Parent attribute set is not recursive (rec)
- No sibling bindings share the same first segment

Explicitly excluded:
- Single-segment paths (nothing to pack)
- Dynamic paths (cannot statically transform)
- Recursive sets (packing may change semantics)
- Sibling conflicts (would create duplicate nested keys)

Test files:
- pack-attrs.md: basic functionality
- pack-attrs-deep.md: deep paths (one-level pack)
- pack-attrs-quoted.md: quoted attribute preservation
- pack-attrs-empty.md: empty attrset value
- pack-attrs-single.md: negative test for single segments
- pack-attrs-rec.md: negative test for recursive sets
- pack-attrs-dynamic.md: negative test for dynamic paths
- pack-attrs-siblings.md: negative test for sibling conflicts
- flatten-attrs-deep.md: verify both actions offered together
@takeokunn takeokunn marked this pull request as ready for review January 5, 2026 14:07
@takeokunn takeokunn requested a review from inclyc as a code owner January 5, 2026 14:07

@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.

Is this possible? #755 (comment)

nixd has SemaAttrs where attributes are lowered into nested structure.

@takeokunn

Copy link
Copy Markdown
Collaborator Author

It seems implementable, so I'll give it a try.

- Add logic to detect and offer bulk pack action when multiple sibling
  bindings share the same prefix
- Quote and escape attribute keys as needed for valid Nix syntax
- Prevent pack action when siblings have dynamic attributes or path
  conflicts
- Refactor code to use getSiblingCount and findSiblingBindingsRange for
  robust sibling analysis
- Add tests for quoted keys, escaping, multiline sets, empty sets,
  complex values, and positive/negative sibling cases
@takeokunn

takeokunn commented Jan 5, 2026

Copy link
Copy Markdown
Collaborator Author

I haven't thoroughly reviewed the source code yet, but it now runs on my local machine.

CleanShot 2026-01-06 at 03 05 10@2x

… keys

- Move attribute key quoting and escaping logic into quoteNixAttrKey
- Replace duplicated key quoting code with quoteNixAttrKey calls
- Improve maintainability and consistency for Nix attribute key handling
- Add logic to process escape sequences (\n, \r, \t, etc.) in strings
- Unescape recognized sequences and add unescaped characters to Parts
- Update comments for clarity on escape handling
- Add test for parsing string escape sequences: \n, \r, \t, \\, \", \$
- Add test for parsing strings with escaped quotes
- Ensure diagnostics are empty and literals match expected values
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

@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 current implementation LGTM. However, I think we can further improve the UX.

I suggest we handle this as a follow-up task in a separate PR to keep this one focused.

Actions.emplace_back(createSingleEditAction(
"Pack dotted path to nested set", CodeAction::REFACTOR_REWRITE_KIND,
FileURI, toLSPRange(Src, Bind.range()), std::move(NewText)));
} else {

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.

Offer both 'Pack One' and 'Pack All' options?

e.g. sometimes user may just want to "pack one"

{
  a.b = 1;
  a.c = 2;
}

->

{
  a = { b = 1; };
  a.c = 2;
}

return;

Actions.emplace_back(createSingleEditAction(
"Pack all '" + FirstSeg + "' bindings to nested set",

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.

Suggested change
"Pack all '" + FirstSeg + "' bindings to nested set",
"Recursively pack all '" + FirstSeg + "' bindings to nested set",

Right now, the 'Pack All' operation actually behaves like a recursive pack (matching SemaAttrs logic).

i.e.

{
  a.a = 1;
  a.f = {
    a.x.x2.x3.x4 = 1;
  };
}

currently it will be packed into this:

{
  a = {
    a = 1;
    f = {
      a = {
        x = {
          x2 = {
            x3 = {
              x4 = 1;
            };
          };
        };
      };
    };
  };
}

Sometimes, users might prefer a 'shallow pack' that only expands the first level of attributes.

@takeokunn

takeokunn commented Jan 5, 2026

Copy link
Copy Markdown
Collaborator Author

This PR ended up with too many changes, so I'll split it into a separate PR!!

Thanks for the review!!

I learned a lot about C++ again this time

@takeokunn takeokunn merged commit afd538e into main Jan 5, 2026
19 checks passed
@takeokunn takeokunn deleted the feature/code-actions-3 branch January 5, 2026 19:26
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