bug#15582: js-mode indent bug

bug#15582: js-mode indent bug

[Jakub Jankiewicz]

[-- Attachment #1: Type: text/plain, Size: 850 bytes --]

Hi,

I found this bug in js-mode indent:

when you have multiline if statement with comment at the end, next line
indent is doubled.

If I have js-indent-level equal 8 the code look like this:

            if (x > left && x < left + image.width &&
                y > top && y < top + image.height) {
                    do_somethig();
            }

but when I put comment after if it look like this:

            if (x > left && x < left + image.width &&
                y > top && y < top + image.height) {  // found
                            do_somethig();


when js-indent-level is 2 it have 4 spaces. The same is with while

          while (10 &&
                 20) { // asd
              xx();

four spaces instead of two

version: GNU Emacs 24.2.1


--
Jakub Jankiewicz, Web Developer
http://jcubic.pl

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

bug#15582: notes on js-mode indentation bug

[Tom Tromey]

I looked into this bug.

With the comment, js--continued-expression-p returns t, causing
js--proper-indentation to take this branch of a cond:

(continued-expr-p
 (+ (current-column) (* 2 js-indent-level)
    js-expr-indent-offset))

Without the comment, this doesn't happen, so I think the problem here is
that js--continued-expression-p returns t.  Digging into this a bit
more, the issue is that js--re-search-backward moves point to the wrong
newline, because the newline at the end of the "//" comment is rejected.

This patch works by introducing a new js--find-newline-backward that
tries to do the right thing for line comments.

I've included a new test case (as a patch to a file first appearing in
another pending patch), but considering that there aren't any other
indentation tests for js-mode, I think some review is necessary.

It would be helpful to know when the continued-expr-p branch really is
supposed to trigger.  Or, if there is an informal corpus of indentation
tests, it would be nice to put them all into js-tests.el.

Tom

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index 1484b79..0551f2a 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -1771,6 +1771,24 @@ js--looking-at-operator-p
                  ;; return NaN anyway.  Shouldn't be a problem.
                  (memq (char-before) '(?, ?} ?{))))))))
 
+(defun js--find-newline-backward ()
+  "Move backward to the nearest newline that is not in a block comment."
+  (let ((continue t)
+        (result t))
+    (while continue
+      (setq continue nil)
+      (if (re-search-backward "\n" nil t)
+          (let ((parse (syntax-ppss)))
+            ;; We match the end of a // comment but not a newline in a
+            ;; block comment.
+            (when (nth 4 parse)
+              (goto-char (nth 8 parse))
+              ;; If we saw a block comment, keep trying.
+              (unless (nth 7 parse)
+                (setq continue t))))
+        (setq result nil)))
+    result))
+
 (defun js--continued-expression-p ()
   "Return non-nil if the current line continues an expression."
   (save-excursion
@@ -1780,7 +1798,7 @@ js--continued-expression-p
             (progn
               (forward-comment (- (point)))
               (not (memq (char-before) '(?, ?\[ ?\()))))
-      (and (js--re-search-backward "\n" nil t)
+      (and (js--find-newline-backward)
            (progn
              (skip-chars-backward " \t")
              (or (bobp) (backward-char))
diff --git a/test/lisp/progmodes/js-tests.el b/test/lisp/progmodes/js-tests.el
index 9bf7258..de322f2 100644
--- a/test/lisp/progmodes/js-tests.el
+++ b/test/lisp/progmodes/js-tests.el
@@ -59,6 +59,16 @@
  * Load the inspector's shared head.js for use by tests that need to
  * open the something or other"))))
 
+(ert-deftest js-mode-indent-bug-15582 ()
+  (with-temp-buffer
+    (js-mode)
+    (setq-local js-indent-level 8)
+    (insert "if (x > 72 &&\n    y < 85) { // found\n\t")
+    (save-excursion (insert "do_something();\n"))
+    (js-indent-line)
+    (should (equal (buffer-substring (point-at-bol) (point-at-eol))
+                   "\tdo_something();"))))
+
 (provide 'js-tests)
 
 ;;; js-tests.el ends here

bug#15582: notes on js-mode indentation bug

[Dmitry Gutov]

On 10.01.2017 08:03, Tom Tromey wrote:

> It would be helpful to know when the continued-expr-p branch really is
> supposed to trigger.  Or, if there is an informal corpus of indentation
> tests, it would be nice to put them all into js-tests.el.

It triggers, for instance, in this example:

b +=
   c

But you have probably found it already.

> Tom
>
> diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
> index 1484b79..0551f2a 100644
> --- a/lisp/progmodes/js.el
> +++ b/lisp/progmodes/js.el
> @@ -1771,6 +1771,24 @@ js--looking-at-operator-p
>                   ;; return NaN anyway.  Shouldn't be a problem.
>                   (memq (char-before) '(?, ?} ?{))))))))
>
> +(defun js--find-newline-backward ()
> +  "Move backward to the nearest newline that is not in a block comment."

You might want to phrase it like "Move ... ignoring comments.", meaning 
treating the openers of non-block comments as newlines, and the contents 
of block comments as entirely invisible.

BTW, here are a couple of examples that the new fix doesn't handle:

b += /* found */
   c

b += /*
    */
   c

(I think they suggest that the block comment-related behavior of the new 
function might be tweaked a little).

Thanks.

bug#15582: notes on js-mode indentation bug

[Tom Tromey]

Dmitry> BTW, here are a couple of examples that the new fix doesn't handle:

Thanks.  I'll try to fix these.

Tom

bug#15582: done

[Tom Tromey]

This was fixed by b47f97218efb8d9966e084bdfd8a86e8c47cf81d.

Tom