Repository: rust-lang/rust-mode (language: rust)

Issue:
`rust-syntax-propertize` assumes text already has the properties it is responsible for placing
The `rust-syntax-propertize` function is responsible for setting `syntax-table` text properties on a given region of text. However, it fails to do its job properly, because it uses `syntax-ppss`, which assumes that `syntax-table` properties have already been set.

The `syntax-ppss` function distributed with Emacs calls `syntax-propertize-function` to place `syntax-table` properties as needed, so that when `rust-match-angle-brackets` is enabled, `parse-partial-sexp` can distinguish between `<` and `>` characters surrounding generic parameters and those that are comparison operators or `->` or `=>` tokens. When they enclose generic parameters, `syntax-ppss` treats `<` and `>` tokens as opening and closing "parentheses", in the sense used by Emacs syntax table code.

However, in `rust-mode` buffers, `syntax-propertize-function` is `rust-syntax-propertize`, which ends up calling `syntax-ppss` itself. Unbounded recursion is prevented by `syntax-propertize` temporarily binding `syntax-propertize--done` to `most-positive-fixnum`, but this also means that the propertization doesn't get done.

I don't understand exactly how this happens yet, but I have verified that `parse-partial-sexp` is being called from `rust-syntax-propertize` on regions of text whose `->` tokens have not been marked with `syntax-table` text properties.


Discussion / hints:
I forgot to mention the symptom: after using `fill-paragraph` on a comment, indentation is incorrect: hitting TAB on a top-level comment line results in a ten-space indentation, when it should be zero. This is because syntax-ppss is returning garbage results for this location (a paren nesting depth of -44, for example).
This bug is hard to trigger because it depends on `rust-macro-scope` first polluting the `syntax-ppss-wide` cache with incorrect information, which it will only do if the position that `syntax-ppss` is asked to analyze is at least `syntax-ppss-max-span` characters past the last position requested. The decision to populate `syntax-ppss-wide` also depends on whether the new position is more than twice the average length of all the scans performed by `syntax-begin-function` so far in this Emacs session. (The code in `syntax.el` seems quite overconfident.)

Fix patch:
diff --git a/rust-mode.el b/rust-mode.el
index 80106b8..780474f 100644
--- a/rust-mode.el
+++ b/rust-mode.el
@@ -362,6 +362,10 @@ See `prettify-symbols-compose-predicate'."
     "bool"
     "str" "char"))
 
+(defconst rust-expression-introducers
+  '("if" "while" "match" "return" "box" "in")
+  "List of Rust keywords that are always followed by expressions.")
+
 (defconst rust-number-with-type
   (eval-when-compile
     (concat
@@ -526,22 +530,36 @@ symbols."
                symbols)))))
 
 (defun rust-looking-back-ident ()
-  "Non-nil if we are looking backwards at a valid rust identifier."
-  (let ((beg-of-symbol (save-excursion (forward-thing 'symbol -1) (point))))
-    (looking-back rust-re-ident beg-of-symbol)))
+  "Non-nil if we are looking backwards at a valid rust identifier.
+If we are, regexp match 0 is the identifier."
+  (let ((outer-point (point)))
+    (save-excursion
+      (forward-thing 'symbol -1)
+      (and (looking-at rust-re-ident)
+           (eq (match-end 0) outer-point)))))
 
 (defun rust-looking-back-macro ()
-  "Non-nil if looking back at an ident followed by a !
-
-This is stricter than rust syntax which allows a space between
-the ident and the ! symbol. If this space is allowed, then we
-would also need a keyword check to avoid `if !(condition)` being
-seen as a macro."
-  (if (> (- (point) (point-min)) 1)
-      (save-excursion
-        (backward-char)
-        (and (= ?! (char-after))
-             (rust-looking-back-ident)))))
+  "Non-nil if looking back at a potential macro name followed by a \"!\".
+If we are, regexp match 0 is the macro name."
+  (save-excursion
+    ;; Look past whitespace and line breaks.
+    ;; > is okay because we only use it for \n and \r, not "*/"
+    (skip-syntax-backward "->")
+    (when (eq (char-before) ?!)
+      (forward-char -1)
+      (skip-syntax-backward "->")
+      (when (rust-looking-back-ident)
+        (let ((ident (match-string 0)))
+          (not (member ident rust-expression-introducers)))))))
+
+(defun rust-looking-back-macro-rules ()
+  "Non-nil if looking back at \"macro_rules IDENT !\"."
+  (save-excursion
+    (skip-syntax-backward "->")
+    (let ((outer-point (point)))
+      (forward-thing 'symbol -2)
+      (and (looking-at (concat "macro_rules\\s-*!\\s-*" rust-re-ident))
+           (eq (match-end 0) outer-point)))))
 
 ;;; Syntax definitions and helpers
 
@@ -562,90 +580,25 @@ seen as a macro."
         ;; Rewind until the point no longer moves
         (setq continue (/= starting (point)))))))
 
-(defvar-local rust-macro-scopes nil
-  "Cache for the scopes calculated by `rust-macro-scope'.
-
-This variable can be `let' bound directly or indirectly around
-`rust-macro-scope' as an optimization but should not be otherwise
-set.")
-
-(defun rust-macro-scope (start end)
-  "Return the scope of macros in the buffer.
-
-The return value is a list of (START END) positions in the
-buffer.
-
-If set START and END are optimizations which limit the return
-value to scopes which are approximately with this range."
-  (save-excursion
-    ;; need to special case macro_rules which has unique syntax
-    (let ((scope nil)
-          (start (or start (point-min)))
-          (end (or end (point-max))))
-      (goto-char start)
-      ;; if there is a start move back to the previous top level,
-      ;; as any macros before that must have closed by this time.
-      (let ((top (syntax-ppss-toplevel-pos (syntax-ppss))))
-        (when top
-          (goto-char top)))
-      (while
-          (and
-           ;; The movement below may have moved us passed end, in
-           ;; which case search-forward will error
-           (< (point) end)
-           (search-forward "!" end t))
-        (let ((pt (point)))
-          (cond
-           ;; in a string or comment is boring, move straight on
-           ((rust-in-str-or-cmnt))
-           ;; in a normal macro,
-           ((and (skip-chars-forward " \t\n\r")
-                 (memq (char-after)
-                       '(?\[ ?\( ?\{))
-                 ;; Check that we have a macro declaration after.
-                 (rust-looking-back-macro))
-            (let ((start (point)))
-              (ignore-errors (forward-list))
-              (setq scope (cons (list start (point)) scope))))
-           ;; macro_rules, why, why, why did you not use macro syntax??
-           ((save-excursion
-              ;; yuck -- last test moves point, even if it fails
-              (goto-char (- pt 1))
-              (skip-chars-backward " \t\n\r")
-              (rust-looking-back-str "macro_rules"))
-            (save-excursion
-              (when (re-search-forward "[[({]" nil t)
-                (backward-char)
-                (let ((start (point)))
-                  (ignore-errors (forward-list))
-                  (setq scope (cons (list start (point)) scope)))))))))
-      ;; Return 'empty rather than nil, to indicate a buffer with no
-      ;; macros at all.
-      (or scope 'empty))))
-
-(defun rust-in-macro (&optional start end)
+(defun rust-in-macro ()
   "Return non-nil when point is within the scope of a macro.
-
-If START and END are set, minimize the buffer analysis to
-approximately this location as an optimization.
-
-Alternatively, if `rust-macro-scopes' is a list use the scope
-information in this variable. This last is an optimization and
-the caller is responsible for ensuring that the data in
-`rust-macro-scopes' is up to date."
-  (when (> (rust-paren-level) 0)
-    (let ((scopes
-           (or
-            rust-macro-scopes
-            (rust-macro-scope start end))))
-      ;; `rust-macro-scope' can return the symbol `empty' if the
-      ;; buffer has no macros at all.
-      (when (listp scopes)
-        (seq-some
-         (lambda (sc)
-           (and (>= (point) (car sc))
-                (< (point) (cadr sc))))
-         scopes)))))
+If we are, return the position of the opening bracket of the macro's arguments."
+  (let ((ppss (syntax-ppss)))
+    ;; If we're in a string or comment, we're definitely not on a token a macro
+    ;; will see.
+    (when (not (or (nth 3 ppss) (nth 4 ppss)))
+      ;; Walk outward to enclosing parens, looking for one preceded by "ident !"
+      ;; or "macro_rules! ident".
+      (let (result
+            (enclosing (reverse (nth 9 ppss))))
+        (save-excursion
+          (while enclosing
+            (goto-char (car enclosing))
+            (if (or (rust-looking-back-macro)
+                    (rust-looking-back-macro-rules))
+                (setq result (point) enclosing nil)
+              (setq enclosing (cdr enclosing)))))
+        result))))
 
 (defun rust-looking-at-where ()
   "Return T when looking at the \"where\" keyword."
@@ -1089,7 +1042,7 @@ outside of this context."
       (cond
 
        ;; Certain keywords always introduce expressions
-       ((rust-looking-back-symbols '("if" "while" "match" "return" "box" "in")) t)
+       ((rust-looking-back-symbols rust-expression-introducers) t)
 
        ;; "as" introduces a type
        ((rust-looking-back-symbols '("as")) nil)
@@ -1411,34 +1364,32 @@ whichever comes first."
 
 (defun rust-syntax-propertize (start end)
   "A `syntax-propertize-function' to apply properties from START to END."
-  ;; Cache all macro scopes as an optimization. See issue #208
-  (let ((rust-macro-scopes (rust-macro-scope start end)))
-    (goto-char start)
-    (let ((str-start (rust-in-str-or-cmnt)))
-      (when str-start
-        (rust--syntax-propertize-raw-string str-start end)))
-    (funcall
-     (syntax-propertize-rules
-      ;; Character literals.
-      (rust--char-literal-rx (1 "\"") (2 "\""))
-      ;; Raw strings.
-      ("\\(r\\)#*\""
-       (0 (ignore
-           (goto-char (match-end 0))
-           (unless (save-excursion (nth 8 (syntax-ppss (match-beginning 0))))
-             (put-text-property (match-beginning 1) (match-end 1)
-                                'syntax-table (string-to-syntax "|"))
-             (rust--syntax-propertize-raw-string (match-beginning 0) end)))))
-      ("[<>]"
-       (0 (ignore
-           (when (save-match-data
-                   (save-excursion
-                     (goto-char (match-beginning 0))
-                     (rust-ordinary-lt-gt-p)))
-             (put-text-property (match-beginning 0) (match-end 0)
-                                'syntax-table (string-to-syntax "."))
-             (goto-char (match-end 0)))))))
-     (point) end)))
+  (goto-char start)
+  (let ((str-start (rust-in-str-or-cmnt)))
+    (when str-start
+      (rust--syntax-propertize-raw-string str-start end)))
+  (funcall
+   (syntax-propertize-rules
+    ;; Character literals.
+    (rust--char-literal-rx (1 "\"") (2 "\""))
+    ;; Raw strings.
+    ("\\(r\\)#*\""
+     (0 (ignore
+         (goto-char (match-end 0))
+         (unless (save-excursion (nth 8 (syntax-ppss (match-beginning 0))))
+           (put-text-property (match-beginning 1) (match-end 1)
+                              'syntax-table (string-to-syntax "|"))
+           (rust--syntax-propertize-raw-string (match-beginning 0) end)))))
+    ("[<>]"
+     (0 (ignore
+         (when (save-match-data
+                 (save-excursion
+                   (goto-char (match-beginning 0))
+                   (rust-ordinary-lt-gt-p)))
+           (put-text-property (match-beginning 0) (match-end 0)
+                              'syntax-table (string-to-syntax "."))
+           (goto-char (match-end 0)))))))
+   (point) end))
 
 (defun rust-fill-prefix-for-comment-start (line-start)
   "Determine what to use for `fill-prefix' based on the text at LINE-START."


Test patch:
diff --git a/rust-mode-tests.el b/rust-mode-tests.el
index e4949b2..abbfcb6 100644
--- a/rust-mode-tests.el
+++ b/rust-mode-tests.el
@@ -3116,7 +3116,7 @@ macro_c!{
      (syntax-ppss))))
 
 
-(ert-deftest rust-test-in-macro-no-caching ()
+(ert-deftest rust-test-in-macro-around-opening ()
   (should-not
    (with-temp-buffer
      (insert
@@ -3125,66 +3125,38 @@ macro_c!{
         struct Boo<D> {}
 ")
      (rust-mode)
-     (search-backward "macro")
-     ;; do not use the cache
-     (let ((rust-macro-scopes nil))
-       (rust-in-macro)))))
-
-(ert-deftest rust-test-in-macro-fake-cache ()
-  (should
-   (with-temp-buffer
-     (insert
-      "fn foo<A>(a:A) {
-    macro_c!{
-        struct Boo<D> {}
-")
-     (rust-mode)
-     (search-backward "macro")
-     ;; make the cache lie to make the whole buffer in scope
-     ;; we need to be at paren level 1 for this to work
-     (let ((rust-macro-scopes `((,(point-min) ,(point-max)))))
-       (rust-in-macro)))))
-
-(ert-deftest rust-test-in-macro-broken-cache ()
-  (should-error
-   (with-temp-buffer
-     (insert
-      "fn foo<A>(a:A) {
-    macro_c!{
-        struct Boo<D> {}
-")
-     (rust-mode)
-     (search-backward "Boo")
-     ;; do we use the cache at all
-     (let ((rust-macro-scopes '(I should break)))
-       (rust-in-macro)))))
+     (search-backward "macro_c")
+     (and
+      (not (rust-in-macro))
+      (progn (forward-thing 'symbol 1) (not (rust-in-macro)))
+      (progn (forward-char 1) (rust-in-macro))
+      (progn (goto-char (point-max)) (rust-in-macro))))))
 
 (ert-deftest rust-test-in-macro-nested ()
-  (should
-   (equal
-    (with-temp-buffer
-      (insert
-       "macro_rules! outer {
+  (with-temp-buffer
+    (insert
+     "macro_rules! outer {
     () => { vec![] };
 }")
-      (rust-mode)
-      (rust-macro-scope (point-min) (point-max)))
-    '((38 40) (20 45)))))
+    (rust-mode)
+    (should (progn (goto-char 20) (not (rust-in-macro))))
+    (should (progn (goto-char 21) (eq (rust-in-macro) 20)))
+    (should (progn (goto-char 38) (eq (rust-in-macro) 20)))
+    (should (progn (goto-char 39) (eq (rust-in-macro) 38)))
+    (should (progn (goto-char 40) (eq (rust-in-macro) 20)))
+    (should (progn (goto-char 44) (eq (rust-in-macro) 20)))
+    (should (progn (goto-char 45) (not (rust-in-macro))))))
 
 (ert-deftest rust-test-in-macro-not-with-space ()
-  (should
-   (equal
-    (with-temp-buffer
-      (insert
+  (with-temp-buffer
+    (insert
        "fn foo<T>() {
     if !(mem::size_of::<T>() > 8) {
         bar()
     }
 }")
-      (rust-mode)
-      (rust-macro-scope (point-min) (point-max)))
-    'empty)))
-
+    (rust-mode)
+    (should (progn (goto-char 24) (not (rust-in-macro))))))
 
 (ert-deftest rust-test-paren-matching-type-with-module-name ()
   (rust-test-matching-parens