Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions libnixf/include/nixf/Basic/Diagnostic.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class TextEdit {
class Fix {
std::vector<TextEdit> Edits;
std::string Message;
bool Preferred = false;

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.

Since libnixf stores fixes in a std::vector and most diagnostics currently yield only a single solution, adding an is_preferred boolean field makes sense primarily as a tie-breaker for multi-fix scenarios. I agree with this addition for LSP alignment (CodeAction.isPreferred), but we should carefully define its interaction with the existing diagnostic structure—specifically how to handle cases where multiple fixes (or none) are marked as preferred to ensure consistent client behavior.

One design detail to consider: if a diagnostic provides only a single fix(), should we mark it as is_preferred by default? In my view, we should be cautious—isPreferred in LSP usually signals that the fix can be safely auto-applied (e.g., via "Fix All"). If a fix has side effects, it might be better to keep it false even if it's the only option. What are your thoughts on the default behavior for single-fix scenarios?


libnixf では現在、fix を std::vector で管理しており、ほとんどの Diagnostic が単一の解決策しか提供していないことを考えると、is_preferred という bool フィールドの導入は、主に複数の候補が存在する場合の優先順位付けとして機能すると理解しています。LSP の仕様(CodeAction.isPreferred)に準拠させるという点では賛成ですが、既存の Diagnostic 構造との論理的な関係については慎重に定義すべきだと考えています。具体的には、複数の fix が preferred とマークされている場合、あるいは一つもマークされていない場合に、クライアント側で一貫した挙動を保証するための設計が必要です。

設計上の詳細について一点確認させてください。ある Diagnostic に対して fix() が一つしかない場合、それをデフォルトで is_preferred とすべきでしょうか?個人的には、少し慎重になる必要があると考えています。LSP における isPreferred は通常、その修正が(「Fix All」などを通じて)安全に自動適用できることを意味します。たとえ解決策が一つしかなくても、副作用があるような修正の場合は false のままにしておく方が適切かもしれません。単一 fix の場合のデフォルト挙動について、どのようにお考えでしょうか?


public:
Fix(std::vector<TextEdit> Edits, std::string Message)
Expand All @@ -69,6 +70,11 @@ class Fix {

[[nodiscard]] const std::vector<TextEdit> &edits() const { return Edits; }
[[nodiscard]] const std::string &message() const { return Message; }
Fix &prefer() {
Preferred = true;
return *this;
}
[[nodiscard]] bool isPreferred() const { return Preferred; }
};

enum class DiagnosticTag {
Expand Down
23 changes: 22 additions & 1 deletion libnixf/src/Sema/VariableLookup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "nixf/Basic/Nodes/Expr.h"
#include "nixf/Basic/Nodes/Lambda.h"
#include "nixf/Sema/PrimOpInfo.h"
#include "nixf/Sema/SemaActions.h"

#include <set>

Expand Down Expand Up @@ -157,13 +158,33 @@ void VariableLookupAnalysis::emitEnvLivenessWarning(
if (D.range().lCur() == FirstName->range().lCur() &&
D.range().rCur() == FirstName->range().rCur()) {
D.fix("remove unused binding")
.edit(TextEdit::mkRemoval(Bind.range()));
.edit(TextEdit::mkRemoval(Bind.range()))
.prefer();
break;
}
}
}
}
}
// Attach fix for unused formal parameters
if (Def->source() == Definition::DS_LambdaNoArg_Formal ||
Def->source() == Definition::DS_LambdaWithArg_Formal) {
// Def->source() guarantees NewEnv->syntax() is ExprLambda*
const auto *Lambda = static_cast<const ExprLambda *>(NewEnv->syntax());
if (Lambda->arg() && Lambda->arg()->formals()) {
const auto &FV = Lambda->arg()->formals()->members();
for (auto It = FV.begin(); It != FV.end(); ++It) {
if (!(*It)->id()) // skip ellipsis
continue;
if ((*It)->id() == Def->syntax()) {
Fix &F = D.fix("remove unused formal `" + Name + "`");
Sema::removeFormal(F, It, FV);
F.prefer();
break;
}
}
}
}
}
}
}
Expand Down
57 changes: 57 additions & 0 deletions libnixf/test/Sema/VariableLookup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,63 @@ TEST_F(VLATest, LivenessFormalWithArg) {
ASSERT_EQ(Diags[0].tags()[0], DiagnosticTag::Faded);
}

TEST_F(VLATest, UnusedFormalFix) {
std::shared_ptr<Node> AST = parse("{x, y}: x", Diags);
VariableLookupAnalysis VLA(Diags);
VLA.runOnAST(*AST);

ASSERT_EQ(Diags.size(), 1);
ASSERT_EQ(Diags[0].kind(), Diagnostic::DK_UnusedDefLambdaNoArg_Formal);
ASSERT_EQ(Diags[0].fixes().size(), 1);
ASSERT_EQ(Diags[0].fixes()[0].message(), "remove unused formal `y`");
ASSERT_TRUE(Diags[0].fixes()[0].isPreferred());
}

TEST_F(VLATest, UnusedFormalFixSingle) {
std::shared_ptr<Node> AST = parse("{ x }: 1", Diags);
VariableLookupAnalysis VLA(Diags);
VLA.runOnAST(*AST);

ASSERT_EQ(Diags.size(), 1);
ASSERT_EQ(Diags[0].kind(), Diagnostic::DK_UnusedDefLambdaNoArg_Formal);
ASSERT_EQ(Diags[0].fixes().size(), 1);
ASSERT_EQ(Diags[0].fixes()[0].edits().size(), 1);
ASSERT_TRUE(Diags[0].fixes()[0].isPreferred());
}

TEST_F(VLATest, UnusedFormalFixFirst) {
std::shared_ptr<Node> AST = parse("{x, y}: y", Diags);
VariableLookupAnalysis VLA(Diags);
VLA.runOnAST(*AST);

ASSERT_EQ(Diags.size(), 1);
ASSERT_EQ(Diags[0].kind(), Diagnostic::DK_UnusedDefLambdaNoArg_Formal);
ASSERT_EQ(Diags[0].fixes().size(), 1);
ASSERT_EQ(Diags[0].fixes()[0].message(), "remove unused formal `x`");
// First formal removal needs 2 edits: remove formal + remove next comma
ASSERT_EQ(Diags[0].fixes()[0].edits().size(), 2);
ASSERT_TRUE(Diags[0].fixes()[0].isPreferred());
}

TEST_F(VLATest, UnusedFormalFixWithArg) {
// {x, y}@args: args — formals are DS_LambdaWithArg_Formal
std::shared_ptr<Node> AST = parse("{x, y}@args: args", Diags);
VariableLookupAnalysis VLA(Diags);
VLA.runOnAST(*AST);

// Both x and y are unused formals (args is used via @-pattern)
ASSERT_EQ(Diags.size(), 2);
ASSERT_EQ(Diags[0].kind(), Diagnostic::DK_UnusedDefLambdaWithArg_Formal);
ASSERT_EQ(Diags[0].fixes().size(), 1);
ASSERT_EQ(Diags[0].fixes()[0].message(), "remove unused formal `x`");
ASSERT_TRUE(Diags[0].fixes()[0].isPreferred());

ASSERT_EQ(Diags[1].kind(), Diagnostic::DK_UnusedDefLambdaWithArg_Formal);
ASSERT_EQ(Diags[1].fixes().size(), 1);
ASSERT_EQ(Diags[1].fixes()[0].message(), "remove unused formal `y`");
ASSERT_TRUE(Diags[1].fixes()[0].isPreferred());
}

TEST_F(VLATest, ToDefAttrs) {
std::shared_ptr<Node> AST = parse("rec { x = 1; y = x; z = x; }", Diags);
VariableLookupAnalysis VLA(Diags);
Expand Down
12 changes: 1 addition & 11 deletions nixd/lib/Controller/CodeAction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,16 +47,6 @@ void Controller::onCodeAction(const lspserver::CodeActionParams &Params,
if (!Range.overlap(DRange))
continue;

// Determine if this diagnostic's fixes should be preferred
bool IsPreferred = false;
switch (D.kind()) {
case nixf::Diagnostic::DK_UnusedDefLet:
IsPreferred = true;
break;
default:
break;
}

// Add fixes.
for (const nixf::Fix &F : D.fixes()) {
std::vector<TextEdit> Edits;
Expand All @@ -74,7 +64,7 @@ void Controller::onCodeAction(const lspserver::CodeActionParams &Params,
Actions.emplace_back(CodeAction{
.title = F.message(),
.kind = std::string(CodeAction::QUICKFIX_KIND),
.isPreferred = IsPreferred,
.isPreferred = F.isPreferred(),
.edit = std::move(WE),
});
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
# RUN: nixd --lit-test < %s | FileCheck %s

Test that no action is offered when all formals are used.

<-- initialize(0)

```json
{
"jsonrpc":"2.0",
"id":0,
"method":"initialize",
"params":{
"processId":123,
"rootPath":"",
"capabilities":{
},
"trace":"off"
}
}
```


<-- textDocument/didOpen

```nix file:///all-used.nix
{x, y}: x + y
```

<-- textDocument/codeAction(2)


```json
{
"jsonrpc":"2.0",
"id":2,
"method":"textDocument/codeAction",
"params":{
"textDocument":{
"uri":"file:///all-used.nix"
},
"range":{
"start":{
"line": 0,
"character": 4
},
"end":{
"line":0,
"character": 5
}
},
"context":{
"diagnostics":[],
"triggerKind":2
}
}
}
```

No remove unused formal action should be offered since all formals are used.

```
CHECK: "id": 2,
CHECK-NEXT: "jsonrpc": "2.0",
CHECK-NEXT: "result": []
```

```json
{"jsonrpc":"2.0","method":"exit"}
```
70 changes: 70 additions & 0 deletions nixd/tools/nixd/test/code-action/remove-unused-formal/basic.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
# RUN: nixd --lit-test < %s | FileCheck %s

Test basic `remove unused formal` action.

<-- initialize(0)

```json
{
"jsonrpc":"2.0",
"id":0,
"method":"initialize",
"params":{
"processId":123,
"rootPath":"",
"capabilities":{
},
"trace":"off"
}
}
```


<-- textDocument/didOpen

```nix file:///basic.nix
{x, y}: x
```

<-- textDocument/codeAction(2)


```json
{
"jsonrpc":"2.0",
"id":2,
"method":"textDocument/codeAction",
"params":{
"textDocument":{
"uri":"file:///basic.nix"
},
"range":{
"start":{
"line": 0,
"character": 4
},
"end":{
"line":0,
"character": 5
}
},
"context":{
"diagnostics":[],
"triggerKind":2
}
}
}
```

The action should remove `, y` from the formals since `y` is unused.

```
CHECK: "id": 2,
CHECK: "isPreferred": true,
CHECK: "kind": "quickfix",
CHECK: "title": "remove unused formal `y`"
```

```json
{"jsonrpc":"2.0","method":"exit"}
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
# RUN: nixd --lit-test < %s | FileCheck %s

Test removing the first formal (special case for comma handling).

<-- initialize(0)

```json
{
"jsonrpc":"2.0",
"id":0,
"method":"initialize",
"params":{
"processId":123,
"rootPath":"",
"capabilities":{
},
"trace":"off"
}
}
```


<-- textDocument/didOpen

```nix file:///first-formal.nix
{x, y}: y
```

<-- textDocument/codeAction(2)


```json
{
"jsonrpc":"2.0",
"id":2,
"method":"textDocument/codeAction",
"params":{
"textDocument":{
"uri":"file:///first-formal.nix"
},
"range":{
"start":{
"line": 0,
"character": 1
},
"end":{
"line":0,
"character": 2
}
},
"context":{
"diagnostics":[],
"triggerKind":2
}
}
}
```

The action should remove `x, ` from the formals since `x` is unused.

```
CHECK: "id": 2,
CHECK: "isPreferred": true,
CHECK: "kind": "quickfix",
CHECK: "title": "remove unused formal `x`"
```

```json
{"jsonrpc":"2.0","method":"exit"}
```
Loading