Fail patch at Emacs init but work fine if form evaluated by hand

[caadar]

Some time ago I modify helm-highlight-bookmark by el-patch and all was fine. Today I update Emacs packages including Helm and observe some nasty thing: this function work as before patching.

What I see:

1. Restart Emacs, describe-function helm-highlight-bookmark => "PATCHED!.." It seems OK, but wait...

2. Eval (helm-filtered-bookmarks). It's my command of interest. It works unexpectedly though. Let's look at function again!

3. Describe-function helm-highlight-bookmark => Oops, I see vanilla docstring only. Where is my patch?

4. Eval following form by hand. Now patch really applied and all works fine.

After playing with various Emacs rc combo without success I return to old `advice-add' technics. It work as expected with the same configs.

So, Emacs Messages without errors, if form evaluated by hand it work, patch validation is OK also, another patched functions work fine. But loading this func at Emacs init is unsufficient. Mystique thing...

(el-patch-defun helm-highlight-bookmark (bookmarks _source)
  (el-patch-swap
  "Used as `filtered-candidate-transformer' to colorize bookmarks."
    "PATCHED! Used as `filtered-candidate-transformer' to colorize bookmarks.

PATCH NOTE: AutoFS adoptation.")
  (let ((non-essential t))
    (cl-loop for i in bookmarks
          for isfile        = (bookmark-get-filename i)
          for hff           = (helm-bookmark-helm-find-files-p i)
          for handlerp      = (and (fboundp 'bookmark-get-handler)
                                   (bookmark-get-handler i))
          for isw3m         = (and (fboundp 'helm-bookmark-w3m-bookmark-p)
                                   (helm-bookmark-w3m-bookmark-p i))
          for isgnus        = (and (fboundp 'helm-bookmark-gnus-bookmark-p)
                                   (helm-bookmark-gnus-bookmark-p i))
          for isman         = (and (fboundp 'helm-bookmark-man-bookmark-p) ; Man
                                   (helm-bookmark-man-bookmark-p i))
          for iswoman       = (and (fboundp 'helm-bookmark-woman-bookmark-p) ; Woman
                                   (helm-bookmark-woman-bookmark-p i))
          for isannotation  = (bookmark-get-annotation i)
          for isabook       = (string= (bookmark-prop-get i 'type)
                                       "addressbook")
          for isinfo        = (eq handlerp 'Info-bookmark-jump)
          for loc = (bookmark-location i)
          for len =  (string-width i)
          for trunc = (if (and helm-bookmark-show-location
                               (> len bookmark-bmenu-file-column))
                          (helm-substring
                           i bookmark-bmenu-file-column)
                        i)
          ;; Add a * if bookmark have annotation
          if (and isannotation (not (string-equal isannotation "")))
          do (setq trunc (concat "*" (if helm-bookmark-show-location trunc i)))
          for sep = (and helm-bookmark-show-location
                         (make-string (- (+ bookmark-bmenu-file-column 2)
                                         (string-width trunc))
                                      ? ))
          for bmk = (cond ( ;; info buffers
                           isinfo
                           (propertize trunc 'face 'helm-bookmark-info
                                       'help-echo isfile))
                          ( ;; w3m buffers
                           isw3m
                           (propertize trunc 'face 'helm-bookmark-w3m
                                       'help-echo isfile))
                          ( ;; gnus buffers
                           isgnus
                           (propertize trunc 'face 'helm-bookmark-gnus
                                       'help-echo isfile))
                          ( ;; Man Woman
                           (or iswoman isman)
                           (propertize trunc 'face 'helm-bookmark-man
                                       'help-echo isfile))
                          ( ;; Addressbook
                           isabook
                           (propertize trunc 'face 'helm-bookmark-addressbook))
                          (;; Directories (helm-find-files)
                           hff
                           (if (and (file-remote-p isfile)
                                    (not (file-remote-p isfile nil t)))
                               (propertize trunc 'face 'helm-bookmark-file-not-found
                                           'help-echo isfile)
                             (propertize trunc 'face 'helm-bookmark-directory
                                         'help-echo isfile)))
                          ( ;; Directories (dired)
                           (and isfile
                                ;; This is needed because `non-essential'
                                ;; is not working on Emacs-24.2 and the behavior
                                ;; of tramp seems to have changed since previous
                                ;; versions (Need to reenter password even if a
                                ;; first connection have been established,
                                ;; probably when host is named differently
                                ;; i.e machine/localhost)
                                (and (not (file-remote-p isfile))
                                     (file-directory-p isfile)))
                           (propertize trunc 'face 'helm-bookmark-directory
                                       'help-echo isfile))
                          ( ;; Non existing files.
                           (and isfile
                                ;; Be safe and call `file-exists-p'
                                ;; only if file is not remote or
                                ;; remote but connected.
                                (or (el-patch-splice 1 1
                                      (and (file-remote-p isfile)
                                           (not (file-remote-p isfile nil t))))
                                    (not (file-exists-p isfile))))
                           (propertize trunc 'face 'helm-bookmark-file-not-found
                                       'help-echo isfile))
                          ( ;; regular files
                           t
                           (propertize trunc 'face 'helm-bookmark-file
                                       'help-echo isfile)))
          collect (if helm-bookmark-show-location
                      (cons (concat bmk sep (if (listp loc) (car loc) loc))
                            i)
                    (cons bmk i)))))

[raxod502]

That behavior happens because when you define the patch, el-patch uses fset to install the function definition. Then, when you call helm-filtered-bookmarks, it triggers an autoload to load helm-bookmark, which evaluates the original defun for helm-highlight-bookmark. That overrides the patch.

The correct way to patch functions that are provided by an as-of-yet-unloaded feature is to wrap them in a with-eval-after-load. See also el-patch-feature, which allows you to validate such patches without previously loading the features.

The issue does not happen with advice because advice does lots of magic to allow for advising a function whose actual definition is not yet loaded. (Although for cleanliness, I still try to put my advices inside with-eval-after-load forms.)

[caadar]

Ah, my missed (require 'helm-bookmark)...

Thank you, will try with-eval-after-load & el-patch-feature mechanism also.

[raxod502]

See example usage for el-patch-feature: https://github.com/raxod502/radian/search?q=el-patch-feature

[caadar]

Great, all work pretty well!