Subject: Re: General delimited literals in ruby-mode patch

From: Dmitry Gutov <[email-masked]>
Date: Sun, 29 Apr 2012 00:20:24 +0400

On 25.04.2012 22:00, Stefan Monnier wrote:
>>> So could it be refined so as to check for a "%" char?  I.e. instead of
>>> removing the old (not (eq ?/ c)), replace it with (not (memq c '(?/ ?%)))?
>> No, 'c' here is the char at the end of the previous line (see comment above
>> "let"), and the closing delimiter can be any punctuation char.
>
> OK, then I don't understand what the code used to do.  That's OK, I have
> plenty of better things to do anyway ;-)

AFAIU, it was just an optimization. '/' was the only binary operator to 
have double syntactic meaning. If c is not it, we definitely have 
continued expression, and can skip checking parse status.

>>> x = toto / foo if /do
>>> bar/ =~ "dobar" # Shortened version.
>>> still has the exact same problem.
>> Yes.  But let's face it, seeing this code in the wild is not very likely.
>
> You might be right.
>
>> And it's as easy to change as // ->  %r//.
>
> I'm not sure what change you're referring to: how should I change the
> above code to make it unambiguous that the first / is a division rather
> than a regexp?

You would move the whole regexp to the next line, or (if the constraint 
I suggested is in place) replace the line break with "\n":

x == toto / foo if /do\nbar/ =~ "dobar"

Anyway, the second patch does away this this approach.

>>> Or is it also linked to the presence of a preceding space?  That might
>>> not be so bad, then.  E.g. " / " is division but "/ ", " /", and "/"
>>> is regexp.
>> The lack of preceding space can turn a regexp into division, but not the
>> other way around.
>
> So you're saying that " / ", "/", and "/ " are for division, whereas " /"
> is for regexps?

Yes, though not without exceptions. "if / boo /" and "if/boo/" are both 
considered to contain regexps. As well as "/ boo /" when not after a word.
The algorithm is likely more complex, I'm just describing how it works 
with specific examples.

>> That's one more reason to do this just like TextMate, though.
>
> You mean because the problem can be fixed on the user's side without
> changing the Ruby code?  Yes, that's a clear advantage, especially when
> browsing other people's code.

If TextMate bundles are easily editable by user, I don't know about that 
(they might be). I just meant that having the same level of syntax 
support across editors is a good thing. So that if you have resort to 
%r//, it's the same for the other team members. And vice versa.

I'm speaking mostly hypothetically, but the percent literals patch is in 
fact indirect result of my colleague using this syntax in TextMate, with 
unclosed paren inside.

>> I'm not exactly sure why, but percent literal starting within a comment is
>> already (not) propertized as expected.
>
> Not in all cases:
>
>     x #= "tot %q/to"; =
>      /toto;
>
> now add&remove a space on the second line; then do the same on the
> first line.
> The approach I suggested is based on years of experience ;-)

I wasn't arguing against the approach, just making an observation. :)
But it's always good to have a counter-example.

I did something close to what you described (patch 0001), but it didn't 
seem proper to call the same function in two different situations 
(before search and after search when we already have match data), so 
there's still two of them.
>>From bebf7f22ef746e1d20a5cdd4312684e02f0222f9 Mon Sep 17 00:00:00 2001
From: Dmitry Gutov <[email-masked]>
Date: Sat, 28 Apr 2012 19:12:13 +0400
Subject: [PATCH 1/2] * lisp/progmodes/ruby-mode.el: Don't propertize percent
 literals in strings or comments (bug#6286)

Change all names and references to "general delimited literals" to "percent
literals", seems to be a shorter and more popular term.

ruby-percent-literal-beg-re: New constant.
(ruby-propertize-containing-percent-literal): Check the type of literal more
carefully. Only propertize it but not other syntax above the point.
(ruby-syntax-propertize-percent-literal): Only propertize when not inside basic
string or comment. When the literal is unclosed, leave the text after it
unpropertized.
---
 lisp/progmodes/ruby-mode.el |   84 ++++++++++++++++++++++++-------------------
 test/indent/ruby.rb         |    7 ++++
 2 files changed, 54 insertions(+), 37 deletions(-)

diff --git a/lisp/progmodes/ruby-mode.el b/lisp/progmodes/ruby-mode.el
index 5d79437..432680f 100644
--- a/lisp/progmodes/ruby-mode.el
+++ b/lisp/progmodes/ruby-mode.el
@@ -794,7 +794,7 @@ and `\\' when preceded by `?'."
 ;;                           (not (or (eolp) (looking-at "#")
 ;;                                    (and (eq (car (nth 1 state)) ?{)
 ;;                                         (looking-at "|"))))))
-                       ;; Not a regexp or general delimited literal.
+                       ;; Not a regexp or percent literal.
                        (null (nth 0 (ruby-parse-region (or begin parse-start)
                                                        (point))))
                        (or (not (eq ?| (char-after (point))))
@@ -1111,17 +1111,21 @@ See `add-log-current-defun-function'."
             mlist)))))
 
 (declare-function ruby-syntax-propertize-heredoc "ruby-mode" (limit))
-(declare-function ruby-syntax-general-delimiters-goto-beg "ruby-mode" ())
-(declare-function ruby-syntax-propertize-general-delimiters "ruby-mode" (limit))
+(declare-function ruby-propertize-containing-percent-literal "ruby-mode" (limit))
+(declare-function ruby-syntax-propertize-percent-literal "ruby-mode" (limit))
 
 (if (eval-when-compile (fboundp #'syntax-propertize-rules))
     ;; New code that works independently from font-lock.
     (progn
+      (defconst ruby-percent-literal-beg-re
+        "\\(%\\)[qQrswWx]?\\([[:punct:]]\\)"
+        "Regexp to match the beginning of percent literal.")
+
       (defun ruby-syntax-propertize-function (start end)
         "Syntactic keywords for Ruby mode.  See `syntax-propertize-function'."
         (goto-char start)
         (ruby-syntax-propertize-heredoc end)
-        (ruby-syntax-general-delimiters-goto-beg)
+        (ruby-propertize-containing-percent-literal end)
         (funcall
          (syntax-propertize-rules
           ;; #{ }, #$hoge, #@foo are not comments.
@@ -1161,8 +1165,8 @@ See `add-log-current-defun-function'."
           ((concat ruby-here-doc-beg-re ".*\\(\n\\)")
            (7 (prog1 "\"" (ruby-syntax-propertize-heredoc end))))
           ;; Handle percent literals: %w(), %q{}, etc.
-          ("\\(?:^\\|[[ \t\n<+(,=]\\)\\(%\\)[qQrswWx]?\\([[:punct:]]\\)"
-           (1 (prog1 "|" (ruby-syntax-propertize-general-delimiters end)))))
+          ((concat "\\(?:^\\|[[ \t\n<+(,=]\\)" ruby-percent-literal-beg-re)
+           (1 (prog1 "|" (ruby-syntax-propertize-percent-literal end)))))
          (point) end))
 
       (defun ruby-syntax-propertize-heredoc (limit)
@@ -1189,40 +1193,46 @@ See `add-log-current-defun-function'."
               ;; inf-loop.
               (if (< (point) start) (goto-char start))))))
 
-      (defun ruby-syntax-general-delimiters-goto-beg ()
-        (let ((state (syntax-ppss)))
-          ;; Move to the start of the literal, in case it's multiline.
-          ;; TODO: determine the literal type more reliably here?
+      (defun ruby-propertize-containing-percent-literal (limit)
+        (let ((state (syntax-ppss))
+              (start (point)))
+          ;; When already inside percent literal, re-propertize it.
           (when (eq t (nth 3 state))
             (goto-char (nth 8 state))
-            (beginning-of-line))))
+            (when (looking-at ruby-percent-literal-beg-re)
+              (ruby-syntax-propertize-percent-literal limit))
+            (when (< (point) start) (goto-char start)))))
 
-      (defun ruby-syntax-propertize-general-delimiters (limit)
+      (defun ruby-syntax-propertize-percent-literal (limit)
         (goto-char (match-beginning 2))
-        (let* ((op (char-after))
-               (ops (char-to-string op))
-               (cl (or (cdr (aref (syntax-table) op))
-                       (cdr (assoc op '((?< . ?>))))))
-               parse-sexp-lookup-properties)
-          (ignore-errors
-            (if cl
-                (progn  ; Paired delimiters.
-                  ;; Delimiter pairs of the same kind can be nested
-                  ;; inside the literal, as long as they are balanced.
-                  ;; Create syntax table that ignores other characters.
-                  (with-syntax-table (make-char-table 'syntax-table nil)
-                    (modify-syntax-entry op (concat "(" (char-to-string cl)))
-                    (modify-syntax-entry cl (concat ")" ops))
-                    (modify-syntax-entry ?\\ "\\")
-                    (save-restriction
-                      (narrow-to-region (point) limit)
-                      (forward-list))))  ; skip to the paired character
-              ;; Single character delimiter.
-              (re-search-forward (concat "[^\\]\\(?:\\\\\\\\\\)*"
-                                         (regexp-quote ops)) limit nil))
-            ;; If we reached here, the closing delimiter was found.
-            (put-text-property (1- (point)) (point)
-                               'syntax-table (string-to-syntax "|")))))
+        ;; Not inside a simple string or comment.
+        (when (eq t (nth 3 (syntax-ppss)))
+          (let* ((op (char-after))
+                 (ops (char-to-string op))
+                 (cl (or (cdr (aref (syntax-table) op))
+                         (cdr (assoc op '((?< . ?>))))))
+                 parse-sexp-lookup-properties)
+            (condition-case nil
+                (progn
+                  (if cl ; Paired delimiters.
+                      ;; Delimiter pairs of the same kind can be nested
+                      ;; inside the literal, as long as they are balanced.
+                      ;; Create syntax table that ignores other characters.
+                      (with-syntax-table (make-char-table 'syntax-table nil)
+                        (modify-syntax-entry op (concat "(" (char-to-string cl)))
+                        (modify-syntax-entry cl (concat ")" ops))
+                        (modify-syntax-entry ?\\ "\\")
+                        (save-restriction
+                          (narrow-to-region (point) limit)
+                          (forward-list))) ; skip to the paired character
+                    ;; Single character delimiter.
+                    (re-search-forward (concat "[^\\]\\(?:\\\\\\\\\\)*"
+                                               (regexp-quote ops)) limit nil))
+                  ;; Found the closing delimiter.
+                  (put-text-property (1- (point)) (point) 'syntax-table
+                                     (string-to-syntax "|")))
+              ;; Unclosed literal, leave the following text unpropertized.
+              ((scan-error search-failed) (goto-char limit))))))
       )
 
   ;; For Emacsen where syntax-propertize-rules is not (yet) available,
@@ -1267,7 +1277,7 @@ This should only be called after matching against `ruby-here-doc-end-re'."
      (4 (7 . ?/))
      (6 (7 . ?/)))
     ("^=en\\(d\\)\\_>" 1 "!")
-    ;; General delimited string.
+    ;; Percent literal.
     ("\\(^\\|[[ \t\n<+(,=]\\)\\(%[xrqQwW]?\\([^<[{(a-zA-Z0-9 \n]\\)[^\n\\\\]*\\(\\\\.[^\n\\\\]*\\)*\\(\\3\\)\\)"
      (3 "\"")
      (5 "\""))
diff --git a/test/indent/ruby.rb b/test/indent/ruby.rb
index c4a747a..7a9d123 100644
--- a/test/indent/ruby.rb
+++ b/test/indent/ruby.rb
@@ -7,6 +7,13 @@ c = %w(foo
  baz)
 d = %!hello!
 
+# Don't propertize percent literals inside strings.
+"(%s, %s)" % [123, 456]
+
+# Nor inside comments.
+x = # "tot %q/to"; =
+y = 2 / 3
+
 # A "do" after a slash means that slash is not a division, but it doesn't imply
 # it's a regexp-ender, since it can be a regexp-starter instead!
 x = toto / foo; if /do bar/ then
-- 
1.7.10.msysgit.1

>>From ac047bcebe08a8f850e3a3400156316088fd9520 Mon Sep 17 00:00:00 2001
From: Dmitry Gutov <[email-masked]>
Date: Sat, 28 Apr 2012 22:11:00 +0400
Subject: [PATCH 2/2] * lisp/progmodes/ruby-mode.el: Go back to method
 whitelisting for regexps.
 ruby-syntax-methods-before-regexp: New variable.
 (ruby-syntax-propertize-function): Use it to recognize
 regexps. Don't look at the text after regexp, just use
 the old approach.

* test/indent/ruby.rb: Update examples, add a new one.
---
 lisp/progmodes/ruby-mode.el |   38 ++++++++++++++++++++------------------
 test/indent/ruby.rb         |   25 +++++++++++++------------
 2 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/lisp/progmodes/ruby-mode.el b/lisp/progmodes/ruby-mode.el
index 432680f..14ce846 100644
--- a/lisp/progmodes/ruby-mode.el
+++ b/lisp/progmodes/ruby-mode.el
@@ -226,6 +226,13 @@ Also ignores spaces after parenthesis when 'space."
   "Use `ruby-encoding-map' to set encoding magic comment if this is non-nil."
   :type 'boolean :group 'ruby)
 
+(defcustom ruby-syntax-methods-before-regexp
+  '("gsub" "gsub!" "sub" "sub!" "scan" "split" "split!" "index" "match"
+    "assert_match" "Given" "Then" "When")
+  "Methods that can take regexp as the first argument.
+It will be properly highlighted even when the call omits parens."
+  :group 'ruby)
+
 ;; Safe file variables
 (put 'ruby-indent-tabs-mode 'safe-local-variable 'booleanp)
 (put 'ruby-indent-level 'safe-local-variable 'integerp)
@@ -1137,28 +1144,23 @@ See `add-log-current-defun-function'."
                         ;; Not within a string.
                         (nth 3 (syntax-ppss (match-beginning 0))))
                 (string-to-syntax "\\"))))
-          ;; Regexps: regexps are distinguished from division either because
-          ;; of the keyword/symbol before them, or because of the code
-          ;; following them.
+          ;; Regexps: regexps are distinguished from division because
+          ;; of the keyword, symbol, or method name before them.
           ((concat
             ;; Special tokens that can't be followed by a division operator.
-            "\\(?:\\(^\\|[[=(,~?:;<>]\\|\\(?:^\\|\\s \\)"
+            "\\(^\\|[[=(,~?:;<>]"
+            ;; Control flow keywords and operators following bol or whitespace.
+            "\\|\\(?:^\\|\\s \\)"
             (regexp-opt '("if" "elsif" "unless" "while" "until" "when" "and"
-                          "or" "&&" "||"
-                          "gsub" "gsub!" "sub" "sub!" "scan" "split" "split!"))
-            "\\)\\s *\\)?"
+                          "or" "not" "&&" "||"))
+            ;; Method name from the list.
+            "\\|\\_<"
+            (regexp-opt ruby-syntax-methods-before-regexp)
+            "\\)\\s *"
             ;; The regular expression itself.
-            "\\(/\\)[^/\n\\\\]*\\(?:\\\\.[^/\n\\\\]*\\)*\\(/\\)"
-            ;; Special code that cannot follow a division operator.
-            ;; FIXME: Just because the second slash of "/foo/ do bar" can't
-            ;; be a division, doesn't mean it can't *start* a regexp, as in
-            ;; "x = toto/foo; if /do bar/".
-            "\\([imxo]*\\s *\\(?:,\\|\\_<do\\_>\\)\\)?")
-           (2 (when (or (match-beginning 1) (match-beginning 4))
-                (string-to-syntax "\"/")))
-           (3 (if (or (match-beginning 1) (match-beginning 4))
-                  (string-to-syntax "\"/")
-                (goto-char (match-end 2)))))
+            "\\(/\\)[^/\n\\\\]*\\(?:\\\\.[^/\n\\\\]*\\)*\\(/\\)")
+           (2 (string-to-syntax "\"/"))
+           (3 (string-to-syntax "\"/")))
           ("^=en\\(d\\)\\_>" (1 "!"))
           ("^\\(=\\)begin\\_>" (1 "!"))
           ;; Handle here documents.
diff --git a/test/indent/ruby.rb b/test/indent/ruby.rb
index 7a9d123..4f2e9e6 100644
--- a/test/indent/ruby.rb
+++ b/test/indent/ruby.rb
@@ -1,24 +1,25 @@
-# Don't mis-match "sub" at the end of words.
-a = asub / aslb + bsub / bslb;
-
+# Percent literals.
 b = %Q{This is a "string"}
-c = %w(foo
+c = %w!foo
  bar
- baz)
-d = %!hello!
+ baz!
+d = %(hello (nested) world)
 
 # Don't propertize percent literals inside strings.
 "(%s, %s)" % [123, 456]
 
-# Nor inside comments.
+# Or inside comments.
 x = # "tot %q/to"; =
 y = 2 / 3
 
-# A "do" after a slash means that slash is not a division, but it doesn't imply
-# it's a regexp-ender, since it can be a regexp-starter instead!
-x = toto / foo; if /do bar/ then
-                  toto = 1
-                end
+# Regexp after whitelisted method.
+"abc".sub /b/, 'd'
+
+# Don't mis-match "sub" at the end of words.
+a = asub / aslb + bsub / bslb;
+
+# Highlight the regexp after "if".
+x = toto / foo if /do bar/ =~ "dobar"
 
 # Some Cucumber code:
 Given /toto/ do
-- 
1.7.10.msysgit.1