ENHANCED: handle renaming variables to existing variable names

* sweeprolog.el (sweeprolog-rename-variable-allow-existing): new user
option, used by...
(sweeprolog-read-new-variable-try): new command, exists the minibuffer
if it contains a valid variable name.  It checks if the selected
variable name already exists.  Used by...
(sweeprolog-read-new-variable): new function, reads a Prolog variable
name in the minibuffer with a dedicated exit command.
(sweeprolog-rename-variable): use it.

diff --git a/README.org b/README.org
index db2fd018f90929481170f961408e23e224f19bb7..0dd0fe3a7e5f5224049ef04643e810cf6c96d573 100644 (file)
--- a/README.org
+++ b/README.org
@@ -1650,7 +1650,7 @@ to provide contextual menus that you interact with using the mouse.
 
 - Command: context-menu-mode :: Toggle Context Menu mode.  When
   enabled, clicking the mouse button ~down-mouse-3~ (i.e. right-click)
-  activates a menu whose contents depends on its surrounding context.
+  activates a menu whose contents depend on its surrounding context.
 - Variable: sweeprolog-context-menu-functions :: List of functions
   that create Context Menu entries for Prolog tokens.  Each function
   should receive as its arguments the menu that is being created, the
@@ -1669,7 +1669,9 @@ If you right-click on a Prolog file specification or module name,
 Sweep suggests visiting it either in the current window or in another.
 If you right-click on a predicate, it lets you view its documentation
 in a dedicated buffer (see also [[#prolog-help][Prolog Help]]).  For variables, it
-suggests to renaming (see .
+enables the ~Rename Variable~ menu entry that you can use to rename the
+variable you click on across its containing clause (see [[#rename-variable][Renaming
+Variables]]).
 
 You can further extend and customize the context menu that
 ~sweeprolog-mode~ provides by adding functions to the variable
@@ -1690,6 +1692,11 @@ following command:
 
 - Key: C-c C-r (sweeprolog-rename-variable) :: Rename a variable
   across the topmost Prolog term at point.
+- User Option: sweeprolog-rename-variable-allow-existing :: If
+  non-nil, allow selecting an existing variable name as the new name
+  of a variable being renamed with ~sweeprolog-rename-variable~.  If it
+  is the symbol ~confirm~, allow but ask for confirmation first.
+  Defaults to ~confirm~.
 
 The command ~sweeprolog-rename-variable~, bound to ~C-c C-r~, prompts for
 two variable names and replaces all occurrences of the first variable
@@ -1697,6 +1704,16 @@ in the term at point with the second.  The prompt for the first (old)
 variable name provides completion based on the existing variable names
 in the current term, and it uses the variable at point as its default.
 
+The user option ~sweeprolog-rename-variable-allow-existing~ controls
+what happens if the second (new) variable name that you insert in the
+minibuffer already occurs in the current clause.  By default it is set
+to ~confirm~, which says to ask for confirmation before selecting an
+existing variable name as the new name.  This is because renaming a
+variable to another existing variable name potentially alters the
+semantics of the term by merging the two variables.  Other
+alternatives for this user option are ~t~ for allowing such merges
+without confirmation, and ~nil~ for refusing them altogether.
+
 If Context Menu mode is enabled, you can also rename variables by
 right-clicking on them with the mouse and selecting =Rename Variable=
 from the top of the context menu.  See [[#context-menu][Context Menu]] for more

diff --git a/sweeprolog.el b/sweeprolog.el
index d59489830f0afe431683cc00cb695b2b5a4e681b..fcb4c85dd39151ed76f2a0f5415d81cd2a6162b6 100644 (file)
--- a/sweeprolog.el
+++ b/sweeprolog.el
@@ -387,6 +387,14 @@ token via its `help-echo' text property."
   :type 'boolean
   :group 'sweeprolog)
 
+(defcustom sweeprolog-rename-variable-allow-existing 'confirm
+  "If non-nil, allow renaming variables to existing variable names.
+If it is the symbol `confirm', allow but ask for confirmation
+first."
+  :package-version '((sweeprolog "0.15.1"))
+  :type 'symbol
+  :group 'sweeprolog)
+
 ;;;; Keymaps
 
 (defvar sweeprolog-mode-map
@@ -5559,15 +5567,95 @@ is the name of the variable at point, if any."
 (defun sweeprolog--format-variable (var)
   (propertize var 'face (sweeprolog-variable-face)))
 
-(defun sweeprolog-rename-variable (&optional old new point interactive)
+(defvar sweeprolog-read-new-variable--existing-vars nil)
+(defvar-local sweeprolog-read-new-variable--warned nil)
+
+(defun sweeprolog--variable-name-p (string)
+  "Return t if STRING is valid Prolog variable name."
+  (save-match-data
+    (let ((case-fold-search nil))
+      (not
+       (not
+        (string-match (rx bos (or "_" upper) (* alnum) eos) string))))))
+
+(defun sweeprolog-read-new-variable-try ()
+  "Try to exit the minibuffer with a new Prolog variable name.
+
+If the minibuffer contains a variable that already occurs in the
+current clause, warn and ask for confirmation before exiting.  If
+the minibuffer does not contain a valid variable name, report it
+and refuse to exit."
+  (interactive)
+  (unless (> (minibuffer-depth) 0)
+    (error "Minibuffer must be active"))
+  (let ((choice (minibuffer-contents)))
+    (if (sweeprolog--variable-name-p choice)
+        (if (member choice sweeprolog-read-new-variable--existing-vars)
+            (pcase sweeprolog-rename-variable-allow-existing
+              ('confirm (if (string= sweeprolog-read-new-variable--warned choice)
+                            (exit-minibuffer)
+                          (minibuffer-message
+                           (substitute-command-keys
+                            "%s already exists, type \\[sweeprolog-read-new-variable-try] again to confirm")
+                           (sweeprolog--format-variable choice))
+                          (setq sweeprolog-read-new-variable--warned choice)))
+              ('nil (minibuffer-message "%s already exists" (sweeprolog--format-variable choice)))
+              (_ (exit-minibuffer)))
+          (exit-minibuffer))
+      (minibuffer-message "Invalid Prolog variable name"))))
+
+(put 'sweeprolog-read-new-variable-try :advertised-binding [?\C-m])
+
+(defvar sweeprolog-read-new-variable-map
+  (let ((map (make-sparse-keymap)))
+    (set-keymap-parent map minibuffer-local-map)
+    (define-key map (kbd "C-m") #'sweeprolog-read-new-variable-try)
+    (define-key map (kbd "C-j") #'sweeprolog-read-new-variable-try)
+    map)
+  "Keymap used by `sweeprolog-read-new-variable'.")
+
+(defun sweeprolog-read-new-variable (prompt existing-vars)
+  "Prompt with PROMPT for a new Prolog variable.
+EXISTING-VARS is a list of existing variable names (strings)."
+  (let ((sweeprolog-read-new-variable--existing-vars existing-vars))
+    (read-from-minibuffer prompt nil sweeprolog-read-new-variable-map)))
+
+(defun sweeprolog-read-existing-variable (occurrences &optional default)
+  (let* ((max-var-len (apply #'max
+                             (mapcar #'length
+                                     (mapcar #'car
+                                             occurrences))))
+         (completion-extra-properties
+          (list :annotation-function
+                (lambda (key)
+                  (let ((n (length (alist-get key
+                                              occurrences
+                                              nil nil
+                                              #'string=))))
+                    (concat (make-string (- max-var-len
+                                            (length key))
+                                         ? )
+                            (format " %d %s" n
+                                    (ngettext "occurrence"
+                                              "occurrences"
+                                              n))))))))
+    (completing-read
+     (concat
+      "Rename variable"
+      (when default
+        (concat " (default " (sweeprolog--format-variable default) ")"))
+      ": ")
+     occurrences nil t nil nil default)))
+
+(defun sweeprolog-rename-variable (&optional old new point verbose)
   "Rename the variable OLD to NEW in the Prolog term at POINT.
 
 If OLD is nil, prompt for it in the minibuffer with completion.
 If NEW is nil, prompt for it as well.  If POINT is nil, it
-defaults to the current point.  If INTERACTIVE is non-nil, also
-print a message with the number of replaced occurrences of OLD.
+defaults to the current point.  If VERBOSE is non-nil, also print
+a message with the number of replaced occurrences of OLD.
 
-Interactively, OLD, NEW and POINT are nil, and INTERACTIVE is t."
+Interactively, OLD, NEW and POINT are nil, and VERBOSE is t."
   (interactive (list nil nil nil t) sweeprolog-mode)
   (setq point (or point (point)))
   (let* ((term-var-occurrences (sweeprolog--variables-at-point point))
@@ -5575,34 +5663,19 @@ Interactively, OLD, NEW and POINT are nil, and INTERACTIVE is t."
          (var-at-point (cdr term-var-occurrences)))
     (unless var-occurrences
       (user-error "No variables to rename here!"))
-    (let* ((max-var-len (apply #'max
-                               (mapcar #'length
-                                       (mapcar #'car
-                                               var-occurrences))))
-           (completion-extra-properties
-            (list :annotation-function
-                  (lambda (key)
-                    (let ((n (length (alist-get key var-occurrences nil nil #'string=))))
-                      (concat (make-string (- max-var-len (length key)) ? )
-                              (format " %d %s" n
-                                      (ngettext "occurrence"
-                                                "occurrences"
-                                                n)))))))
-           (old-name
+    (let* ((old-name
             (or old
-                (completing-read
-                 (concat
-                  "Rename variable"
-                  (when-let ((def var-at-point))
-                    (concat " (default " (sweeprolog--format-variable def) ")"))
-                  ": ")
-                 var-occurrences nil t nil nil var-at-point)))
+                (sweeprolog-read-existing-variable var-occurrences
+                                                   var-at-point)))
+           (old-formatted (sweeprolog--format-variable old-name))
+           (existing-vars (mapcar #'car var-occurrences))
            (new-name
             (or new
-                (read-string
-                 (concat
-                  "Rename " (sweeprolog--format-variable old-name) " to: ")
-                 nil nil old-name)))
+                (sweeprolog-read-new-variable
+                 (concat "Rename "
+                         old-formatted
+                         " to: ")
+                 existing-vars)))
            (old-occurrences
             (alist-get old-name var-occurrences nil nil #'string=))
            (num (length old-occurrences)))
@@ -5614,11 +5687,13 @@ Interactively, OLD, NEW and POINT are nil, and INTERACTIVE is t."
               (delete-region occurrence-beg occurrence-end)
               (goto-char occurrence-beg)
               (insert new-name)))))
-      (when interactive
-        (message "Replaced %d %s of %s to %s."
+      (when verbose
+        (message (if (member new-name existing-vars)
+                     "Merged %d %s of %s to %s."
+                   "Replaced %d %s of %s to %s.")
                  num
                  (ngettext "occurrence" "occurrences" num)
-                 (sweeprolog--format-variable old-name)
+                 old-formatted
                  (sweeprolog--format-variable new-name))))))