Skip to content

/ clojure-mode

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

Jump to bottom

namespace caching breaks updating namespace name #480

Open
dpsutton opened this issue Jul 28, 2018 · 5 comments
Open

namespace caching breaks updating namespace name #480

dpsutton opened this issue Jul 28, 2018 · 5 comments
Labels
bug enhancement

Comments

@dpsutton
Copy link
Contributor

@dpsutton dpsutton commented Jul 28, 2018 

 (defun clojure-update-ns ()
      "Update the namespace of the current buffer.
    Useful if a file has been renamed."
      (interactive)
      (let ((nsname (funcall clojure-expected-ns-function)))
        (when nsname
          (save-excursion
            (save-match-data
              (if (clojure-find-ns)
                  (progn
                    (replace-match nsname nil nil nil 4)
                    (message "ns form updated to `%s'" nsname)
                    (setq clojure-cached-ns nsname))
                (error "Namespace not found")))))))

This is the helpful function to update the namespace and reset the cache for namespaces. Unfortunately it relies on the replace match generated in clojure-find-ns. The problem is that this match does not happen if namespace caching is enbled.

    (defun clojure-find-ns ()
      "Return the namespace of the current Clojure buffer.
    Return the namespace closest to point and above it.  If there are
    no namespaces above point, return the first one in the buffer.

    The results will be cac(match-string-no-properties 4)hed if `clojure-cache-ns' is set to t."
      (if (and clojure-cache-ns clojure-cached-ns)
          clojure-cached-ns
        (let ((ns (save-excursion
                    (save-restriction
                      (widen)

                      ;; Move to top-level to avoid searching from inside ns
                      (ignore-errors (while t (up-list nil t t)))

                      ;; The closest ns form above point.
                      (when (or (re-search-backward clojure-namespace-name-regex nil t)
                                ;; Or any form at all.
                                (and (goto-char (point-min))
                                     (re-search-forward clojure-namespace-name-regex nil t)))
                        (match-string-no-properties 4))))))
          (setq clojure-cached-ns ns)
          ns)))

Note the top level bails if the cache is already set and therefore does not call (match-string-no-properties 4) yielding an error.

This leaves an unfortunate catch-22: if you use the cache, you need to update it with update-ns. If you update the ns with the cache on, it doesn't create the match to update the cache.
@dpsutton
Copy link
Contributor Author

@dpsutton dpsutton commented Jul 28, 2018

This also affects clojure-sort-ns except not tragically. It calls (comment-normalize-vars) at the top which creates match data which usually coincides with the ns form. But if you put another form before the ns form this breaks.
@dpsutton
Copy link
Contributor Author

@dpsutton dpsutton commented Jul 28, 2018

This also affects CIDER as well.

    (defun cider-eval-ns-form ()
      "Evaluate the current buffer's namespace form."
      (interactive)
      (when (clojure-find-ns)
        (save-excursion
          (goto-char (match-beginning 0))
          (cider-eval-defun-at-point))))

    (defun cider-ns-form ()
      "Retrieve the ns form."
      (when (clojure-find-ns)
        (save-excursion
          (goto-char (match-beginning 0))
          (cider-defun-at-point))))

The caching breaks the contract of side-effecting the match data unfortunately.
@vspinu
Copy link
Contributor

@vspinu vspinu commented Jul 28, 2018

Extracting the search logic into clojure-goto-ns would be a way to go here.
@bbatsov
Copy link
Member

@bbatsov bbatsov commented Jul 29, 2018

Yeah, probably we need two functions indeed - one that finds the form and another that simply returns the current namespace. Right now things are way too convoluted.
@bbatsov
Copy link
Member

@bbatsov bbatsov commented Aug 18, 2018

@dpsutton Interested in tackling this?

I was thinking of adding a clojure-current-ns function that the caching would only apply to and rename the current function to clojure-find-ns-form (or something like this). There are two approaches we can take with it:

  • have it return some richer datastructure like the location of the ns form and it's various components. We should also add another function that can modify an ns form based on this data.
  • keep the current behaviour
  • something better I can't think of :-)

I'd really love for us to get rid of relying on the implementation details and the regexp matching, that's for sure. Won't have time for this for a while, but as it's not complex and you're pretty familiar with the matter I hope you'd "volunteer" to improve this. 😉
@bbatsov bbatsov mentioned this issue Sep 23, 2018
Closed
@jr0cket jr0cket mentioned this issue Jul 7, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug enhancement
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
@bbatsov @vspinu @dpsutton
You can’t perform that action at this time.