bug#25904: Formatting bug with js-mode

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#25904: Formatting bug with js-mode

Michael Snead
Hi there. Apologies if this is the wrong place to submit a bug on js-mode for emacs. I am an emacs newbie (in fact, I've only used spacemacs and still relatively new there too) and not sure where to post about this.

Basically I am trying to find the right place to report this issue, which was determined to be an upstream issue with js-mode:


In essence, a fat arrow expression followed by a linefeed, followed by an expression (such as a JSX tag with children) that is not wrapped in parenthesis is valid syntax and recognized by editors like WebStorm and VSCode but doesn't work correctly in this editor.

If this is not the right place I would be grateful for directions on where this bug belongs.

Thanks!
Reply | Threaded
Open this post in threaded view
|

bug#25904: Formatting bug with js-mode

Felipe Ochoa-2
Here's an initial implementation of a solution for this
issue. (Also filed as js2#314 [1])

foo.bar.baz(very =>
  very  // current code aligns return value to paren above
).biz(baz =>
  baz
);

I've been using this code successfully for a number of days now,
but since it's my first time touching indentation code, it likely
needs review for I comments and whitespace handling, etc.

(Note: disabling `js-indent-align-list-continuation' also
addresses this issue, but disabling it changes indentation in
other parts.)

[1]: https://github.com/mooz/js2-mode/issues/314

---
 lisp/progmodes/js.el | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index 1f86909..18f888d 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -1848,7 +1848,9 @@ This performs fontification according to `js--class-styles'."
              (skip-chars-backward " \t")
              (or (bobp) (backward-char))
              (and (> (point) (point-min))
-                  (save-excursion (backward-char) (not (looking-at "[/*]/")))
+                  ;; Need to exclude => here since js--looking-at-operator-p thinks
+                  ;; it's looking at an assignment operator
+                  (save-excursion (backward-char) (not (looking-at "[/*]/\\|=>")))
                   (js--looking-at-operator-p)
                   (and (progn (backward-char)
                               (not (looking-at "+\\+\\|--\\|/[/*]"))))))))))
@@ -2107,7 +2109,18 @@ indentation is aligned to that column."
                  (continued-expr-p (js--continued-expression-p)))
              (goto-char (nth 1 parse-status)) ; go to the opening char
              (if (or (not js-indent-align-list-continuation)
-                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)"))
+                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)")
+                     ;; check for a arrow function without parens
+                     (and (looking-at "(\\s-*\\(async\\s-*\\)?")
+                          ;; TODO: should call (forward-comment most-positive-fixnum)?
+                          (save-excursion
+                            (goto-char (match-end 0))
+                            (cond
+                             ((eq (char-after) ?\()
+                              (forward-sexp)
+                              (looking-at-p "\\s-*=>\\s-*\\(/[/*]\\|$\\)"))
+                             (t (looking-at-p
+                                 (concat js--name-re "\\s-*=>\\s-*\\(/[/*]\\|$\\)")))))))
                  (progn ; nothing following the opening paren/bracket
                    (skip-syntax-backward " ")
                    (when (eq (char-before) ?\)) (backward-list))
--
2.7.4



Reply | Threaded
Open this post in threaded view
|

bug#25904: Formatting bug with js-mode

Dmitry Gutov
Hi Felipe!

On 11/8/17 1:55 AM, Felipe Ochoa wrote:

> Here's an initial implementation of a solution for this issue. (Also
> filed as js2#314 [1])
>
> foo.bar.baz(very =>  very  // current code aligns return value to paren
> above ).biz(baz =>  baz
> );
>
> I've been using this code successfully for a number of days now, but
> since it's my first time touching indentation code, it likely needs
> review for I comments and whitespace handling, etc.

I think the code is reasonable, but the patch needs a few examples for
emacs/test/manual/js*.js. Maybe add a new file, or maybe reuse an
existing one.

It also wouldn't hurt that the existing examples are unchanged with the
new code.

>               (skip-chars-backward " \t")
>               (or (bobp) (backward-char))
>               (and (> (point) (point-min))
> -                  (save-excursion (backward-char) (not (looking-at
> "[/*]/")))
> +                  ;; Need to exclude => here since
> js--looking-at-operator-p thinks
> +                  ;; it's looking at an assignment operator
> +                  (save-excursion (backward-char) (not (looking-at
> "[/*]/\\|=>")))

OK.

> @@ -2107,7 +2109,18 @@ indentation is aligned to that column."
>                   (continued-expr-p (js--continued-expression-p)))
>               (goto-char (nth 1 parse-status)) ; go to the opening char
>               (if (or (not js-indent-align-list-continuation)
> -                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)"))
> +                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)")
> +                     ;; check for a arrow function without parens
> +                     (and (looking-at "(\\s-*\\(async\\s-*\\)?")
> +                          ;; TODO: should call (forward-comment
> most-positive-fixnum)?

To allow comments between the opening paren and the arglist? Does
anybody write them there?

> +                          (save-excursion
> +                            (goto-char (match-end 0))
> +                            (cond
> +                             ((eq (char-after) ?\()
> +                              (forward-sexp)
> +                              (looking-at-p
> "\\s-*=>\\s-*\\(/[/*]\\|$\\)"))
> +                             (t (looking-at-p
> +                                 (concat js--name-re
> "\\s-*=>\\s-*\\(/[/*]\\|$\\)")))))))

I imagine this (*) could be transformed into a single regexp, though it
would be pretty complex (rx could help, though!).

(*) Looking at an opening paren followed by ([optional] lambda arglist
and an arrow) then [optional] comment and newline.

Arglist matcher might be on the big side, but I'm guessing the
performance will be better. Not sure how big of an issue this is.

If it's not one regexp, moving some of the new code into a helper
function (with a sensible name) seems prudent.



Reply | Threaded
Open this post in threaded view
|

bug#25904: Formatting bug with js-mode

Felipe Ochoa-2
Hi Dmitry!

> I think the code is reasonable, but the patch needs a few
> examples for emacs/test/manual/js*.js. Maybe add a new file, or
> maybe reuse an existing one.

I've added a test to js.js in the latest patch below.
 
> It also wouldn't hurt that the existing examples are unchanged
> with the new code.

I checked all the existing examples manually and all were
unchanged when applying indent-region on the entire buffer.

> To allow comments between the opening paren and the arglist?
> Does anybody write them there?

Oops -- this comment was supposed to be after the arrow. I was
thinking of return type annotation comments, but I just checked
flow and I don't think they support this. We can just remove the
comment

> I imagine this (*) could be transformed into a single regexp,
> though it would be pretty complex (rx could help, though!).
>
> (*) Looking at an opening paren followed by ([optional] lambda
> arglist and an arrow) then [optional] comment and newline.

Is there an arglist regexp already in use somewhere? I'd rather
not roll my own since dealing with default argument values and
spreads and such seems quite challenging.

> If it's not one regexp, moving some of the new code into a
> helper function (with a sensible name) seems prudent.

I've done this in the latest patch.

---
 lisp/progmodes/js.el     | 26 ++++++++++++++++++++++++--
 test/manual/indent/js.js |  9 +++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index 1f86909..6e057b0 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -1848,7 +1848,9 @@ This performs fontification according to `js--class-styles'."
              (skip-chars-backward " \t")
              (or (bobp) (backward-char))
              (and (> (point) (point-min))
-                  (save-excursion (backward-char) (not (looking-at "[/*]/")))
+                  ;; Need to exclude => here since js--looking-at-operator-p thinks
+                  ;; it's looking at an assignment operator
+                  (save-excursion (backward-char) (not (looking-at "[/*]/\\|=>")))
                   (js--looking-at-operator-p)
                   (and (progn (backward-char)
                               (not (looking-at "+\\+\\|--\\|/[/*]"))))))))))
@@ -2077,6 +2079,21 @@ indentation is aligned to that column."
         (when comma-p
           (goto-char (1+ declaration-keyword-end))))))))

+(defun js--looking-at-broken-arrow-function-p ()
+  "Helper function for `js--proper-indentation'.
+Return t if point is at the start of a (possibly async) arrow
+function and the last non-comment, non-whitespace token of the
+current like is the \"=>\" token."
+  (and (looking-at "\\s-*\\(async\\s-*\\)?")
+       (save-excursion
+         (goto-char (match-end 0))
+         (cond
+          ((eq (char-after) ?\()
+           (forward-list) ; Is this better than forward-sexp?
+           (looking-at-p "\\s-*=>\\s-*\\(/[/*]\\|$\\)"))
+          (t (looking-at-p
+              (concat js--name-re "\\s-*=>\\s-*\\(/[/*]\\|$\\)")))))))
+
 (defun js--proper-indentation (parse-status)
   "Return the proper indentation for the current line."
   (save-excursion
@@ -2107,7 +2124,12 @@ indentation is aligned to that column."
                  (continued-expr-p (js--continued-expression-p)))
              (goto-char (nth 1 parse-status)) ; go to the opening char
              (if (or (not js-indent-align-list-continuation)
-                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)"))
+                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)")
+                     (when (eq (char-after) ?\()
+                       (save-excursion
+                         (forward-char)
+                         (skip-syntax-forward " ")
+                         (js--looking-at-broken-arrow-function-p))))
                  (progn ; nothing following the opening paren/bracket
                    (skip-syntax-backward " ")
                    (when (eq (char-before) ?\)) (backward-list))
diff --git a/test/manual/indent/js.js b/test/manual/indent/js.js
index b0d8bca..2286cc1 100644
--- a/test/manual/indent/js.js
+++ b/test/manual/indent/js.js
@@ -144,6 +144,15 @@ bar(
   /abc/
 )

+// #25904
+foo.bar.baz(very =>
+  very
+).biz(([baz={a: [123]}, boz]) =>
+  baz
+).snarf((snorf) =>
+  snorf
+);
+
 // Local Variables:
 // indent-tabs-mode: nil
 // js-indent-level: 2
--
2.7.4



Reply | Threaded
Open this post in threaded view
|

bug#25904: Formatting bug with js-mode

Dmitry Gutov
On 11/21/17 12:22 AM, Felipe Ochoa wrote:

> I've added a test to js.js in the latest patch below.

Thanks.

>> To allow comments between the opening paren and the arglist? Does
>> anybody write them there?
>
> Oops -- this comment was supposed to be after the arrow. I was thinking
> of return type annotation comments, but I just checked flow and I don't
> think they support this. We can just remove the comment

Sure.

> Is there an arglist regexp already in use somewhere? I'd rather not roll
> my own since dealing with default argument values and spreads and such
> seems quite challenging.

No, sorry, I forgot about destructuring and etc. forward-list is fine here.

>> If it's not one regexp, moving some of the new code into a helper
>> function (with a sensible name) seems prudent.
>
> I've done this in the latest patch.

Thanks.

> +(defun js--looking-at-broken-arrow-function-p ()
> +  "Helper function for `js--proper-indentation'.
> +Return t if point is at the start of a (possibly async) arrow
> +function and the last non-comment, non-whitespace token of the
> +current like is the \"=>\" token."
> +  (and (looking-at "\\s-*\\(async\\s-*\\)?")

This looks weird: the regexp will always match. Better to write it as

(if (looking-at NON-OPT-REGEXP) (goto-char ...)), where NON-OPT-REGEXP
does not include the optional qualifier (?).

And the (save-excursion...) form seems unnecessary, since the caller
already uses it.

> +       (save-excursion
> +         (goto-char (match-end 0))
> +         (cond
> +          ((eq (char-after) ?\()
> +           (forward-list) ; Is this better than forward-sexp?

I think so: it should be a bit faster, and it doesn't require you to
bind forward-sexp-function to nil (for e.g. js2-mode).

> +           (looking-at-p "\\s-*=>\\s-*\\(/[/*]\\|$\\)"))
> +          (t (looking-at-p
> +              (concat js--name-re "\\s-*=>\\s-*\\(/[/*]\\|$\\)")))))))

Should we extract "\\s-*=>\\s-*\\(/[/*]\\|$\\)" to a local variable, or
a constant?

> (defun js--proper-indentation (parse-status)
>    "Return the proper indentation for the current line."
>    (save-excursion
> @@ -2107,7 +2124,12 @@ indentation is aligned to that column."
>                   (continued-expr-p (js--continued-expression-p)))
>               (goto-char (nth 1 parse-status)) ; go to the opening char
>               (if (or (not js-indent-align-list-continuation)
> -                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)"))
> +                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)")
> +                     (when (eq (char-after) ?\()
> +                       (save-excursion
> +                         (forward-char)
> +                         (skip-syntax-forward " ")

This seems a bit extraneous: the regexps in the called function all
start with "\\s-*", right? Let's maybe have the one or the other.

> +                         (js--looking-at-broken-arrow-function-p))))
>                   (progn ; nothing following the opening paren/bracket
>                     (skip-syntax-backward " ")
>                     (when (eq (char-before) ?\)) (backward-list))
> diff --git a/test/manual/indent/js.js b/test/manual/indent/js.js
> index b0d8bca..2286cc1 100644
> --- a/test/manual/indent/js.js
> +++ b/test/manual/indent/js.js
> @@ -144,6 +144,15 @@ bar(
>    /abc/
> )
>
> +// #25904

We write references to debbugs as bug#25904. bug-reference-mode can
linkify the result.

> +foo.bar.baz(very =>
> +  very
> +).biz(([baz={a: [123]}, boz]) =>
> +  baz
> +).snarf((snorf) =>
> +  snorf
> +);
> +

Thank you.

Please attach the resulting patch as a file produced by 'git
format-patch', with a ChangeLog style message entry, as described in
CONTRIBUTE.



Reply | Threaded
Open this post in threaded view
|

bug#25904: Formatting bug with js-mode

Felipe Ochoa
This has taken a *little* longer than I'd hoped, but here's a new
patch with all the discussed changes. Hopefully I got the commit
message formatting and content right

From 0c7eed3fd9f7f77a071296c191e8569471bac4e5 Mon Sep 17 00:00:00 2001
From: Felipe Ochoa <[hidden email]>
Date: Tue, 7 Nov 2017 16:50:42 -0500
Subject: [PATCH] Fixes js indentation after arrow function without parens

From: felipe <[hidden email]>

* lisp/progmodes/js.el (js--proper-indentation)
(js--looking-at-broken-arrow-function-p)
* test/manual/indent/js.js: Indent expression following an arrow as
though it were enclosed in parentheses

(Bug#25904)
---
 lisp/progmodes/js.el     | 24 ++++++++++++++++++++++--
 test/manual/indent/js.js |  9 +++++++++
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index f30e591..5a333a9 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -1849,7 +1849,9 @@ js--continued-expression-p
              (skip-chars-backward " \t")
              (or (bobp) (backward-char))
              (and (> (point) (point-min))
-                  (save-excursion (backward-char) (not (looking-at "[/*]/")))
+                  ;; Need to exclude => here since
js--looking-at-operator-p thinks
+                  ;; it's looking at an assignment operator
+                  (save-excursion (backward-char) (not (looking-at
"[/*]/\\|=>")))
                   (js--looking-at-operator-p)
                   (and (progn (backward-char)
                               (not (looking-at "+\\+\\|--\\|/[/*]"))))))))))
@@ -2078,6 +2080,23 @@ js--maybe-goto-declaration-keyword-end
         (when comma-p
           (goto-char (1+ declaration-keyword-end))))))))

+(defconst js--line-terminating-arrow "\\s-*=>\\s-*\\(/[/*]\\|$\\)"
+  "Regexp to match a \"=>\" token if it's the last non-whitespace,
non-comment token in a line.")
+
+(defun js--looking-at-broken-arrow-function-p ()
+  "Helper function for `js--proper-indentation'.
+Return t if point is at the start of a (possibly async) arrow
+function and the last non-comment, non-whitespace token of the
+current like is the \"=>\" token."
+  (when (looking-at "\\s-*async\\s-*")
+    (goto-char (match-end 0)))
+  (cond
+   ((eq (char-after) ?\()
+    (forward-list)
+    (looking-at-p js--line-terminating-arrow))
+   (t (looking-at-p
+       (concat js--name-re js--line-terminating-arrow)))))
+
 (defun js--proper-indentation (parse-status)
   "Return the proper indentation for the current line."
   (save-excursion
@@ -2108,7 +2127,8 @@ js--proper-indentation
                  (continued-expr-p (js--continued-expression-p)))
              (goto-char (nth 1 parse-status)) ; go to the opening char
              (if (or (not js-indent-align-list-continuation)
-                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)"))
+                     (looking-at "[({[]\\s-*\\(/[/*]\\|$\\)")
+                     (progn (forward-char)
(js--looking-at-broken-arrow-function-p)))
                  (progn ; nothing following the opening paren/bracket
                    (skip-syntax-backward " ")
                    (when (eq (char-before) ?\)) (backward-list))
diff --git a/test/manual/indent/js.js b/test/manual/indent/js.js
index b0d8bca..fc39c12 100644
--- a/test/manual/indent/js.js
+++ b/test/manual/indent/js.js
@@ -144,6 +144,15 @@ bar(
   /abc/
 )

+// bug#25904
+foo.bar.baz(very => // A comment
+  very
+).biz(([baz={a: [123]}, boz]) =>
+  baz
+).snarf((snorf) => /* Another comment */
+  snorf
+);
+
 // Local Variables:
 // indent-tabs-mode: nil
 // js-indent-level: 2
--
2.7.4



Reply | Threaded
Open this post in threaded view
|

bug#25904: bug#24896: JSX prop indentation after fat arrow

Jimmy Yuen Ho Wong
In reply to this post by Michael Snead
If this patch works, can we make it into Emacs 26 before the release?

Thanks a lot for working on this. This has been the most annoying bug in
js-mode.





Reply | Threaded
Open this post in threaded view
|

bug#25904: bug#24896: JSX prop indentation after fat arrow

Eli Zaretskii
> From: Jimmy Yuen Ho Wong <[hidden email]>
> Date: Sun, 29 Apr 2018 02:20:39 +0100
> Cc: Felipe Ochoa <[hidden email]>,
> Dmitry Gutov <[hidden email]>
>
> If this patch works, can we make it into Emacs 26 before the release?

No, it's too late for that.  Sorry.



Reply | Threaded
Open this post in threaded view
|

bug#25904: Formatting bug with js-mode

Dmitry Gutov
In reply to this post by Felipe Ochoa
On 3/16/18 3:06 AM, Felipe Ochoa wrote:
> This has taken a *little* longer than I'd hoped, but here's a new
> patch with all the discussed changes. Hopefully I got the commit
> message formatting and content right

Not really. First of all, I asked for an attached file ('git am' needs a
file), and not its contents inside the message body. Which suffered
word-wrap damage during transmission (because that's what email clients do).

I would commit it this time anyway, though, if not for the next point.

>  From 0c7eed3fd9f7f77a071296c191e8569471bac4e5 Mon Sep 17 00:00:00 2001
> From: Felipe Ochoa <[hidden email]>
> Date: Tue, 7 Nov 2017 16:50:42 -0500
> Subject: [PATCH] Fixes js indentation after arrow function without parens
>
> From: felipe <[hidden email]>
>
> * lisp/progmodes/js.el (js--proper-indentation)
> (js--looking-at-broken-arrow-function-p)

I think there was going to be some description here.

And there's no mention of changes to js--continued-expression-p and
js--proper-indentation. They would have the meat of the explanation.

> * test/manual/indent/js.js: Indent expression following an arrow as
> though it were enclosed in parentheses

This needs a period, at least.

> (Bug#25904)

We usually put the bug number inside one of the sentences of the change
log entry, not just at the end of it.

The patch does work, but if you could make the details right, it would
be great. We've missed the emacs-26 train, so there's no real hurry.



Reply | Threaded
Open this post in threaded view
|

bug#25904: Patches

Jackson Ray Hamilton-2
In reply to this post by Michael Snead

Hello Dmitry and Felipe,

I’ve taken a stab at formatting Felipe’s fix for arrow function indentation according to the coding standards.  Compared to his patch, I made some trivial name/doc changes, and changed

(progn (forward-char) (js--looking-at-broken-arrow-function-p))

to

(save-excursion (forward-char) (js--looking-at-broken-arrow-function-p))

since we probably want to undo the forward-char in case the form returns nil.

In order to get all the JS indentation tests to pass with make, I also had to make a variable safe file-locally.

Both changes are attached.  They could be applied to the emacs-26 branch.  (Or master, but emacs-26 would be preferable so we can deliver it sooner.  I tested on both branches.)

Jackson


0001-Indent-arrows-expression-bodies-like-function-bodies.patch (5K) Download Attachment
0001-js-indent-align-list-continuation-Make-variable-safe.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#25904: Patches

Dmitry Gutov
Version: 27.1

On 11.02.2019 01:03, Jackson Ray Hamilton wrote:

> Both changes are attached.  They could be applied to the emacs-26
> branch.  (Or master, but emacs-26 would be preferable so we can deliver
> it sooner.  I tested on both branches.)

Pushed to master, thank you.

Regarding emacs-26, please feel free to lobby it with Eli. I think it's
a fine patch, but I'm not sure it's important enough regression-wise.



Reply | Threaded
Open this post in threaded view
|

bug#25904: Patches

Jackson Ray Hamilton-2
Fair enough, thanks for the merge.

> On February 12, 2019 at 4:27 PM Dmitry Gutov <[hidden email]> wrote:
>
>
> Version: 27.1
>
> On 11.02.2019 01:03, Jackson Ray Hamilton wrote:
>
> > Both changes are attached.  They could be applied to the emacs-26
> > branch.  (Or master, but emacs-26 would be preferable so we can deliver
> > it sooner.  I tested on both branches.)
>
> Pushed to master, thank you.
>
> Regarding emacs-26, please feel free to lobby it with Eli. I think it's
> a fine patch, but I'm not sure it's important enough regression-wise.
Reply | Threaded
Open this post in threaded view
|

bug#25904: Patches

Eli Zaretskii
In reply to this post by Dmitry Gutov
> From: Dmitry Gutov <[hidden email]>
> Date: Wed, 13 Feb 2019 03:27:26 +0300
>
> Regarding emacs-26, please feel free to lobby it with Eli. I think it's
> a fine patch, but I'm not sure it's important enough regression-wise.

The second one, the one which makes the variable safe in some cases,
can be cherry-picked to emacs-26.  I believe it's independent of the
main change, quite safe, and is a Good Thing anyway.

Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#25904: Patches

Dmitry Gutov
On 13.02.2019 18:15, Eli Zaretskii wrote:
> The second one, the one which makes the variable safe in some cases,
> can be cherry-picked to emacs-26.  I believe it's independent of the
> main change, quite safe, and is a Good Thing anyway.

OK, done.

Not what the question was about, though.