From 133c8f2c0d7e9618afeaf574e6c7cd752feda993 Mon Sep 17 00:00:00 2001 From: takeokunn Date: Mon, 5 Jan 2026 11:20:47 +0900 Subject: [PATCH 1/7] feat(code-action): add pack dotted path to nested set refactor action 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 --- nixd/lib/Controller/CodeAction.cpp | 115 ++++++++++++++++++ .../test/code-action/flatten-attrs-deep.md | 26 +--- .../nixd/test/code-action/pack-attrs-deep.md | 93 ++++++++++++++ .../test/code-action/pack-attrs-dynamic.md | 71 +++++++++++ .../nixd/test/code-action/pack-attrs-empty.md | 71 +++++++++++ .../test/code-action/pack-attrs-quoted.md | 93 ++++++++++++++ .../nixd/test/code-action/pack-attrs-rec.md | 70 +++++++++++ .../test/code-action/pack-attrs-siblings.md | 70 +++++++++++ .../test/code-action/pack-attrs-single.md | 70 +++++++++++ .../tools/nixd/test/code-action/pack-attrs.md | 93 ++++++++++++++ 10 files changed, 751 insertions(+), 21 deletions(-) create mode 100644 nixd/tools/nixd/test/code-action/pack-attrs-deep.md create mode 100644 nixd/tools/nixd/test/code-action/pack-attrs-dynamic.md create mode 100644 nixd/tools/nixd/test/code-action/pack-attrs-empty.md create mode 100644 nixd/tools/nixd/test/code-action/pack-attrs-quoted.md create mode 100644 nixd/tools/nixd/test/code-action/pack-attrs-rec.md create mode 100644 nixd/tools/nixd/test/code-action/pack-attrs-siblings.md create mode 100644 nixd/tools/nixd/test/code-action/pack-attrs-single.md create mode 100644 nixd/tools/nixd/test/code-action/pack-attrs.md diff --git a/nixd/lib/Controller/CodeAction.cpp b/nixd/lib/Controller/CodeAction.cpp index 2a576cd16..fd59b55fa 100644 --- a/nixd/lib/Controller/CodeAction.cpp +++ b/nixd/lib/Controller/CodeAction.cpp @@ -162,6 +162,120 @@ void addFlattenAttrsAction(const nixf::Node &N, FileURI, toLSPRange(Src, Bind.range()), std::move(NewText))); } +/// \brief Check if sibling bindings share the same first path segment. +/// Used to block Pack action when conflicts would occur. +bool hasConflictingSiblings(const nixf::Binding &Bind, + const nixf::ParentMapAnalysis &PM) { + // Get the first segment of this binding's path + const auto &Names = Bind.path().names(); + if (Names.empty() || !Names[0]->isStatic()) + return true; // Can't determine, block action + + const std::string &FirstSeg = Names[0]->staticName(); + + // Find parent Binds node + const nixf::Node *Parent = PM.query(Bind); + if (!Parent || Parent->kind() != nixf::Node::NK_Binds) + return true; // Can't find parent, block action + + const auto &ParentBinds = static_cast(*Parent); + + // Check all sibling bindings for conflicts + for (const auto &Sibling : ParentBinds.bindings()) { + if (Sibling.get() == &Bind) + continue; // Skip self + + if (Sibling->kind() != nixf::Node::NK_Binding) + continue; // Skip Inherit nodes + + const auto &SibBind = static_cast(*Sibling); + const auto &SibNames = SibBind.path().names(); + + if (SibNames.empty()) + continue; + + // Check if first segment matches + if (SibNames[0]->isStatic() && SibNames[0]->staticName() == FirstSeg) + return true; // Conflict found + } + + return false; +} + +/// \brief Add pack action for dotted attribute paths. +/// Transforms: { foo.bar = 1; } -> { foo = { bar = 1; }; } +void addPackAttrsAction(const nixf::Node &N, const nixf::ParentMapAnalysis &PM, + const std::string &FileURI, llvm::StringRef Src, + std::vector &Actions) { + // Find if we're inside a Binding + const nixf::Node *BindingNode = PM.upTo(N, nixf::Node::NK_Binding); + if (!BindingNode) + return; + + const auto &Bind = static_cast(*BindingNode); + const auto &Names = Bind.path().names(); + + // Must have at least 2 path segments (e.g., foo.bar) + if (Names.size() < 2) + return; + + // All path segments must be static + for (const auto &Name : Names) { + if (!Name->isStatic()) + return; + } + + // Check parent ExprAttrs is not recursive + const nixf::Node *BindsNode = PM.query(Bind); + if (!BindsNode || BindsNode->kind() != nixf::Node::NK_Binds) + return; + + const nixf::Node *AttrsNode = PM.query(*BindsNode); + if (!AttrsNode || AttrsNode->kind() != nixf::Node::NK_ExprAttrs) + return; + + const auto &ParentAttrs = static_cast(*AttrsNode); + if (ParentAttrs.isRecursive()) + return; + + // Check for sibling conflicts + if (hasConflictingSiblings(Bind, PM)) + return; + + // Build the packed text: first = { rest = value; } + std::string NewText; + + // First segment + const std::string_view FirstName = Names[0]->src(Src); + + // Pre-allocate to reduce reallocations + size_t ValueSize = Bind.value() ? Bind.value()->src(Src).size() : 0; + NewText.reserve(FirstName.size() + Bind.path().src(Src).size() + ValueSize + + 15); // 15 for " = { ", " = ", "; };" + NewText += FirstName; + NewText += " = { "; + + // Remaining path segments (from second name to end) + // Get source from second name start to path end + const nixf::LexerCursor RestStart = Names[1]->range().lCur(); + const nixf::LexerCursor RestEnd = Bind.path().range().rCur(); + std::string_view RestPath = + Src.substr(RestStart.offset(), RestEnd.offset() - RestStart.offset()); + + NewText += RestPath; + NewText += " = "; + + // Value + if (Bind.value()) { + NewText += Bind.value()->src(Src); + } + NewText += "; };"; + + Actions.emplace_back(createSingleEditAction( + "Pack dotted path to nested set", CodeAction::REFACTOR_REWRITE_KIND, + FileURI, toLSPRange(Src, Bind.range()), std::move(NewText))); +} + /// \brief Add refactoring code actions for attribute names (quote/unquote). void addAttrNameActions(const nixf::Node &N, const nixf::ParentMapAnalysis &PM, const std::string &FileURI, llvm::StringRef Src, @@ -240,6 +354,7 @@ void Controller::onCodeAction(const lspserver::CodeActionParams &Params, addAttrNameActions(*N, *TU->parentMap(), FileURI, TU->src(), Actions); addFlattenAttrsAction(*N, *TU->parentMap(), FileURI, TU->src(), Actions); + addPackAttrsAction(*N, *TU->parentMap(), FileURI, TU->src(), Actions); } } diff --git a/nixd/tools/nixd/test/code-action/flatten-attrs-deep.md b/nixd/tools/nixd/test/code-action/flatten-attrs-deep.md index 7d45ae6c7..5a9f30bc0 100644 --- a/nixd/tools/nixd/test/code-action/flatten-attrs-deep.md +++ b/nixd/tools/nixd/test/code-action/flatten-attrs-deep.md @@ -57,33 +57,17 @@ Test that flatten action works with already-dotted outer paths. ``` No Quote action since "a.b" is a path, not a simple identifier. +Both Flatten and Pack actions are offered since "a.b" is a dotted path with nested value. ``` CHECK: "id": 2, CHECK-NEXT: "jsonrpc": "2.0", CHECK-NEXT: "result": [ CHECK-NEXT: { -CHECK-NEXT: "edit": { -CHECK-NEXT: "changes": { -CHECK-NEXT: "file:///flatten-attrs-deep.nix": [ -CHECK-NEXT: { -CHECK-NEXT: "newText": "a.b.c = 1;", -CHECK-NEXT: "range": { -CHECK-NEXT: "end": { -CHECK-NEXT: "character": 19, -CHECK-NEXT: "line": 0 -CHECK-NEXT: }, -CHECK-NEXT: "start": { -CHECK-NEXT: "character": 2, -CHECK-NEXT: "line": 0 -CHECK-NEXT: } -CHECK-NEXT: } -CHECK-NEXT: } -CHECK-NEXT: ] -CHECK-NEXT: } -CHECK-NEXT: }, -CHECK-NEXT: "kind": "refactor.rewrite", -CHECK-NEXT: "title": "Flatten nested attribute set" +CHECK: "title": "Flatten nested attribute set" +CHECK-NEXT: }, +CHECK-NEXT: { +CHECK: "title": "Pack dotted path to nested set" CHECK-NEXT: } CHECK-NEXT: ] ``` diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-deep.md b/nixd/tools/nixd/test/code-action/pack-attrs-deep.md new file mode 100644 index 000000000..da7df13e9 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/pack-attrs-deep.md @@ -0,0 +1,93 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that Pack action packs only one level for deep dotted paths. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///pack-attrs-deep.nix +{ a.b.c = 1; } +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-deep.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":2 + }, + "end":{ + "line":0, + "character":7 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Pack should convert only one level: a.b.c -> a = { b.c = 1; } + +``` + CHECK: "id": 2, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [ +CHECK-NEXT: { +CHECK-NEXT: "edit": { +CHECK-NEXT: "changes": { +CHECK-NEXT: "file:///pack-attrs-deep.nix": [ +CHECK-NEXT: { +CHECK-NEXT: "newText": "a = { b.c = 1; };", +CHECK-NEXT: "range": { +CHECK-NEXT: "end": { +CHECK-NEXT: "character": 12, +CHECK-NEXT: "line": 0 +CHECK-NEXT: }, +CHECK-NEXT: "start": { +CHECK-NEXT: "character": 2, +CHECK-NEXT: "line": 0 +CHECK-NEXT: } +CHECK-NEXT: } +CHECK-NEXT: } +CHECK-NEXT: ] +CHECK-NEXT: } +CHECK-NEXT: }, +CHECK-NEXT: "kind": "refactor.rewrite", +CHECK-NEXT: "title": "Pack dotted path to nested set" +CHECK-NEXT: } +CHECK-NEXT: ] +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-dynamic.md b/nixd/tools/nixd/test/code-action/pack-attrs-dynamic.md new file mode 100644 index 000000000..8b058b46c --- /dev/null +++ b/nixd/tools/nixd/test/code-action/pack-attrs-dynamic.md @@ -0,0 +1,71 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that Pack action is NOT offered for dynamic attribute paths. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///pack-attrs-dynamic.nix +{ foo.${bar} = 1; } +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-dynamic.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":2 + }, + "end":{ + "line":0, + "character":5 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +No Pack action should be offered for paths with dynamic interpolation. +Quote action may still be offered for the static "foo" segment. + +``` + CHECK: "id": 2, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [ +CHECK-NOT: "Pack dotted path to nested set" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-empty.md b/nixd/tools/nixd/test/code-action/pack-attrs-empty.md new file mode 100644 index 000000000..a08acbe53 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/pack-attrs-empty.md @@ -0,0 +1,71 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that Pack action works correctly with empty attribute set values. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///pack-attrs-empty.nix +{ foo.bar = { }; } +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-empty.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":2 + }, + "end":{ + "line":0, + "character":9 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Pack action should be offered even when the value is an empty attribute set. +Both Pack and Flatten actions are available since the value is also a flattenable attrset. + +``` + CHECK: "id": 2, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [ +CHECK: "title": "Pack dotted path to nested set" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-quoted.md b/nixd/tools/nixd/test/code-action/pack-attrs-quoted.md new file mode 100644 index 000000000..bcaaed286 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/pack-attrs-quoted.md @@ -0,0 +1,93 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that Pack action preserves quoted attribute names. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///pack-attrs-quoted.nix +{ "foo-bar".baz = 1; } +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-quoted.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":2 + }, + "end":{ + "line":0, + "character":14 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Pack action should preserve the quoted first segment. + +``` + CHECK: "id": 2, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [ +CHECK-NEXT: { +CHECK-NEXT: "edit": { +CHECK-NEXT: "changes": { +CHECK-NEXT: "file:///pack-attrs-quoted.nix": [ +CHECK-NEXT: { +CHECK-NEXT: "newText": "\"foo-bar\" = { baz = 1; };", +CHECK-NEXT: "range": { +CHECK-NEXT: "end": { +CHECK-NEXT: "character": 20, +CHECK-NEXT: "line": 0 +CHECK-NEXT: }, +CHECK-NEXT: "start": { +CHECK-NEXT: "character": 2, +CHECK-NEXT: "line": 0 +CHECK-NEXT: } +CHECK-NEXT: } +CHECK-NEXT: } +CHECK-NEXT: ] +CHECK-NEXT: } +CHECK-NEXT: }, +CHECK-NEXT: "kind": "refactor.rewrite", +CHECK-NEXT: "title": "Pack dotted path to nested set" +CHECK-NEXT: } +CHECK-NEXT: ] +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-rec.md b/nixd/tools/nixd/test/code-action/pack-attrs-rec.md new file mode 100644 index 000000000..40733a9b0 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/pack-attrs-rec.md @@ -0,0 +1,70 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that Pack action is NOT offered inside recursive attribute sets. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///pack-attrs-rec.nix +rec { foo.bar = 1; } +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-rec.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":6 + }, + "end":{ + "line":0, + "character":13 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +No Pack action should be offered in recursive attribute sets. + +``` + CHECK: "id": 2, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [] +CHECK-NOT: "Pack dotted path to nested set" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-siblings.md b/nixd/tools/nixd/test/code-action/pack-attrs-siblings.md new file mode 100644 index 000000000..e63895678 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/pack-attrs-siblings.md @@ -0,0 +1,70 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that Pack action is NOT offered when sibling bindings share the same prefix. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///pack-attrs-siblings.nix +{ foo.bar = 1; foo.baz = 2; } +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-siblings.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":2 + }, + "end":{ + "line":0, + "character":9 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +No Pack action should be offered when siblings share the same prefix (foo). + +``` + CHECK: "id": 2, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [] +CHECK-NOT: "Pack dotted path to nested set" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-single.md b/nixd/tools/nixd/test/code-action/pack-attrs-single.md new file mode 100644 index 000000000..ac5609c32 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/pack-attrs-single.md @@ -0,0 +1,70 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that Pack action is NOT offered for single-segment paths. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///pack-attrs-single.nix +{ foo = 1; } +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-single.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":2 + }, + "end":{ + "line":0, + "character":5 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +No Pack action should be offered for single-segment paths (nothing to pack). + +``` + CHECK: "id": 2, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [ +CHECK-NOT: "Pack dotted path to nested set" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/pack-attrs.md b/nixd/tools/nixd/test/code-action/pack-attrs.md new file mode 100644 index 000000000..64bb2e615 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/pack-attrs.md @@ -0,0 +1,93 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that "Pack dotted path to nested set" action is offered for dotted attribute paths. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///pack-attrs.nix +{ foo.bar = 1; } +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":2 + }, + "end":{ + "line":0, + "character":9 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +The Pack action should be offered for dotted path "foo.bar". + +``` + CHECK: "id": 2, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [ +CHECK-NEXT: { +CHECK-NEXT: "edit": { +CHECK-NEXT: "changes": { +CHECK-NEXT: "file:///pack-attrs.nix": [ +CHECK-NEXT: { +CHECK-NEXT: "newText": "foo = { bar = 1; };", +CHECK-NEXT: "range": { +CHECK-NEXT: "end": { +CHECK-NEXT: "character": 14, +CHECK-NEXT: "line": 0 +CHECK-NEXT: }, +CHECK-NEXT: "start": { +CHECK-NEXT: "character": 2, +CHECK-NEXT: "line": 0 +CHECK-NEXT: } +CHECK-NEXT: } +CHECK-NEXT: } +CHECK-NEXT: ] +CHECK-NEXT: } +CHECK-NEXT: }, +CHECK-NEXT: "kind": "refactor.rewrite", +CHECK-NEXT: "title": "Pack dotted path to nested set" +CHECK-NEXT: } +CHECK-NEXT: ] +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` From 1c92d7c00d1f5a6063914f5c13416d3c35fb5dc7 Mon Sep 17 00:00:00 2001 From: takeokunn Date: Tue, 6 Jan 2026 02:54:37 +0900 Subject: [PATCH 2/7] feat(code-action): support bulk pack for sibling dotted attribute paths - 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 --- nixd/lib/Controller/CodeAction.cpp | 226 +++++++++++++----- .../code-action/pack-attrs-bulk-quoted.md | 94 ++++++++ .../nixd/test/code-action/pack-attrs-bulk.md | 93 +++++++ ...trs-siblings.md => pack-attrs-conflict.md} | 16 +- .../test/code-action/pack-attrs-escape.md | 73 ++++++ .../test/code-action/pack-attrs-multiline.md | 95 ++++++++ ...empty.md => pack-attrs-sibling-dynamic.md} | 19 +- .../pack-attrs-siblings-positive.md | 117 +++++++++ .../test/code-action/pack-attrs-values.md | 151 ++++++++++++ 9 files changed, 812 insertions(+), 72 deletions(-) create mode 100644 nixd/tools/nixd/test/code-action/pack-attrs-bulk-quoted.md create mode 100644 nixd/tools/nixd/test/code-action/pack-attrs-bulk.md rename nixd/tools/nixd/test/code-action/{pack-attrs-siblings.md => pack-attrs-conflict.md} (67%) create mode 100644 nixd/tools/nixd/test/code-action/pack-attrs-escape.md create mode 100644 nixd/tools/nixd/test/code-action/pack-attrs-multiline.md rename nixd/tools/nixd/test/code-action/{pack-attrs-empty.md => pack-attrs-sibling-dynamic.md} (62%) create mode 100644 nixd/tools/nixd/test/code-action/pack-attrs-siblings-positive.md create mode 100644 nixd/tools/nixd/test/code-action/pack-attrs-values.md diff --git a/nixd/lib/Controller/CodeAction.cpp b/nixd/lib/Controller/CodeAction.cpp index fd59b55fa..8a616d0ad 100644 --- a/nixd/lib/Controller/CodeAction.cpp +++ b/nixd/lib/Controller/CodeAction.cpp @@ -13,6 +13,7 @@ #include +#include #include namespace nixd { @@ -162,48 +163,131 @@ void addFlattenAttrsAction(const nixf::Node &N, FileURI, toLSPRange(Src, Bind.range()), std::move(NewText))); } -/// \brief Check if sibling bindings share the same first path segment. -/// Used to block Pack action when conflicts would occur. -bool hasConflictingSiblings(const nixf::Binding &Bind, - const nixf::ParentMapAnalysis &PM) { - // Get the first segment of this binding's path +/// \brief Get the number of sibling bindings sharing the same first path segment. +/// Uses SemaAttrs to count nested attributes for the first segment. +/// Returns 0 if the segment is not found or has conflicts (non-path binding). +size_t getSiblingCount(const nixf::Binding &Bind, + const nixf::ExprAttrs &ParentAttrs) { const auto &Names = Bind.path().names(); if (Names.empty() || !Names[0]->isStatic()) - return true; // Can't determine, block action + return 0; const std::string &FirstSeg = Names[0]->staticName(); + const nixf::SemaAttrs &SA = ParentAttrs.sema(); - // Find parent Binds node - const nixf::Node *Parent = PM.query(Bind); - if (!Parent || Parent->kind() != nixf::Node::NK_Binds) - return true; // Can't find parent, block action + auto It = SA.staticAttrs().find(FirstSeg); + if (It == SA.staticAttrs().end()) + return 0; - const auto &ParentBinds = static_cast(*Parent); + const nixf::Attribute &Attr = It->second; - // Check all sibling bindings for conflicts - for (const auto &Sibling : ParentBinds.bindings()) { - if (Sibling.get() == &Bind) - continue; // Skip self + // Check if value is a nested ExprAttrs (path was desugared) + if (!Attr.value() || Attr.value()->kind() != nixf::Node::NK_ExprAttrs) + return 0; // Non-ExprAttrs value = conflict with non-path binding + const auto &NestedAttrs = static_cast(*Attr.value()); + const nixf::SemaAttrs &NestedSA = NestedAttrs.sema(); + + // Return 0 if there are dynamic attrs (can't safely pack) + if (!NestedSA.dynamicAttrs().empty()) + return 0; + + return NestedSA.staticAttrs().size(); +} + +/// \brief Maximum recursion depth for nested text generation. +/// Prevents stack overflow on maliciously crafted deeply nested inputs. +constexpr size_t MAX_NESTED_DEPTH = 100; + +/// \brief Recursively generate nested attribute set text from SemaAttrs. +/// This produces the packed/nested form of attributes. +/// \param Depth Current recursion depth (for safety limit) +void generateNestedText(const nixf::SemaAttrs &SA, llvm::StringRef Src, + std::string &Out, size_t Depth = 0) { + // Safety check: prevent stack overflow from deeply nested structures + if (Depth > MAX_NESTED_DEPTH) { + Out += "{ /* max depth exceeded */ }"; + return; + } + + Out += "{ "; + bool First = true; + for (const auto &[Key, Attr] : SA.staticAttrs()) { + if (!First) + Out += " "; + First = false; + + // Output the key, quoting if necessary + if (isValidNixIdentifier(Key)) { + Out += Key; + } else { + Out += "\""; + // Escape special characters in string keys + for (char C : Key) { + if (C == '"' || C == '\\' || C == '$') + Out += '\\'; + Out += C; + } + Out += "\""; + } + Out += " = "; + + // Check if value is a nested ExprAttrs that needs recursive generation + if (Attr.value() && Attr.value()->kind() == nixf::Node::NK_ExprAttrs) { + const auto &NestedAttrs = + static_cast(*Attr.value()); + const nixf::SemaAttrs &NestedSA = NestedAttrs.sema(); + + // If all nested attrs come from dotted paths (no inherit, no dynamic), + // we can generate recursively + if (NestedSA.dynamicAttrs().empty() && !Attr.fromInherit()) { + generateNestedText(NestedSA, Src, Out, Depth + 1); + } else { + // Use original source text + Out += Attr.value()->src(Src); + } + } else if (Attr.value()) { + Out += Attr.value()->src(Src); + } + Out += ";"; + } + Out += " }"; +} + +/// \brief Find all sibling bindings that share the same first path segment. +/// Returns the range covering all such bindings, or nullopt if not applicable. +std::optional +findSiblingBindingsRange(const nixf::Binding &Bind, const nixf::Binds &Binds, + const std::string &FirstSeg) { + nixf::LexerCursor Start = Bind.range().lCur(); + nixf::LexerCursor End = Bind.range().rCur(); + + for (const auto &Sibling : Binds.bindings()) { if (Sibling->kind() != nixf::Node::NK_Binding) - continue; // Skip Inherit nodes + continue; const auto &SibBind = static_cast(*Sibling); const auto &SibNames = SibBind.path().names(); - if (SibNames.empty()) + if (SibNames.empty() || !SibNames[0]->isStatic()) continue; - // Check if first segment matches - if (SibNames[0]->isStatic() && SibNames[0]->staticName() == FirstSeg) - return true; // Conflict found + if (SibNames[0]->staticName() == FirstSeg) { + // Expand range to include this sibling + if (SibBind.range().lCur().offset() < Start.offset()) + Start = SibBind.range().lCur(); + if (SibBind.range().rCur().offset() > End.offset()) + End = SibBind.range().rCur(); + } } - return false; + return nixf::LexerCursorRange{Start, End}; } /// \brief Add pack action for dotted attribute paths. /// Transforms: { foo.bar = 1; } -> { foo = { bar = 1; }; } +/// Also offers bulk pack when siblings share a prefix: +/// { foo.bar = 1; foo.baz = 2; } -> { foo = { bar = 1; baz = 2; }; } void addPackAttrsAction(const nixf::Node &N, const nixf::ParentMapAnalysis &PM, const std::string &FileURI, llvm::StringRef Src, std::vector &Actions) { @@ -238,42 +322,76 @@ void addPackAttrsAction(const nixf::Node &N, const nixf::ParentMapAnalysis &PM, if (ParentAttrs.isRecursive()) return; - // Check for sibling conflicts - if (hasConflictingSiblings(Bind, PM)) - return; + const std::string &FirstSeg = Names[0]->staticName(); + size_t SiblingCount = getSiblingCount(Bind, ParentAttrs); - // Build the packed text: first = { rest = value; } - std::string NewText; + if (SiblingCount == 0) + return; // Can't pack (dynamic attrs or other conflicts) - // First segment - const std::string_view FirstName = Names[0]->src(Src); - - // Pre-allocate to reduce reallocations - size_t ValueSize = Bind.value() ? Bind.value()->src(Src).size() : 0; - NewText.reserve(FirstName.size() + Bind.path().src(Src).size() + ValueSize + - 15); // 15 for " = { ", " = ", "; };" - NewText += FirstName; - NewText += " = { "; - - // Remaining path segments (from second name to end) - // Get source from second name start to path end - const nixf::LexerCursor RestStart = Names[1]->range().lCur(); - const nixf::LexerCursor RestEnd = Bind.path().range().rCur(); - std::string_view RestPath = - Src.substr(RestStart.offset(), RestEnd.offset() - RestStart.offset()); - - NewText += RestPath; - NewText += " = "; - - // Value - if (Bind.value()) { - NewText += Bind.value()->src(Src); - } - NewText += "; };"; + if (SiblingCount == 1) { + // Single binding - offer simple pack action + std::string NewText; + const std::string_view FirstName = Names[0]->src(Src); - Actions.emplace_back(createSingleEditAction( - "Pack dotted path to nested set", CodeAction::REFACTOR_REWRITE_KIND, - FileURI, toLSPRange(Src, Bind.range()), std::move(NewText))); + size_t ValueSize = Bind.value() ? Bind.value()->src(Src).size() : 0; + NewText.reserve(FirstName.size() + Bind.path().src(Src).size() + ValueSize + + 15); + NewText += FirstName; + NewText += " = { "; + + const nixf::LexerCursor RestStart = Names[1]->range().lCur(); + const nixf::LexerCursor RestEnd = Bind.path().range().rCur(); + + // Safety check: ensure valid range to prevent integer underflow + if (RestEnd.offset() < RestStart.offset()) + return; + + std::string_view RestPath = + Src.substr(RestStart.offset(), RestEnd.offset() - RestStart.offset()); + + NewText += RestPath; + NewText += " = "; + + if (Bind.value()) { + NewText += Bind.value()->src(Src); + } + NewText += "; };"; + + Actions.emplace_back(createSingleEditAction( + "Pack dotted path to nested set", CodeAction::REFACTOR_REWRITE_KIND, + FileURI, toLSPRange(Src, Bind.range()), std::move(NewText))); + } else { + // Multiple siblings share the prefix - offer bulk pack action + const nixf::SemaAttrs &SA = ParentAttrs.sema(); + auto It = SA.staticAttrs().find(FirstSeg); + if (It == SA.staticAttrs().end()) + return; + + const nixf::Attribute &Attr = It->second; + if (!Attr.value() || Attr.value()->kind() != nixf::Node::NK_ExprAttrs) + return; + + const auto &NestedAttrs = + static_cast(*Attr.value()); + + // Generate the packed text using SemaAttrs + std::string NewText; + NewText += FirstSeg; + NewText += " = "; + generateNestedText(NestedAttrs.sema(), Src, NewText); + NewText += ";"; + + // Find the range covering all sibling bindings + const auto &ParentBinds = static_cast(*BindsNode); + auto Range = findSiblingBindingsRange(Bind, ParentBinds, FirstSeg); + if (!Range) + return; + + Actions.emplace_back(createSingleEditAction( + "Pack all '" + FirstSeg + "' bindings to nested set", + CodeAction::REFACTOR_REWRITE_KIND, FileURI, toLSPRange(Src, *Range), + std::move(NewText))); + } } /// \brief Add refactoring code actions for attribute names (quote/unquote). diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-bulk-quoted.md b/nixd/tools/nixd/test/code-action/pack-attrs-bulk-quoted.md new file mode 100644 index 000000000..ca36f70b5 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/pack-attrs-bulk-quoted.md @@ -0,0 +1,94 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that bulk Pack action correctly handles keys requiring quotes. +The key "1foo" starts with a digit, requiring quotes in the output. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///pack-attrs-bulk-quoted.nix +{ "1foo".a = 1; "1foo".b = 2; } +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-bulk-quoted.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":2 + }, + "end":{ + "line":0, + "character":9 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Bulk Pack action should output quoted key "1foo" since it starts with a digit. + +``` + CHECK: "id": 2, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [ +CHECK-NEXT: { +CHECK-NEXT: "edit": { +CHECK-NEXT: "changes": { +CHECK-NEXT: "file:///pack-attrs-bulk-quoted.nix": [ +CHECK-NEXT: { +CHECK-NEXT: "newText": "\"1foo\" = { a = 1; b = 2; };", +CHECK-NEXT: "range": { +CHECK-NEXT: "end": { +CHECK-NEXT: "character": 29, +CHECK-NEXT: "line": 0 +CHECK-NEXT: }, +CHECK-NEXT: "start": { +CHECK-NEXT: "character": 2, +CHECK-NEXT: "line": 0 +CHECK-NEXT: } +CHECK-NEXT: } +CHECK-NEXT: } +CHECK-NEXT: ] +CHECK-NEXT: } +CHECK-NEXT: }, +CHECK-NEXT: "kind": "refactor.rewrite", +CHECK-NEXT: "title": "Pack all '1foo' bindings to nested set" +CHECK-NEXT: } +CHECK-NEXT: ] +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-bulk.md b/nixd/tools/nixd/test/code-action/pack-attrs-bulk.md new file mode 100644 index 000000000..137932767 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/pack-attrs-bulk.md @@ -0,0 +1,93 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that bulk Pack action is offered when multiple sibling bindings share the same prefix. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///pack-attrs-bulk.nix +{ foo.bar = 1; foo.baz = 2; } +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-bulk.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":2 + }, + "end":{ + "line":0, + "character":9 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Bulk Pack action should combine all 'foo' bindings into a single nested set. + +``` + CHECK: "id": 2, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [ +CHECK-NEXT: { +CHECK-NEXT: "edit": { +CHECK-NEXT: "changes": { +CHECK-NEXT: "file:///pack-attrs-bulk.nix": [ +CHECK-NEXT: { +CHECK-NEXT: "newText": "foo = { bar = 1; baz = 2; };", +CHECK-NEXT: "range": { +CHECK-NEXT: "end": { +CHECK-NEXT: "character": 27, +CHECK-NEXT: "line": 0 +CHECK-NEXT: }, +CHECK-NEXT: "start": { +CHECK-NEXT: "character": 2, +CHECK-NEXT: "line": 0 +CHECK-NEXT: } +CHECK-NEXT: } +CHECK-NEXT: } +CHECK-NEXT: ] +CHECK-NEXT: } +CHECK-NEXT: }, +CHECK-NEXT: "kind": "refactor.rewrite", +CHECK-NEXT: "title": "Pack all 'foo' bindings to nested set" +CHECK-NEXT: } +CHECK-NEXT: ] +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-siblings.md b/nixd/tools/nixd/test/code-action/pack-attrs-conflict.md similarity index 67% rename from nixd/tools/nixd/test/code-action/pack-attrs-siblings.md rename to nixd/tools/nixd/test/code-action/pack-attrs-conflict.md index e63895678..7d1bf8a8f 100644 --- a/nixd/tools/nixd/test/code-action/pack-attrs-siblings.md +++ b/nixd/tools/nixd/test/code-action/pack-attrs-conflict.md @@ -1,6 +1,6 @@ # RUN: nixd --lit-test < %s | FileCheck %s -Test that Pack action is NOT offered when sibling bindings share the same prefix. +Test that Pack action is NOT offered when a non-path binding conflicts with dotted path. <-- initialize(0) @@ -22,8 +22,8 @@ Test that Pack action is NOT offered when sibling bindings share the same prefix <-- textDocument/didOpen -```nix file:///pack-attrs-siblings.nix -{ foo.bar = 1; foo.baz = 2; } +```nix file:///pack-attrs-conflict.nix +{ foo = 1; foo.bar = 2; } ``` <-- textDocument/codeAction(2) @@ -36,16 +36,16 @@ Test that Pack action is NOT offered when sibling bindings share the same prefix "method":"textDocument/codeAction", "params":{ "textDocument":{ - "uri":"file:///pack-attrs-siblings.nix" + "uri":"file:///pack-attrs-conflict.nix" }, "range":{ "start":{ "line": 0, - "character":2 + "character":11 }, "end":{ "line":0, - "character":9 + "character":18 } }, "context":{ @@ -56,13 +56,13 @@ Test that Pack action is NOT offered when sibling bindings share the same prefix } ``` -No Pack action should be offered when siblings share the same prefix (foo). +No Pack action should be offered when a non-path binding (foo = 1) conflicts with dotted path (foo.bar). ``` CHECK: "id": 2, CHECK-NEXT: "jsonrpc": "2.0", CHECK-NEXT: "result": [] -CHECK-NOT: "Pack dotted path to nested set" +CHECK-NOT: "Pack" ``` ```json diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-escape.md b/nixd/tools/nixd/test/code-action/pack-attrs-escape.md new file mode 100644 index 000000000..50ca63a70 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/pack-attrs-escape.md @@ -0,0 +1,73 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that Pack action correctly escapes special characters in quoted attribute keys. +Tests escaping of: double quotes ("), backslashes (\), and dollar signs ($). + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +Bulk pack with keys containing special characters that need escaping. + +```nix file:///pack-attrs-escape.nix +{ "has\"quote".a = 1; "has\"quote".b = 2; } +``` + +<-- textDocument/codeAction(2) + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-escape.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":2 + }, + "end":{ + "line":0, + "character":17 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Bulk Pack action should correctly escape the double quote in the key. + +``` + CHECK: "id": 2, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [ +CHECK: "newText": "\"has\\\"quote\" = { a = 1; b = 2; };" +CHECK: "title": "Pack all 'has\"quote' bindings to nested set" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-multiline.md b/nixd/tools/nixd/test/code-action/pack-attrs-multiline.md new file mode 100644 index 000000000..d131275c3 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/pack-attrs-multiline.md @@ -0,0 +1,95 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that Pack action works correctly with multiline attribute sets. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///pack-attrs-multiline.nix +{ + foo.bar = 1; +} +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-multiline.nix" + }, + "range":{ + "start":{ + "line": 1, + "character":2 + }, + "end":{ + "line":1, + "character":9 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Pack action should work on multiline format, transforming the binding on line 1. + +``` + CHECK: "id": 2, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [ +CHECK-NEXT: { +CHECK-NEXT: "edit": { +CHECK-NEXT: "changes": { +CHECK-NEXT: "file:///pack-attrs-multiline.nix": [ +CHECK-NEXT: { +CHECK-NEXT: "newText": "foo = { bar = 1; };", +CHECK-NEXT: "range": { +CHECK-NEXT: "end": { +CHECK-NEXT: "character": 14, +CHECK-NEXT: "line": 1 +CHECK-NEXT: }, +CHECK-NEXT: "start": { +CHECK-NEXT: "character": 2, +CHECK-NEXT: "line": 1 +CHECK-NEXT: } +CHECK-NEXT: } +CHECK-NEXT: } +CHECK-NEXT: ] +CHECK-NEXT: } +CHECK-NEXT: }, +CHECK-NEXT: "kind": "refactor.rewrite", +CHECK-NEXT: "title": "Pack dotted path to nested set" +CHECK-NEXT: } +CHECK-NEXT: ] +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-empty.md b/nixd/tools/nixd/test/code-action/pack-attrs-sibling-dynamic.md similarity index 62% rename from nixd/tools/nixd/test/code-action/pack-attrs-empty.md rename to nixd/tools/nixd/test/code-action/pack-attrs-sibling-dynamic.md index a08acbe53..0fe8e18ae 100644 --- a/nixd/tools/nixd/test/code-action/pack-attrs-empty.md +++ b/nixd/tools/nixd/test/code-action/pack-attrs-sibling-dynamic.md @@ -1,6 +1,6 @@ # RUN: nixd --lit-test < %s | FileCheck %s -Test that Pack action works correctly with empty attribute set values. +Test that Pack action is NOT offered when sibling bindings contain dynamic attributes. <-- initialize(0) @@ -22,8 +22,8 @@ Test that Pack action works correctly with empty attribute set values. <-- textDocument/didOpen -```nix file:///pack-attrs-empty.nix -{ foo.bar = { }; } +```nix file:///pack-attrs-sibling-dynamic.nix +let x = "baz"; in { foo.bar = 1; foo.${x} = 2; } ``` <-- textDocument/codeAction(2) @@ -36,16 +36,16 @@ Test that Pack action works correctly with empty attribute set values. "method":"textDocument/codeAction", "params":{ "textDocument":{ - "uri":"file:///pack-attrs-empty.nix" + "uri":"file:///pack-attrs-sibling-dynamic.nix" }, "range":{ "start":{ "line": 0, - "character":2 + "character":20 }, "end":{ "line":0, - "character":9 + "character":27 } }, "context":{ @@ -56,14 +56,13 @@ Test that Pack action works correctly with empty attribute set values. } ``` -Pack action should be offered even when the value is an empty attribute set. -Both Pack and Flatten actions are available since the value is also a flattenable attrset. +No Pack action should be offered when sibling has dynamic attribute (foo.${x}). ``` CHECK: "id": 2, CHECK-NEXT: "jsonrpc": "2.0", -CHECK-NEXT: "result": [ -CHECK: "title": "Pack dotted path to nested set" +CHECK-NEXT: "result": [] +CHECK-NOT: "Pack" ``` ```json diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-siblings-positive.md b/nixd/tools/nixd/test/code-action/pack-attrs-siblings-positive.md new file mode 100644 index 000000000..48adce4dd --- /dev/null +++ b/nixd/tools/nixd/test/code-action/pack-attrs-siblings-positive.md @@ -0,0 +1,117 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that Pack action works correctly with various non-conflicting sibling bindings: +- Sibling with inherit (no path conflict) +- Siblings with different prefixes (no path conflict) + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen (inherit sibling) + +```nix file:///pack-attrs-siblings-positive-1.nix +{ foo.bar = 1; inherit baz; } +``` + +<-- textDocument/codeAction(2) - Test with inherit sibling + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-siblings-positive-1.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":2 + }, + "end":{ + "line":0, + "character":9 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Pack action should be offered when sibling is inherit (no path conflict). + +``` + CHECK: "id": 2, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [ +CHECK: "newText": "foo = { bar = 1; };" +CHECK: "title": "Pack dotted path to nested set" +``` + +<-- textDocument/didOpen (different prefix siblings) + +```nix file:///pack-attrs-siblings-positive-2.nix +{ foo.bar = 1; baz.qux = 2; } +``` + +<-- textDocument/codeAction(3) - Test with different prefix siblings + +```json +{ + "jsonrpc":"2.0", + "id":3, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-siblings-positive-2.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":2 + }, + "end":{ + "line":0, + "character":9 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Pack action should transform only the selected foo.bar binding. + +``` + CHECK: "id": 3, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [ +CHECK: "newText": "foo = { bar = 1; };" +CHECK: "title": "Pack dotted path to nested set" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-values.md b/nixd/tools/nixd/test/code-action/pack-attrs-values.md new file mode 100644 index 000000000..5e31964d4 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/pack-attrs-values.md @@ -0,0 +1,151 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that Pack action correctly handles various value types: +- Empty attribute sets +- Complex expressions (let) +- Nested attribute sets + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen (empty value) + +```nix file:///pack-attrs-values.nix +{ empty.value = { }; complex.value = let x = 1; in x + 1; nested.value = { a = 1; b.c = 2; }; } +``` + +<-- textDocument/codeAction(2) - Test empty attribute set value + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-values.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":2 + }, + "end":{ + "line":0, + "character":13 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Pack action should be offered for empty attribute set values. + +``` + CHECK: "id": 2, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [ +CHECK: "newText": "empty = { value = { }; };" +CHECK: "title": "Pack dotted path to nested set" +``` + +<-- textDocument/codeAction(3) - Test complex let expression value + +```json +{ + "jsonrpc":"2.0", + "id":3, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-values.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":21 + }, + "end":{ + "line":0, + "character":34 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Pack action should preserve complex let expressions. + +``` + CHECK: "id": 3, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [ +CHECK: "newText": "complex = { value = let x = 1; in x + 1; };" +CHECK: "title": "Pack dotted path to nested set" +``` + +<-- textDocument/codeAction(4) - Test nested attribute set value + +```json +{ + "jsonrpc":"2.0", + "id":4, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-values.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":58 + }, + "end":{ + "line":0, + "character":70 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Pack action should preserve nested attribute set values exactly. + +``` + CHECK: "id": 4, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [ +CHECK: "newText": "nested = { value = { a = 1; b.c = 2; }; };" +CHECK: "title": "Pack dotted path to nested set" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` From 75de29c999a14050108f577a33a4b20ce957fadb Mon Sep 17 00:00:00 2001 From: takeokunn Date: Tue, 6 Jan 2026 03:01:57 +0900 Subject: [PATCH 3/7] docs: update comments for getSiblingCount to clarify segment handling --- nixd/lib/Controller/CodeAction.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nixd/lib/Controller/CodeAction.cpp b/nixd/lib/Controller/CodeAction.cpp index 8a616d0ad..361174abd 100644 --- a/nixd/lib/Controller/CodeAction.cpp +++ b/nixd/lib/Controller/CodeAction.cpp @@ -163,8 +163,8 @@ void addFlattenAttrsAction(const nixf::Node &N, FileURI, toLSPRange(Src, Bind.range()), std::move(NewText))); } -/// \brief Get the number of sibling bindings sharing the same first path segment. -/// Uses SemaAttrs to count nested attributes for the first segment. +/// \brief Get the number of sibling bindings sharing the same first path +/// segment. Uses SemaAttrs to count nested attributes for the first segment. /// Returns 0 if the segment is not found or has conflicts (non-path binding). size_t getSiblingCount(const nixf::Binding &Bind, const nixf::ExprAttrs &ParentAttrs) { From 2ce6c53db73458283dcc210727ffccbf6cd39cad Mon Sep 17 00:00:00 2001 From: takeokunn Date: Tue, 6 Jan 2026 03:15:33 +0900 Subject: [PATCH 4/7] refactor(codeaction): extract and reuse quoteNixAttrKey for attribute 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 --- nixd/lib/Controller/CodeAction.cpp | 34 ++++++++++++++++++------------ 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/nixd/lib/Controller/CodeAction.cpp b/nixd/lib/Controller/CodeAction.cpp index 361174abd..4fee86926 100644 --- a/nixd/lib/Controller/CodeAction.cpp +++ b/nixd/lib/Controller/CodeAction.cpp @@ -69,6 +69,25 @@ bool isValidNixIdentifier(const std::string &S) { return Keywords.find(S) == Keywords.end(); } +/// \brief Quote and escape a Nix attribute key if necessary. +/// Returns the key as-is if it's a valid identifier, otherwise quotes and +/// escapes special characters (", \, $). +std::string quoteNixAttrKey(const std::string &Key) { + if (isValidNixIdentifier(Key)) + return Key; + + std::string Result; + Result.reserve(Key.size() + 4); + Result += '"'; + for (char C : Key) { + if (C == '"' || C == '\\' || C == '$') + Result += '\\'; + Result += C; + } + Result += '"'; + return Result; +} + /// \brief Check if an ExprAttrs can be flattened (no rec, inherit, dynamic). /// Returns the Binds node if flattenable, nullptr otherwise. const nixf::Binds *getFlattenableBinds(const nixf::ExprAttrs &Attrs) { @@ -218,18 +237,7 @@ void generateNestedText(const nixf::SemaAttrs &SA, llvm::StringRef Src, First = false; // Output the key, quoting if necessary - if (isValidNixIdentifier(Key)) { - Out += Key; - } else { - Out += "\""; - // Escape special characters in string keys - for (char C : Key) { - if (C == '"' || C == '\\' || C == '$') - Out += '\\'; - Out += C; - } - Out += "\""; - } + Out += quoteNixAttrKey(Key); Out += " = "; // Check if value is a nested ExprAttrs that needs recursive generation @@ -376,7 +384,7 @@ void addPackAttrsAction(const nixf::Node &N, const nixf::ParentMapAnalysis &PM, // Generate the packed text using SemaAttrs std::string NewText; - NewText += FirstSeg; + NewText += quoteNixAttrKey(FirstSeg); NewText += " = "; generateNestedText(NestedAttrs.sema(), Src, NewText); NewText += ";"; From f72cf252381730c7e85a5c86c0e790db4e940fa2 Mon Sep 17 00:00:00 2001 From: takeokunn Date: Tue, 6 Jan 2026 03:27:22 +0900 Subject: [PATCH 5/7] fix(ParseStrings): handle string escape sequences and unescape properly - 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 --- libnixf/src/Parse/ParseStrings.cpp | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/libnixf/src/Parse/ParseStrings.cpp b/libnixf/src/Parse/ParseStrings.cpp index 691c2f48a..1ee5ddfbc 100644 --- a/libnixf/src/Parse/ParseStrings.cpp +++ b/libnixf/src/Parse/ParseStrings.cpp @@ -71,11 +71,34 @@ std::shared_ptr Parser::parseStringParts() { consume(); continue; } - case tok_string_escape: - // If this is a part of string, just push it. + case tok_string_escape: { + // Process escape sequence and add the unescaped character to Parts. + // The token view contains the full escape sequence (e.g., "\\n", "\\\"") + std::string_view EscapeSeq = Tok.view(); + std::string Unescaped; + if (EscapeSeq.size() >= 2) { + char Escaped = EscapeSeq[1]; + switch (Escaped) { + case 'n': + Unescaped = "\n"; + break; + case 'r': + Unescaped = "\r"; + break; + case 't': + Unescaped = "\t"; + break; + default: + // For \\, \", \$, and any other escape, just use the character itself + Unescaped = Escaped; + break; + } + } + if (!Unescaped.empty()) + Parts.emplace_back(std::move(Unescaped)); consume(); - // TODO: escape and emplace_back continue; + } default: assert(LastToken && "LastToken should be set in `parseString`"); return std::make_shared( From a595f1a2610d415f0c2061dd58543f9c654c671a Mon Sep 17 00:00:00 2001 From: takeokunn Date: Tue, 6 Jan 2026 03:32:21 +0900 Subject: [PATCH 6/7] test(parser): add tests for string escape sequences and escaped quotes - 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 --- libnixf/test/Parse/ParseSimple.cpp | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/libnixf/test/Parse/ParseSimple.cpp b/libnixf/test/Parse/ParseSimple.cpp index 148863a27..182f3237c 100644 --- a/libnixf/test/Parse/ParseSimple.cpp +++ b/libnixf/test/Parse/ParseSimple.cpp @@ -87,6 +87,35 @@ TEST(Parser, StringSimple) { ASSERT_TRUE(Expr->range().rCur().isAt(0, 5, 5)); } +TEST(Parser, StringEscapeSequences) { + // Test escape sequences: \n, \r, \t, \\, \", \$ + auto Src = R"("a\nb\rc\td\\e\"f\$g")"sv; + std::vector Diags; + auto Expr = nixf::parse(Src, Diags); + ASSERT_TRUE(Expr); + ASSERT_EQ(Expr->kind(), Node::NK_ExprString); + ASSERT_EQ(Diags.size(), 0); + + const auto &Parts = static_cast(Expr.get())->parts(); + ASSERT_TRUE(Parts.isLiteral()); + // Expected: a + \n + b + \r + c + \t + d + \ + e + " + f + $ + g + ASSERT_EQ(Parts.literal(), "a\nb\rc\td\\e\"f$g"); +} + +TEST(Parser, StringEscapeQuote) { + // Test that escaped quotes are properly parsed + auto Src = R"("has\"quote")"sv; + std::vector Diags; + auto Expr = nixf::parse(Src, Diags); + ASSERT_TRUE(Expr); + ASSERT_EQ(Expr->kind(), Node::NK_ExprString); + ASSERT_EQ(Diags.size(), 0); + + const auto &Parts = static_cast(Expr.get())->parts(); + ASSERT_TRUE(Parts.isLiteral()); + ASSERT_EQ(Parts.literal(), "has\"quote"); +} + TEST(Parser, StringMissingDQuote) { auto Src = R"("aaa)"sv; std::vector Diags; From 2c6773aa7130738e1eecfdc79223d860deb18a6b Mon Sep 17 00:00:00 2001 From: takeokunn Date: Tue, 6 Jan 2026 03:49:44 +0900 Subject: [PATCH 7/7] fix(test): split multi-request test to avoid race condition in UBSan CI 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 --- .../pack-attrs-sibling-different-prefix.md | 70 +++++++++++ .../code-action/pack-attrs-sibling-inherit.md | 70 +++++++++++ .../pack-attrs-siblings-positive.md | 117 ------------------ 3 files changed, 140 insertions(+), 117 deletions(-) create mode 100644 nixd/tools/nixd/test/code-action/pack-attrs-sibling-different-prefix.md create mode 100644 nixd/tools/nixd/test/code-action/pack-attrs-sibling-inherit.md delete mode 100644 nixd/tools/nixd/test/code-action/pack-attrs-siblings-positive.md diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-sibling-different-prefix.md b/nixd/tools/nixd/test/code-action/pack-attrs-sibling-different-prefix.md new file mode 100644 index 000000000..e64c74488 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/pack-attrs-sibling-different-prefix.md @@ -0,0 +1,70 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that Pack action transforms only the selected binding when sibling has different prefix. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///pack-attrs-sibling-different-prefix.nix +{ foo.bar = 1; baz.qux = 2; } +``` + +<-- textDocument/codeAction(2) + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-sibling-different-prefix.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":2 + }, + "end":{ + "line":0, + "character":9 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Pack action should transform only the selected foo.bar binding. + +``` + CHECK: "id": 2, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [ +CHECK: "newText": "foo = { bar = 1; };" +CHECK: "title": "Pack dotted path to nested set" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-sibling-inherit.md b/nixd/tools/nixd/test/code-action/pack-attrs-sibling-inherit.md new file mode 100644 index 000000000..97c13b2b7 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/pack-attrs-sibling-inherit.md @@ -0,0 +1,70 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that Pack action works correctly with inherit sibling binding (no path conflict). + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///pack-attrs-sibling-inherit.nix +{ foo.bar = 1; inherit baz; } +``` + +<-- textDocument/codeAction(2) + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///pack-attrs-sibling-inherit.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":2 + }, + "end":{ + "line":0, + "character":9 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Pack action should be offered when sibling is inherit (no path conflict). + +``` + CHECK: "id": 2, +CHECK-NEXT: "jsonrpc": "2.0", +CHECK-NEXT: "result": [ +CHECK: "newText": "foo = { bar = 1; };" +CHECK: "title": "Pack dotted path to nested set" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/pack-attrs-siblings-positive.md b/nixd/tools/nixd/test/code-action/pack-attrs-siblings-positive.md deleted file mode 100644 index 48adce4dd..000000000 --- a/nixd/tools/nixd/test/code-action/pack-attrs-siblings-positive.md +++ /dev/null @@ -1,117 +0,0 @@ -# RUN: nixd --lit-test < %s | FileCheck %s - -Test that Pack action works correctly with various non-conflicting sibling bindings: -- Sibling with inherit (no path conflict) -- Siblings with different prefixes (no path conflict) - -<-- initialize(0) - -```json -{ - "jsonrpc":"2.0", - "id":0, - "method":"initialize", - "params":{ - "processId":123, - "rootPath":"", - "capabilities":{ - }, - "trace":"off" - } -} -``` - - -<-- textDocument/didOpen (inherit sibling) - -```nix file:///pack-attrs-siblings-positive-1.nix -{ foo.bar = 1; inherit baz; } -``` - -<-- textDocument/codeAction(2) - Test with inherit sibling - -```json -{ - "jsonrpc":"2.0", - "id":2, - "method":"textDocument/codeAction", - "params":{ - "textDocument":{ - "uri":"file:///pack-attrs-siblings-positive-1.nix" - }, - "range":{ - "start":{ - "line": 0, - "character":2 - }, - "end":{ - "line":0, - "character":9 - } - }, - "context":{ - "diagnostics":[], - "triggerKind":2 - } - } -} -``` - -Pack action should be offered when sibling is inherit (no path conflict). - -``` - CHECK: "id": 2, -CHECK-NEXT: "jsonrpc": "2.0", -CHECK-NEXT: "result": [ -CHECK: "newText": "foo = { bar = 1; };" -CHECK: "title": "Pack dotted path to nested set" -``` - -<-- textDocument/didOpen (different prefix siblings) - -```nix file:///pack-attrs-siblings-positive-2.nix -{ foo.bar = 1; baz.qux = 2; } -``` - -<-- textDocument/codeAction(3) - Test with different prefix siblings - -```json -{ - "jsonrpc":"2.0", - "id":3, - "method":"textDocument/codeAction", - "params":{ - "textDocument":{ - "uri":"file:///pack-attrs-siblings-positive-2.nix" - }, - "range":{ - "start":{ - "line": 0, - "character":2 - }, - "end":{ - "line":0, - "character":9 - } - }, - "context":{ - "diagnostics":[], - "triggerKind":2 - } - } -} -``` - -Pack action should transform only the selected foo.bar binding. - -``` - CHECK: "id": 3, -CHECK-NEXT: "jsonrpc": "2.0", -CHECK-NEXT: "result": [ -CHECK: "newText": "foo = { bar = 1; };" -CHECK: "title": "Pack dotted path to nested set" -``` - -```json -{"jsonrpc":"2.0","method":"exit"} -```