Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
takeokunn merged 7 commits into from
Jan 5, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
133c8f2
feat(code-action): add pack dotted path to nested set refactor action
takeokunn Jan 5, 2026
1c92d7c
feat(code-action): support bulk pack for sibling dotted attribute paths
takeokunn Jan 5, 2026
75de29c
docs: update comments for getSiblingCount to clarify segment handling
takeokunn Jan 5, 2026
2ce6c53
refactor(codeaction): extract and reuse quoteNixAttrKey for attribute…
takeokunn Jan 5, 2026
f72cf25
fix(ParseStrings): handle string escape sequences and unescape properly
takeokunn Jan 5, 2026
a595f1a
test(parser): add tests for string escape sequences and escaped quotes
takeokunn Jan 5, 2026
2c6773a
fix(test): split multi-request test to avoid race condition in UBSan CI
takeokunn Jan 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions libnixf/src/Parse/ParseStrings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,34 @@ std::shared_ptr<InterpolatedParts> 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<InterpolatedParts>(
Expand Down
29 changes: 29 additions & 0 deletions libnixf/test/Parse/ParseSimple.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Diagnostic> 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<ExprString *>(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<Diagnostic> 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<ExprString *>(Expr.get())->parts();
ASSERT_TRUE(Parts.isLiteral());
ASSERT_EQ(Parts.literal(), "has\"quote");
}

TEST(Parser, StringMissingDQuote) {
auto Src = R"("aaa)"sv;
std::vector<Diagnostic> Diags;
Expand Down
241 changes: 241 additions & 0 deletions nixd/lib/Controller/CodeAction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

#include <boost/asio/post.hpp>

#include <optional>
#include <set>

namespace nixd {
Expand Down Expand Up @@ -68,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) {
Expand Down Expand Up @@ -162,6 +182,226 @@ 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.
/// 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 0;

const std::string &FirstSeg = Names[0]->staticName();
const nixf::SemaAttrs &SA = ParentAttrs.sema();

auto It = SA.staticAttrs().find(FirstSeg);
if (It == SA.staticAttrs().end())
return 0;

const nixf::Attribute &Attr = It->second;

// 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<const nixf::ExprAttrs &>(*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
Out += quoteNixAttrKey(Key);
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<const nixf::ExprAttrs &>(*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<nixf::LexerCursorRange>
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;

const auto &SibBind = static_cast<const nixf::Binding &>(*Sibling);
const auto &SibNames = SibBind.path().names();

if (SibNames.empty() || !SibNames[0]->isStatic())
continue;

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 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<CodeAction> &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<const nixf::Binding &>(*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<const nixf::ExprAttrs &>(*AttrsNode);
if (ParentAttrs.isRecursive())
return;

const std::string &FirstSeg = Names[0]->staticName();
size_t SiblingCount = getSiblingCount(Bind, ParentAttrs);

if (SiblingCount == 0)
return; // Can't pack (dynamic attrs or other conflicts)

if (SiblingCount == 1) {
// Single binding - offer simple pack action
std::string NewText;
const std::string_view FirstName = Names[0]->src(Src);

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 {

@inclyc inclyc Jan 5, 2026

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;
}

// 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<const nixf::ExprAttrs &>(*Attr.value());

// Generate the packed text using SemaAttrs
std::string NewText;
NewText += quoteNixAttrKey(FirstSeg);
NewText += " = ";
generateNestedText(NestedAttrs.sema(), Src, NewText);
NewText += ";";

// Find the range covering all sibling bindings
const auto &ParentBinds = static_cast<const nixf::Binds &>(*BindsNode);
auto Range = findSiblingBindingsRange(Bind, ParentBinds, FirstSeg);
if (!Range)
return;

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

@inclyc inclyc Jan 5, 2026

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.

CodeAction::REFACTOR_REWRITE_KIND, FileURI, toLSPRange(Src, *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,
Expand Down Expand Up @@ -240,6 +480,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);
}
}

Expand Down
26 changes: 5 additions & 21 deletions nixd/tools/nixd/test/code-action/flatten-attrs-deep.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: ]
```
Expand Down
Loading