search

clojure-lsp / clojure-lsp

Clojure & ClojureScript Language Server (LSP) implementation
https://clojure-lsp.io
MIT License
1.17k stars 153 forks source link

Dragging a file and the ensuing refactoring breaks destructuring of auto-resolved keywords #1659

Open wevre opened 1 year ago

wevre commented 1 year ago

Describe the bug Dragging a file to a new folder triggers some refactoring which breaks auto-resolved keyword destructuring in function parameters.

To Reproduce I'm doing this in VS Code with Calva. Steps to reproduce the behavior:

  1. Create an empty project.
  2. Add a bare-bones deps.edn file: 
    {:paths ["src"]}
  3. Add a file at src/acme/ns1/test.clj: 
    
(ns acme.ns1.test)

(defn my-fn [{::keys [payload]}] (println "this is my-fn in the ns1 namespace"))

4. Create a second file at `src/acme/ns2/refer.clj`:
```clj
(ns acme.ns2.refer
  (:require [acme.ns1.test :as test]))

(defn other-fn [{::test/keys [payload]}]
  (println "This is the other fn"))
  1. Drag test.clj file from the ns1 folder into same ns2 folder as refer.clj.
  2. The test.clj file now looks like this (with some comments about what is right and what is not): 
    
(ns acme.ns2.test)
;;  ^^^^^^^^^^^^^ This change is correct.

(def foo "A string")

(defn my-fn [{::keys [:acme.ns2.test/payload]}] ;; ^^^^^^^^^^^^^^^ Strange, none of this should be here. (println "this is my-fn in the ns1 namespace"))

7. Similarly, the `refer.clj` file now looks like this:
```clj
(ns acme.ns2.refer
  (:require [acme.ns2.test :as test]))
;;           ^^^^^^^^^^^^^ This change is correct.

(defn other-fn [{::test/keys [:acme.ns2.test/payload]}]
  ;;                          ^^^^^^^^^^^^^^^ This should not be here.
  (println "This is the other fn"))

Expected behavior See the final examples above in steps 6 and 7 for some comments about what changed incorrectly.

Log - client <-> server 
[Trace - 10:10:44 AM] Sending request 'workspace/willRenameFiles - (36)'.
Params: {
    "files": [
        {
            "oldUri": "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns1/test.clj",
            "newUri": "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/test.clj"
        }
    ]
}

[Trace - 10:10:44 AM] Received response 'workspace/willRenameFiles - (36)' in 3ms.
Result: {
    "documentChanges": [
        {
            "textDocument": {
                "version": 126,
                "uri": "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns1/test.clj"
            },
            "edits": [
                {
                    "range": {
                        "start": {
                            "line": 0,
                            "character": 4
                        },
                        "end": {
                            "line": 0,
                            "character": 17
                        }
                    },
                    "newText": "acme.ns2.test"
                },
                {
                    "range": {
                        "start": {
                            "line": 4,
                            "character": 22
                        },
                        "end": {
                            "line": 4,
                            "character": 29
                        }
                    },
                    "newText": ":acme.ns2.test/payload"
                }
            ]
        },
        {
            "textDocument": {
                "version": 145,
                "uri": "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/refer.clj"
            },
            "edits": [
                {
                    "range": {
                        "start": {
                            "line": 1,
                            "character": 13
                        },
                        "end": {
                            "line": 1,
                            "character": 26
                        }
                    },
                    "newText": "acme.ns2.test"
                },
                {
                    "range": {
                        "start": {
                            "line": 3,
                            "character": 30
                        },
                        "end": {
                            "line": 3,
                            "character": 37
                        }
                    },
                    "newText": ":acme.ns2.test/payload"
                }
            ]
        }
    ]
}

[Trace - 10:10:44 AM] Sending notification 'textDocument/didChange'.
Params: {
    "textDocument": {
        "uri": "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/refer.clj",
        "version": 146
    },
    "contentChanges": [
        {
            "text": "(ns acme.ns2.refer\n  (:require [acme.ns2.test :as test]))\n\n(defn other-fn [{::test/keys [:acme.ns2.test/payload]}]\n  (println \"This is the other fn\"))\n"
        }
    ]
}

[Trace - 10:10:44 AM] Sending notification 'textDocument/didChange'.
Params: {
    "textDocument": {
        "uri": "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns1/test.clj",
        "version": 127
    },
    "contentChanges": [
        {
            "text": "(ns acme.ns2.test)\n\n(def foo \"A string\")\n\n(defn my-fn [{::keys [:acme.ns2.test/payload]}]\n  (println \"this is my-fn in the ns1 namespace\"))\n"
        }
    ]
}

[Trace - 10:10:44 AM] Sending notification 'textDocument/didClose'.
Params: {
    "textDocument": {
        "uri": "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns1/test.clj"
    }
}

[Trace - 10:10:44 AM] Sending notification 'textDocument/didOpen'.
Params: {
    "textDocument": {
        "uri": "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/test.clj",
        "languageId": "clojure",
        "version": 1,
        "text": "(ns acme.ns2.test)\n\n(def foo \"A string\")\n\n(defn my-fn [{::keys [:acme.ns2.test/payload]}]\n  (println \"this is my-fn in the ns1 namespace\"))\n"
    }
}

[Trace - 10:10:44 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {
    "uri": "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns1/test.clj",
    "diagnostics": []
}

[Trace - 10:10:44 AM] Sending request 'textDocument/codeAction - (37)'.
Params: {
    "textDocument": {
        "uri": "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/test.clj"
    },
    "range": {
        "start": {
            "line": 0,
            "character": 0
        },
        "end": {
            "line": 0,
            "character": 0
        }
    },
    "context": {
        "diagnostics": [],
        "triggerKind": 2
    }
}

[Trace - 10:10:44 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {
    "uri": "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/refer.clj",
    "diagnostics": [
        {
            "range": {
                "start": {
                    "line": 3,
                    "character": 6
                },
                "end": {
                    "line": 3,
                    "character": 14
                }
            },
            "tags": [
                1
            ],
            "message": "Unused public var 'acme.ns2.refer/other-fn'",
            "code": "clojure-lsp/unused-public-var",
            "langs": [],
            "severity": 3,
            "source": "clojure-lsp"
        },
        {
            "range": {
                "start": {
                    "line": 3,
                    "character": 30
                },
                "end": {
                    "line": 3,
                    "character": 52
                }
            },
            "tags": [
                1
            ],
            "message": "unused binding payload",
            "code": "unused-binding",
            "langs": [],
            "severity": 2,
            "source": "clj-kondo"
        }
    ]
}

[Trace - 10:10:44 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {
    "uri": "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns1/test.clj",
    "diagnostics": [
        {
            "range": {
                "start": {
                    "line": 0,
                    "character": 4
                },
                "end": {
                    "line": 0,
                    "character": 17
                }
            },
            "tags": [],
            "message": "Namespace name does not match file name: acme.ns2.test",
            "code": "namespace-name-mismatch",
            "langs": [],
            "severity": 1,
            "source": "clj-kondo"
        },
        {
            "range": {
                "start": {
                    "line": 2,
                    "character": 5
                },
                "end": {
                    "line": 2,
                    "character": 8
                }
            },
            "tags": [
                1
            ],
            "message": "Unused public var 'acme.ns2.test/foo'",
            "code": "clojure-lsp/unused-public-var",
            "langs": [],
            "severity": 3,
            "source": "clojure-lsp"
        },
        {
            "range": {
                "start": {
                    "line": 4,
                    "character": 6
                },
                "end": {
                    "line": 4,
                    "character": 11
                }
            },
            "tags": [
                1
            ],
            "message": "Unused public var 'acme.ns2.test/my-fn'",
            "code": "clojure-lsp/unused-public-var",
            "langs": [],
            "severity": 3,
            "source": "clojure-lsp"
        },
        {
            "range": {
                "start": {
                    "line": 4,
                    "character": 22
                },
                "end": {
                    "line": 4,
                    "character": 44
                }
            },
            "tags": [
                1
            ],
            "message": "unused binding payload",
            "code": "unused-binding",
            "langs": [],
            "severity": 2,
            "source": "clj-kondo"
        }
    ]
}

[Trace - 10:10:44 AM] Received response 'textDocument/codeAction - (37)' in 97ms.
Result: [
    {
        "title": "Change coll to vector",
        "kind": "refactor",
        "command": {
            "title": "Change coll",
            "command": "change-coll",
            "arguments": [
                "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/test.clj",
                0,
                0,
                "vector"
            ]
        }
    },
    {
        "title": "Change coll to set",
        "kind": "refactor",
        "command": {
            "title": "Change coll",
            "command": "change-coll",
            "arguments": [
                "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/test.clj",
                0,
                0,
                "set"
            ]
        }
    },
    {
        "title": "Change coll to map",
        "kind": "refactor",
        "command": {
            "title": "Change coll",
            "command": "change-coll",
            "arguments": [
                "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/test.clj",
                0,
                0,
                "map"
            ]
        }
    },
    {
        "title": "Move to let",
        "kind": "refactor.extract",
        "command": {
            "title": "Move to let",
            "command": "move-to-let",
            "arguments": [
                "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/test.clj",
                0,
                0,
                "new-binding"
            ]
        }
    },
    {
        "title": "Extract to def",
        "kind": "refactor.extract",
        "command": {
            "title": "Extract to def",
            "command": "extract-to-def",
            "arguments": [
                "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/test.clj",
                0,
                0,
                null
            ]
        }
    },
    {
        "title": "Move another expression to get/get-in",
        "kind": "refactor.rewrite",
        "command": {
            "title": "Move another expression to get/get-in",
            "command": "get-in-more",
            "arguments": [
                "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/test.clj",
                0,
                0
            ]
        }
    },
    {
        "title": "Move all expressions to get/get-in",
        "kind": "refactor.rewrite",
        "command": {
            "title": "Move all expressions to get/get-in",
            "command": "get-in-all",
            "arguments": [
                "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/test.clj",
                0,
                0
            ]
        }
    },
    {
        "title": "Sort list",
        "kind": "refactor.rewrite",
        "command": {
            "title": "Sort list",
            "command": "sort-clauses",
            "arguments": [
                "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/test.clj",
                0,
                0
            ]
        }
    },
    {
        "title": "Drag forward",
        "kind": "refactor.rewrite",
        "command": {
            "title": "Drag forward",
            "command": "drag-forward",
            "arguments": [
                "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/test.clj",
                0,
                0
            ]
        }
    },
    {
        "title": "Introduce let",
        "kind": "refactor.extract",
        "command": {
            "title": "Introduce let",
            "command": "introduce-let",
            "arguments": [
                "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/test.clj",
                0,
                0,
                "new-binding"
            ]
        }
    },
    {
        "title": "Clean namespace",
        "kind": "source.organizeImports",
        "command": {
            "title": "Clean namespace",
            "command": "clean-ns",
            "arguments": [
                "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/test.clj",
                0,
                0
            ]
        }
    }
]

[Trace - 10:10:44 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {
    "uri": "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/test.clj",
    "diagnostics": [
        {
            "range": {
                "start": {
                    "line": 2,
                    "character": 5
                },
                "end": {
                    "line": 2,
                    "character": 8
                }
            },
            "tags": [
                1
            ],
            "message": "Unused public var 'acme.ns2.test/foo'",
            "code": "clojure-lsp/unused-public-var",
            "langs": [],
            "severity": 3,
            "source": "clojure-lsp"
        },
        {
            "range": {
                "start": {
                    "line": 4,
                    "character": 6
                },
                "end": {
                    "line": 4,
                    "character": 11
                }
            },
            "tags": [
                1
            ],
            "message": "Unused public var 'acme.ns2.test/my-fn'",
            "code": "clojure-lsp/unused-public-var",
            "langs": [],
            "severity": 3,
            "source": "clojure-lsp"
        },
        {
            "range": {
                "start": {
                    "line": 4,
                    "character": 22
                },
                "end": {
                    "line": 4,
                    "character": 44
                }
            },
            "tags": [
                1
            ],
            "message": "unused binding payload",
            "code": "unused-binding",
            "langs": [],
            "severity": 2,
            "source": "clj-kondo"
        }
    ]
}

[Trace - 10:10:44 AM] Sending notification 'workspace/didChangeWatchedFiles'.
Params: {
    "changes": [
        {
            "uri": "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/test.clj",
            "type": 1
        },
        {
            "uri": "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns1/test.clj",
            "type": 3
        }
    ]
}

[Trace - 10:10:44 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {
    "uri": "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns1/test.clj",
    "diagnostics": []
}

[Trace - 10:10:45 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {
    "uri": "file:///Users/wevrem/Documents/sites/byt.com/test-clojure-lsp-refactor/src/acme/ns2/test.clj",
    "diagnostics": [
        {
            "range": {
                "start": {
                    "line": 0,
                    "character": 4
                },
                "end": {
                    "line": 0,
                    "character": 17
                }
            },
            "tags": [],
            "message": "Namespace name does not match file name: acme.ns1.test",
            "code": "namespace-name-mismatch",
            "langs": [],
            "severity": 1,
            "source": "clj-kondo"
        },
        {
            "range": {
                "start": {
                    "line": 2,
                    "character": 5
                },
                "end": {
                    "line": 2,
                    "character": 8
                }
            },
            "tags": [
                1
            ],
            "message": "Unused public var 'acme.ns1.test/foo'",
            "code": "clojure-lsp/unused-public-var",
            "langs": [],
            "severity": 3,
            "source": "clojure-lsp"
        },
        {
            "range": {
                "start": {
                    "line": 4,
                    "character": 6
                },
                "end": {
                    "line": 4,
                    "character": 11
                }
            },
            "tags": [
                1
            ],
            "message": "Unused public var 'acme.ns1.test/my-fn'",
            "code": "clojure-lsp/unused-public-var",
            "langs": [],
            "severity": 3,
            "source": "clojure-lsp"
        },
        {
            "range": {
                "start": {
                    "line": 4,
                    "character": 22
                },
                "end": {
                    "line": 4,
                    "character": 29
                }
            },
            "tags": [
                1
            ],
            "message": "unused binding payload",
            "code": "unused-binding",
            "langs": [],
            "severity": 2,
            "source": "clj-kondo"
        }
    ]
}

User details (please complete the following information):

Additional context This happened in a large project I was working on. What I created above was just a simple example to illustrate the problem.

ericdallo commented 1 year ago

Thanks for the report, looks like a bug indeed, we will probably need to add a check when renaming the auto resolved keywords for when renaming the ns as well