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

resolve-missing doesn't find newly defined symbols due to cache #160

Closed
plexus opened this issue Apr 24, 2016 · 7 comments · Fixed by #161
Closed

resolve-missing doesn't find newly defined symbols due to cache #160

plexus opened this issue Apr 24, 2016 · 7 comments · Fixed by #161
Labels

Comments

@plexus
Copy link
Contributor

plexus commented Apr 24, 2016

I'm using resolve-missing a lot from clj-refactor.el to add missing require/refer statements to the namespace declaration.

I noticed that when defining new functions and using them from another namespace I had to require them manually, refactor-nrepl would not find them. It would work fine after restarting the Clojure process.

I finally traced it down to the fact that in refactor-nrepl.ns.slam.hound.regrow the symbols are cached, so anything defined/loaded later on gets ignored.

I worked around this by disabling the cache, and now it works great.

(add-hook 'nrepl-connected-hook
          (lambda ()
            (cider-nrepl-request:eval
             "(alter-var-root #'refactor-nrepl.ns.slam.hound.regrow/*cache* (constantly nil))"
             (lambda (x)))))

I would like to help to come up with a general solution, but I'm not sure what the best course of action is. Dropping the caching is one option, but I assume it's there for a reason. Alternatively it would need some kind of invalidation, maybe marking namespaces as "dirty" when evaluating forms in them.

Thoughts?

@expez
Copy link
Member

expez commented Apr 25, 2016

I've noticed the same and I tried taking a look at the cache code. After a quick glance I couldn't figure out why it doesn't work because it checks the cache first and then falls back to whatever it was gonna do without the cache in place.

When we copied this stuff from slamhound the cache was disabled and I only recently enabled it. I did this because it takes around 20s to scan for stuff on the clojure project at work, without caching.

If you want to investigate why 'plan b', when there's a cache miss, doesn't work as it should, that would be great.

@expez expez added the bug label Apr 25, 2016
@plexus
Copy link
Contributor Author

plexus commented Apr 25, 2016

I had a closer look. Candidates are found by calling (symbols->ns-syms) which returns a map of Symbol => set of namespace names, e.g.

{'join #{'clojure.set 'clojure.string}
 ,,,}

This whole map gets cached and reused the next time, the only "cache miss" is when this function has never been called before.

As a stopgap we could recompute this when there are no candidates found. This wouldn't work when using a var name that already exists in another namespace, but it would certainly be an improvement.

Ideally we would detect when new vars are defined and amend or expire the cache, but I don't suppose there's a var-defined-hook, that would be too easy :)

@expez
Copy link
Member

expez commented Apr 25, 2016

As a stopgap we could recompute this when there are no candidates found. This wouldn't work when using a var name that already exists in another namespace, but it would certainly be an improvement.

Yeah, that sounsd like a good idea! Care to take a stab at it?

@plexus
Copy link
Contributor Author

plexus commented Apr 25, 2016

Alright, I'll give it a shot. I still feel this is a very imperfect solution, as the few cases where it doesn't work will really puzzle people.

It seems nREPL in the end delegates to clojure.main/repl, we could wrap it and mark the corresponding namespaces as dirty.

(def dirty-ns (atom #{}))

(defn wrap-repl [f]
  (fn [& args]
    (swap! dirty-ns conj (ns-name *ns*))
    (apply f args)))

(alter-var-root #'clojure.main/repl wrap-repl)

What do you think?

@expez
Copy link
Member

expez commented Apr 25, 2016

What do you think?

If this works, it looks like a good way to invalidate certain parts of the cache 👍

plexus added a commit to plexus/refactor-nrepl that referenced this issue May 1, 2016
resolve-missing keeps a cache of known vars and types. Vars/types that
are added after the cache has been populated, either by eval-ing code,
or by hotloading new dependencies will be missing from the cache.

To find newly defined vars/types we instrument clojure.main/repl, and
mark any affected namespace as dirty. Dirty namespaces are always
re-scanned when invoking resolve-missing.

We currently don't bother clearing this list of dirty namespaces,
because multiple cache items would have to be updated in sync before we
can clear it. Having the majority of namespaces cached should still
provide a significant speedup.

When adding and hotloading a new dependency the cache is reset. The next
call to resolve-missing will rebuild the cache. This way all symbols on
the newly added classpath can be found.
@expez expez closed this as completed in #161 May 1, 2016
@benedekfazekas
Copy link
Member

@malcolmsparks want to reopen this or file a new one so I don't forget about it? ;)

@malcolmsparks
Copy link

Hi @benedekfazekas - I'm not sure this issue is the issue I'm having - so I'll raise a new one now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants