radian-software/el-patch

Bug: templates that don't contain `...` and don't deal with their original form completely are incorrectly considered valid

hab25 opened this issue · 3 comments

Reproduce with emacs -Q:

;; assumed to be in `load-path`
(require 'el-patch)
(require 'el-patch-template)

(setq
 el-patch-use-aggressive-defvar t
 el-patch-warn-on-eval-template nil)

(el-patch-define-and-eval-template
 (defcustom remote-file-name-inhibit-auto-save-visited)
 (el-patch-swap nil t)) ; this is an incomplete template! E.g. it doesn't deal with the original form's docstring or `:type`
(el-patch-validate-template
 'remote-file-name-inhibit-auto-save-visited 'defcustom) ; passes without error or warning! This shouldn't be happening

(message (pp-to-string remote-file-name-inhibit-auto-save-visited)) ; t

I don't think this is a bug. According to the documentation, templates consist of a series of diffs which are matched against the original definition. Unlike a normal patch, they don't have to match the entire original definition, and as such (unlike a normal patch) the original definition has to be accessible for them to be compiled.

I don't think this is a bug. According to the documentation, templates consist of a series of diffs which are matched against the original definition. Unlike a normal patch, they don't have to match the entire original definition, and as such (unlike a normal patch) the original definition has to be accessible for them to be compiled.

Thanks, I had misunderstood.

Also, I think this means that my recent comment #37 (comment) is incorrect.

For example one can't use el-patch-template to accurately emulate :after advice (i.e. including not breaking when implementation changes):

(el-patch-define-and-eval-template

 (defun restart-emacs)
 ...
 (el-patch-add (message "hi")))

fails.

I assume this is because the template can't determine where to insert (message "hi") since the entire definition doesn't have to be matched.

Yes, that's right.