Closed OGAWAHirofumi closed 5 years ago
npostavs
commented
6 years ago Would this be the bug of c-mode or yasnippet?
I think it's mainly due to a (design) bug in yasnippet, that it inserts the raw snippet syntax into the buffer, and then edits it. This has caused various problems, though c-mode is the most sensitive to it. Anyway, it looks like we'll need to incorporate something like your workaround into yasnippet.
joaotavora
commented
6 years ago I think it's mainly due to a (design) bug in yasnippet, that it inserts the raw snippet syntax into the buffer, and then edits it
Indeed, this misdesign is only decently fixed in the incomplete snippet-engine branch. But in the meantime it shouldn't be very hard to expand in a temp buffer, collect text and markers, paste into the target buffer and proceed with indentation.
OGAWAHirofumi
commented
6 years ago FWIW, this is the version to use temporary buffer, and may work as start.
---
yasnippet.el | 43 +++++++++++++++++++++++++++++--------------
1 file changed, 29 insertions(+), 14 deletions(-)
diff -puN yasnippet.el~use-temp-buffer yasnippet.el
--- yasnippet/yasnippet.el~use-temp-buffer 2018-08-25 18:24:08.585687824 +0900
+++ yasnippet-hirofumi/yasnippet.el 2018-08-25 18:24:28.042691331 +0900
@@ -4031,20 +4031,10 @@ Returns the newly created snippet."
(save-restriction
(let ((snippet (yas--make-snippet expand-env)))
(yas--letenv expand-env
- ;; Put a single undo action for the expanded snippet's
- ;; content.
- (let ((buffer-undo-list t))
- ;; Some versions of cc-mode fail when inserting snippet
- ;; content in a narrowed buffer.
- (goto-char begin)
- (insert content)
- (setq end (+ end (length content)))
- (narrow-to-region begin end)
- (goto-char (point-min))
- (yas--snippet-parse-create snippet))
- (when (listp buffer-undo-list)
- (push (cons (point-min) (point-max))
- buffer-undo-list))
+ (goto-char begin)
+ (setq end (+ end (yas--snippet-insert-create snippet)))
+ ;; Currently below of this needs narrow-to-region.
+ (narrow-to-region begin end)
;; Indent, collecting undo information normally.
(yas--indent snippet)
@@ -4267,6 +4257,31 @@ Meant to be called in a narrowed buffer,
(yas--update-mirrors snippet) ; Update mirrors for the first time.
(goto-char parse-start)))
+(defun yas--snippet-insert-create (snippet)
+ "Expand SNIPPET with temp-buffer, then insert result into current position."
+ ;; Insert the result of snippet with temp-buffer.
+ (let ((orig-buf (current-buffer))
+ (pos (point)))
+ (with-temp-buffer
+ (insert content)
+ (goto-char (point-min))
+ (yas--snippet-parse-create snippet)
+ (let ((result (buffer-string)))
+ (with-current-buffer orig-buf
+ (insert result)
+ ;; Mapping marker pos on temp-buffer to pos on current buffer.
+ (yas--snippet-map-markers
+ (lambda (m)
+ (when (markerp m)
+ (move-marker m (+ (1- pos) (marker-position m))))
+ m)
+ snippet)
+ (dolist (m yas--indent-markers)
+ (move-marker m (+ (1- pos) (marker-position m))))
+ ;; Restore position
+ (goto-char pos)
+ (length result))))))
+
;; HACK: Some implementations of `indent-line-function' (called via
;; `indent-according-to-mode') delete text before they insert (like
;; cc-mode), some make complicated regexp replacements (looking at
_FWIW, this is the version to use temporary buffer, and may work as start.
You almost certinaly have to pass orig-buffer to yas--snippet-parse-create, or somehow store it in the snippet, so that the backquoted evaluations happen in the correct buffer.
OGAWAHirofumi
commented
6 years ago Hmm.. if eval modified buffer, it happens on orig-buffer, not temp-buffer. If this is required feature, maybe temp-buffer strategy doesn't work?
joaotavora
commented
6 years ago Hmm.. if eval modified buffer, it happens on orig-buffer, not temp-buffer.
Sure, but that's how it has always worked.
Anyway, backquoted evaluations aren't supposed to change the buffer, or even access itscontents. But they can (and usually do) refer to things like buffer-file-name, etc... So you really do have to evaluate in the original buffer.
OGAWAHirofumi
commented
6 years ago FWIF, this is the version eval with original buffer.
---
yasnippet.el | 81 ++++++++++++++++++++++++++++++++++++----------------------
1 file changed, 51 insertions(+), 30 deletions(-)
diff -puN yasnippet.el~use-temp-buffer-eval2 yasnippet.el
--- yasnippet/yasnippet.el~use-temp-buffer-eval2 2018-08-26 18:49:48.083741803 +0900
+++ yasnippet-hirofumi/yasnippet.el 2018-08-26 18:50:02.052700674 +0900
@@ -1446,12 +1446,14 @@ Also tries to work around Emacs Bug#3093
,@body)
(yas--remove-misc-free-from-undo old-undo-list))))
-(defun yas--eval-for-string (form)
+(defun yas--eval-for-string (form &optional buffer-for-eval)
"Evaluate FORM and convert the result to string."
(let ((debug-on-error (and (not (memq yas-good-grace '(t inline)))
debug-on-error)))
(condition-case oops
(save-excursion
+ (when buffer-for-eval
+ (set-buffer buffer-for-eval))
(yas--save-restriction-and-widen
(save-match-data
(let ((result (eval form)))
@@ -3174,7 +3176,7 @@ Update each field with the result of cal
snippet)
t))
-(defun yas--apply-transform (field-or-mirror field &optional empty-on-nil-p)
+(defun yas--apply-transform (field-or-mirror field &optional empty-on-nil-p buffer-for-eval)
"Calculate transformed string for FIELD-OR-MIRROR from FIELD.
If there is no transform for ht field, return nil.
@@ -3192,7 +3194,8 @@ string iff EMPTY-ON-NIL-P is true."
(transformed (and transform
(save-excursion
(goto-char start-point)
- (let ((ret (yas--eval-for-string transform)))
+ (let ((ret (yas--eval-for-string transform
+ buffer-for-eval)))
(or ret (and empty-on-nil-p "")))))))
transformed))
@@ -4031,20 +4034,10 @@ Returns the newly created snippet."
(save-restriction
(let ((snippet (yas--make-snippet expand-env)))
(yas--letenv expand-env
- ;; Put a single undo action for the expanded snippet's
- ;; content.
- (let ((buffer-undo-list t))
- ;; Some versions of cc-mode fail when inserting snippet
- ;; content in a narrowed buffer.
- (goto-char begin)
- (insert content)
- (setq end (+ end (length content)))
- (narrow-to-region begin end)
- (goto-char (point-min))
- (yas--snippet-parse-create snippet))
- (when (listp buffer-undo-list)
- (push (cons (point-min) (point-max))
- buffer-undo-list))
+ (goto-char begin)
+ (setq end (+ end (yas--snippet-insert-create content snippet)))
+ ;; Currently below of this needs narrow-to-region.
+ (narrow-to-region begin end)
;; Indent, collecting undo information normally.
(yas--indent snippet)
@@ -4218,7 +4211,7 @@ cons cells to this var.")
(defvar yas--indent-markers nil
"List of markers for manual indentation.")
-(defun yas--snippet-parse-create (snippet)
+(defun yas--snippet-parse-create (snippet original-buffer)
"Parse a recently inserted snippet template, creating all
necessary fields, mirrors and exit points.
@@ -4231,7 +4224,8 @@ Meant to be called in a narrowed buffer,
(setq yas--dollar-regions nil) ; Reset the yas--dollar-regions.
(yas--protect-escapes nil '(?`)) ; Protect just the backquotes.
(goto-char parse-start)
- (setq saved-quotes (yas--save-backquotes)) ; `expressions`.
+ (setq saved-quotes
+ (yas--save-backquotes original-buffer)) ; `expressions`.
(yas--protect-escapes) ; Protect escaped characters.
(goto-char parse-start)
(yas--indent-parse-create) ; Parse indent markers: `$>'.
@@ -4264,9 +4258,35 @@ Meant to be called in a narrowed buffer,
(yas--restore-backquotes saved-quotes) ; Restore `expression` values.
(goto-char parse-start)
(yas--restore-escapes) ; Restore escapes.
- (yas--update-mirrors snippet) ; Update mirrors for the first time.
+ (yas--update-mirrors snippet
+ original-buffer) ; Update mirrors for the first time.
(goto-char parse-start)))
+(defun yas--snippet-insert-create (content snippet)
+ "Expand SNIPPET with temp-buffer, then insert result into current position."
+ ;; Insert the result of snippet with temp-buffer.
+ (let ((original-buffer (current-buffer))
+ (pos (point)))
+ (with-temp-buffer
+ (insert content)
+ (goto-char (point-min))
+ (yas--snippet-parse-create snippet original-buffer)
+ (let ((result (buffer-string)))
+ (with-current-buffer original-buffer
+ (insert result)
+ ;; Mapping marker pos on temp-buffer to pos on current buffer.
+ (yas--snippet-map-markers
+ (lambda (m)
+ (when (markerp m)
+ (move-marker m (+ (1- pos) (marker-position m))))
+ m)
+ snippet)
+ (dolist (m yas--indent-markers)
+ (move-marker m (+ (1- pos) (marker-position m))))
+ ;; Restore position
+ (goto-char pos)
+ (length result))))))
+
;; HACK: Some implementations of `indent-line-function' (called via
;; `indent-according-to-mode') delete text before they insert (like
;; cc-mode), some make complicated regexp replacements (looking at
@@ -4484,15 +4504,14 @@ With optional string TEXT do it in strin
(or escaped yas--escaped-characters))
changed-text))
-(defun yas--save-backquotes ()
+(defun yas--save-backquotes (original-buffer)
"Save all \"\\=`(lisp-expression)\\=`\"-style expressions.
Return a list of (MARKER . STRING) entires for each backquoted
Lisp expression."
(let* ((saved-quotes nil)
- (yas--snippet-buffer (current-buffer))
(yas--change-detected nil)
(detect-change (lambda (_beg _end)
- (when (eq (current-buffer) yas--snippet-buffer)
+ (when (eq (current-buffer) original-buffer)
(setq yas--change-detected t)))))
(while (re-search-forward yas--backquote-lisp-expression-regexp nil t)
(let ((current-string (match-string-no-properties 1)) transformed)
@@ -4500,9 +4519,10 @@ Lisp expression."
(delete-region (match-beginning 0) (match-end 0)))
(let ((before-change-functions
(cons detect-change before-change-functions)))
- (setq transformed (yas--eval-for-string (yas--read-lisp
- (yas--restore-escapes
- current-string '(?`))))))
+ (setq transformed (yas--eval-for-string
+ (yas--read-lisp (yas--restore-escapes
+ current-string '(?`)))
+ original-buffer)))
(goto-char (match-beginning 0))
(when transformed
(let ((marker (make-marker))
@@ -4706,7 +4726,7 @@ When multiple expressions are found, onl
(parent 1)
(t 0))))))
-(defun yas--update-mirrors (snippet)
+(defun yas--update-mirrors (snippet &optional buffer-for-eval)
"Update all the mirrors of SNIPPET."
(yas--save-restriction-and-widen
(save-excursion
@@ -4730,7 +4750,7 @@ When multiple expressions are found, onl
(when parent-field
(yas--advance-start-maybe mirror (yas--fom-start parent-field))))
;; Update this mirror.
- do (yas--mirror-update-display mirror field)
+ do (yas--mirror-update-display mirror field buffer-for-eval)
;; Delay indenting until we're done all mirrors. We must do
;; this to avoid losing whitespace between fields that are
;; still empty (i.e., they will be non-empty after updating).
@@ -4747,13 +4767,14 @@ When multiple expressions are found, onl
(cl-loop for (beg . end) in (cl-sort indent-regions #'< :key #'car)
do (yas--indent-region beg end snippet)))))))
-(defun yas--mirror-update-display (mirror field)
+(defun yas--mirror-update-display (mirror field &optional buffer-for-eval)
"Update MIRROR according to FIELD (and mirror transform)."
(let* ((mirror-parent-field (yas--mirror-parent-field mirror))
(reflection (and (not (and mirror-parent-field
(yas--field-modified-p mirror-parent-field)))
- (or (yas--apply-transform mirror field 'empty-on-nil)
+ (or (yas--apply-transform
+ mirror field 'empty-on-nil buffer-for-eval)
(yas--field-text-for-display field)))))
(when (and reflection
(not (string= reflection (buffer-substring-no-properties (yas--mirror-start mirror)
_The detection of side effect of eval is not tackled.
Nor was it ever, AFAIK.
narrowing is not called for eval (narrow-to-region is calling after yas--snippet-insert-create)
I don't think that's a problem (but i might me missing something)
A few points:
buffer field to the yas--snippet struct. yas--buffer-for-eval and let-bind it in yas--snippet-insert-create.And finally we have to wait for @npostavs to weigh in on this :-)
OGAWAHirofumi
commented
6 years ago Nor was it ever, AFAIK.
I meant, this patch is not yet supporting yas--change-detected mechanism via before-change-functions. Because before-change-functions (buffer local) is not setup for buffer-for-eval in this patch.
I don't think that's a problem (but i might me missing something)
This may not be the issue though, for compatibility and safety, narrow-to-region may make sandbox env for eval.
- A simple, if possibly silly, question: does the patch fix the problem reported here?
My original issue seems to be fixed. And yasnippet-tests.el seems to be passed with 3 additional tests for (buffer-name).
- I think this is better implemented by adding a buffer field to the yas--snippet struct.
- An alternative to 2. is to use a special variable yas--buffer-for-eval and let-bind it in yas--snippet-insert-create.
I will see what happens with it later.
And finally we have to wait for @npostavs to weigh in on this :-)
Thanks. :)
OGAWAHirofumi
commented
6 years ago Another update for this strategy.
[Maybe I found the bug around before-change-functions stuff. yas--save-backquotes setup before-change-functions then remove. But setup is in let-bind, so remove is needless. True?
- (let ((marker (make-marker))
- (before-change-functions (cdr before-change-functions)))
+ (let ((marker (make-marker))]
diff -urNp -x patchset -x '*~*' yasnippet.orig/yasnippet-tests.el yasnippet/yasnippet-tests.el
--- yasnippet.orig/yasnippet-tests.el 2018-08-27 19:20:22.877023387 +0900
+++ yasnippet/yasnippet-tests.el 2018-08-27 19:20:38.356004268 +0900
@@ -1578,6 +1578,40 @@ add the snippets associated with the giv
(yas-should-not-expand '("_"))
(yas-should-expand '(("car" . "(car )")))))))
+(ert-deftest issue-953-c-mode-indent ()
+ (with-temp-buffer
+ (c-mode)
+ (yas-minor-mode 1)
+ (yas-expand-snippet
+ "#ifndef `guard`\n#define `guard`\n#endif /* !`guard` */\n"
+ (point-min) (point-max) '((guard "FOO")))
+ (should (string= (yas--buffer-contents) "#ifndef FOO\n#define FOO\n#endif /* !FOO */\n"))))
+
+(ert-deftest temp-buffer-backslashes ()
+ (with-temp-buffer
+ (yas-minor-mode 1)
+ (yas-expand-snippet "bla`(upcase (buffer-name))`ble")
+ (should (string= (yas--buffer-contents) (format "bla%sble" (upcase (buffer-name)))))))
+
+(ert-deftest temp-buffer-field-transformation ()
+ (with-temp-buffer
+ (yas-minor-mode 1)
+ (let ((snippet (concat "${1:$$(if (string= \"" (buffer-name) "\" (buffer-name)) yas-text \"\")}${1:$(concat \"bar\" yas-text (buffer-name))}")))
+ (yas-expand-snippet snippet)
+ (should (string= (yas--buffer-contents) (concat "bar" (buffer-name))))
+ (yas-mock-insert "foo")
+ (should (string= (yas--buffer-contents) (concat "foobarfoo" (buffer-name)))))))
+
+(ert-deftest temp-buffer-mirror-with-transformation ()
+ (with-temp-buffer
+ (yas-minor-mode 1)
+ (yas-expand-snippet "${1:brother} from another ${1:$(upcase yas-text)} ${1:$(upcase (buffer-name))}")
+ (should (string= (yas--buffer-contents)
+ (concat "brother from another BROTHER " (upcase (buffer-name)))))
+ (yas-mock-insert "bla")
+ (should (string= (yas--buffer-contents)
+ (concat "bla from another BLA " (upcase (buffer-name)))))))
+
(provide 'yasnippet-tests)
diff -urNp -x patchset -x '*~*' yasnippet.orig/yasnippet.el yasnippet/yasnippet.el
--- yasnippet.orig/yasnippet.el 2018-08-27 19:22:51.856836502 +0900
+++ yasnippet/yasnippet.el 2018-08-27 19:20:39.429002966 +0900
@@ -1446,12 +1446,17 @@ Also tries to work around Emacs Bug#3093
,@body)
(yas--remove-misc-free-from-undo old-undo-list))))
+;; If buffer was set, eval is switching to this buffer
+(defvar yas--buffer-for-eval nil)
+
(defun yas--eval-for-string (form)
"Evaluate FORM and convert the result to string."
(let ((debug-on-error (and (not (memq yas-good-grace '(t inline)))
debug-on-error)))
(condition-case oops
(save-excursion
+ (when yas--buffer-for-eval
+ (set-buffer yas--buffer-for-eval))
(yas--save-restriction-and-widen
(save-match-data
(let ((result (eval form)))
@@ -4031,20 +4036,10 @@ Returns the newly created snippet."
(save-restriction
(let ((snippet (yas--make-snippet expand-env)))
(yas--letenv expand-env
- ;; Put a single undo action for the expanded snippet's
- ;; content.
- (let ((buffer-undo-list t))
- ;; Some versions of cc-mode fail when inserting snippet
- ;; content in a narrowed buffer.
- (goto-char begin)
- (insert content)
- (setq end (+ end (length content)))
- (narrow-to-region begin end)
- (goto-char (point-min))
- (yas--snippet-parse-create snippet))
- (when (listp buffer-undo-list)
- (push (cons (point-min) (point-max))
- buffer-undo-list))
+ (goto-char begin)
+ (setq end (+ end (yas--snippet-insert-create content snippet)))
+ ;; Currently below of this needs narrow-to-region.
+ (narrow-to-region begin end)
;; Indent, collecting undo information normally.
(yas--indent snippet)
@@ -4267,6 +4262,31 @@ Meant to be called in a narrowed buffer,
(yas--update-mirrors snippet) ; Update mirrors for the first time.
(goto-char parse-start)))
+(defun yas--snippet-insert-create (content snippet)
+ "Expand SNIPPET with temp-buffer, then insert result into current position."
+ ;; Insert the result of snippet with temp-buffer.
+ (let ((yas--buffer-for-eval (current-buffer))
+ (pos (point)))
+ (with-temp-buffer
+ (insert content)
+ (goto-char (point-min))
+ (yas--snippet-parse-create snippet)
+ (let ((result (buffer-string)))
+ (with-current-buffer yas--buffer-for-eval
+ (insert result)
+ ;; Mapping marker pos on temp-buffer to pos on current buffer.
+ (yas--snippet-map-markers
+ (lambda (m)
+ (when (markerp m)
+ (move-marker m (+ (1- pos) (marker-position m))))
+ m)
+ snippet)
+ (dolist (m yas--indent-markers)
+ (move-marker m (+ (1- pos) (marker-position m))))
+ ;; Restore position
+ (goto-char pos)
+ (length result))))))
+
;; HACK: Some implementations of `indent-line-function' (called via
;; `indent-according-to-mode') delete text before they insert (like
;; cc-mode), some make complicated regexp replacements (looking at
@@ -4489,24 +4509,24 @@ With optional string TEXT do it in strin
Return a list of (MARKER . STRING) entires for each backquoted
Lisp expression."
(let* ((saved-quotes nil)
- (yas--snippet-buffer (current-buffer))
(yas--change-detected nil)
(detect-change (lambda (_beg _end)
- (when (eq (current-buffer) yas--snippet-buffer)
+ (when (eq (current-buffer) yas--buffer-for-eval)
(setq yas--change-detected t)))))
(while (re-search-forward yas--backquote-lisp-expression-regexp nil t)
(let ((current-string (match-string-no-properties 1)) transformed)
(yas--save-restriction-and-widen
(delete-region (match-beginning 0) (match-end 0)))
- (let ((before-change-functions
- (cons detect-change before-change-functions)))
- (setq transformed (yas--eval-for-string (yas--read-lisp
- (yas--restore-escapes
- current-string '(?`))))))
+ ;; Setup before-change-functions for yas--buffer-for-eval buffer
+ (with-current-buffer yas--buffer-for-eval
+ (let ((before-change-functions
+ (cons detect-change before-change-functions)))
+ (setq transformed (yas--eval-for-string (yas--read-lisp
+ (yas--restore-escapes
+ current-string '(?`)))))))
(goto-char (match-beginning 0))
(when transformed
- (let ((marker (make-marker))
- (before-change-functions (cdr before-change-functions)))
+ (let ((marker (make-marker)))
(yas--save-restriction-and-widen
(insert "Y") ;; quite horrendous, I love it :)
(set-marker marker (point))GitHub has this thing called "pull requests"...
npostavs
commented
6 years ago [Maybe I found the bug around before-change-functions stuff. yas--save-backquotes setup before-change-functions then remove. But setup is in let-bind, so remove is needless. True?
Ouch, yes, that looks like quite a bad bug. Not sure what I was thinking when I wrote that.
Anyway, the main drawback to fixing this is that it will finally break all those snippets which rely on insertion by side-effect. That's the main reason I was putting this off, and using workarounds instead. There has been a warning for a quite while though, so maybe it's okay to do this by now. Maybe.
Have you assigned copyright for Emacs? Your main patch is too big to accept without that (the before-change-functions thing is small enough to install regardless, I plan to do that soon).
OGAWAHirofumi
commented
6 years ago Anyway, the main drawback to fixing this is that it will finally break all those snippets which rely on insertion by side-effect. That's the main reason I was putting this off, and using workarounds instead. There has been a warning for a quite while though, so maybe it's okay to do this by now. Maybe.
Right. Some compatibility code would be possible though (narrow region for eval, then copy to temp-buffer), however if eval depending on by moving "point", it would be impossible to be fully compatible.
Have you assigned copyright for Emacs? Your main patch is too big to accept without that (the before-change-functions thing is small enough to install regardless, I plan to do that soon).
Ah, it was again. I'm ok to give copyright though (I don't care copyright for this code at all), I was lazy to sign and I'm not sure what happens with sign.
OGAWAHirofumi
commented
6 years ago Hi, so to contribute, I have to sign to give copyright? I don't know the details of process, what happens, and what I have to do.
Can you help to process?
npostavs
commented
6 years ago (the
before-change-functionsthing is small enough to install regardless, I plan to do that soon).
Done, albeit a little late (7a178a2ca016ae205df4b6dc191fd42debc99c8d).
Hi, so to contribute, I have to sign to give copyright? I don't know the details of process, what happens, and what I have to do.
We can't accept more than (about) 15 lines from you, if you haven't assigned copyright. To start the copyright assignment process, fill in this form, with "Emacs" as the program name, and send it to the mentioned email address.
By the way, regardless of the copyright situation, I'm still not entirely decided if we should go with the temp buffer approach.
OGAWAHirofumi
commented
6 years ago We can't accept more than (about) 15 lines from you, if you haven't assigned copyright. To start the copyright assignment process, fill in this form, with "Emacs" as the program name, and send it to the mentioned email address.
By the way, regardless of the copyright situation, I'm still not entirely decided if we should go with the temp buffer approach.
It's fine. I don't care what fix was made if this bug was fixed.
emacs version: GNU Emacs 27.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.30) of 2018-08-04
With "emacs -Q", evaluate the following code to reproduce,
=>
"#define FOO" is mis indented like above.
With some debugging, "yas--snippet-parse-create" in "yas--snippet-create" confuses c-mode. More specifically it seems to be c-macro-cache is caching the wrong state. Then "yas--indent" uses wrong cache.
I'm not sure, this is the bug of c-mode or yasnippet though, clearing cache seems to fix the issue. Actual workaround is
And I noticed the result code of around "yas--snippet-parse-create" is similar with "combine-change-calls" that recently added to emacs/lisp/subr.el.
Would this be the bug of c-mode or yasnippet?