bug#22983: [ Patch ] Re: bug#22983: syntax-ppss returns wrong result.

[Alan Mackenzie]

Hello, Dmitry and Philipp.

On Sat, Sep 09, 2017 at 12:44:02 +0300, Dmitry Gutov wrote:
> Hi Alan,

> On 9/7/17 11:45 PM, Alan Mackenzie wrote:

> > The solution I propose is to introduce a second cache into syntax-ppss,
> > and this cache would be used whenever (not (eq (point-min) 1)).
> > Whenever point-min changes, and isn't 1, this second cached would be
> > calculated again from scratch.

Here is a patch implementing this.  Comments about it would be welcome.

[ .... ]

> And since the API doesn't change, and the observable behavior doesn't 
> either (in the vast majority of cases; probably all except the broken 
> ones), we can refine this solution easily, or even swap it for something 
> else, with little cost.

[ .... ]

> Caveats:

> - This solves the dependency on point-min, but does nothing about the 
> dependency on the current syntax-table (which can change). I'm not 
> necessarily suggesting we try to solve that now, though.

> - Before this change is pushed to master, or shortly after, I'd like to 
> know that it actually fixed the problem Philipp experienced with 
> python-mode, so we can revert 4fbd330. If it was caused by e.g. 
> syntax-table changing, we've not improved much.

Philipp, any chance of you trying out python mode with this patch but
without 4fbd330?



diff --git a/lisp/emacs-lisp/syntax.el b/lisp/emacs-lisp/syntax.el
index d1d5176944..952ea8bb83 100644
--- a/lisp/emacs-lisp/syntax.el
+++ b/lisp/emacs-lisp/syntax.el
@@ -386,11 +386,103 @@ syntax-ppss-cache
 (defvar-local syntax-ppss-last nil
   "Cache of (LAST-POS . LAST-PPSS).")
 
-(defalias 'syntax-ppss-after-change-function 'syntax-ppss-flush-cache)
-(defun syntax-ppss-flush-cache (beg &rest ignored)
-  "Flush the cache of `syntax-ppss' starting at position BEG."
-  ;; Set syntax-propertize to refontify anything past beg.
-  (setq syntax-propertize--done (min beg syntax-propertize--done))
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;; Several caches.
+;;
+;; Because `syntax-ppss' is equivalent to (parse-partial-sexp
+;; (POINT-MIN) x), we need either to empty the cache when we narrow
+;; the buffer, which is suboptimal, or we need to use several caches.
+;;
+;; The implementation which follows uses three caches, the current one
+;; (in `syntax-ppss-cache' and `syntax-ppss-last') and two inactive
+;; ones (in `syntax-ppss-{cache,last}-{wide,narrow}'), which store the
+;; former state of the active cache as it was used in widened and
+;; narrowed buffers respectively.  There are also the variables
+;; `syntax-ppss-max-valid-{wide,narrow}' which hold the maximum
+;; position where the caches are valid, due to buffer changes.
+;;
+;; At the first call to `syntax-ppss' after a widening or narrowing of
+;; the buffer, the pertinent inactive cache is swapped into the
+;; current cache by calling `syntax-ppss-set-cache'.  Note that there
+;; is currently just one inactive cache for narrowed buffers, so only
+;; one inactive narrowed cache can be stored at a time.
+;;
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+
+(defvar-local syntax-ppss-cache-wide nil
+  "Holds the value of `syntax-ppss-cache' for a widened buffer.")
+(defvar-local syntax-ppss-last-wide nil
+  "Holds the value of `syntax-ppss-last' for a widened buffer.")
+(defvar-local syntax-ppss-max-valid-wide most-positive-fixnum
+  "The buffer position after which `syntax-ppss-cache-wide' is invalid.")
+
+(defvar-local syntax-ppss-cache-narrow nil
+  "Holds the value of `syntax-ppss-cache' for a narrowed buffer.")
+(defvar-local syntax-ppss-last-narrow nil
+  "Holds the value of `syntax-ppss-last' for a narrowed buffer.")
+(defvar-local syntax-ppss-max-valid-narrow most-positive-fixnum
+  "The buffer position after which `syntax-ppss-cache-narrow' is invalid.")
+
+(defvar-local syntax-ppss-narrow-point-min 1
+  "Value of `point-min' for which the stored \"narrow\" cache is valid.")
+
+(defvar-local syntax-ppss-supremum most-positive-fixnum
+  "Lowest change position since previous restriction change.")
+
+(defvar-local syntax-ppss-cache-point-min 1
+  "Value of `point-min' for which the current cache is valid.")
+
+(defun syntax-ppss-set-cache ()
+  "Swap in and out the cache pertinent to the new point-min."
+  (unless (eq (point-min) syntax-ppss-cache-point-min)
+    ;; Update the stored `...max-valid' values.
+    (setq syntax-ppss-max-valid-wide
+          (if (eq syntax-ppss-cache-point-min 1)
+              (or (caar syntax-ppss-cache)
+                  1)
+            (min syntax-ppss-max-valid-wide syntax-ppss-supremum)))
+    (setq syntax-ppss-max-valid-narrow
+          (if (eq syntax-ppss-cache-point-min syntax-ppss-narrow-point-min)
+              (or (caar syntax-ppss-cache)
+                  syntax-ppss-cache-point-min)
+            (min syntax-ppss-max-valid-narrow syntax-ppss-supremum)))
+    (setq syntax-ppss-supremum most-positive-fixnum)
+
+    ;; Store away the current values of the cache.
+    (cond
+     ((eq syntax-ppss-cache-point-min 1)
+      (setq syntax-ppss-cache-wide syntax-ppss-cache
+            syntax-ppss-last-wide syntax-ppss-last))
+     ((eq syntax-ppss-cache-point-min syntax-ppss-narrow-point-min)
+      (setq syntax-ppss-cache-narrow syntax-ppss-cache
+            syntax-ppss-last-narrow syntax-ppss-last))
+     (syntax-ppss-cache
+      (setq syntax-ppss-narrow-point-min syntax-ppss-cache-point-min
+            syntax-ppss-cache-narrow syntax-ppss-cache
+            syntax-ppss-last-narrow syntax-ppss-last))
+     (t nil))
+
+    ;; Restore/initialize the cache for the new point-min.
+    (cond
+     ((eq (point-min) 1)
+      (setq syntax-ppss-cache syntax-ppss-cache-wide
+            syntax-ppss-last syntax-ppss-last-wide)
+      (save-restriction
+        (widen)
+        (syntax-ppss-invalidate-cache syntax-ppss-max-valid-wide)))
+     ((eq (point-min) syntax-ppss-narrow-point-min)
+      (setq syntax-ppss-cache syntax-ppss-cache-narrow
+            syntax-ppss-last syntax-ppss-last-narrow)
+      (save-restriction
+        (widen)
+        (syntax-ppss-invalidate-cache syntax-ppss-max-valid-narrow)))
+     (t
+      (setq syntax-ppss-cache nil
+            syntax-ppss-last nil)))
+    (setq syntax-ppss-cache-point-min (point-min))))
+
+(defun syntax-ppss-invalidate-cache (beg &rest ignored)
+  "Invalidate the cache of `syntax-ppss' starting at position BEG."
   ;; Flush invalid cache entries.
   (while (and syntax-ppss-cache (> (caar syntax-ppss-cache) beg))
     (setq syntax-ppss-cache (cdr syntax-ppss-cache)))
@@ -411,6 +503,16 @@ syntax-ppss-flush-cache
   ;;   (remove-hook 'before-change-functions 'syntax-ppss-flush-cache t))
   )
 
+;; Retain the following two for compatibility reasons.
+(defalias 'syntax-ppss-after-change-function 'syntax-ppss-flush-cache)
+(defun syntax-ppss-flush-cache (beg &rest ignored)
+  "Flush the `syntax-ppss' caches and set `syntax-propertize--done'."
+  (setq syntax-ppss-supremum (min beg syntax-ppss-supremum))
+  ;; Ensure the appropriate cache is active.
+  (syntax-ppss-set-cache)
+  (setq syntax-propertize--done (min beg syntax-propertize--done))
+  (syntax-ppss-invalidate-cache beg ignored))
+
 (defvar syntax-ppss-stats
   [(0 . 0.0) (0 . 0.0) (0 . 0.0) (0 . 0.0) (0 . 0.0) (1 . 2500.0)])
 (defun syntax-ppss-stats ()
@@ -434,6 +536,8 @@ syntax-ppss
 this function is called while `before-change-functions' is
 temporarily let-bound, or if the buffer is modified without
 running the hook."
+  ;; Ensure the appropriate cache is active.
+  (syntax-ppss-set-cache)
   ;; Default values.
   (unless pos (setq pos (point)))
   (syntax-propertize pos)


> All the best,
> Dmitry.

-- 
Alan Mackenzie (Nuremberg, Germany).