bug#25936: Fix for whitespace.el to make it read tab-width and indent-tabs-mode correctly

bug#25936: Fix for whitespace.el to make it read tab-width and indent-tabs-mode correctly

[Reuben Thomas]

[-- Attachment #1.1: Type: text/plain, Size: 728 bytes --]

I noticed that whitespace-mode was not correctly diagnosing whitespace
problems in some files.

On investigation, this was because whitespace-report-region didn't call
whitespace-ensure-local-variables, so it didn't catch changes to
indent-tabs-mode in file-find hook functions.

However, on further investigation, I can't see why these local variables
are needed. Currently, whitespace-mode makes buffer-local copies of
indent-tabs-mode and tab-width, with names prefixed by "whitespace-". But
these variables are buffer-local if necessary already, and whitespace-mode
never tries to change them.

Hence, I attach a patch for review which simply removes these (apparently
unnecessary) local variables.

-- 
http://rrt.sc3d.org

[-- Attachment #1.2: Type: text/html, Size: 1313 bytes --]

[-- Attachment #2: 0001-Fix-reading-of-tab-settings-in-whitespace-mode.patch --]
[-- Type: text/x-patch, Size: 6204 bytes --]

From 1ca64fa2f7c44112f27dc56139d7319d3e95cca5 Mon Sep 17 00:00:00 2001
From: Reuben Thomas <rrt@sc3d.org>
Date: Thu, 2 Mar 2017 12:50:06 +0000
Subject: [PATCH] Fix reading of tab settings in whitespace-mode

lisp/whitespace.el (whitespace-indent-tabs-mode)
whitespace-tab-width): Remove these variables. The underlying
variables `indent-tabs-mode' and `tab-width' are already buffer-local
when needed, and whitespace-mode never changes them.
(whitespace-ensure-local-variables): Remove this function, which only
existed to set the above variables.
(whitespace-cleanup-region, whitespace-regexp)
(whitespace-indentation-regexp, whitespace-report-region)
(whitespace-turn-on, whitespace-color-on): Adjust these functions to
use `indent-tabs-mode' and `tab-width' directly, and not call
`whitespace-ensure-local-variables'.
---
 lisp/whitespace.el | 44 ++++++++++++++------------------------------
 1 file changed, 14 insertions(+), 30 deletions(-)

diff --git a/lisp/whitespace.el b/lisp/whitespace.el
index 6c4f59d..c44e2be 100644
--- a/lisp/whitespace.el
+++ b/lisp/whitespace.el
@@ -1134,12 +1134,6 @@ whitespace-toggle-option-alist
 (defvar whitespace-active-style nil
   "Used to save locally `whitespace-style' value.")
 
-(defvar whitespace-indent-tabs-mode indent-tabs-mode
-  "Used to save locally `indent-tabs-mode' value.")
-
-(defvar whitespace-tab-width tab-width
-  "Used to save locally `tab-width' value.")
-
 (defvar whitespace-point (point)
   "Used to save locally current point value.
 Used by function `whitespace-trailing-regexp' (which see).")
@@ -1415,12 +1409,6 @@ whitespace-cleanup
     ;; PROBLEM 6: `tab-width' or more SPACEs after TAB
     (whitespace-cleanup-region (point-min) (point-max)))))
 
-(defun whitespace-ensure-local-variables ()
-  "Set `whitespace-indent-tabs-mode' and `whitespace-tab-width' locally."
-  (set (make-local-variable 'whitespace-indent-tabs-mode)
-       indent-tabs-mode)
-  (set (make-local-variable 'whitespace-tab-width)
-       tab-width))
 
 ;;;###autoload
 (defun whitespace-cleanup-region (start end)
@@ -1467,11 +1455,8 @@ whitespace-cleanup-region
       ;; read-only buffer
       (whitespace-warn-read-only "cleanup region")
     ;; non-read-only buffer
-    (whitespace-ensure-local-variables)
     (let ((rstart           (min start end))
 	  (rend             (copy-marker (max start end)))
-	  (indent-tabs-mode whitespace-indent-tabs-mode)
-	  (tab-width        whitespace-tab-width)
 	  overwrite-mode		; enforce no overwrite
 	  tmp)
       (save-excursion
@@ -1512,7 +1497,7 @@ whitespace-cleanup-region
          ;; by SPACEs.
          ((memq 'space-after-tab whitespace-style)
           (whitespace-replace-action
-           (if whitespace-indent-tabs-mode 'tabify 'untabify)
+           (if indent-tabs-mode 'tabify 'untabify)
            rstart rend (whitespace-space-after-tab-regexp) 1))
          ;; ACTION: replace `tab-width' or more SPACEs by TABs.
          ((memq 'space-after-tab::tab whitespace-style)
@@ -1531,9 +1516,9 @@ whitespace-cleanup-region
          ;; by SPACEs.
          ((memq 'space-before-tab whitespace-style)
           (whitespace-replace-action
-           (if whitespace-indent-tabs-mode 'tabify 'untabify)
+           (if indent-tabs-mode 'tabify 'untabify)
            rstart rend whitespace-space-before-tab-regexp
-           (if whitespace-indent-tabs-mode 0 2)))
+           (if indent-tabs-mode 0 2)))
          ;; ACTION: replace SPACEs before TAB by TABs.
          ((memq 'space-before-tab::tab whitespace-style)
           (whitespace-replace-action
@@ -1564,25 +1549,25 @@ whitespace-replace-action
 
 
 (defun whitespace-regexp (regexp &optional kind)
-  "Return REGEXP depending on `whitespace-indent-tabs-mode'."
+  "Return REGEXP depending on `indent-tabs-mode'."
   (format
    (cond
     ((or (eq kind 'tab)
-         whitespace-indent-tabs-mode)
+         indent-tabs-mode)
      (car regexp))
     ((or (eq kind 'space)
-         (not whitespace-indent-tabs-mode))
+         (not indent-tabs-mode))
      (cdr regexp)))
-   whitespace-tab-width))
+   tab-width))
 
 
 (defun whitespace-indentation-regexp (&optional kind)
-  "Return the indentation regexp depending on `whitespace-indent-tabs-mode'."
+  "Return the indentation regexp depending on `indent-tabs-mode'."
   (whitespace-regexp whitespace-indentation-regexp kind))
 
 
 (defun whitespace-space-after-tab-regexp (&optional kind)
-  "Return the space-after-tab regexp depending on `whitespace-indent-tabs-mode'."
+  "Return the space-after-tab regexp depending on `indent-tabs-mode'."
   (whitespace-regexp whitespace-space-after-tab-regexp kind))
 
 
@@ -1744,10 +1729,10 @@ whitespace-report-region
              whitespace-report-list)))
       (when (pcase report-if-bogus (`nil t) (`never nil) (_ has-bogus))
         (whitespace-kill-buffer whitespace-report-buffer-name)
-        ;; `whitespace-indent-tabs-mode' is local to current buffer
-        ;; `whitespace-tab-width' is local to current buffer
-        (let ((ws-indent-tabs-mode whitespace-indent-tabs-mode)
-              (ws-tab-width whitespace-tab-width))
+        ;; `indent-tabs-mode' may be local to current buffer
+        ;; `tab-width' may be local to current buffer
+        (let ((ws-indent-tabs-mode indent-tabs-mode)
+              (ws-tab-width tab-width))
           (with-current-buffer (get-buffer-create
                                 whitespace-report-buffer-name)
             (erase-buffer)
@@ -2027,7 +2012,6 @@ whitespace-turn-on
        (if (listp whitespace-style)
 	   whitespace-style
 	 (list whitespace-style)))
-  (whitespace-ensure-local-variables)
   ;; turn on whitespace
   (when whitespace-active-style
     (whitespace-color-on)
@@ -2123,7 +2107,7 @@ whitespace-color-on
               ,(cond
                 ((memq 'space-before-tab whitespace-active-style)
                  ;; Show SPACEs before TAB (indent-tabs-mode).
-                 (if whitespace-indent-tabs-mode 1 2))
+                 (if indent-tabs-mode 1 2))
                 ((memq 'space-before-tab::tab whitespace-active-style)
                  1)
                 ((memq 'space-before-tab::space whitespace-active-style)
-- 
2.7.4

bug#25936: Fix for whitespace.el to make it read tab-width and indent-tabs-mode correctly

[npostavs]

Reuben Thomas <rrt@sc3d.org> writes:

> However, on further investigation, I can't see why these local variables
> are needed. Currently, whitespace-mode makes buffer-local copies of
> indent-tabs-mode and tab-width, with names prefixed by "whitespace-". But
> these variables are buffer-local if necessary already, and whitespace-mode
> never tries to change them.

These variables seem to have been introduced in [1: 55d1cfe870].  I
agree that it looks like there is no need for them, but I'll wait
another couple weeks in case someone else has some insights.  Adding
author of that commit (and the person mentioned as "suggesting" it) on
CC in case they might remember anything about it.

1: 2008-04-16 03:41:17 +0000 55d1cfe8703c5829bacc8d129277f1f9b33950f6
  Honor the indent-tabs-mode and tab-width setting from user.

bug#25936: Fix for whitespace.el to make it read tab-width and indent-tabs-mode correctly

[npostavs]

npostavs@users.sourceforge.net writes:

> Reuben Thomas <rrt@sc3d.org> writes:
>
>> However, on further investigation, I can't see why these local variables
>> are needed. Currently, whitespace-mode makes buffer-local copies of
>> indent-tabs-mode and tab-width, with names prefixed by "whitespace-". But
>> these variables are buffer-local if necessary already, and whitespace-mode
>> never tries to change them.
>
> These variables seem to have been introduced in [1: 55d1cfe870].  I
> agree that it looks like there is no need for them, but I'll wait
> another couple weeks in case someone else has some insights.  Adding
> author of that commit (and the person mentioned as "suggesting" it) on
> CC in case they might remember anything about it.
>
> 1: 2008-04-16 03:41:17 +0000 55d1cfe8703c5829bacc8d129277f1f9b33950f6
>   Honor the indent-tabs-mode and tab-width setting from user.

I guess they won't answer.  The compiler is telling me you missed a
reference of whitespace-tab-width though.

whitespace.el:2095:45:Warning: reference to free variable
    ‘whitespace-tab-width’

bug#25936: Fix for whitespace.el to make it read tab-width and indent-tabs-mode correctly

[Reuben Thomas]

[-- Attachment #1: Type: text/plain, Size: 410 bytes --]

On 18 April 2017 at 13:06,  <npostavs@users.sourceforge.net> wrote:
> npostavs@users.sourceforge.net writes:
>
> I guess they won't answer.  The compiler is telling me you missed a
> reference of whitespace-tab-width though.
>
> whitespace.el:2095:45:Warning: reference to free variable
>     ‘whitespace-tab-width’

Thanks very much for this. Revised patch attached.

-- 
http://rrt.sc3d.org

[-- Attachment #2: 0006-Fix-reading-of-tab-settings-in-whitespace-mode.patch --]
[-- Type: text/x-patch, Size: 6941 bytes --]

From 9529e6499aa480342babba7524b98255610066ee Mon Sep 17 00:00:00 2001
From: Reuben Thomas <rrt@sc3d.org>
Date: Thu, 2 Mar 2017 12:50:06 +0000
Subject: [PATCH 6/6] Fix reading of tab settings in whitespace-mode

lisp/whitespace.el (whitespace-indent-tabs-mode)
whitespace-tab-width): Remove these variables. The underlying
variables `indent-tabs-mode' and `tab-width' are already buffer-local
when needed, and whitespace-mode never changes them.
(whitespace-ensure-local-variables): Remove this function, which only
existed to set the above variables.
(whitespace-cleanup-region, whitespace-regexp)
(whitespace-indentation-regexp, whitespace-report-region)
(whitespace-turn-on, whitespace-color-on): Adjust these functions to
use `indent-tabs-mode' and `tab-width' directly, and not call
`whitespace-ensure-local-variables'.
---
 lisp/whitespace.el | 52 ++++++++++++++++++----------------------------------
 1 file changed, 18 insertions(+), 34 deletions(-)

diff --git a/lisp/whitespace.el b/lisp/whitespace.el
index 6c4f59d..6aca47c 100644
--- a/lisp/whitespace.el
+++ b/lisp/whitespace.el
@@ -1134,12 +1134,6 @@ whitespace-toggle-option-alist
 (defvar whitespace-active-style nil
   "Used to save locally `whitespace-style' value.")
 
-(defvar whitespace-indent-tabs-mode indent-tabs-mode
-  "Used to save locally `indent-tabs-mode' value.")
-
-(defvar whitespace-tab-width tab-width
-  "Used to save locally `tab-width' value.")
-
 (defvar whitespace-point (point)
   "Used to save locally current point value.
 Used by function `whitespace-trailing-regexp' (which see).")
@@ -1415,12 +1409,6 @@ whitespace-cleanup
     ;; PROBLEM 6: `tab-width' or more SPACEs after TAB
     (whitespace-cleanup-region (point-min) (point-max)))))
 
-(defun whitespace-ensure-local-variables ()
-  "Set `whitespace-indent-tabs-mode' and `whitespace-tab-width' locally."
-  (set (make-local-variable 'whitespace-indent-tabs-mode)
-       indent-tabs-mode)
-  (set (make-local-variable 'whitespace-tab-width)
-       tab-width))
 
 ;;;###autoload
 (defun whitespace-cleanup-region (start end)
@@ -1467,11 +1455,8 @@ whitespace-cleanup-region
       ;; read-only buffer
       (whitespace-warn-read-only "cleanup region")
     ;; non-read-only buffer
-    (whitespace-ensure-local-variables)
     (let ((rstart           (min start end))
 	  (rend             (copy-marker (max start end)))
-	  (indent-tabs-mode whitespace-indent-tabs-mode)
-	  (tab-width        whitespace-tab-width)
 	  overwrite-mode		; enforce no overwrite
 	  tmp)
       (save-excursion
@@ -1512,7 +1497,7 @@ whitespace-cleanup-region
          ;; by SPACEs.
          ((memq 'space-after-tab whitespace-style)
           (whitespace-replace-action
-           (if whitespace-indent-tabs-mode 'tabify 'untabify)
+           (if indent-tabs-mode 'tabify 'untabify)
            rstart rend (whitespace-space-after-tab-regexp) 1))
          ;; ACTION: replace `tab-width' or more SPACEs by TABs.
          ((memq 'space-after-tab::tab whitespace-style)
@@ -1531,9 +1516,9 @@ whitespace-cleanup-region
          ;; by SPACEs.
          ((memq 'space-before-tab whitespace-style)
           (whitespace-replace-action
-           (if whitespace-indent-tabs-mode 'tabify 'untabify)
+           (if indent-tabs-mode 'tabify 'untabify)
            rstart rend whitespace-space-before-tab-regexp
-           (if whitespace-indent-tabs-mode 0 2)))
+           (if indent-tabs-mode 0 2)))
          ;; ACTION: replace SPACEs before TAB by TABs.
          ((memq 'space-before-tab::tab whitespace-style)
           (whitespace-replace-action
@@ -1564,25 +1549,25 @@ whitespace-replace-action
 
 
 (defun whitespace-regexp (regexp &optional kind)
-  "Return REGEXP depending on `whitespace-indent-tabs-mode'."
+  "Return REGEXP depending on `indent-tabs-mode'."
   (format
    (cond
     ((or (eq kind 'tab)
-         whitespace-indent-tabs-mode)
+         indent-tabs-mode)
      (car regexp))
     ((or (eq kind 'space)
-         (not whitespace-indent-tabs-mode))
+         (not indent-tabs-mode))
      (cdr regexp)))
-   whitespace-tab-width))
+   tab-width))
 
 
 (defun whitespace-indentation-regexp (&optional kind)
-  "Return the indentation regexp depending on `whitespace-indent-tabs-mode'."
+  "Return the indentation regexp depending on `indent-tabs-mode'."
   (whitespace-regexp whitespace-indentation-regexp kind))
 
 
 (defun whitespace-space-after-tab-regexp (&optional kind)
-  "Return the space-after-tab regexp depending on `whitespace-indent-tabs-mode'."
+  "Return the space-after-tab regexp depending on `indent-tabs-mode'."
   (whitespace-regexp whitespace-space-after-tab-regexp kind))
 
 
@@ -1744,10 +1729,10 @@ whitespace-report-region
              whitespace-report-list)))
       (when (pcase report-if-bogus (`nil t) (`never nil) (_ has-bogus))
         (whitespace-kill-buffer whitespace-report-buffer-name)
-        ;; `whitespace-indent-tabs-mode' is local to current buffer
-        ;; `whitespace-tab-width' is local to current buffer
-        (let ((ws-indent-tabs-mode whitespace-indent-tabs-mode)
-              (ws-tab-width whitespace-tab-width))
+        ;; `indent-tabs-mode' may be local to current buffer
+        ;; `tab-width' may be local to current buffer
+        (let ((ws-indent-tabs-mode indent-tabs-mode)
+              (ws-tab-width tab-width))
           (with-current-buffer (get-buffer-create
                                 whitespace-report-buffer-name)
             (erase-buffer)
@@ -2027,7 +2012,6 @@ whitespace-turn-on
        (if (listp whitespace-style)
 	   whitespace-style
 	 (list whitespace-style)))
-  (whitespace-ensure-local-variables)
   ;; turn on whitespace
   (when whitespace-active-style
     (whitespace-color-on)
@@ -2105,10 +2089,10 @@ whitespace-color-on
            `((,(let ((line-column (or whitespace-line-column fill-column)))
                  (format
                   "^\\([^\t\n]\\{%s\\}\\|[^\t\n]\\{0,%s\\}\t\\)\\{%d\\}%s\\(.+\\)$"
-                  whitespace-tab-width
-                  (1- whitespace-tab-width)
-                  (/ line-column whitespace-tab-width)
-                  (let ((rem (% line-column whitespace-tab-width)))
+                  tab-width
+                  (1- tab-width)
+                  (/ line-column tab-width)
+                  (let ((rem (% line-column tab-width)))
                     (if (zerop rem)
                         ""
                       (format ".\\{%d\\}" rem)))))
@@ -2123,7 +2107,7 @@ whitespace-color-on
               ,(cond
                 ((memq 'space-before-tab whitespace-active-style)
                  ;; Show SPACEs before TAB (indent-tabs-mode).
-                 (if whitespace-indent-tabs-mode 1 2))
+                 (if indent-tabs-mode 1 2))
                 ((memq 'space-before-tab::tab whitespace-active-style)
                  1)
                 ((memq 'space-before-tab::space whitespace-active-style)
-- 
2.7.4

bug#25936: Fix for whitespace.el to make it read tab-width and indent-tabs-mode correctly

[npostavs]

tags 25936 fixed
close 25936 26.1
quit

Reuben Thomas <rrt@sc3d.org> writes:

> On 18 April 2017 at 13:06,  <npostavs@users.sourceforge.net> wrote:
>
> Thanks very much for this. Revised patch attached.

Thanks, pushed to master [1: a6b375ba4b].

[1: a6b375ba4b]: 2017-04-20 22:39:15 -0400
  Fix reading of tab settings in whitespace-mode
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=a6b375ba4bfc9453abc428dcb73e65bfcf61b794