Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement find-usages #51

Closed
bbatsov opened this issue May 1, 2019 · 7 comments · Fixed by #141
Closed

Implement find-usages #51

bbatsov opened this issue May 1, 2019 · 7 comments · Fixed by #141
Labels
enhancement Improvement to an existing feature

Comments

@bbatsov
Copy link
Member

bbatsov commented May 1, 2019

Cognitect recently released a new version of REBL that has a find usages feature. @SevereOverfl0w asked Rich about the implementation and Rich was kind enough to share the crux of it:

(defn fdeps [val]
    (set (some->> val class .getDeclaredFields
                  (keep (fn [^java.lang.reflect.Field f]
                          (or (and (identical? clojure.lang.Var (.getType f))
                                   (java.lang.reflect.Modifier/isPublic (.getModifiers f))
                                   (java.lang.reflect.Modifier/isStatic (.getModifiers f))
                                   (-> f .getName (.startsWith "const__"))                                  
                                   (.get f val))
                              nil))))))

Some notes from Rich:

It reflects against the fn classes themselves, every closure put fields in for vars,
that’s what makes vars fast.
So these are ‘runtime’ deps, not source deps. You won’t see inlined fns or macros.
fdeps works on the fn value, not the var.

So people have already been building cool things with that idea https://gist.github.com/vlaaad/0350b61e127e82165d195b490999ec0a

So, what we need to do now is add this to Orchard and then wrap it in CIDER nREPL. That should be pretty easy and have a huge impact for the end users.

@bbatsov bbatsov added the enhancement Improvement to an existing feature label May 1, 2019
@SevereOverfl0w
Copy link
Collaborator

We may want to consider removing the isPublic constraint.

bbatsov added a commit that referenced this issue May 1, 2019
This functionality is limited only to functions in loaded namespaces,
but it still pretty handy.

One notable problem that we should ideally solve is that `fdeps`
doesn't track function dependencies in lambdas.
@bbatsov
Copy link
Member Author

bbatsov commented May 1, 2019

I've hacked together a simple prototype of the idea. It works pretty well IMO (especially compared to nothing), but it has one caveat - fdeps doesn't find functions used in lambdas. E.g.

(defn- dummy-fn [x]
  (map #(* % 2) (filter even? (range 1 10))))

Here * is not registered. I wonder if we can account for this somehow...

We may want to consider removing the isPublic constraint.

I'm not sure what exactly this filters, but it's certainly not public vars. I see that currently private function deps are part of the results.

@SevereOverfl0w
Copy link
Collaborator

I wonder if the nested function ends up in a field in the outer function?

@bbatsov
Copy link
Member Author

bbatsov commented May 2, 2019

I can only hope so. Haven't played much with this yet.

@bbatsov
Copy link
Member Author

bbatsov commented May 2, 2019

It seems that the only reasonable way to get access to the lambdas would be to do a classpath search for the classes that they result in. Then we can get their fn object and get its dependencies.

@vemv
Copy link
Member

vemv commented Jan 5, 2022

I think that in principle the "REBL approach" is too hacky, it relies on compiler internals, which not only are subject to change but also might make for a worse solution - essentially our solution would be limited by whatever the compiler can offer.

If we needed an improvement, the compiler code is not something we can fork, and also any improvements could be as well rejected.

There's the alternative of tools.analyzer, as refactor-nrepl's find-usages uses. It's slow, side-effectful and there's no immediate plan to change that.

So that leaves clj-kondo as a viable option. I think it's a good investment: people are using clj-kondo / clojure-lsp because they 'just work' and false positives are rarer than one might think.

I summarized my thinking / tentative intentions in clojure-lsp/clojure-lsp#658. I keep believing invoking clj-kondo from the same JVM that hosts the repl is a very nice hybrid, and one that I have used for a few years now.

It also would be a pretty good thing for the community to consolidate the tooling landscape - less fragmented, with more manpower, and therefore higher-quality.

@bbatsov
Copy link
Member Author

bbatsov commented Jan 5, 2022

In general I agree with the reasoning, but I also don't see the harm in having some find-usages implementation that's basic, but doesn't require any additional setup/tools. That's the main reason I wrote this basic implementation back in the day.

I summarized my thinking / tentative intentions in clojure-lsp/clojure-lsp#658. I keep believing invoking clj-kondo from the same JVM that hosts the repl is a very nice hybrid, and one that I have used for a few years now.

Yeah, I also thought it'd be a good idea to use clj-kondo from nREPL, but never got to doing work in this direction.

@vemv vemv closed this as completed in #141 Jan 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants