[PATCH] Update js-mode function heading regexes

[PATCH] Update js-mode function heading regexes

[Adam Niederer]

Hello,

I've modified the js-mode function heading regular expressions to accept
some modern additions to JavaScript. This causes function names to be
highlighted in more cases, and makes js-beginning-of-defun and
js-end-of-defun more consistent. The changelog and behavioral changes
are as follows:

* js--function-heading-1-re - Accept "async function foo" as well as
"function foo". This should only affect highlighting.

* js--function-heading-2-re - Accept any character before the beginning
of a function in an object. This causes the function's name to be
correctly highlighted.

Pre-patch, beginning-of-defun (C-M-a) with the point in location 1
(below) will place point at the beginning of the function keyword,
whereas at location 2 it will place point at the beginning of "bur".

Post-patch, beginning-of-defun at location 1 will place point at the
beginning of "foo", and at location 2 it will place point at the
beginning of "bur"

{ foo: function() {<point location 1>}
  bur: function() {<point location 2>} }

* js--function-heading-3-re - Accept let, const, and async anonymous
functions

Pre-patch, beginning-of-defun (C-M-a) with the point in location 1
(below) will place point at the beginning of "x", whereas at locations
2, 3 and 4, it will place point at the beginning of the respective
function keywords.

Post-patch, beginning-of-defun (C-M-a) with the point in location 1
(below) will place point at the beginning of "x", whereas at location 2,
3 and 4, it will place point at the beginning of "y", "z" and "w",
respectively.

var x = function() {<point location 1>}
var y = async function() {<point location 2>}

let z = function() {<point location 3>}
let w = async function() {<point location 4>}

Below is the file I used to test these changes. make check is failing
for me, but it looks like the failing tests are unrelated (filenotify &
inotify).

This is my first contribution via a mailing list, so please let me know
if I'm missing anything :).

-Adam

{ foo: function() {}
  bar: function() {} }

[{ foo: function() {}
   bar: function() {} }]

let x = [{ foo: function() {}
           bar: function() {} }]

function foo() {}
async function bar() {}

var x = function() {}
var x = async function() {}

let x = function() {}
let x = async function() {}

const x = function() {}
const x = async function() {}

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index bae9e52bf0..126181f5bb 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -250,20 +250,20 @@ js--available-frameworks
 
 (defconst js--function-heading-1-re
   (concat
-   "^\\s-*function\\(?:\\s-\\|\\*\\)+\\(" js--name-re "\\)")
+   "^\\s-*\\(?:async\\s-+\\)?function\\(?:\\s-\\|\\*\\)+\\("
js--name-re "\\)")
   "Regexp matching the start of a JavaScript function header.
 Match group 1 is the name of the function.")
 
 (defconst js--function-heading-2-re
   (concat
-   "^\\s-*\\(" js--name-re "\\)\\s-*:\\s-*function\\_>")
+   "^.*?\\(" js--name-re "\\)\\s-*:\\s-*function\\_>")
   "Regexp matching the start of a function entry in an associative array.
 Match group 1 is the name of the function.")
 
 (defconst js--function-heading-3-re
   (concat
-   "^\\s-*\\(?:var\\s-+\\)?\\(" js--dotted-name-re "\\)"
-   "\\s-*=\\s-*function\\_>")
+   "^\\s-*\\(?:\\(?:var\\|let\\|const\\)\\s-+\\)?\\("
js--dotted-name-re "\\)"
+   "\\s-*=\\s-*\\(?:async\\s-+\\)?function\\_>")
   "Regexp matching a line in the JavaScript form \"var MUMBLE = function\".
 Match group 1 is MUMBLE.")
 

Re: [PATCH] Update js-mode function heading regexes

[Dmitry Gutov]

Hi!

On 6/17/17 10:30 AM, Adam Niederer wrote:
> Below is the file I used to test these changes. make check is failing
> for me, but it looks like the failing tests are unrelated (filenotify &
> inotify).

These are manual tests, right? 'make check' doesn't sound relevant.

But we have some automated tests for js.el too. Check out 
test/lisp/progmodes/js-tests.el. Maybe these scenarios would be better 
coded with ERT.

> This is my first contribution via a mailing list, so please let me know
> if I'm missing anything :).

Not much. Patch submissions via the bug tracker have higher odds of not 
being forgotten is they're not applied right away, though.

Thanks for the patch.

Re: [PATCH] Update js-mode function heading regexes

[Adam Niederer]

> These are manual tests, right? 'make check' doesn't sound relevant. 
> But we have some automated tests for js.el too. Check out
> test/lisp/progmodes/js-tests.el. Maybe these scenarios would be better
> coded with ERT. 
The file I included was primarily for visual evaluation of the changes.
Adding some automated tests is a good idea, though, so I've included an
updated patchset below. This also makes js--function-heading-2-re work
on async functions.
> Not much. Patch submissions via the bug tracker have higher odds of
> not being forgotten is they're not applied right away, though.
>
> Thanks for the patch.

Would reposting these changes to bug-gnu-emacs be a good idea?

Thanks a lot,

-Adam

--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -250,20 +250,20 @@ js--available-frameworks
 
 (defconst js--function-heading-1-re
   (concat
-   "^\\s-*function\\(?:\\s-\\|\\*\\)+\\(" js--name-re "\\)")
+   "^\\s-*\\(?:async\\s-+\\)?function\\(?:\\s-\\|\\*\\)+\\("
js--name-re "\\)")
   "Regexp matching the start of a JavaScript function header.
 Match group 1 is the name of the function.")
 
 (defconst js--function-heading-2-re
   (concat
-   "^\\s-*\\(" js--name-re "\\)\\s-*:\\s-*function\\_>")
+   "^.*?\\(" js--name-re "\\)\\s-*:\\s-*\\(?:async\\s-+\\)?function\\_>")
   "Regexp matching the start of a function entry in an associative array.
 Match group 1 is the name of the function.")
 
 (defconst js--function-heading-3-re
   (concat
-   "^\\s-*\\(?:var\\s-+\\)?\\(" js--dotted-name-re "\\)"
-   "\\s-*=\\s-*function\\_>")
+   "^\\s-*\\(?:\\(?:var\\|let\\|const\\)\\s-+\\)?\\("
js--dotted-name-re "\\)"
+   "\\s-*=\\s-*\\(?:async\\s-+\\)?function\\_>")
   "Regexp matching a line in the JavaScript form \"var MUMBLE = function\".
 Match group 1 is MUMBLE.")

diff --git a/test/lisp/progmodes/js-tests.el
b/test/lisp/progmodes/js-tests.el
index 8e1bac10cd..313e2e2e52 100644
--- a/test/lisp/progmodes/js-tests.el
+++ b/test/lisp/progmodes/js-tests.el
@@ -177,6 +177,44 @@
     ;; The bug was a hang.
     (should t)))
 
+(ert-deftest js-mode-highlight-function-names ()
+  "Ensure js--function-heading-1-re and js--function-heading-2-re match all
+possible function declarations and highlight the function names
accordingly."
+  (with-temp-buffer
+    (js-mode)
+    (insert "function foo() {}\n"
+            "async function bar() {}\n"
+            "let x = [{ baz: function() {}\n"
+            "           quux: function() {}\n"
+            "           kek: async function() {}\n"
+            "/**/       bur: async function() {} }]\n")
+    (font-lock-ensure)
+    (dolist (name '("foo" "bar" "baz" "quux" "kek" "bur"))
+      (save-excursion
+        (search-backward name)
+        (should (equal (face-at-point) font-lock-function-name-face))))))
+
+(ert-deftest js-mode-accept-function-bindings ()
+  "Ensure js--function-heading-3-re matches all function declarations
of the
+form (var|let|const) foo = (async)? function."
+  (with-temp-buffer
+    (js-mode)
+    (insert "var foo = async function() {}\n"
+            "const bar = async function() {}\n"
+            "let baz = async function() {}\n"
+            "var quux = function() {}\n"
+            "const kek = function() {}\n"
+            "let bur = function() {}\n")
+    (dolist (name '("foo" "bar" "baz" "quux" "kek" "bur"))
+      ;; re-search-backward will throw if one the strings doesn't match.
+      (re-search-backward js--function-heading-3-re))
+    ;; Ensure we have exactly six matches
+    (should-error (re-search-backward js--function-heading-3-re))))
+
 (provide 'js-tests)
 
 ;;; js-tests.el ends here

Re: [PATCH] Update js-mode function heading regexes

[Dmitry Gutov]

Hi Adam,

Sorry for the late reply.

On 6/19/17 10:04 AM, Adam Niederer wrote:

> The file I included was primarily for visual evaluation of the changes.
> Adding some automated tests is a good idea, though, so I've included an
> updated patchset below. This also makes js--function-heading-2-re work
> on async functions.

Sometimes it's easier to evaluate the improvement just by looking at the 
tests, though it depends on how they are written, of course.

To comment on the patch and your original post:

- The change to heading-3-re is not mentioned, but it's fairly obvious.

- I don't quite understand the description of the change in 
heading-2-re. Why not at least require a colon?

>> Not much. Patch submissions via the bug tracker have higher odds of
>> not being forgotten is they're not applied right away, though.
>>
>> Thanks for the patch.
> 
> Would reposting these changes to bug-gnu-emacs be a good idea?

By now it's looking even better than it did back then. As you can see, 
it's been lost in the mailing list.

And I'd like for someone else to look at the patch, because it touches 
the area of js-mode than I've never had to deal with.

> +(ert-deftest js-mode-highlight-function-names ()
> +  "Ensure js--function-heading-1-re and js--function-heading-2-re match all
> +possible function declarations and highlight the function names
> accordingly."

LGTM.

> +(ert-deftest js-mode-accept-function-bindings ()
> +  "Ensure js--function-heading-3-re matches all function declarations
> of the
> +form (var|let|const) foo = (async)? function."

Regarding this test, can we check something higher level than whether 
js--function-heading-3-re matches? It's an implementation detail, after all.