From e7fef63ee30f968866294f375b20d461de52faf4 Mon Sep 17 00:00:00 2001 From: takeokunn Date: Sun, 11 Jan 2026 18:57:59 +0900 Subject: [PATCH 1/4] feat(code-action): add "with" to "let/inherit" refactor action - Introduce code action to convert `with` expressions to `let/inherit` - Add implementation and tests for various scenarios, including: - Attribute set bodies, lambda bodies, multiple and nested variables - Alphabetical sorting and correct handling of variable shadowing - Action only offered when cursor is on `with` and variables are used - Update build configuration to include new code action sources --- nixd/lib/Controller/CodeAction.cpp | 6 + nixd/lib/Controller/CodeActions/WithToLet.cpp | 128 ++++++++++++++++++ nixd/lib/Controller/CodeActions/WithToLet.h | 39 ++++++ nixd/lib/meson.build | 1 + .../with-to-let/with-to-let-attrset-body.md | 70 ++++++++++ .../with-to-let/with-to-let-basic.md | 71 ++++++++++ .../with-to-let/with-to-let-complex-source.md | 70 ++++++++++ .../with-to-let-cursor-not-on-with.md | 69 ++++++++++ .../with-to-let/with-to-let-lambda-body.md | 70 ++++++++++ .../with-to-let/with-to-let-multiple-vars.md | 70 ++++++++++ .../with-to-let/with-to-let-nested.md | 73 ++++++++++ .../with-to-let/with-to-let-shadowing.md | 77 +++++++++++ .../with-to-let/with-to-let-unused.md | 70 ++++++++++ 13 files changed, 814 insertions(+) create mode 100644 nixd/lib/Controller/CodeActions/WithToLet.cpp create mode 100644 nixd/lib/Controller/CodeActions/WithToLet.h create mode 100644 nixd/tools/nixd/test/code-action/with-to-let/with-to-let-attrset-body.md create mode 100644 nixd/tools/nixd/test/code-action/with-to-let/with-to-let-basic.md create mode 100644 nixd/tools/nixd/test/code-action/with-to-let/with-to-let-complex-source.md create mode 100644 nixd/tools/nixd/test/code-action/with-to-let/with-to-let-cursor-not-on-with.md create mode 100644 nixd/tools/nixd/test/code-action/with-to-let/with-to-let-lambda-body.md create mode 100644 nixd/tools/nixd/test/code-action/with-to-let/with-to-let-multiple-vars.md create mode 100644 nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested.md create mode 100644 nixd/tools/nixd/test/code-action/with-to-let/with-to-let-shadowing.md create mode 100644 nixd/tools/nixd/test/code-action/with-to-let/with-to-let-unused.md diff --git a/nixd/lib/Controller/CodeAction.cpp b/nixd/lib/Controller/CodeAction.cpp index 6d6fbb04e..6eecf076a 100644 --- a/nixd/lib/Controller/CodeAction.cpp +++ b/nixd/lib/Controller/CodeAction.cpp @@ -12,6 +12,7 @@ #include "CodeActions/JsonToNix.h" #include "CodeActions/NoogleDoc.h" #include "CodeActions/PackAttrs.h" +#include "CodeActions/WithToLet.h" #include "nixd/Controller/Controller.h" @@ -79,6 +80,11 @@ void Controller::onCodeAction(const lspserver::CodeActionParams &Params, addExtractToFileAction(*N, *TU->parentMap(), *TU->variableLookup(), FileURI, TU->src(), Actions); } + // Add with-to-let action (requires VLA for variable tracking) + if (TU->variableLookup()) { + addWithToLetAction(*N, *TU->parentMap(), *TU->variableLookup(), + FileURI, TU->src(), Actions); + } } } diff --git a/nixd/lib/Controller/CodeActions/WithToLet.cpp b/nixd/lib/Controller/CodeActions/WithToLet.cpp new file mode 100644 index 000000000..1fa946a04 --- /dev/null +++ b/nixd/lib/Controller/CodeActions/WithToLet.cpp @@ -0,0 +1,128 @@ +/// \file +/// \brief Implementation of with-to-let code action. + +#include "WithToLet.h" +#include "Utils.h" + +#include "../Convert.h" + +#include + +#include + +namespace nixd { + +namespace { + +/// \brief Check if cursor position is on the `with` keyword. +bool isCursorOnWithKeyword(const nixf::ExprWith &With, const nixf::Node &N) { + // The node N should be or contain the `with` keyword + const auto &KwWith = With.kwWith(); + auto KwRange = KwWith.range(); + auto NRange = N.range(); + + // Check if N's range overlaps with the `with` keyword range + return NRange.lCur().offset() <= KwRange.rCur().offset() && + NRange.rCur().offset() >= KwRange.lCur().offset(); +} + +/// \brief Collect unique variable names used from the with scope. +/// Returns empty set if the Definition cannot be obtained. +std::set +collectWithVariables(const nixf::ExprWith &With, + const nixf::VariableLookupAnalysis &VLA) { + std::set VarNames; + + // Get the Definition for the `with` keyword + const nixf::Definition *Def = VLA.toDef(With.kwWith()); + if (!Def) + return VarNames; + + // Extract unique variable names from all uses + for (const nixf::ExprVar *Var : Def->uses()) { + if (Var) + VarNames.insert(std::string(Var->id().name())); + } + + return VarNames; +} + +/// \brief Generate the let/inherit replacement text. +/// Format: let inherit (source) var1 var2 ...; in body +std::string generateLetInherit(const nixf::ExprWith &With, + const std::set &VarNames, + llvm::StringRef Src) { + std::string Result; + + // Get source expression text + std::string_view SourceExpr; + if (With.with()) + SourceExpr = With.with()->src(Src); + else + return ""; // Cannot generate without source expression + + // Get body expression text + std::string_view BodyExpr; + if (With.expr()) + BodyExpr = With.expr()->src(Src); + else + return ""; // Cannot generate without body expression + + // Build: let inherit (source) vars; in body + Result.reserve(SourceExpr.size() + BodyExpr.size() + VarNames.size() * 10 + + 30); + + Result += "let inherit ("; + Result += SourceExpr; + Result += ")"; + + // Add sorted variable names + for (const auto &Name : VarNames) { + Result += " "; + Result += Name; + } + + Result += "; in "; + Result += BodyExpr; + + return Result; +} + +} // namespace + +void addWithToLetAction(const nixf::Node &N, const nixf::ParentMapAnalysis &PM, + const nixf::VariableLookupAnalysis &VLA, + const std::string &FileURI, llvm::StringRef Src, + std::vector &Actions) { + // Find enclosing ExprWith node + const nixf::Node *WithNode = PM.upTo(N, nixf::Node::NK_ExprWith); + if (!WithNode) + return; + + const auto &With = static_cast(*WithNode); + + // Check if cursor is on the `with` keyword (not in the body or source expr) + if (!isCursorOnWithKeyword(With, N)) + return; + + // Collect variables used from this with scope + std::set VarNames = collectWithVariables(With, VLA); + + // Skip if no variables are used (unused with) + // The existing "remove with" quickfix handles this case + if (VarNames.empty()) + return; + + // Generate the replacement text + std::string NewText = generateLetInherit(With, VarNames, Src); + if (NewText.empty()) + return; + + // Create the code action + Actions.emplace_back(createSingleEditAction( + "Convert `with` to `let/inherit`", + lspserver::CodeAction::REFACTOR_REWRITE_KIND, FileURI, + toLSPRange(Src, With.range()), std::move(NewText))); +} + +} // namespace nixd diff --git a/nixd/lib/Controller/CodeActions/WithToLet.h b/nixd/lib/Controller/CodeActions/WithToLet.h new file mode 100644 index 000000000..ef5577bef --- /dev/null +++ b/nixd/lib/Controller/CodeActions/WithToLet.h @@ -0,0 +1,39 @@ +/// \file +/// \brief Code action for converting `with` expressions to `let/inherit`. +/// +/// Transforms: with lib; expr -> let inherit (lib) usedVars; in expr +/// This makes variable sources explicit and improves code clarity. + +#pragma once + +#include + +#include +#include + +#include + +#include +#include + +namespace nixf { +class Node; +} // namespace nixf + +namespace nixd { + +/// \brief Add code action to convert `with` expression to `let/inherit`. +/// +/// This action is offered when the cursor is on the `with` keyword. +/// It transforms `with source; body` into `let inherit (source) vars; in body` +/// where vars are all variables used from the with scope. +/// +/// The action is NOT offered when: +/// - The cursor is not on the `with` keyword +/// - No variables are used from the with scope (unused with) +void addWithToLetAction(const nixf::Node &N, const nixf::ParentMapAnalysis &PM, + const nixf::VariableLookupAnalysis &VLA, + const std::string &FileURI, llvm::StringRef Src, + std::vector &Actions); + +} // namespace nixd diff --git a/nixd/lib/meson.build b/nixd/lib/meson.build index a37168ee2..7ffb9e12c 100644 --- a/nixd/lib/meson.build +++ b/nixd/lib/meson.build @@ -15,6 +15,7 @@ libnixd_lib = library( 'Controller/CodeActions/NoogleDoc.cpp', 'Controller/CodeActions/PackAttrs.cpp', 'Controller/CodeActions/Utils.cpp', + 'Controller/CodeActions/WithToLet.cpp', 'Controller/Completion.cpp', 'Controller/Configuration.cpp', 'Controller/Convert.cpp', diff --git a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-attrset-body.md b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-attrset-body.md new file mode 100644 index 000000000..2e40042aa --- /dev/null +++ b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-attrset-body.md @@ -0,0 +1,70 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test `with` to `let/inherit` with attribute set body. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///with-to-let-attrset.nix +with lib; { foo = optionalString true "x"; } +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///with-to-let-attrset.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":0 + }, + "end":{ + "line":0, + "character":4 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Body is an attribute set. + +``` + CHECK: "id": 2, + CHECK: "newText": "let inherit (lib) optionalString; in { foo = optionalString true \"x\"; }", + CHECK: "kind": "refactor.rewrite", + CHECK: "title": "Convert `with` to `let/inherit`" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-basic.md b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-basic.md new file mode 100644 index 000000000..e396f1929 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-basic.md @@ -0,0 +1,71 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test basic `with` to `let/inherit` conversion. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///with-to-let-basic.nix +with lib; optionalString true "yes" +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///with-to-let-basic.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":0 + }, + "end":{ + "line":0, + "character":4 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +The action should convert `with lib; optionalString true "yes"` to +`let inherit (lib) optionalString; in optionalString true "yes"` + +``` + CHECK: "id": 2, + CHECK: "newText": "let inherit (lib) optionalString; in optionalString true \"yes\"", + CHECK: "kind": "refactor.rewrite", + CHECK: "title": "Convert `with` to `let/inherit`" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-complex-source.md b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-complex-source.md new file mode 100644 index 000000000..462a3457b --- /dev/null +++ b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-complex-source.md @@ -0,0 +1,70 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test `with` to `let/inherit` with complex source expression. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///with-to-let-complex.nix +with pkgs.lib; optionalString true "yes" +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///with-to-let-complex.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":0 + }, + "end":{ + "line":0, + "character":4 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Source expression is `pkgs.lib` (attribute selection). + +``` + CHECK: "id": 2, + CHECK: "newText": "let inherit (pkgs.lib) optionalString; in optionalString true \"yes\"", + CHECK: "kind": "refactor.rewrite", + CHECK: "title": "Convert `with` to `let/inherit`" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-cursor-not-on-with.md b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-cursor-not-on-with.md new file mode 100644 index 000000000..15afc93da --- /dev/null +++ b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-cursor-not-on-with.md @@ -0,0 +1,69 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that NO action is offered when cursor is NOT on the `with` keyword. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///with-to-let-cursor.nix +with lib; optionalString foo "yes" +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///with-to-let-cursor.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":10 + }, + "end":{ + "line":0, + "character":24 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Cursor is on "optionalString" (characters 10-24), not on "with" keyword. +No action should be offered. + +``` + CHECK: "id": 2, +CHECK-NOT: "Convert `with` to `let/inherit`" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-lambda-body.md b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-lambda-body.md new file mode 100644 index 000000000..e7dad59ba --- /dev/null +++ b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-lambda-body.md @@ -0,0 +1,70 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test `with` to `let/inherit` with lambda body expression. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///with-to-let-lambda-body.nix +with lib; x: optionalString x "yes" +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///with-to-let-lambda-body.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":0 + }, + "end":{ + "line":0, + "character":4 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Body is a lambda function. + +``` + CHECK: "id": 2, + CHECK: "newText": "let inherit (lib) optionalString; in x: optionalString x \"yes\"", + CHECK: "kind": "refactor.rewrite", + CHECK: "title": "Convert `with` to `let/inherit`" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-multiple-vars.md b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-multiple-vars.md new file mode 100644 index 000000000..b231b6c70 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-multiple-vars.md @@ -0,0 +1,70 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test `with` to `let/inherit` conversion with multiple variables. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///with-to-let-multiple.nix +with lib; { x = optionalString true "yes"; y = concatStrings [ "a" "b" ]; } +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///with-to-let-multiple.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":0 + }, + "end":{ + "line":0, + "character":4 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +Variables should be sorted alphabetically in the inherit list. + +``` + CHECK: "id": 2, + CHECK: "newText": "let inherit (lib) concatStrings optionalString; in { x = optionalString true \"yes\"; y = concatStrings [ \"a\" \"b\" ]; }", + CHECK: "kind": "refactor.rewrite", + CHECK: "title": "Convert `with` to `let/inherit`" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested.md b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested.md new file mode 100644 index 000000000..86c1a5bcd --- /dev/null +++ b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested.md @@ -0,0 +1,73 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test `with` to `let/inherit` conversion with nested `with` expressions. + +When the cursor is on the outer `with`, the outer `with` is converted to `let/inherit`, +preserving the inner `with` in the body. The VLA tracks all unresolved variables +(`bar`, `foo`, `inner`) under the outer `with` scope's Definition, so they are all +inherited. Variables are sorted alphabetically. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///with-to-let-nested.nix +with outer; with inner; foo bar +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///with-to-let-nested.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":0 + }, + "end":{ + "line":0, + "character":4 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +``` + CHECK: "id": 2, + CHECK: "newText": "let inherit (outer) bar foo inner; in with inner; foo bar", + CHECK: "kind": "refactor.rewrite", + CHECK: "title": "Convert `with` to `let/inherit`" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-shadowing.md b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-shadowing.md new file mode 100644 index 000000000..398984b56 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-shadowing.md @@ -0,0 +1,77 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test `with` to `let/inherit` conversion with variable shadowing. + +When a variable from the `with` scope is shadowed by a local binding, +it should NOT be included in the inherit list. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///with-to-let-shadowing.nix +with lib; let foo = 1; in foo + optionalString true "x" +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///with-to-let-shadowing.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":0 + }, + "end":{ + "line":0, + "character":4 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +The action should convert `with lib; let foo = 1; in foo + optionalString true "x"` to +`let inherit (lib) optionalString; in let foo = 1; in foo + optionalString true "x"` + +The variable `foo` is shadowed by the inner let binding, so only `optionalString` +should be inherited from `lib`. + +``` + CHECK: "id": 2, + CHECK: "newText": "let inherit (lib) optionalString; in let foo = 1; in foo + optionalString true \"x\"", + CHECK: "kind": "refactor.rewrite", + CHECK: "title": "Convert `with` to `let/inherit`" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-unused.md b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-unused.md new file mode 100644 index 000000000..17d48d5c5 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-unused.md @@ -0,0 +1,70 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that NO action is offered for unused `with` expressions. +The "remove with" quickfix handles this case instead. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///with-to-let-unused.nix +with lib; 42 +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///with-to-let-unused.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":0 + }, + "end":{ + "line":0, + "character":4 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +No "Convert with to let/inherit" action should be offered when no variables +are used from the with scope. + +``` + CHECK: "id": 2, +CHECK-NOT: "Convert `with` to `let/inherit`" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` From cf5d0c4ec6fe2e0a48d57ee7ad9c9519cd66af83 Mon Sep 17 00:00:00 2001 From: takeokunn Date: Mon, 12 Jan 2026 13:16:55 +0900 Subject: [PATCH 2/4] feat(code-action): prevent with-to-let conversion for non-innermost nested with - Add unwrapParen and hasDirectlyNestedWith helpers to detect nested with chains - Only offer with-to-let code action for innermost with in nested expressions - Update tests to verify correct behavior for nested, parenthesized, and triple-nested cases - Document new restriction in WithToLet.h and test descriptions --- nixd/lib/Controller/CodeActions/WithToLet.cpp | 23 ++++++ nixd/lib/Controller/CodeActions/WithToLet.h | 1 + .../with-to-let/with-to-let-nested-inner.md | 74 ++++++++++++++++++ .../with-to-let/with-to-let-nested-paren.md | 74 ++++++++++++++++++ .../with-to-let/with-to-let-nested-triple.md | 75 +++++++++++++++++++ .../with-to-let/with-to-let-nested.md | 20 ++--- 6 files changed, 258 insertions(+), 9 deletions(-) create mode 100644 nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested-inner.md create mode 100644 nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested-paren.md create mode 100644 nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested-triple.md diff --git a/nixd/lib/Controller/CodeActions/WithToLet.cpp b/nixd/lib/Controller/CodeActions/WithToLet.cpp index 1fa946a04..e7d883897 100644 --- a/nixd/lib/Controller/CodeActions/WithToLet.cpp +++ b/nixd/lib/Controller/CodeActions/WithToLet.cpp @@ -7,6 +7,7 @@ #include "../Convert.h" #include +#include #include @@ -14,6 +15,21 @@ namespace nixd { namespace { +/// \brief Unwrap parenthesized expressions to get the inner expression. +/// For example, `(expr)` returns `expr`, `((expr))` returns `expr`. +const nixf::Expr *unwrapParen(const nixf::Expr *E) { + while (E && E->kind() == nixf::Node::NK_ExprParen) + E = static_cast(E)->expr(); + return E; +} + +/// \brief Check if the with body is directly another with expression. +/// This includes parenthesized with, e.g., `with a; (with b; x)`. +bool hasDirectlyNestedWith(const nixf::ExprWith &With) { + const nixf::Expr *Body = unwrapParen(With.expr()); + return Body && Body->kind() == nixf::Node::NK_ExprWith; +} + /// \brief Check if cursor position is on the `with` keyword. bool isCursorOnWithKeyword(const nixf::ExprWith &With, const nixf::Node &N) { // The node N should be or contain the `with` keyword @@ -105,6 +121,13 @@ void addWithToLetAction(const nixf::Node &N, const nixf::ParentMapAnalysis &PM, if (!isCursorOnWithKeyword(With, N)) return; + // Skip non-innermost with in nested chains to avoid semantic issues. + // Converting outer `with` to `let/inherit` can change variable resolution + // because `let` bindings shadow inner `with` scopes. + // See: https://github.com/nix-community/nixd/pull/768#discussion_r2679465713 + if (hasDirectlyNestedWith(With)) + return; + // Collect variables used from this with scope std::set VarNames = collectWithVariables(With, VLA); diff --git a/nixd/lib/Controller/CodeActions/WithToLet.h b/nixd/lib/Controller/CodeActions/WithToLet.h index ef5577bef..b0480124a 100644 --- a/nixd/lib/Controller/CodeActions/WithToLet.h +++ b/nixd/lib/Controller/CodeActions/WithToLet.h @@ -31,6 +31,7 @@ namespace nixd { /// The action is NOT offered when: /// - The cursor is not on the `with` keyword /// - No variables are used from the with scope (unused with) +/// - The with body is directly another with expression (nested with chains) void addWithToLetAction(const nixf::Node &N, const nixf::ParentMapAnalysis &PM, const nixf::VariableLookupAnalysis &VLA, const std::string &FileURI, llvm::StringRef Src, diff --git a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested-inner.md b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested-inner.md new file mode 100644 index 000000000..df353c6b3 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested-inner.md @@ -0,0 +1,74 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that the code action IS offered for the innermost `with` in nested chains. + +The innermost `with` is safe to convert because its `let` bindings would shadow +outer `with` scopes anyway, preserving the original variable resolution semantics. + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///with-to-let-nested-inner.nix +with outer; with inner; foo bar +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///with-to-let-nested-inner.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":12 + }, + "end":{ + "line":0, + "character":16 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +The action should convert the inner `with` to `let/inherit`, preserving the +outer `with` in place. + +``` + CHECK: "id": 2, + CHECK: "newText": "let inherit (inner) bar foo; in foo bar", + CHECK: "kind": "refactor.rewrite", + CHECK: "title": "Convert `with` to `let/inherit`" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested-paren.md b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested-paren.md new file mode 100644 index 000000000..a4ee15a1f --- /dev/null +++ b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested-paren.md @@ -0,0 +1,74 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that NO action is offered for outer `with` when the inner `with` is parenthesized. + +The `unwrapParen()` helper correctly unwraps parentheses to detect nested `with`, +so `with a; (with b; x)` is treated the same as `with a; with b; x`. + +See: https://github.com/nix-community/nixd/pull/768#discussion_r2679465713 + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///with-to-let-nested-paren.nix +with outer; (with inner; foo) +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///with-to-let-nested-paren.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":0 + }, + "end":{ + "line":0, + "character":4 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +No "Convert with to let/inherit" action should be offered when cursor is +on the outer `with`, even when the inner `with` is wrapped in parentheses. + +``` + CHECK: "id": 2, +CHECK-NOT: "Convert `with` to `let/inherit`" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested-triple.md b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested-triple.md new file mode 100644 index 000000000..e4e84af0b --- /dev/null +++ b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested-triple.md @@ -0,0 +1,75 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that NO action is offered for middle `with` in a triple-nested chain. + +In `with a; with b; with c; x`, only the innermost `with c` should get the action. +Both `with a` and `with b` have directly nested `with` expressions in their bodies. + +See: https://github.com/nix-community/nixd/pull/768#discussion_r2679465713 + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///with-to-let-nested-triple.nix +with a; with b; with c; foo +``` + +<-- textDocument/codeAction(2) + +Request code action on middle `with b` (character 8-12). + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///with-to-let-nested-triple.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":8 + }, + "end":{ + "line":0, + "character":12 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +No "Convert with to let/inherit" action should be offered for middle `with b` +because its body contains another `with c`. + +``` + CHECK: "id": 2, +CHECK-NOT: "Convert `with` to `let/inherit`" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested.md b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested.md index 86c1a5bcd..ef2f354a3 100644 --- a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested.md +++ b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-nested.md @@ -1,11 +1,12 @@ # RUN: nixd --lit-test < %s | FileCheck %s -Test `with` to `let/inherit` conversion with nested `with` expressions. +Test that NO action is offered for outer `with` in nested `with` expressions. -When the cursor is on the outer `with`, the outer `with` is converted to `let/inherit`, -preserving the inner `with` in the body. The VLA tracks all unresolved variables -(`bar`, `foo`, `inner`) under the outer `with` scope's Definition, so they are all -inherited. Variables are sorted alphabetically. +Converting outer `with` to `let/inherit` can change variable resolution semantics +because `let` bindings shadow inner `with` scopes. To avoid this semantic issue, +the code action is only offered for the innermost `with` in a nested chain. + +See: https://github.com/nix-community/nixd/pull/768#discussion_r2679465713 <-- initialize(0) @@ -61,11 +62,12 @@ with outer; with inner; foo bar } ``` +No "Convert with to let/inherit" action should be offered when cursor is +on the outer `with` of a nested chain. + ``` - CHECK: "id": 2, - CHECK: "newText": "let inherit (outer) bar foo inner; in with inner; foo bar", - CHECK: "kind": "refactor.rewrite", - CHECK: "title": "Convert `with` to `let/inherit`" + CHECK: "id": 2, +CHECK-NOT: "Convert `with` to `let/inherit`" ``` ```json From a40adaa017e1e8b39bc0a0501b5bf6008bd3e66e Mon Sep 17 00:00:00 2001 From: takeokunn Date: Tue, 13 Jan 2026 21:53:25 +0900 Subject: [PATCH 3/4] refactor: use semantic analysis for nested with detection Address review feedback to detect indirect nested with scopes. - Add getWithScopes() to VariableLookupAnalysis for tracking with scopes - Replace AST-based hasDirectlyNestedWith with semantic hasNestedWithScope - Add unit tests for getWithScopes in various nesting scenarios - Add integration tests for indirect nesting cases Ref: https://github.com/nix-community/nixd/pull/768#discussion_r2681198142 --- libnixf/include/nixf/Sema/VariableLookup.h | 28 ++++ libnixf/src/Sema/VariableLookup.cpp | 6 + libnixf/test/Sema/VariableLookup.cpp | 146 ++++++++++++++++++ nixd/lib/Controller/CodeActions/WithToLet.cpp | 58 +++++-- .../with-to-let-indirect-attrset.md | 76 +++++++++ .../with-to-let-indirect-inner-ok.md | 77 +++++++++ .../with-to-let-indirect-lambda.md | 76 +++++++++ .../with-to-let/with-to-let-indirect-let.md | 77 +++++++++ 8 files changed, 528 insertions(+), 16 deletions(-) create mode 100644 nixd/tools/nixd/test/code-action/with-to-let/with-to-let-indirect-attrset.md create mode 100644 nixd/tools/nixd/test/code-action/with-to-let/with-to-let-indirect-inner-ok.md create mode 100644 nixd/tools/nixd/test/code-action/with-to-let/with-to-let-indirect-lambda.md create mode 100644 nixd/tools/nixd/test/code-action/with-to-let/with-to-let-indirect-let.md diff --git a/libnixf/include/nixf/Sema/VariableLookup.h b/libnixf/include/nixf/Sema/VariableLookup.h index b30938ea9..eb1625fc1 100644 --- a/libnixf/include/nixf/Sema/VariableLookup.h +++ b/libnixf/include/nixf/Sema/VariableLookup.h @@ -164,6 +164,12 @@ class VariableLookupAnalysis { std::map Results; + /// \brief For variables resolved from `with` scopes, track all enclosing + /// `with` expressions that could potentially provide the binding. + /// This is needed to detect indirect nested `with` scenarios where + /// converting an outer `with` to `let/inherit` could change semantics. + std::map> VarWithScopes; + public: VariableLookupAnalysis(std::vector &Diags); @@ -198,6 +204,28 @@ class VariableLookupAnalysis { } const EnvNode *env(const Node *N) const; + + /// \brief Get all `with` expressions that could provide the binding for a + /// variable. + /// + /// For variables resolved from `with` scopes, multiple enclosing `with` + /// expressions may potentially provide the binding. This method returns + /// all such `with` expressions, ordered from innermost to outermost. + /// + /// This is useful for detecting indirect nested `with` scenarios where + /// converting an outer `with` to `let/inherit` could change semantics. + /// + /// \param Var The variable to query. + /// \return A vector of `ExprWith` pointers representing all `with` scopes + /// that could provide the variable's binding. Returns an empty + /// vector if the variable is not resolved from a `with` scope. + [[nodiscard]] std::vector + getWithScopes(const ExprVar &Var) const { + auto It = VarWithScopes.find(&Var); + if (It != VarWithScopes.end()) + return It->second; + return {}; + } }; } // namespace nixf diff --git a/libnixf/src/Sema/VariableLookup.cpp b/libnixf/src/Sema/VariableLookup.cpp index 814167369..7bf8dddde 100644 --- a/libnixf/src/Sema/VariableLookup.cpp +++ b/libnixf/src/Sema/VariableLookup.cpp @@ -157,10 +157,16 @@ void VariableLookupAnalysis::lookupVar(const ExprVar &Var, Def->usedBy(Var); Results.insert({&Var, LookupResult{LookupResultKind::Defined, Def}}); } else if (!WithEnvs.empty()) { // comes from enclosed "with" expressions. + // Collect all `with` expressions that could provide this variable's binding. + // This is stored for later queries (e.g., by code actions that need to + // determine if converting a `with` to `let/inherit` is safe). + std::vector WithScopes; for (const auto *WithEnv : WithEnvs) { Def = WithDefs.at(WithEnv->syntax()); Def->usedBy(Var); + WithScopes.push_back(static_cast(WithEnv->syntax())); } + VarWithScopes.insert({&Var, std::move(WithScopes)}); Results.insert({&Var, LookupResult{LookupResultKind::FromWith, Def}}); } else { // Check if this is a primop. diff --git a/libnixf/test/Sema/VariableLookup.cpp b/libnixf/test/Sema/VariableLookup.cpp index 8c7882f3c..d509d8871 100644 --- a/libnixf/test/Sema/VariableLookup.cpp +++ b/libnixf/test/Sema/VariableLookup.cpp @@ -581,4 +581,150 @@ TEST_F(VLATest, PrimOp_Override_Namespace) { ASSERT_EQ(Diags.size(), 0); } +//===----------------------------------------------------------------------===// +// getWithScopes() tests - for detecting nested with scopes +//===----------------------------------------------------------------------===// + +TEST_F(VLATest, GetWithScopes_SingleWith) { + // A single `with` scope should return exactly one element + std::shared_ptr AST = parse("with lib; foo", Diags); + VariableLookupAnalysis VLA(Diags); + VLA.runOnAST(*AST); + + ASSERT_TRUE(AST); + const auto &With = *static_cast(AST.get()); + const auto &Var = *static_cast(With.expr()); + + auto Scopes = VLA.getWithScopes(Var); + ASSERT_EQ(Scopes.size(), 1); + ASSERT_EQ(Scopes[0], &With); +} + +TEST_F(VLATest, GetWithScopes_DirectNested) { + // Direct nested with: variable should have 2 scopes (inner first, outer last) + const char *Src = R"(with outer; with inner; foo)"; + std::shared_ptr AST = parse(Src, Diags); + VariableLookupAnalysis VLA(Diags); + VLA.runOnAST(*AST); + + ASSERT_TRUE(AST); + const auto &OuterWith = *static_cast(AST.get()); + const auto &InnerWith = *static_cast(OuterWith.expr()); + const auto &Var = *static_cast(InnerWith.expr()); + + auto Scopes = VLA.getWithScopes(Var); + ASSERT_EQ(Scopes.size(), 2); + // Innermost first + ASSERT_EQ(Scopes[0], &InnerWith); + ASSERT_EQ(Scopes[1], &OuterWith); +} + +TEST_F(VLATest, GetWithScopes_IndirectNested_Let) { + // Indirect nested with through let: `with a; let x = with b; foo; in x` + // The variable `foo` should have 2 scopes + const char *Src = R"(with outer; let x = with inner; foo; in x)"; + std::shared_ptr AST = parse(Src, Diags); + VariableLookupAnalysis VLA(Diags); + VLA.runOnAST(*AST); + + ASSERT_TRUE(AST); + // Navigate: ExprWith -> ExprLet -> Binds -> Binding[0].Value -> ExprWith + // -> ExprVar + const auto &OuterWith = *static_cast(AST.get()); + const auto *Let = static_cast(OuterWith.expr()); + ASSERT_TRUE(Let); + + // Find the inner with by navigating the AST + // The binding value should be the inner with + const auto *Binds = Let->binds(); + ASSERT_TRUE(Binds); + const auto &Bindings = Binds->bindings(); + ASSERT_EQ(Bindings.size(), 1); + const auto *BindingNode = static_cast(Bindings[0].get()); + ASSERT_TRUE(BindingNode); + const auto *InnerWith = + static_cast(BindingNode->value().get()); + ASSERT_TRUE(InnerWith); + ASSERT_EQ(InnerWith->kind(), Node::NK_ExprWith); + + const auto *Var = static_cast(InnerWith->expr()); + ASSERT_TRUE(Var); + ASSERT_EQ(Var->kind(), Node::NK_ExprVar); + + auto Scopes = VLA.getWithScopes(*Var); + ASSERT_EQ(Scopes.size(), 2); + // Innermost first + ASSERT_EQ(Scopes[0], InnerWith); + ASSERT_EQ(Scopes[1], &OuterWith); +} + +TEST_F(VLATest, GetWithScopes_IndirectNested_AttrSet) { + // Indirect nested with through attrset: `with a; { y = with b; bar; }` + const char *Src = R"(with outer; { y = with inner; bar; })"; + std::shared_ptr AST = parse(Src, Diags); + VariableLookupAnalysis VLA(Diags); + VLA.runOnAST(*AST); + + ASSERT_TRUE(AST); + const auto &OuterWith = *static_cast(AST.get()); + + // Navigate to the inner with through the attrset + const auto *Attrs = static_cast(OuterWith.expr()); + ASSERT_TRUE(Attrs); + const auto *Binds = Attrs->binds(); + ASSERT_TRUE(Binds); + const auto &Bindings = Binds->bindings(); + ASSERT_EQ(Bindings.size(), 1); + + const auto *BindingNode = static_cast(Bindings[0].get()); + ASSERT_TRUE(BindingNode); + const auto *InnerWith = + static_cast(BindingNode->value().get()); + ASSERT_TRUE(InnerWith); + + const auto *Var = static_cast(InnerWith->expr()); + ASSERT_TRUE(Var); + + auto Scopes = VLA.getWithScopes(*Var); + ASSERT_EQ(Scopes.size(), 2); + ASSERT_EQ(Scopes[0], InnerWith); + ASSERT_EQ(Scopes[1], &OuterWith); +} + +TEST_F(VLATest, GetWithScopes_NoWith) { + // Variable not from with should return empty + const char *Src = R"(let x = 1; in x)"; + std::shared_ptr AST = parse(Src, Diags); + VariableLookupAnalysis VLA(Diags); + VLA.runOnAST(*AST); + + ASSERT_TRUE(AST); + const auto *Let = static_cast(AST.get()); + const auto *Var = static_cast(Let->expr()); + + auto Scopes = VLA.getWithScopes(*Var); + ASSERT_TRUE(Scopes.empty()); +} + +TEST_F(VLATest, GetWithScopes_TripleNested) { + // Triple nested with: variable should have 3 scopes + const char *Src = R"(with a; with b; with c; foo)"; + std::shared_ptr AST = parse(Src, Diags); + VariableLookupAnalysis VLA(Diags); + VLA.runOnAST(*AST); + + ASSERT_TRUE(AST); + const auto &WithA = *static_cast(AST.get()); + const auto &WithB = *static_cast(WithA.expr()); + const auto &WithC = *static_cast(WithB.expr()); + const auto &Var = *static_cast(WithC.expr()); + + auto Scopes = VLA.getWithScopes(Var); + ASSERT_EQ(Scopes.size(), 3); + // Innermost first + ASSERT_EQ(Scopes[0], &WithC); + ASSERT_EQ(Scopes[1], &WithB); + ASSERT_EQ(Scopes[2], &WithA); +} + } // namespace diff --git a/nixd/lib/Controller/CodeActions/WithToLet.cpp b/nixd/lib/Controller/CodeActions/WithToLet.cpp index e7d883897..e6e428768 100644 --- a/nixd/lib/Controller/CodeActions/WithToLet.cpp +++ b/nixd/lib/Controller/CodeActions/WithToLet.cpp @@ -15,19 +15,43 @@ namespace nixd { namespace { -/// \brief Unwrap parenthesized expressions to get the inner expression. -/// For example, `(expr)` returns `expr`, `((expr))` returns `expr`. -const nixf::Expr *unwrapParen(const nixf::Expr *E) { - while (E && E->kind() == nixf::Node::NK_ExprParen) - E = static_cast(E)->expr(); - return E; -} +/// \brief Check if any variable used by this `with` could also be provided by +/// a nested (inner) `with` expression. +/// +/// This uses semantic analysis to detect indirect nested `with` scenarios. +/// Converting an outer `with` to `let/inherit` is unsafe when variables could +/// come from inner `with` scopes, because `let` bindings shadow `with` scopes. +/// +/// \param With The `with` expression to check. +/// \param VLA The variable lookup analysis containing scope information. +/// \return true if this `with` has nested `with` scopes that affect its +/// variables (conversion unsafe), false if it's safe to convert. +bool hasNestedWithScope(const nixf::ExprWith &With, + const nixf::VariableLookupAnalysis &VLA) { + // Get the Definition for the `with` keyword to find all variables it provides + const nixf::Definition *Def = VLA.toDef(With.kwWith()); + if (!Def) + return false; + + // Check each variable used from this with scope + for (const nixf::ExprVar *Var : Def->uses()) { + if (!Var) + continue; + + // Get all `with` scopes that could provide this variable + auto WithScopes = VLA.getWithScopes(*Var); + + // If there are multiple `with` scopes, check if this `with` is not the + // innermost one. The WithScopes vector is ordered innermost-to-outermost, + // so if our `with` is not the first one, there's a nested `with`. + if (WithScopes.size() > 1 && WithScopes.front() != &With) { + // This variable could come from a different (inner) `with`, + // so converting this outer `with` to `let/inherit` would change semantics + return true; + } + } -/// \brief Check if the with body is directly another with expression. -/// This includes parenthesized with, e.g., `with a; (with b; x)`. -bool hasDirectlyNestedWith(const nixf::ExprWith &With) { - const nixf::Expr *Body = unwrapParen(With.expr()); - return Body && Body->kind() == nixf::Node::NK_ExprWith; + return false; } /// \brief Check if cursor position is on the `with` keyword. @@ -121,11 +145,13 @@ void addWithToLetAction(const nixf::Node &N, const nixf::ParentMapAnalysis &PM, if (!isCursorOnWithKeyword(With, N)) return; - // Skip non-innermost with in nested chains to avoid semantic issues. - // Converting outer `with` to `let/inherit` can change variable resolution + // Skip `with` expressions that have nested `with` scopes (direct or indirect). + // Converting such a `with` to `let/inherit` can change variable resolution // because `let` bindings shadow inner `with` scopes. - // See: https://github.com/nix-community/nixd/pull/768#discussion_r2679465713 - if (hasDirectlyNestedWith(With)) + // This semantic check handles both direct nesting (with a; with b; x) and + // indirect nesting (with a; let y = with b; x; in y). + // See: https://github.com/nix-community/nixd/pull/768#discussion_r2681198142 + if (hasNestedWithScope(With, VLA)) return; // Collect variables used from this with scope diff --git a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-indirect-attrset.md b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-indirect-attrset.md new file mode 100644 index 000000000..6ee59e42b --- /dev/null +++ b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-indirect-attrset.md @@ -0,0 +1,76 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that NO action is offered for outer `with` when there's an indirect nested +`with` inside an attribute set. + +This is another "indirect nested with" case. The inner `with` is not directly +in the body of the outer `with`, but inside an attrset binding. + +See: https://github.com/nix-community/nixd/pull/768#discussion_r2681198142 + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///with-to-let-indirect-attrset.nix +with outer; { y = with inner; bar; } +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///with-to-let-indirect-attrset.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":0 + }, + "end":{ + "line":0, + "character":4 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +No "Convert with to let/inherit" action should be offered when there's an +indirect nested `with` inside an attrset. The variable `bar` could resolve +from either `outer` or `inner` at evaluation time. + +``` + CHECK: "id": 2, +CHECK-NOT: "Convert `with` to `let/inherit`" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-indirect-inner-ok.md b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-indirect-inner-ok.md new file mode 100644 index 000000000..0cb75fe8d --- /dev/null +++ b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-indirect-inner-ok.md @@ -0,0 +1,77 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that the code action IS offered for the innermost `with` in an indirect +nesting scenario. + +Even when there's indirect nesting, the innermost `with` is safe to convert +because its variables cannot come from an outer `with` scope. + +See: https://github.com/nix-community/nixd/pull/768#discussion_r2681198142 + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///with-to-let-indirect-inner-ok.nix +with outer; let x = with inner; foo; in x +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///with-to-let-indirect-inner-ok.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":20 + }, + "end":{ + "line":0, + "character":24 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +The action should be offered for the inner `with` because it's the innermost +scope for the variable `foo`. Converting it to `let/inherit` is safe. + +``` + CHECK: "id": 2, + CHECK: "newText": "let inherit (inner) foo; in foo", + CHECK: "kind": "refactor.rewrite", + CHECK: "title": "Convert `with` to `let/inherit`" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-indirect-lambda.md b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-indirect-lambda.md new file mode 100644 index 000000000..55b7a4344 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-indirect-lambda.md @@ -0,0 +1,76 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that NO action is offered for outer `with` when there's an indirect nested +`with` inside a lambda body. + +This is another "indirect nested with" case. The inner `with` is not directly +in the body of the outer `with`, but inside a lambda expression. + +See: https://github.com/nix-community/nixd/pull/768#discussion_r2681198142 + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///with-to-let-indirect-lambda.nix +with outer; x: with inner; baz +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///with-to-let-indirect-lambda.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":0 + }, + "end":{ + "line":0, + "character":4 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +No "Convert with to let/inherit" action should be offered when there's an +indirect nested `with` inside a lambda. The variable `baz` could resolve +from either `outer` or `inner` at evaluation time. + +``` + CHECK: "id": 2, +CHECK-NOT: "Convert `with` to `let/inherit`" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` diff --git a/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-indirect-let.md b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-indirect-let.md new file mode 100644 index 000000000..57c2367f6 --- /dev/null +++ b/nixd/tools/nixd/test/code-action/with-to-let/with-to-let-indirect-let.md @@ -0,0 +1,77 @@ +# RUN: nixd --lit-test < %s | FileCheck %s + +Test that NO action is offered for outer `with` when there's an indirect nested +`with` inside a `let` binding. + +This is the "indirect nested with" case that the semantic check must handle. +Converting the outer `with` to `let/inherit` would change variable resolution +because `let` bindings shadow inner `with` scopes. + +See: https://github.com/nix-community/nixd/pull/768#discussion_r2681198142 + +<-- initialize(0) + +```json +{ + "jsonrpc":"2.0", + "id":0, + "method":"initialize", + "params":{ + "processId":123, + "rootPath":"", + "capabilities":{ + }, + "trace":"off" + } +} +``` + + +<-- textDocument/didOpen + +```nix file:///with-to-let-indirect-let.nix +with outer; let x = with inner; foo; in x +``` + +<-- textDocument/codeAction(2) + + +```json +{ + "jsonrpc":"2.0", + "id":2, + "method":"textDocument/codeAction", + "params":{ + "textDocument":{ + "uri":"file:///with-to-let-indirect-let.nix" + }, + "range":{ + "start":{ + "line": 0, + "character":0 + }, + "end":{ + "line":0, + "character":4 + } + }, + "context":{ + "diagnostics":[], + "triggerKind":2 + } + } +} +``` + +No "Convert with to let/inherit" action should be offered when there's an +indirect nested `with` inside a `let` binding. The variable `foo` could +resolve from either `outer` or `inner` at evaluation time. + +``` + CHECK: "id": 2, +CHECK-NOT: "Convert `with` to `let/inherit`" +``` + +```json +{"jsonrpc":"2.0","method":"exit"} +``` From 670354939cc5226190ee33cc2c61fecb22e4d6fe Mon Sep 17 00:00:00 2001 From: takeokunn Date: Tue, 13 Jan 2026 21:55:25 +0900 Subject: [PATCH 4/4] style: improve clarity and grammar in comments for with-to-let code action - Update comments in VariableLookup.cpp and WithToLet.cpp for better grammar and readability - Clarify intent and semantics of code action logic - No functional code changes made --- libnixf/src/Sema/VariableLookup.cpp | 6 +++--- nixd/lib/Controller/CodeActions/WithToLet.cpp | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/libnixf/src/Sema/VariableLookup.cpp b/libnixf/src/Sema/VariableLookup.cpp index 7bf8dddde..e9132d225 100644 --- a/libnixf/src/Sema/VariableLookup.cpp +++ b/libnixf/src/Sema/VariableLookup.cpp @@ -157,9 +157,9 @@ void VariableLookupAnalysis::lookupVar(const ExprVar &Var, Def->usedBy(Var); Results.insert({&Var, LookupResult{LookupResultKind::Defined, Def}}); } else if (!WithEnvs.empty()) { // comes from enclosed "with" expressions. - // Collect all `with` expressions that could provide this variable's binding. - // This is stored for later queries (e.g., by code actions that need to - // determine if converting a `with` to `let/inherit` is safe). + // Collect all `with` expressions that could provide this variable's + // binding. This is stored for later queries (e.g., by code actions that + // need to determine if converting a `with` to `let/inherit` is safe). std::vector WithScopes; for (const auto *WithEnv : WithEnvs) { Def = WithDefs.at(WithEnv->syntax()); diff --git a/nixd/lib/Controller/CodeActions/WithToLet.cpp b/nixd/lib/Controller/CodeActions/WithToLet.cpp index e6e428768..648710158 100644 --- a/nixd/lib/Controller/CodeActions/WithToLet.cpp +++ b/nixd/lib/Controller/CodeActions/WithToLet.cpp @@ -145,12 +145,12 @@ void addWithToLetAction(const nixf::Node &N, const nixf::ParentMapAnalysis &PM, if (!isCursorOnWithKeyword(With, N)) return; - // Skip `with` expressions that have nested `with` scopes (direct or indirect). - // Converting such a `with` to `let/inherit` can change variable resolution - // because `let` bindings shadow inner `with` scopes. - // This semantic check handles both direct nesting (with a; with b; x) and - // indirect nesting (with a; let y = with b; x; in y). - // See: https://github.com/nix-community/nixd/pull/768#discussion_r2681198142 + // Skip `with` expressions that have nested `with` scopes (direct or + // indirect). Converting such a `with` to `let/inherit` can change variable + // resolution because `let` bindings shadow inner `with` scopes. This semantic + // check handles both direct nesting (with a; with b; x) and indirect nesting + // (with a; let y = with b; x; in y). See: + // https://github.com/nix-community/nixd/pull/768#discussion_r2681198142 if (hasNestedWithScope(With, VLA)) return;