Fork me on GitHub
#lsp
<
2021-07-15
>
lassemaatta13:07:51

(context: I'm taking my first steps in learning emacs & clojure-lsp) I noticed that although navigating between definitions and references (e.g. function definitions and call sites) works quite nicely, I seem to be unable to navigate from the concrete Java type of a record to the record definition. For example, if I define a record in one namespace and import it (or rather the generated Java class to be precise) in another namespace (e.g. for using it in a extend-type ) -> should I be able to somehow navigate back to the defrecord form from the corresponding java class or is this something that clojure-lsp doesn't support?

ericdallo13:07:15

clojure-lsp is quite limited related to java classes manipulation, as clj-kondo doesn't have a deep java/classes analysis

ericdallo13:07:36

could you paste a minimal code just to confirm that?

borkdude13:07:04

> concrete Java type of a record to the record definition. I think clj-kondo should be able to support this

borkdude13:07:18

as it's just record names right

ericdallo13:07:24

Yeah, that 'd be great, navigation from record usages to definition

borkdude13:07:04

Feel free to post some repro code

borkdude13:07:08

at clj-kondo on Github

ericdallo13:07:57

@U0178V2SLAY could you help with the minimal repro please?

lassemaatta13:07:40

the above may or may not be valid clojure, as I don't have a repl at hand 🙂

borkdude13:07:51

why don't you write the record as:

(defrecord SomeRecord [foo bar] SomeProtocol (do-stuff [this] 42))
That is probably the more idiomatic way

ericdallo13:07:59

@U0178V2SLAY your code actually works to me, I can navigate to definition and references of both record, protocol and the extend-type

lassemaatta13:07:19

Ah, most likely a case of pebkac then. And yes, I could most likely solve this by merging the namespaces.

borkdude13:07:12

@UKFSJSM38 from where to where are you navigation to the record definition?

borkdude13:07:30

for me it doesn't work

borkdude13:07:30

and I would be surprised if it did

ericdallo13:07:31

I tried with different namespaces and it worked as well. if on namespace my.other-namespace I navigate from the SomeRecord symbol, I go to my.fabulous-namespace/SomeRecord

borkdude13:07:18

can you record a gif? I don't get it. I will do it too.

👍 2
lassemaatta13:07:46

(why am I using extend-typeinstead of implementing the protocol inline? good question. I'm playing around with the hexagonal architecture and trying to learn it. I thought of implementing the ports as protocols and actual adapters as records. At the moment I'm thinking it might be cleaner to avoid complecting the different protocols and records directly and instead use separate namespaces to enhance the different records with appropriate protocols. This may turn out to be a bad idea and thus I may merge the protocol implementations inline with the records.)

borkdude13:07:30

it's the first time I've seen this construction

borkdude13:07:36

but it's valid Clojure

lassemaatta13:07:08

I'll take that as a compliment 😄

👍 2
ericdallo13:07:05

find definition works the references doesn't work though, I managed to make it work somehow before, but now it doesn't. (I renamed the ns to match my current proejct, not sure this related to issue)

ericdallo13:07:48

it's weird the definition find the defrecord, but there is no references to it, looks a corner case bug

borkdude13:07:25

you should use the underscore namespace name

borkdude13:07:32

as what you wrote now isn't valid clojure

ericdallo13:07:53

oh, in the import, you are right

borkdude13:07:58

it's interesting that this subtle difference fixes the problem though

borkdude13:07:05

it seems that it's only a tiny bugfix in clj-kondo then

borkdude13:07:24

please post issues with what should get fixed + optional PRs

ericdallo13:07:25

it works too

borkdude13:07:35

as this is a correctness bug I will give it the highest priority

Joshua Suskalo13:07:45

This hexagonal architecture looks like a cousin to polylith, but with less of an eye for deployment.

ericdallo13:07:59

with underline it works as well

ericdallo13:07:17

@U04V15CAJ did you find the bug already? I'm not sure what is the issue yet

borkdude13:07:47

oh with the underline my navigation to the record from the imported class didn't work. are you sure you're not connected to CIDER or something?

ericdallo13:07:50

let me try with the original namespace name

ericdallo13:07:17

works too, odd

borkdude13:07:59

can you try this code, literally?

(ns my.fabulous-namespace)

(defrecord SomeRecord [foo bar])

(defprotocol SomeProtocol
  (do-stuff [this]))

(ns my.other-namespace
  (:require [my.fabulous-namespace :as foobar])
  (:import [my.fabulous_namespace SomeRecord])) ;; same

(extend-type SomeRecord ;; no navigation to record
  foobar/SomeProtocol ;; this worked
  (do-stuff [this]
    42))

borkdude13:07:10

I'm not in a .clj-kondo project

borkdude13:07:02

doesn't make a difference if I do

ericdallo13:07:22

removed kondo and lsp cache, and with your code still works 😅

borkdude13:07:18

maybe you're using a clj-kondo from the future?

ericdallo13:07:40

still I think this could be improved, there is no hover information on the SomeRecord inside the import form, but definition works

ericdallo13:07:02

hahah, I'm using clojure-lsp from master, which didn't bump clj-kondo, it's the same from latest release

borkdude13:07:09

are you absolutely sure you have no other tools which just make a guess to jump to an earlier symbol in the file or so?

borkdude13:07:33

what if you split these namespaces into two different files?

ericdallo13:07:12

yeah, it was doom using ripgrep fallback because lsp-find-definition was not working facepalm

ericdallo13:07:19

I forgot about that one

ericdallo13:07:53

so yeah, the find-definition to the defrecord doesn't work for LSP indeed

ericdallo13:07:30

and I think it's related with the _ indeed, we can confirm that checking the analysis filename

ericdallo13:07:31

acutally, not related with filename, I meant, if we change the _ to - which is not a valid clojure code, it works though

ericdallo13:07:52

also the references/code lens get fixed

borkdude13:07:28

yes, it's a subtle bug. clj-kondo just has to "try" if this might be a record or deftype

ericdallo14:07:04

yes, found the issue, it's in the :to

ericdallo14:07:08

my.fabulous_namespace

ericdallo14:07:19

{:name-end-col 24,
  :name-end-row 12,
  :name-row 12,
  :name SomeRecord,
  :filename "/home/greg/dev/clojure-lsp/src/clojure_lsp/foo.clj",
  :from my.other-namespace,
  :col 14,
  :name-col 14,
  :bucket :var-usages,
  :row 12,
  :to my.fabulous_namespace}

ericdallo14:07:43

actually, not sure it's a clj-kondo bug

borkdude14:07:43

actually defrecord doesn't really create a var SomeRecord

borkdude14:07:50

but whatever :)

borkdude14:07:03

I think we can improve this somehow

ericdallo14:07:52

yes, my point is

ericdallo14:07:23

everything inside a :import, should be a java package like right?

ericdallo14:07:31

so the :to is actually correct?

ericdallo14:07:50

if that would be a java package, the :to would be indeed the package with _ not -

ericdallo14:07:15

unless clj-kondo recognize that is a defrecord and change the :to only for those

ericdallo14:07:21

but not for java packages

borkdude14:07:43

right, we should improve this specifically for defrecord and deftype

borkdude14:07:51

but it's kind of a niche use case I'd say

lassemaatta14:07:36

Thanks to both of you for investigating this 👍

ericdallo14:07:02

I'll open the issue on clj-kondo, thanks for the report!

👍 2
ericdallo20:09:28

@U0178V2SLAY latest clojure-lsp nightly should work

👍 1
borkdude20:09:45

wow, a slack thread where the previous reply was 1 year ago :)

😅 1
ericdallo13:09:45

At least ended with a happy ending 😂

NoahTheDuke15:07:44

cycle-coll doesn’t work if my collection has one element and I have cycled it into a map

ericdallo15:07:22

It's because it's a invalid clojure code right?

NoahTheDuke15:07:33

probably, yeah

NoahTheDuke15:07:43

if I add an extra element to make it a valid map, it works again

ericdallo16:07:05

It's because rewrite-clj can't parse and invalid code

ericdallo16:07:17

Not sure the best approach here, probably make rewrite-clj understand that code somehow

👍 2
NoahTheDuke16:07:51

would the best current solution be to remove cycling into maps? to me, it makes sense to cycle between the others (none have special restrictions about their creation) whereas maps are special because of their requirement for even entries

ericdallo17:07:33

Hum, I think this is still useful for when user wants to cycle to map and fix the key val pairs later

👍 2
ericdallo17:07:07

Also @UEENNMX0T, you probably is calling this refactoring manually right?

ericdallo17:07:21

I'd suggest you use the code actions

ericdallo17:07:49

as clojure-lsp already return 3 code actions when iniside a coll: • cycle to map • cycle to vec • cycle to set for example

ericdallo17:07:54

and user chooses which one

NoahTheDuke17:07:21

yeah, i normally do that, right now i’ve exposed the refactoring commands like lsp-mode (`lsp-clojure-cycle-coll`, for example) and am making sure they all work

ericdallo17:07:22

I don't see vim users use code actions normally, most of the time because they don't know it exists

ericdallo17:07:47

> yeah, i normally do that, right now i’ve exposed the refactoring commands like lsp-mode (`lsp-clojure-cycle-coll`, for example) and am making sure they all work Looks a good approach

Proctor19:07:01

> I don’t see vim users use code actions normally, most of the time because they don’t know it exists I just stumbled across some refactoring with the code actions in (Neo)Vim, where I went to using “execute command” Is there a way to code action and give the action for it to use if it makes sense?

Proctor19:07:08

was trying to hotkey things like inline-symbol or extract function or the let bindings, where I prompt for the name….

ericdallo19:07:44

yeah, ATM the code action doesn't support ask to user some input, that's why it uses mocked function names

Proctor12:07:08

just making sure I wasn’t missing something in the way I was doing it from NeoVim and it’s LSP client

👍 2
NoahTheDuke16:07:30

I’m writing some documentation for my forthcoming coc.nvim plugin for clojure-lsp, and I can’t quite figure out what expand-let does

ericdallo16:07:28

It should do this

NoahTheDuke16:07:02

ahhh interesting. doesn’t seem to quite do that on my machine. i’ll see if i can record to demonstrate

ericdallo16:07:07

Maybe it's missing improve the docs for those refactorings?

ericdallo16:07:33

Maybe it's a bug, it's not a common used feature

NoahTheDuke16:07:44

https://clojure-lsp.github.io/clojure-lsp/features/#refactoring doesn’t have any docs, and this one in particular doesn’t have a demonstration gif, so the one you linked probably just needs to be added to that page

NoahTheDuke16:07:08

here’s what i see when i use it

ericdallo16:07:45

Yeah, almost sure it's a bug

ericdallo16:07:24

feel free to open an issue or PR fixing it 🙂

NoahTheDuke16:07:40

sure thing! i’ll poke around and see what i can do

👍 2