Patch function without providing complete source in patch

[haji-ali]

I have a question regarding a possible extension of el-patch. In most cases, I just need to change a single form in a possibly long definition of a particular function. Such changes frequently break when the other code in the function, which I haven't patched, changes even though I know that such changes should not break my patch.

So I am wondering if it would be possible, in theory, to build a patch given a form. Something like this

(el-patch-patch
   'foo
   (function-call arg1 arg2 (el-patch-add arg3) )
)

and one would search the definition of foo for all forms which match (function-call arg1 arg2) and add an argument arg3. One could also add key forms to make sure that they exist in the function definition if such forms are necessary for the patch. Of course this assumes that one has the definition of foo rather than its compilation.

To be clear, I know that this is not implemented in el-patch, but I am wondering if people see a problem with such a methodology.

[raxod502]

I certainly do see the utility, as I have also experienced frequent breakage of patches due to unrelated changes in function internals. The feature would be nontrivial to implement for a few reasons:

• You'd need to look up the source code of the function during macroexpansion (yikes!).
• When using this feature, compiling your init-file would be basically required, otherwise you'd be doing this incredibly slow operation at every startup.
• There are definitely open questions as far as a reasonable and robust user interface to the interface in terms of what the patch would look like.

That said, I think it would be a great idea!

[haji-ali]

• You'd need to look up the source code of the function during macroexpansion (yikes!).

I thought that el-patch does something similar when validating the patch and I was hoping that I can leverage that implementation.

• When using this feature, compiling your init-file would be basically required, otherwise you'd be doing this incredibly slow operation at every startup.

Good point. I wonder if it’s better to output a full el-patch into a file and only do the slow operation of actually generating the full el-patch when the patch validation fails.

• There are definitely open questions as far as a reasonable and robust user interface to the interface in terms of what the patch would look like.

Indeed, that’s why I wanted to get input before actually working on the pull request.

Do you see a problem with the interface I suggested above? One issue that I thought about is what if the user wants to do multiple patches to the same function in different locations. But that can be done by passing multiple forms to the fictional ‘patch-el-patch’.

EDIT: A different proposal that might also mitigate the same problem (and be easier to implement but a bit more complicated to use) is to introduce a function like el-patch-until which simply copies the same forms from the source until a particular car (in the current form) is found. Something like

(el-patch-defun foo nil
  (el-patch-until 'let)  ;; This matches all and any forms until the form starting with let is found
  (let ((el-patch-until))
      (el-patch-until 'bar) 
      (bar arg (el-patch-add new-arg) )
     (el-patch-until)   ;; This matches all forms until the end of the current one
))

EDIT2: Another way (sorry for spamming). While implementing the previous solution, I realized that the user doesn't actually need to provide the form until which matching is done (since it should be the one right after that keyword). So the final solution would look like this.

(el-patch-defun foo nil
  el-patch-filler
  (let (el-patch-filler)
      el-patch-filler
      (bar arg (el-patch-add new-arg) )
     el-patch-filler  ;; This matches all forms until the end of the current one
))

BTW, this way is not great if, for example, one wants to change the n'th let in a form, but it is still better than repeating the source

(el-patch-defun foo nil
  el-patch-filler
  (let  el-patch-filler)
  el-patch-filler
  (let  el-patch-filler)
  el-patch-filler
  (let (el-patch-filler)
      el-patch-filler
      (bar arg (el-patch-add new-arg) )
     el-patch-filler  ;; This matches all forms until the end of the current one
))

[raxod502]

I was hoping that I can leverage that implementation

Yes, that is certainly the way to do it. I was just saying that if you did so, then it would Emacs would be opening a file and doing various other sketchy operations during macroexpansion, whereas typically macroexpansion is expected to be stateless and fast. There's no specific problem that will be caused by doing something more complicated; it's just a sign to be careful and consider if there is another approach. Not that I have a better suggestion for how to implement a feature like this.

I wonder if it’s better to output a full el-patch into a file and only do the slow operation of actually generating the full el-patch when the patch validation fails

This gave me an idea for how to avoid the usability problems that I described in my previous comment. If we were to implement what you've suggested, notice that it's kind of the same as what we have currently, except:

1. the patch is kept in a different file, not directly in your init-file
2. you have an automated way of regenerating your patch based on a template

So why not solve the underlying problem directly, and provide a command that will take a patch template, and generate a patch for you? That more or less eliminates the busywork of addressing irrelevant changes in the upstream code, without introducing any issues for the user like having to byte-compile their init-file to use this feature with reasonable performance.

open questions as far as a reasonable and robust user interface

OK, I thought about it for a while, and I have a suggestion. I translated all instances of el-patch from my current configuration into my proposed syntax:

(el-patch-template (defvar ruby-electric-mode-map)
  (el-patch-remove
    (dolist ...))
  (el-patch-concat
    "Keymap used in ruby-electric-mode"
    (el-patch-add ".\n\nThe single-character bindings have been removed.")))

(el-patch-template (defun TeX-update-style)
  (el-patch-concat
    "Run style specific hooks"
    (el-patch-add
      ", silently,")
    " for the current document.

Only do this if it has not been done before, or if optional argument
FORCE is not nil.")
  (el-patch-remove
    (message "Applying style hooks..."))
  (el-patch-remove
    (message "Applying style hooks...done")))

(el-patch-template (defun with-editor--setup)
  (el-patch-splice 2
    (when (server-running-p server-name)
      (setq server-name (format "server%s" (emacs-pid)))
      ...)))

(el-patch-template (defun magit-maybe-start-credential-cache-daemon)
  (el-patch-wrap 2
    (with-current-buffer
        (get-buffer-create " *git-credential-cache--daemon*")
      (start-process "git-credential-cache--daemon"
                     (el-patch-swap
                       " *git-credential-cache--daemon*"
                       (current-buffer))
                     magit-git-executable
                     "credential-cache--daemon"
                     magit-credential-cache-daemon-socket)
      (el-patch-add
        (set-process-query-on-exit-flag
         (get-buffer-process (current-buffer)) nil)))))

(el-patch-template (defun (el-patch-swap restart-emacs radian-new-emacs))
  (el-patch-concat
    (el-patch-swap
      "Restart Emacs."
      "Start a new Emacs session without killing the current one.")
    "

When called interactively ARGS is interpreted as follows

- with a single `universal-argument' (`C-u') Emacs is "
    (el-patch-swap "restarted" "started")
    "
  with `--debug-init' flag
- with two `universal-argument' (`C-u') Emacs is "
    (el-patch-swap "restarted" "started")
    " with
  `-Q' flag
- with three `universal-argument' (`C-u') the user prompted for
  the arguments

When called non-interactively ARGS should be a list of arguments
with which Emacs should be "
    (el-patch-swap "restarted" "started")
    ".")
  (el-patch-swap
    (save-buffers-kill-emacs)
    (restart-emacs--launch-other-emacs restart-args)))

Notice:

• I figure we can use the symbol ... instead of el-patch-filler
• the arguments to el-patch-template (which will generate a full el-patch that can be placed into the user's init-file or perhaps saved to another version-controllable file, details TBD) are: the type of patch, the name of the patch, and then a list of subpatches
• each subpatch is resolved in favor of the original version, and then pattern-matched against the actual function source; it must match in exactly one place or the patch generation fails
• the actual function source is transformed by substituting each subpatch into it at the appropriate location, and this forms an el-patch form

Thoughts?

[haji-ali]

Very nice syntax! I think this should cover a majority of cases. Having the requirement of exactly one match is really important to avoid unintended consequences. The implementation of the patch template should not require any change to el-patch (except perhaps for some refactoring).

* I figure we can use the symbol `...` instead of `el-patch-filler`

Yeah, I think combining both ideas (pattern matching and a filler form) would provide the most power and the shortest templates.
Are you thinking of ... being only at the end of a form? Or can one have something like this

(let ( ... (var (el-patch-swap 1 2)) ... ) 
...
)

I thought about using smaller symbols (although I did not think of the natural ...). However, one must be careful about such symbols being used in the original definition of the function. I don't know how relevant such a potential problem is and if we should provide a way to override the symbol in such cases.

One thing that would need to be ironed out is the behaviour of el-patch-splice and el-patch-wrap when ... is on the left or right and a form is to be trimmed there.

* the arguments to `el-patch-template` (which will generate a full `el-patch` that can be placed into the user's init-file or perhaps saved to another version-controllable file, details TBD) are: the type of patch, the name of the patch, and then a list of subpatches

We can go from the least-automated workflow:

• User writes the patch template in some file (that is not called from the init.el).
• User runs the template and the el-patch is yanked to the kill ring (much like straight-get-recipe).
• User pastes the el-patch in their preferred location.
• After sometime, when el-patch validation fails the user has to repeat the above steps manually.

To the most automated workflow:

• User defines the template in their init.el
• At the end the user calls a function, let call it el-template-finalize, to produce the templates to a file.
• el-template-finalize reads the file to check which el-patch'es are defined.
• If there are any new templates without a correspondingly defined patch, append the newly defined patch to the file.

The automated workflow is kinda ugly and I prefer the manual one. In any case, I think the manual workflow should be the first target.

* each subpatch is resolved in favor of the original version, and then pattern-matched against the actual function source; it must match in exactly one place or the patch generation fails

* the actual function source is transformed by substituting each subpatch into it at the appropriate location, and this forms an `el-patch` form

Sounds good!

[raxod502]

Are you thinking of ... being only at the end of a form?

Ideally (i.e. from a user perspective), you'd be able to put it anywhere. I guess it depends how challenging the pattern-matching turns out to be, though, in terms of implementation.

one must be careful about such symbols being used in the original definition of the function. I don't know how relevant such a potential problem is

I've personally never seen ... used as a symbol in any Elisp code I've worked with. We should be pretty safe. Of course, if somebody does dredge up an example in a bug report, we can reconsider :)

One thing that would need to be ironed out is the behaviour of el-patch-splice and el-patch-wrap when ... is on the left or right and a form is to be trimmed there

Good catch, I'm not sure what to do in that case. For el-patch-splice, ostensibly we might be able to get away with supporting it somehow. For el-patch-wrap it'd be impossible because the ... would contain the forms that were supposed to be patched in. I suspect we just want to report an error in that case; the pattern-matching algorithm will be hard enough even if we can always deterministically resolve the subpatch.

The automated workflow is kinda ugly and I prefer the manual one. In any case, I think the manual workflow should be the first target

Agreed on all points.

User writes the patch template in some file (that is not called from the init.el)

This seems fine. Here's a wild idea for the future:

• invoking el-patch-template has different effects depending on context
• if it's evaluated interactively, it copies the patch form to the kill ring
• if it's evaluated as part of normal startup or loading, it reports an error
• if it's evaluated as part of byte-compilation, it transparently macroexpands to the patch form (or throws an error if validation fails)

That way, we'd support the manual workflow you suggested, but also, people who byte-compile their init-file could just put in the template form and have it magically work without added startup cost (just slower byte-compilation).

[haji-ali]

Ideally (i.e. from a user perspective), you'd be able to put it anywhere. I guess it depends how challenging the pattern-matching turns out to be, though, in terms of implementation.

EDIT: The matching with backtracking turned out to be easier than expected. We can see the effect on the performance and re-adjust if necessary.

In any case, I think we are converging on a similar viewpoint. I will see how complicated the implementation is and we can discuss further.

EDIT 2: the implementation ideas above do not work nicely with el-patch-let (when ... is used in the definition).. sigh...