feat(code-action): add remove unused formal parameter quickfix#778
feat(code-action): add remove unused formal parameter quickfix#778takeokunn wants to merge 2 commits into
Conversation
|
@inclyc That said, I agree that attaching the fix directly to the diagnostic in VariableLookup.cpp would be the more natural fit for this case, since the Formals context is already available at diagnostic emission time — unlike AddToFormals, which needs ParentMap to find the enclosing lambda. What would you prefer?
|
|
I believe this PR (#778) should be refactored to be primarily based on diagnostic information in The reasons are: (1) The existing structure based on fix-it hints is very prevalent and common in (2) On the other hand, if a formal parameter is unused, automatically removing it feels natural and provides a unique solution. In contrast, "AddToFormals" appears to be "one of several possible solutions," which makes it more suitable to be implemented as a separate Code Action. Furthermore, considering "technical perfectionism," open-source communities generally don't share the same sense of "urgency" found in commercial software. If you're willing, I would also prefer to maintain a more robust project rather than leaving this as technical debt indefinitely. Implementing this specific task within Code Actions feels somewhat "awkward" and counter-intuitive. この PR (#778) は 理由は以下の通りです： (1) (2) 一方で、形式引数が**未使用（unused）**である場合、それを自動的に削除することは自然であり、かつ解決策も一意です。それに対して "AddToFormals" は「複数の解決策のうちの一つ」という側面が強いため、個別の Code Action として記述する方が適しています。 また、「技術的なこだわり（技術潔癖）」という面から見ても、オープンソースコミュニティは通常、商用ソフトウェアのような「急ぎの対応」を追求する場ではないと考えています。もし許されるのであれば、これを技術負債として放置するのではなく、より堅牢なプロジェクトとして維持していきたいです。この機能を Code Actions の中で実装し続けるのは、どこか「不自然（拧巴）」で無理があると感じています。 |
|
@inclyc |
takeokunn
force-pushed
the
feature/add-code-action-12
branch
2 times, most recently
from
ea556ea to
8558171
Compare
…eters Attach fix-it hints to unused formal parameter diagnostics in VariableLookup, using Sema::removeFormal() to generate TextEdits. The Controller simply converts these to LSP code actions. Handles comma placement for first, middle, and last formals, as well as formals with default values and ellipsis.
takeokunn
force-pushed
the
feature/add-code-action-12
branch
from
8558171 to
14e726b
Compare
|
@inclyc |
| class Fix { | ||
| std::vector<TextEdit> Edits; | ||
| std::string Message; | ||
| bool Preferred = false; |
There was a problem hiding this comment.
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 の場合のデフォルト挙動について、どのようにお考えでしょうか？
|
Also see #796 |
3 participants
Summary
Add code action to remove unused formal parameters from lambda expressions.
Example Transformation:
{x, y}: x{x}: x{x, y, z}: x + z{x, z}: x + z{x ? 1, y}: y{y}: y{x, y, ...}: y{y, ...}: yChanges
nixd/lib/Controller/CodeActions/RemoveUnusedFormal.hnixd/lib/Controller/CodeActions/RemoveUnusedFormal.cppnixd/lib/Controller/CodeAction.cppnixd/lib/meson.buildBehavior
Action offered when:
DK_UnusedDefLambdaNoArg_FormalorDK_UnusedDefLambdaWithArg_FormalexistsAction NOT offered when:
...)Handles comma placement:
{x, y}:→ removexand next formal's leading comma{x, y}:→ removey(comma is part of its range)Test Plan
basic.md: Remove last formalfirst-formal.md: Remove first formal (special comma handling)middle-formal.md: Remove middle formalwith-default.md: Remove formal with default valuewith-ellipsis.md: Remove formal when ellipsis presentall-used-negative.md: Negative - all formals used (no action offered)Related