bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Let .emacs solely consist of:

   (setq prolog-indent-width 8
         prolog-electric-tab-flag nil
         prolog-electric-if-then-else-flag t
         prolog-paren-indent 4
         prolog-electric-dot-flag nil
         prolog-paren-indent-p t
         prolog-char-quote-workaround nil)

When I then do:

   $ emacs --no-splash -f prolog-mode

and press the following key sequence:

   t e s t SPC : - RET ( a SPC - > RET b RET ; c RET ) .

then I get the following indentation (shown in untabified form):


test :-
        (   a ->
                    b
         ;c
                  ).

This is highly non-idiomatic and very unexpected layout of Prolog code.

In contrast, in the current version (1.25) of the Prolog mode supplied
directly from its maintainer Stefan Bruda and available from:

   https://bruda.ca/_media/emacs/prolog.el

and using the exact same steps, I get the intended indentation:

test :-
        (   a ->
            b
        ;   c
        ).

Thank you for looking into this!

All the best,
Markus



In GNU Emacs 24.5.1 (x86_64-apple-darwin14.0.0, GTK+ Version 2.24.28)
 of 2015-09-20 on mt-mbpro
Windowing system distributor `The X.Org Foundation', version 11.0.11502000
Configured using:
 `configure --prefix=/opt/local --without-dbus --without-libotf
 --without-m17n-flt --without-gpm --without-gnutls --with-xml2 --infodir
 /opt/local/share/info/emacs --without-xaw3d --without-imagemagick
 --with-xpm --with-jpeg --with-tiff --with-gif --with-png --with-xft
 --with-x-toolkit=gtk2 --with-gconf --with-rsvg 'CFLAGS=-pipe -Os -arch
 x86_64' CPPFLAGS=-I/opt/local/include 'LDFLAGS=-L/opt/local/lib
 -Wl,-headerpad_max_install_names -lfreetype -lfontconfig -Wl,-no_pie
 -arch x86_64''

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

>    (setq prolog-indent-width 8
>          prolog-electric-tab-flag nil
>          prolog-electric-if-then-else-flag t
>          prolog-paren-indent 4
>          prolog-electric-dot-flag nil
>          prolog-paren-indent-p t
>          prolog-char-quote-workaround nil)

Thanks.  We indeed have some problems with the new indentation code, and
I hope you can help me figure out what it should do.

Could you give me some examples of what prolog-paren-indent-p
should do?  Currently it's simply unused :-(

>    t e s t SPC : - RET ( a SPC - > RET b RET ; c RET ) .

> then I get the following indentation (shown in untabified form):

> test :-
>         (   a ->
>                     b
>          ;c
>                   ).

I see a few different problems:
- The indentation of ) is wrong simply because nothing caused it to be
  reindented (hitting TAB or RET at the end brings the close paren to
  the right spot).  I guess "." should cause re-indentation.  Adding it
  to electric-indent-chars should do the trick.

- there are two desired indentations for "b".  IIUC You want

    (a ->
     b1,
     b2
    ;c1,
     c2)

  whereas the current code tries to accommodate

    (a ->
         b1,
         b2;
     c1,
     c2)

- the current code also tries to accommodate

    (    a
     ->
         b1,
         b2
     ;   c1,
         c2)

  so I'm not sure how to combine this with your use case.  E.g. would
  you prefer
   
    (    a
    ->
         b1,
         b2
    ;    c1,
         c2)

  or do you only want the ; at paren-level in the case where -> was not
  at the beginning of the line?

- After hitting "b RET" you get indented to a bogus column.  This is
  because SMIE thinks that

       predicate
           (arg2, arg2)

  is a possibility, so after "b RET" it thinks you might be about to
  enter a list of arguments to "b".  This is a general problem with
  SMIE's handling of empty lines (where it's often valid but unlikely),
  and even more so here in Prolog where such things aren't even valid.


The patch below fixes some of those problems.  After this patch, you
should hopefully see something more like

    (   a ->
            b
     ;  c
    ).

in your buffer.


        Stefan


diff --git a/lisp/progmodes/prolog.el b/lisp/progmodes/prolog.el
index b36df21..2f4c03e 100644
--- a/lisp/progmodes/prolog.el
+++ b/lisp/progmodes/prolog.el
@@ -1121,6 +1121,9 @@ Commands:
   (dolist (ar prolog-align-rules) (add-to-list 'align-rules-list ar))
   (add-hook 'post-self-insert-hook #'prolog-post-self-insert nil t)
   ;; `imenu' entry moved to the appropriate hook for consistency.
+  (when prolog-electric-dot-flag
+    (setq-local electric-indent-chars
+                (cons ?\. electric-indent-chars)))
 
   ;; Load SICStus debugger if suitable
   (if (and (eq prolog-system 'sicstus)
@@ -2078,6 +2081,7 @@ whitespace characters, parentheses, or then/else branches."
   (when prolog-electric-if-then-else-flag
     (save-excursion
       (let ((regexp (concat "(\\|" prolog-left-indent-regexp))
+            (pos (point))
             level)
         (beginning-of-line)
         (skip-chars-forward " \t")
@@ -2087,6 +2091,9 @@ whitespace characters, parentheses, or then/else branches."
         ;;             prolog-paren-indent))
 
         ;; work on all subsequent "->", "(", ";"
+        (and (looking-at regexp)
+             (= pos (match-end 0))
+             (indent-according-to-mode))
         (while (looking-at regexp)
           (goto-char (match-end 0))
           (setq level (+ (prolog-find-unmatched-paren) prolog-paren-indent))

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Thanks.  We indeed have some problems with the new indentation code, and
> I hope you can help me figure out what it should do.

Thank you for looking into this! I will help in every way I can to
achieve the excellent indentation of Stefan Bruda's original version.

> Could you give me some examples of what prolog-paren-indent-p
> should do?  Currently it's simply unused :-(

Basically, when I set prolog-paren-indent-p to t (and the other settings
I posted), then, after I have already typed a Prolog clause like:

   test :-
           HERE

(throughout the following, "HERE" is a placeholder that only denotes the
location of point, with point at the "H"), and then I press "(", then I
expect to get:

   test :-
           (   HERE

with point again being at the "H". In other words, the electric
parenthesis indents `prolog-paren-indent', which is typically 4. This
way, you spare users having to enter spaces manually to achieve the
typical indentation of the if-then-else construct.

>> test :-
>>         (   a ->
>>                     b
>>          ;c
>>                   ).
>
> I see a few different problems:
> - The indentation of ) is wrong simply because nothing caused it to be
>   reindented (hitting TAB or RET at the end brings the close paren to
>   the right spot).  I guess "." should cause re-indentation.  Adding it
>   to electric-indent-chars should do the trick.

In Stefan Bruda's original version, after I typed:

   test :-
           (   a ->
               b,
               cHERE

with point again at the "H", and then I press RET, I get:

   test :-
           (   a ->
               b,
               c
           HERE

That is, notice in particular that this new line is NOT indented the
same amount as the immediately preceding Prolog goals. There is a clear
reason for this: The immediately preceding Prolog goal, namely c, is
NOT followed by a comma. This means that where HERE appears, only two
things can realistically follow:

   1) ";", denoting a further disjunct
   2) ")", the closing parenthesis for the whole construct

For example, the preceding block can either end with:

   test :-
           (   a ->
               b,
               c
           ;   HERE

(notice the next indentation), or with:

   test :-
           (   a ->
               b,
               c
           )

after which the whole construct is finished. This is exactly the
behaviour users expect from a good indentation engine for Prolog, like
PceEmacs that ships with SWI-Prolog, and also Stefan Bruda's prolog.el.

To (unexpectedly) first indent the block and then only re-indent it when
"." is pressed would in my view be a regression compared to that.

>
> - there are two desired indentations for "b".  IIUC You want
>
>     (a ->
>      b1,
>      b2
>     ;c1,
>      c2)
>
>   whereas the current code tries to accommodate
>
>     (a ->
>          b1,
>          b2;
>      c1,
>      c2)

The two most common ways to indent Prolog's if-then-else are:

        (   a ->
            b,
            c
        ;   d,
            e
        ),

and:

        (   a
        ->  b,
            c
        ;   d,
            e
        )

Notice that only the placement of the arrow is different.

I am using the first style throughout all Prolog code I write. The
second version is also common in the Prolog community, and notably a lot
of library code of SWI-Prolog is written in that style. I made sure (via
patches that are integrated as of version 1.15) that both styles are
well supported in the prolog.el supplied directly by Stefan Bruda, even
in the same program and predicate, if you set electric parentheses and
the other settings I posted. Please try it for reference to see the
expected indentation of Prolog code written in either style.

Relatedly, notice that the indentation must also work for disjunctions:

        (   a,
            b
        ;   c,
            d
        )

i.e., in such cases, there is no arrow at all.


>
> - After hitting "b RET" you get indented to a bogus column.  This is
>   because SMIE thinks that
>
>        predicate
>            (arg2, arg2)
>
>   is a possibility, so after "b RET" it thinks you might be about to
>   enter a list of arguments to "b".  This is a general problem with
>   SMIE's handling of empty lines (where it's often valid but unlikely),
>   and even more so here in Prolog where such things aren't even valid.

Exactly, the main point here is that this would be invalid.

> The patch below fixes some of those problems.  After this patch, you
> should hopefully see something more like
>
>     (   a ->
>             b
>      ;  c
>     ).

I've never seen this indentation style in a decade of heavy Prolog use
including publishing, so my guess is that this will not help many users.

Many thanks for looking into this!

All the best,
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

>    test :-
>            (   HERE

> with point again being at the "H". In other words, the electric
> parenthesis indents `prolog-paren-indent', which is typically 4.

Good, thanks.  I think this does work correctly, currently, right?

> That is, notice in particular that this new line is NOT indented the
> same amount as the immediately preceding Prolog goals. There is a clear
> reason for this: The immediately preceding Prolog goal, namely c, is
> NOT followed by a comma. This means that where HERE appears, only two
> things can realistically follow:

Right, this is a current problem in SMIE, as mentioned: it doesn't know
about such "realistically".  It just knows that the grammar would allow
a comma at that point just as much as a semi-colon or lots of other
things, and it even thinks that there are yet further possibilities
(which actually aren't allowed by Prolog).

I'll have to think about how to best teach SMIE about this.

> To (unexpectedly) first indent the block and then only re-indent it when
> "." is pressed would in my view be a regression compared to that.

No doubt it's a regression.

>> 
>> - there are two desired indentations for "b".  IIUC You want
>> 
>> (a ->
>> b1,
>> b2
>> ;c1,
>> c2)
>> 
>> whereas the current code tries to accommodate
>> 
>> (a ->
>> b1,
>> b2;
>> c1,
>> c2)

> The two most common ways to indent Prolog's if-then-else are:
>
>         (   a ->
>             b,
>             c
>         ;   d,
>             e
>         ),
>
> and:
>
>         (   a
>         ->  b,
>             c
>         ;   d,
>             e
>         )

OK, the latter should be easy to support.  The former might require
a bit more works (or a config var).

>> (   a ->
>>         b
>> ;  c
>> ).

> I've never seen this indentation style in a decade

I'm not surprised.  This indentation is only the indirect result of
supporting

   (a ->
        b1,
        b2;
    c).

and since indentation normally only looks at the previous lines, the
indentation of "b1," can't depend on whether the following semi-colon is
at the end of a line, or at the beginning of a line.


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

I installed the patch below which should fix many of your problems.

IIUC the only remaining issue is the indentation we get when the line is
empty.


        Stefan


diff --git a/lisp/progmodes/prolog.el b/lisp/progmodes/prolog.el
index b36df21..24ac8d7 100644
--- a/lisp/progmodes/prolog.el
+++ b/lisp/progmodes/prolog.el
@@ -925,12 +925,30 @@ This is really kludgy, and unneeded (i.e. obsolete) in Emacs>=24."
     (`(:after . ".") '(column . 0)) ;; To work around smie-closer-alist.
     ;; Allow indentation of if-then-else as:
     ;;    (   test
-    ;;     -> thenrule
-    ;;     ;  elserule
+    ;;    ->  thenrule
+    ;;    ;   elserule
     ;;    )
     (`(:before . ,(or `"->" `";"))
-     (and (smie-rule-bolp) (smie-rule-parent-p "(") (smie-rule-parent 1)))
-    (`(:after . ,(or `":-" `"->" `"-->")) prolog-indent-width)))
+     (and (smie-rule-bolp) (smie-rule-parent-p "(") (smie-rule-parent 0)))
+    (`(:after . ,(or `"->" `"*->"))
+     ;; We distinguish
+     ;;
+     ;;     (a ->
+     ;;          b;
+     ;;      c)
+     ;; and
+     ;;     (    a ->
+     ;;          b
+     ;;     ;    c)
+     ;;
+     ;; based on the space between the open paren and the "a".
+     (unless (and (smie-rule-parent-p "(")
+                  (save-excursion
+                    (smie-indent-forward-token)
+                    (smie-backward-sexp 'halfsexp)
+                    (not (eq ?\( (char-before)))))
+       prolog-indent-width))
+    (`(:after . ,(or `":-" `"-->")) prolog-indent-width)))
 
 \f
 ;;-------------------------------------------------------------------
@@ -1121,6 +1139,9 @@ Commands:
   (dolist (ar prolog-align-rules) (add-to-list 'align-rules-list ar))
   (add-hook 'post-self-insert-hook #'prolog-post-self-insert nil t)
   ;; `imenu' entry moved to the appropriate hook for consistency.
+  (when prolog-electric-dot-flag
+    (setq-local electric-indent-chars
+                (cons ?\. electric-indent-chars)))
 
   ;; Load SICStus debugger if suitable
   (if (and (eq prolog-system 'sicstus)
@@ -2078,6 +2099,7 @@ whitespace characters, parentheses, or then/else branches."
   (when prolog-electric-if-then-else-flag
     (save-excursion
       (let ((regexp (concat "(\\|" prolog-left-indent-regexp))
+            (pos (point))
             level)
         (beginning-of-line)
         (skip-chars-forward " \t")
@@ -2087,6 +2109,9 @@ whitespace characters, parentheses, or then/else branches."
         ;;             prolog-paren-indent))
 
         ;; work on all subsequent "->", "(", ";"
+        (and (looking-at regexp)
+             (= pos (match-end 0))
+             (indent-according-to-mode))
         (while (looking-at regexp)
           (goto-char (match-end 0))
           (setq level (+ (prolog-find-unmatched-paren) prolog-paren-indent))
diff --git a/test/indent/prolog.prolog b/test/indent/prolog.prolog
index 5b5d272..ca4d2c9 100644
--- a/test/indent/prolog.prolog
+++ b/test/indent/prolog.prolog
@@ -1,5 +1,27 @@
 %% -*- mode: prolog; coding: utf-8; fill-column: 78 -*-
 
+%% bug#21526
+test1 :-
+    (   a ->
+        b
+    ;   c
+    ).
+
+test2 :-
+    (    a
+    ->   b1,
+         b2
+    ;    c1,
+         c2
+    )
+
+test3 :-
+    (   a,
+        b
+    ;   c
+    ).
+
+
 %% Testing correct tokenizing.
 foo(X) :- 0'= = X.
 foo(X) :- 8'234 = X.
@@ -50,9 +72,9 @@ subst(X, V, FV, lambda(Y, Ti, Bi), lambda(Y1, To, Bo)) :-
      %% Perform substitution on the body.
      subst(X, V, FV, Bi1, Bo)),
     (	X = Y
-     %% If X is equal to Y, X is shadowed, so no subst can take place.
-     ->	Y1 = Y, Bo = Bi
-     ;	(member((Y, _), FV)
+    %% If X is equal to Y, X is shadowed, so no subst can take place.
+    ->	Y1 = Y, Bo = Bi
+    ;	(member((Y, _), FV)
 	 %% If Y appears in FV, it can appear in V, so we need to
 	 %% rename it to avoid name capture.
 	 -> new_atom(Y, Y1),

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I installed the patch below which should fix many of your problems.

Thank you! There are several remaining issues. First, if-then-else
constructs can be nested. For example, when I have:


   test :-
           (   a,
               HERE

and press "(", then, in the prolog.el supplied by Stefan Bruda and using
the settings I posted, I get:


   test :-
           (   a,
               (   HERE

That is, it automatically indents for placing the next "if" as expected.

Eventually, a nested mixed disjunction/if-then-else may either look like:

   test :-
           (   a,
               (   b ->
                   (   c ->
                       d
                   ;   e
                   )
               ;   f
               )
           ;   g
           ).

or:

   test :-
           (   a,
               (   b
               ->  (   c
                   ->  d
                   ;   e
                   )
               ;   f
               )
           ;   g
           ).

When I try it with your patch, it fails to indent subsequent lines.

> IIUC the only remaining issue is the indentation we get when the line is
> empty.

This issue also remains. Thank you!

All the best,
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

> When I try it with your patch, it fails to indent subsequent lines.

Ugh!  That should be fixed now (see patch below).

>> IIUC the only remaining issue is the indentation we get when the line is
>> empty.
> This issue also remains. Thank you!

Right.  This one requires thinking (but will benefit other modes as well).


        Stefan



diff --git a/lisp/progmodes/prolog.el b/lisp/progmodes/prolog.el
index 24ac8d7..8c02e54 100644
--- a/lisp/progmodes/prolog.el
+++ b/lisp/progmodes/prolog.el
@@ -2081,7 +2081,7 @@ Argument BOUND is a buffer position limiting searching."
 (defun prolog-find-unmatched-paren ()
   "Return the column of the last unmatched left parenthesis."
   (save-excursion
-    (goto-char (or (car (nth 9 (syntax-ppss))) (point-min)))
+    (goto-char (or (nth 1 (syntax-ppss)) (point-min)))
     (current-column)))
 
 

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> When I try it with your patch, it fails to indent subsequent lines.
>
> Ugh!  That should be fixed now (see patch below).

Great, thank you!

> Right.  This one requires thinking (but will benefit other modes as
> well).

This will make the new mode much more useable, so thank you for looking
into this! Currently, not only is the new empty line indented in a
distracting and unexpected way, but closing the bock does not indent it
as expected either. For example, when I press RET at:

   test :-
           (   a
           ;   bHERE

then I get:

   test :-
           (   a
           ;   b
                       HERE

and when I then press ")" followed by "." to end the block, I get:

   test :-
           (   a
           ;   b
                       ).

Previously you suggested to make "." electric, this would fix this,
however, I expect the correct indentation already when pressing ")":

   test :-
           (   a
           ;   b
           )

because the block need not end with "." but can look for example like:

   test :-
           (   a
           ;   b
           ),
           c.

I hope this helps.

All the best,
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

>>> IIUC the only remaining issue is the indentation we get when the line is
>>> empty.
>> This issue also remains. Thank you!
> Right.  This one requires thinking (but will benefit other modes as well).

I believe I found a solution for this one.  Please test the new code in
master.


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> I believe I found a solution for this one.  Please test the new code in
> master.

It's amazing, thank you! I found two remaining issues:

(1) Parentheses matching does not work yet. When I have a fact like:

   test(a, b).HERE

(with point again at the H) and press C-M-b (backward-sexp), I expect to
see, as with prolog.el directly supplied by Stefan Bruda:

   testHERE(a, b).

Instead, I currently unexpectedly get:

   HERE
   test(a, b).

or, depending on the empty lines before this fact, even things like:

   HERE


   test(a, b).

This also applies in cases like:

   test :-
           (   a
           ;   b
           ).HERE

where, when pressing C-M-b, I expect to see:

   test :-
           HERE(   a
           ;   b
           ).

instead of (as currently) placing point before the whole clause.


(2) When I set `prolog-electric-dot-flag' to t, and enter, at
(point-min) of an empty Prolog mode buffer from "emacs -Q -nw":

   RET RET RET t e s t .

then I see that the first line of the buffer is briefly highlighted,
apparently in connection with parsing. This may be connected to the
previous issue, and I hope that fixing (1) also fixes this.


All the best!
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Here are further test cases I found when trying this patch:

(1) After evaluating (show-paren-mode 1) and using the Prolog content:

   t1 :- a.
   t2 :- b.HERE

The dot in the immediately preceding line is highlighted as the
(mistakenly classified as such) "matching" element.

(2) Please enable syntax highlighting for DCGs with zero arguments.

For example, the following is colored as expected:

   test(X, Y) -->
           a,
           b.

In contrast, in the following DCG, "test" is unexpectedly not colored:

   test -->
           a,
           b.

(3) Please extend the patch to cover multiple conditions.

For example, when I press RET in the following partial construct:


   test :-
           (   a ->
               b
           ;   c ->HERE

I unexpectedly get:

   test :-
           (   a ->
               b
           ;   c ->
                       HERE

Whereas the expected result is:

   test :-
           (   a ->
               b
           ;   c ->
               HERE

Thank you and all the best,
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Here are some further cases I found when trying the latest version:

(a) The indentation of (\+)/1 (not provable) is not correct and yields
an error. For example, when I press RET when point is at the "H":

   test :-
           \+ a,HERE

then I get:

   test :-
   \+ a,
   HERE

and the message:

   Error: (error "Bumped into unknown token")

The expected result is:

   test :-
           \+ a,
           HERE

(and no error). For example, the clause may eventually look like:

   test :-
           \+ a,
           b,
           \+ \+ c,
           d.

In connection with if-then-else, we expect the layout:

   test :-
           (   \+ a ->
               b
           ;   \+ c,
               \+ d
           ).

but currently get:

   test :-
           (   \+ a ->
               b
           ;   \+ c,
   \+ d
           ).


(b) Comments within if-then-else constructs are also not correctly
indented. For example, a block like:

   test :-
           (   a ->
               % what should we do
               % with two lines?
               b

is still indented exactly as expected. However, when I then add two
subsequent comment lines also in the else branch, I get:

   test :-
           (   a ->
               % what should we do
               % on two lines?
               b
           ;   % what else should
           % we do on two lines
           c
           ).

Whereas we expect (and Stefan Bruda's version also yields):

   test :-
           (   a ->
               % what should we do
               % on two lines?
               b
           ;   % what else should
               % we do on two lines
               c
           ).

The remaining two issues are not related to if-then-else, but I still
include them in this report for easy reference. I will of course file
separate issues for them if you prefer that.

(c) Arguments of directives should be more indented. For example, a
directive whose arguments span several lines should look like this:

   :- multifile
           pred1,
           pred2,
           pred3.

Instead, when I press RET when point is at the H:

   :- multifileHERE

I unexpectedly get:


   :- multifile
      HERE

`prolog-keywords' contains the directives where a hanging indentation of
prolog-indent-width in subsequent lines would be very welcome.


(d) Code patterns like the following are quite common:

   test_predicate(a) :- !.
   test_predicate(b) :- !.
   test_predicate(c) :- !,
           test_goal1,
           test_goal2.
   test_predicate(d) :- !,
           test_goal3.

For a block like that, I get with the current git version:

   test_predicate(a) :- !.
   test_predicate(b) :- !.
   test_predicate(c) :- !,
                        test_goal1,
                        test_goal2.
   test_predicate(d) :- !,
                        test_goal3.

Whereas with Stefan Bruda's original version, I get the first layout.


I have used the following two source files, together amounting to more
than 8,000 lines of Prolog code written using Stefan Bruda's version
with the settings I posted, to find such regressions:

One is a constraint solver over Boolean variables:

   https://github.com/SWI-Prolog/swipl-devel/blob/master/library/clp/clpb.pl

And the other is a constraint solver over integers:

   https://github.com/SWI-Prolog/swipl-devel/blob/master/library/clp/clpfd.pl   
I hope that these files are useful as further test cases for you.

All the best!
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Here is a further test case I found in connection with if-then-else,
using your latest patch and the settings I posted after emacs -Q:

   test :-
           (a
           ;   b
           ).

When I place point on the "a" and press TAB, the expected result is:

   test :-
           (   a
           ;   b
           ).

However, in contrast to the prolog.el by Stefan Bruda, I get again:

   test :-
           (a
           ;   b
           ).

Thank you and all the best!
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

> When I place point on the "a" and press TAB, the expected result is:
>
>    test :-
>            (   a
>            ;   b
>            ).
>
> However, in contrast to the prolog.el by Stefan Bruda, I get again:
>
>    test :-
>            (a
>            ;   b
>            ).

That's expected: the TAB behavior prolog.el provides only changes the
spacing at the beginning of the line, not the spacing within elements of
the line.


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

>    Error: (error "Bumped into unknown token")

Should be fixed now.

>            ;   % what else should
>                % we do on two lines
>                c

OK, I have a patch which does that, but note that M-; on the first line
above will move that % to comment-column (40) and at that point, we
probably don't want to align the "c" with that.  My patch works around
this problem by using as heuristic that we only align if the offset is
within prolog-indent-width.

> (c) Arguments of directives should be more indented.  For example, a
> directive whose arguments span several lines should look like this:
>
>    :- multifile
>            pred1,
>            pred2,
>            pred3.

I have no idea what "directives" are nor what the above "multifile"
means nor what is its syntax.  Does Bruda's code handle that?

> `prolog-keywords' contains the directives where a hanging indentation of
> prolog-indent-width in subsequent lines would be very welcome.

That's vague: what means "subsequent lines"?
Anything until a "."?  What about a ";" or a ":-" or a "-->"?

>    test_predicate(c) :- !,
>            test_goal1,
>            test_goal2.

Should work now.


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> (c) Arguments of directives should be more indented.  For example, a
>> directive whose arguments span several lines should look like this:
>>
>>    :- multifile
>>            pred1,
>>            pred2,
>>            pred3.
>
> I have no idea what "directives" are nor what the above "multifile"
> means nor what is its syntax.  Does Bruda's code handle that?

Yes. All issues I reported so far are deviations from Stefan Bruda's
original version. His version also highlights DCG heads with zero
arguments correctly.

A directive starts with :- or ?- in a source file.

>
>> `prolog-keywords' contains the directives where a hanging indentation of
>> prolog-indent-width in subsequent lines would be very welcome.
>
> That's vague: what means "subsequent lines"?
> Anything until a "."?  What about a ";" or a ":-" or a "-->"?

(:-)/1 can be read just like a Prolog rule, only without head. So, after
:-, I expect the same indentation like in a rule. A directive can
include any regular Prolog goal.

>>    test_predicate(c) :- !,
>>            test_goal1,
>>            test_goal2.
>
> Should work now.

Awesome, thank you!

Regarding comments, one more thing I noticed is that in a multiline
comment like /* ... */, we actually expect a small indentation like:

/*
  Hello, indented world!
*/

as in the default C mode, but currently get:

/*
Not indented.
*/

In Stefan Bruda's version, we get the former layout.

All the best,
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

> Yes.  All issues I reported so far are deviations from Stefan Bruda's
> original version.  His version also highlights DCG heads with zero
> arguments correctly.

Hmm... I wonder how we lost this.  Has it always highlighted those, or
is it something newish (e.g. newer than say Emacs-23.3)?

>>> (c) Arguments of directives should be more indented.  For example, a
>>> directive whose arguments span several lines should look like this:
>>> 
>>> :- multifile
>>>        pred1,
>>>        pred2,
>>>        pred3.
>> 
>> I have no idea what "directives" are nor what the above "multifile"
>> means nor what is its syntax.  Does Bruda's code handle that?

> Yes. All issues I reported so far are deviations from Stefan Bruda's
> original version. His version also highlights DCG heads with zero
> arguments correctly.

> A directive starts with :- or ?- in a source file.

> (:-)/1 can be read just like a Prolog rule, only without head. So, after
> :-, I expect the same indentation like in a rule. A directive can
> include any regular Prolog goal.

I don't understand.  You say you want

    :- multifile
           pred1,
           pred2,
           pred3.

and you say that it is indented like in a normal rule, but in normal rule,
I expect neither

    foo(X):- multifile
                 pred1,
                 pred2,
                 pred3.
nor
    foo(X) :- multifile
        pred1,
        pred2,
        pred3.

AFAIK the problem is not with the ":- without a left-hand side", but
with how to parse the

       multifile
           pred1,
           pred2,
           pred3

Currently it's parsed as:

       (multifile
           pred1),
           pred2,
           pred3

whereas clearly you'd like

       multifile
           (pred1,
           pred2,
           pred3)

hence my questions: What means "subsequent lines"?
Anything until a "."?  What about a ";" or a ":-" or a "-->"?
IOW what should happen with something like

     :- multifile
           pred1;
           pred2 -->
           pred3.

Furthermore, it seems that this is linked to the (:-)/1 operator, so
what should happen if "multifile" appears elsewhere?  what other keyword
can appear where you have "multifile" and do they all use this
same syntax?

> Regarding comments, one more thing I noticed is that in a multiline
> comment like /* ... */, we actually expect a small indentation like:

> as in the default C mode, but currently get:
>
> /*
> Not indented.
> */

Indeed, we have some problem here (it seems TAB doesn't even do
anything).  Patch welcome.


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I don't understand.  You say you want
>
>     :- multifile
>            pred1,
>            pred2,
>            pred3.

Yes, exactly. The most frequent such directives are:

    discontiguous dynamic meta_predicate module_transparent multifile 

Their unary argument is always a valid Prolog term itself, and the whole
directive is, like everything that appears in a Prolog file (except
comments), also a valid Prolog term, and ends with a '.'.

As a good approximation, you can expect these directives to have the
form:

   :- <keyword> pred1,
           pred2,
           ...,
           pred_n.

Where pred_k is a predicate indicator of the form Name/Arity, where
Name is a Prolog atom and Arity is an integer.

The atom may of course also contain a '.', for example, the directive
may look like:

   :- multifile 'unusual.predicate'/1.

This situation is very rare, but at least it should not produce errors.

> whereas clearly you'd like
>
>        multifile
>            (pred1,
>            pred2,
>            pred3)

Yes, this is because of the built-in precedences: multifile has
precedence 1150, and thus (,)/2 with arity 1000 binds more tightly.

> what should happen if "multifile" appears elsewhere?  what other keyword
> can appear where you have "multifile" and do they all use this
> same syntax?

The term above is just like any other Prolog term, so if multifile
appears elsewhere then, from a purely semantic point of view, I would
expect it to also indent just according to the structure of the
resulting Prolog term. However, not even Stefan Bruda's mode goes so far
as to ensure this kind of semantic indentation. For example, from a
purely semantic view, in the following example, the atom 'x' is an
argument of the operator multifile:

   test :-
           multifile
                   x.

However, as a first approximation, it would already be great to handle
this indentation of multifile just in directives (= lines where (:-)/1
is the primary functor). This is also what Stefan Bruda's mode does.

The same goes for the other keywords shown above.

The ability to define custom operators makes Prolog code harder to parse
and indent semantically than other languages. If you are interested in
truly pushing this to its ultimate conclusion, you will have to
dynamically take operator precedences into account, using Prolog's
current_op/3 predicate to look up the current precedences.

Independently of all this, would you please consider to set the default
value of prolog-electric-if-then-else-flag to t in Emacs? I find this
behaviour extremely useful, and other modes like C also have electric
indentation turned on by default in recent Emacs versions.

Thank you and all the best!
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

> Yes, this is because of the built-in precedences: multifile has
> precedence 1150, and thus (,)/2 with arity 1000 binds more tightly.

Ah, there it is!
Is there a precedence table somewhere that includes "multifile"
and friends?  The one I have doesn't include it.


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Is there a precedence table somewhere that includes "multifile"
> and friends?  The one I have doesn't include it.

You can generate such a table directly from Prolog, using for example:

   ?- current_op(P, T, N), portray_clause(P-T-N), false.

This yields the precedence (smaller = tighter), types and names of all
operators that are available by default. Users may change these
precedences, but it is unusual and bad practice to change the precedence
of default operators. So I would use this as a good approximation.

Using SWI-Prolog, you obtain for example:

   250-yfx- (?).
   1-fx- ($).
   990-xfx- (:=).
   200-fy- (@).
   1150-fx- (volatile).
   700-xfx- (=@=).
   200-xfy- (^).
   700-xfx- (@>).
   100-yfx- ('.').
   1150-fx- (meta_predicate).
   400-yfx- (rdiv).
   500-yfx- (\/).
   1150-fx- (initialization).
   600-xfy- (:).
   1200-xfx- (-->).
   700-xfx- (=..).
   400-yfx- (*).
   700-xfx- (@>=).
   1100-xfy- (;).
   700-xfx- (\=@=).
   700-xfx- (>:<).
   1150-fx- (dynamic).
   700-xfx- (@<).
   700-xfx- (:<).
   700-xfx- (@=<).
   400-yfx- (xor).
   1150-fx- (discontiguous).
   700-xfx- (>=).
   400-yfx- (//).
   700-xfx- (>).
   400-yfx- (div).
   1150-fx- (module_transparent).
   1200-fx- (?-).
   500-yfx- (/\).
   400-yfx- (/).
   200-fy- (+).
   500-yfx- (+).
   1150-fx- (multifile).
   400-yfx- (<<).
   1150-fx- (public).
   1200-fx- (:-).
   1200-xfx- (:-).
   400-yfx- (rem).
   1000-xfy- (',').
   1150-fx- (thread_initialization).
   700-xfx- (\=).
   700-xfx- (=).
   1050-xfy- (*->).
   700-xfx- (=<).
   700-xfx- (<).
   1050-xfy- (->).
   700-xfx- (as).
   1150-fx- (thread_local).
   400-yfx- (>>).
   200-fy- (-).
   500-yfx- (-).
   700-xfx- (is).
   900-fy- (\+).
   200-fy- (\).
   700-xfx- (=:=).
   400-yfx- (mod).
   200-xfx- (**).
   700-xfx- (\==).
   700-xfx- (==).
   1105-xfy- ('|').
   700-xfx- (=\=).

In SWI-Prolog, try the query:

   ?- help(op/3).

to see what these type indicators (xfx, fx etc.) mean.

I hope this helps you to indent according to operator precedence.

All the best,
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

> You can generate such a table directly from Prolog, using for example:
>    ?- current_op(P, T, N), portray_clause(P-T-N), false.

Thanks.  Could you update prolog-smie-grammar accordingly?


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Thanks.  Could you update prolog-smie-grammar accordingly?

Here is a patch:

2015-10-01  Markus Triska  <triska@metalevel.at>

	* prolog.el: Add common directives to operator table.

	(prolog-smie-grammar): Add discontiguous, dynamic etc.

diff --git a/lisp/progmodes/prolog.el b/lisp/progmodes/prolog.el
index ff2769e..a305b03 100644
--- a/lisp/progmodes/prolog.el
+++ b/lisp/progmodes/prolog.el
@@ -913,6 +913,13 @@ prolog-smie-grammar
     (">>" -400 -400)
     ("**" -200 -200)
     ("^" -200 -200)
+    ;; directives declaring predicate properties
+    ("discontiguous" -1150 -1150)
+    ("dynamic" -1150 -1150)
+    ("meta_predicate" -1150 -1150)
+    ("module_transparent" -1150 -1150)
+    ("multifile" -1150 -1150)
+    ("public" -1150 -1150)
     ;; Prefix
     ;; ("+" 200 200)
     ;; ("-" 200 200)

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

> +    ;; directives declaring predicate properties

Please capitalize and punctuate comments.

> +    ("discontiguous" -1150 -1150)
> +    ("dynamic" -1150 -1150)
> +    ("meta_predicate" -1150 -1150)
> +    ("module_transparent" -1150 -1150)
> +    ("multifile" -1150 -1150)
> +    ("public" -1150 -1150)

IIUC these are all prefix, so I think it'd be better to write them as

       ("foo" nil -1150)

Other than that, looks good, please install.


-- Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

> Other than that, looks good, please install.

I cannot push to the Emacs repository, but an updated and extended patch
follows. Importantly, this makes the mode also support *-> (soft cut).

All the best,
Markus

2015-10-02  Markus Triska  <triska@metalevel.at>

	* prolog.el: Update and extend operator table.
	(prolog-smie-grammar): Add multifile, public etc.

diff --git a/lisp/progmodes/prolog.el b/lisp/progmodes/prolog.el
index ff2769e..81aeb8d 100644
--- a/lisp/progmodes/prolog.el
+++ b/lisp/progmodes/prolog.el
@@ -877,12 +877,21 @@ prolog-smie-grammar
   ;; manual uses precedence levels in the opposite sense (higher
   ;; numbers bind less tightly) than SMIE, so we use negative numbers.
   '(("." -10000 -10000)
+    ("?-" nil -1200)
     (":-" -1200 -1200)
     ("-->" -1200 -1200)
+    ("discontiguous" nil -1150)
+    ("dynamic" nil -1150)
+    ("meta_predicate" nil -1150)
+    ("module_transparent" nil -1150)
+    ("multifile" nil -1150)
+    ("public" nil -1150)
+    ("|" -1105 -1105)
     (";" -1100 -1100)
+    ("*->" -1050 -1050)
     ("->" -1050 -1050)
     ("," -1000 -1000)
-    ("\\+" -900 -900)
+    ("\\+" nil -900)
     ("=" -700 -700)
     ("\\=" -700 -700)
     ("=.." -700 -700)

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

> I cannot push to the Emacs repository, but an updated and extended patch
> follows. Importantly, this makes the mode also support *-> (soft cut).

Looks good, installed, thanks.


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Looks good, installed, thanks.

Great! One further devation from Stefan Bruda's mode is:

   test :-
           X = [
                   a,
                   b
           ].


We expect instead, as with Stefan Bruda's version, the layout:

   test :-
           X = [
                   a,
                   b
               ].

Notice that this regression is very visible, since it affects the module
declaration that most Prolog files begin with. For example, consider:

   :- module(x, [
                     a,
                     bHERE

when I, with point at "HERE", press RET, then I get with Emacs git:

   :- module(x, [
                     a,
                     b
                     HERE

The indentation for this new line is already different from Stefan
Bruda's version, but not in itself a serious mistake at that point: It
is theoretically possible that further list elements still follow.  In
practice though, that is extremely unlikely, and we would therefore
prefer the level of the opening "[" (as with Stefan Bruda's version):

   :- module(x, [
                     a,
                     b
                 HERE

However, suppose you retain the current behaviour of Emacs git, and then
close the whole module declaration by entering "]).", yielding:

   :- module(x, [
                     a,
                     b
                     ]).HERE

Then, if you press RET (as most users certainly would here), you get:

   :- module(x, [
                     a,
                     b
             ]).

and this is clearly not the intended layout for a term like that.
Instead, the indentation level of "[" should equal that of "]".

Another deviation from Stefan Bruda's mode is an unusually large
indentation for arguments of declarations like:

   :- public
                   a,
                   b.


Please consider indenting arguments less, for example, as in:

   :- public
             a,
             b.

Thank you and all the best,
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

>    test :-
>            X = [
>                    a,
>                    b
>            ].


> We expect instead, as with Stefan Bruda's version, the layout:

>    test :-
>            X = [
>                    a,
>                    b
>                ].

Try M-x smie-edebug RET to see where this is decided.  The above is just
the standard behavior used in many other major modes.  I prefer the
behavior above, so it probably requires a config var.

> The indentation for this new line is already different from Stefan
> Bruda's version, but not in itself a serious mistake at that point

Disagreement with Bruda's version is indeed not considered a bug, in
general, since Bruda's indentation code is far from perfect.

> Then, if you press RET (as most users certainly would here), you get:

>    :- module(x, [
>                      a,
>                      b
>              ]).

> and this is clearly not the intended layout for a term like that.
> Instead, the indentation level of "[" should equal that of "]".

Actually, I would personally prefer something like

       :- module(x, [
                         a,
                         b
                ]).
or
       :- module(x, [
                         a,
                         b
          ]).
or even
       :- module(x, [
                         a,
                         b
       ]).

> Another deviation from Stefan Bruda's mode is an unusually large
> indentation for arguments of declarations like:

>    :- public
>                    a,
>                    b.

Indeed, that's a poor choice, and oddly enough it's not the result of
a simple bug.  The patch below should improve the behavior.


        Stefan


diff --git a/lisp/progmodes/prolog.el b/lisp/progmodes/prolog.el
index 81aeb8d..61d3a3c 100644
--- a/lisp/progmodes/prolog.el
+++ b/lisp/progmodes/prolog.el
@@ -988,7 +988,16 @@ This is really kludgy, and unneeded (i.e. obsolete) in Emacs>=24."
             (smie-indent-backward-token) ;Skip !
             (equal ":-" (car (smie-indent-backward-token))))
           (smie-rule-parent prolog-indent-width)))
-    (`(:after . ,(or `":-" `"-->")) prolog-indent-width)))
+    (`(:after . ":-")
+     (if (bolp)
+         (save-excursion
+           (smie-indent-forward-token)
+           (skip-chars-forward " \t")
+           (if (eolp)
+               prolog-indent-width
+             (min prolog-indent-width (current-column))))
+       prolog-indent-width))
+    (`(:after . "-->") prolog-indent-width)))
 
 \f
 ;;-------------------------------------------------------------------

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>    test_predicate(c) :- !,
>>            test_goal1,
>>            test_goal2.
>
> Should work now.

Here are cases I found that still deviate from Stefan Bruda's version,
in case you are interested in fixing them. His version yields:

   my_nice_test(X) :- var(X), !,
           test_goal1,
           test_goal2.

   my_nice_test --> !,
           test_goal1,
           test_goal2.

whereas the current Emacs version yields:

   my_nice_test(X) :- var(X), !,
                      test_goal1,
                      test_goal2.

   my_nice_test --> !,
                    test_goal1,
                    test_goal2.

Notice that !/0 typically appears as the last goal on a line, but other
short goals often appear on the same line before !/0 in such patterns.

All the best,
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

>    my_nice_test(X) :- var(X), !,
>            test_goal1,
>            test_goal2.

This looks plain wrong to me.  So we'd need a config var to select
this behavior.

How 'but you try and write the path for it (you only need to change
prolog-smie-rules).


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

> Here are further test cases I found when trying this patch:
>
> (1) After evaluating (show-paren-mode 1) and using the Prolog content:
>
>    t1 :- a.
>    t2 :- b.HERE
>
> The dot in the immediately preceding line is highlighted as the
> (mistakenly classified as such) "matching" element.

Hmm... indeed it seems to be a problem in the show-paren-mode provided
by SMIE.  There's some logic to it, but to the extent that C-M-b jumps
to just before "t2", we should either highlight "t2" or just the "t" or
nothing at all.

> (2) Please enable syntax highlighting for DCGs with zero arguments.

The current code provides very minimal support for DCG, indeed.

AFAIK, the same was true of Bruda's version (after all, the current code
is Bruda's just with the new SMIE indentation swapped in and the
electric self-insert keys re-implemented via post-self-insert-hook).
If his version has evolved in the mean time, we/he should merge
the changes.

> Whereas the expected result is:
>
>    test :-
>            (   a ->
>                b
>            ;   c ->
>                HERE

This should work now.


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

> (1) Parentheses matching does not work yet. When I have a fact like:
>
>    test(a, b).HERE
>
> (with point again at the H) and press C-M-b (backward-sexp), I expect to
> see, as with prolog.el directly supplied by Stefan Bruda:
>
>    testHERE(a, b).
>
> Instead, I currently unexpectedly get:
>
>    HERE
>    test(a, b).

Hmmm you definitely shouldn't get

    testHERE(a, b).

but you should get

    HEREtest(a, b).

IOW, it's not a bug, it's a feature: the new code knows about infix
operators and knows how to jump over the left-hand-side or
right-hand-side of an infix operator.

Tho it seems the feature is not working 100%.  Indeed, I can reproduce
this problem, but only at the beginning of the buffer.  IOW if I have

   test(a).

   test(b).HERE

and I hit C-M-b I correctly end at

   HEREtest(b).

Since the problem only shows up on the very first element, it's not
too serious, but of course, we'd welcome a patch to fix it.

> (2) When I set `prolog-electric-dot-flag' to t, and enter, at
> (point-min) of an empty Prolog mode buffer from "emacs -Q -nw":

>    RET RET RET t e s t .

> then I see that the first line of the buffer is briefly highlighted,

Right, that's the same problem: you should see the first "t"
highlighted instead (since the dot closes the rule, which starts at the
first "t").


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Hmmm you definitely shouldn't get
>
>     testHERE(a, b).
>
> but you should get
>
>     HEREtest(a, b).
>
> IOW, it's not a bug, it's a feature: the new code knows about infix
> operators and knows how to jump over the left-hand-side or
> right-hand-side of an infix operator.

But it does not do so consistently: C-M-b jumps back up to including the
predicate name, but C-M-f from that position only jumps forward up to
the opening parenthesis, and not including the whole construct.

In other words, starting from:

    test(a,b).HERE

pressing C-M-b yields:

    HEREtest(a,b).

but then C-M-f yields:

    testHERE(a,b).

whereas one would expect, for internal consistency:

    test(a,b).HERE

All the best,
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

> But it does not do so consistently: C-M-b jumps back up to including the
> predicate name, but C-M-f from that position only jumps forward up to
> the opening parenthesis, and not including the whole construct.

It does, if you do the C-M-f from just before the dot.
The fact that you have to "jump over the dot" is the crucial hint you
(the user) give to Emacs that you want to jump over "the whole LHS/RHS
of that dot" rather than over a deeper part of the AST.


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> It does, if you do the C-M-f from just before the dot.
> The fact that you have to "jump over the dot" is the crucial hint you
> (the user) give to Emacs that you want to jump over "the whole LHS/RHS
> of that dot" rather than over a deeper part of the AST.

Still, currently C-M-b jumps back way too far even if we accept that.

An example is here:

    my_test(X) :-
            goal1,
            goal2,
            goal3.
    
    %?- my_test(X).HERE

Notice the comment on the last line is an embedded query that I can
evaluate for example with ediprolog. When I press C-M-b in that
situation, point is moved even before the whole snippet (!), whereas I
get, with the mode supplied by Stefan Bruda:

    my_test(X) :-
            goal1,
            goal2,
            goal3.
    
    %?- my_testHERE(X).

i.e., point is moved to a sensible position within that query, and we
obtain the property that from then on, C-M-f and C-M-b can be used
repeatedly to cycle between two positions, making it easier to navigate
consistently between symbolic expressions. Please reconsider the
criteria that determine how much C-M-b and C-M-f should skip, so that
the mode does not skip beyond the intuitively expected positions.

Thank you and all the best,
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> It does, if you do the C-M-f from just before the dot.
> The fact that you have to "jump over the dot" is the crucial hint you
> (the user) give to Emacs that you want to jump over "the whole LHS/RHS
> of that dot" rather than over a deeper part of the AST.

Please consider the following additional test case for navigating Prolog
code with C-M-f and C-M-b, which now unexpectedly move too far.

The snippet I am using to demonstrate the regression is:

   test :-
           (   a,
               b
           ;   (   c,
                   d
               ;   e,
                   f
               ),
               g,
               h
           ).

I begin with point at HERE:

   test :-
           (   a,
               bHERE
           ;   (   c,
                   d
               ;   e,
                   f
               ),
               g,
               h
           ).

From that position, I press C-M-f, and get:

   test :-
           (   a,
               b
           ;   (   c,
                   d
               ;   e,
                   f
               ),
               g,
               hHERE
           ).

This, in my view, already skips too far ahead, over too many other
constructs. (For comparison, with Stefan Bruda's mode, C-M-f in the same
situation moves point to the comma before the goal "g/0").

The most unusual aspect though is, in my view, that not only does it
take a disproportionate amount of the inverse command (C-M-b) to get
approximately back to the original position, but it is in fact not
possible to get satisfactorily close to it (say, within an offset of 1)
with C-M-b alone. This is what happens on repeated C-M-b:

   test :-
           (   a,
               b
           ;   (   c,
                   d
               ;   e,
                   f
               ),
               g,
               HEREh
           ).

followed by:

   test :-
           (   a,
               b
           ;   (   c,
                   d
               ;   e,
                   f
               ),
               HEREg,
               h
           ).

followed by:

   test :-
           (   a,
               b
           ;   HERE(   c,
                   d
               ;   e,
                   f
               ),
               g,
               h
           ).

followed by:

   test :-
           (   HEREa,
               b
           ;   (   c,
                   d
               ;   e,
                   f
               ),
               g,
               h
           ).

I tried navigating several such code snippets with Stefan Bruda's mode,
starting from various positions. In all cases, the important invariant
seems to be preserved that, give or take an offset of at most 1, we can
invert a C-M-f with a subsequent C-M-b in most if not all situations
that are of great practical importance when moving through Prolog code.

All the best,
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

>> From that position, I press C-M-f, and get:

>    test :-
>            (   a,
>                b
>            ;   (   c,
>                    d
>                ;   e,
>                    f
>                ),
>                g,
>                hHERE
>            ).

That's not a bug, it's a feature: you jumped over all the
right-hand-side of the ";".  The old behavior is easy to get, but is
less useful.
Basically the new behavior lets you jump over "any" subtree in the AST,
whereas the old behavior only allowed you to jump over those subtrees
which are made of a single identifier or are delimited by parentheses.

E.g. if you want to swap the order of the two top-level alternatives in
the above example, just put point before or after the ";" and hit C-M-t.

> I tried navigating several such code snippets with Stefan Bruda's mode,
> starting from various positions. In all cases, the important invariant
> seems to be preserved that, give or take an offset of at most 1, we can
> invert a C-M-f with a subsequent C-M-b in most if not all situations
> that are of great practical importance when moving through Prolog code.

Not sure what you mean by "offset of at most 1".  E.g. with

     abd
     toto
     titi

starting at the end of "abd", after C-M-f I don't see how you can get
back to within 1 char of the original position with some number of C-M-b.

The new behavior is different, there's no doubt about it.  I think the
fact that it knows about infix operators and their relative precedence
is a great improvement, bringing sexp navigation closer to what you
get in Lisp.


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> The new behavior is different, there's no doubt about it.  I think the
> fact that it knows about infix operators and their relative precedence
> is a great improvement, bringing sexp navigation closer to what you
> get in Lisp.

I would like to test the mode more seriously, and the most impeding
aspect of the new navigation is currently that Prolog terms within
comments are skipped over way too far, and comments in general make the
mode's navigation hard to predict in my view.  Consider for example:

   % Hello,
   % there.

   it_is_a_fact.

   %?- test(a,b).HERE

In that sitation, pressing C-M-b yields:

   HERE% Hello,
   % there.

   it_is_a_fact.

   %?- test(a,b).

However, when I press C-M-b for example with:

   x.

   % Hello,
   % there.

   it_is_a_fact.

   %?- test(a,b).HERE

Then I get:

   x.

   % Hello,
   % there.

   HEREit_is_a_fact.

   %?- test(a,b).

Thus, syntactic constructs that are comparatively far away from the
Prolog terms near point influence the navigation with C-M-b. In this
case, placing the fact x/0 at the beginning made the mode NOT jump over
a fact and a comment, whereas it otherwise jumps over both of them.

In other modes I tested, syntactic elements of the language are handled
as expected also within comments when using C-M-b and C-M-f.

For example, in the case of Prolog, we have with the recent git version:

   ?- test(a,b).
   ?- test(x,y).HERE

Pressing C-M-b yields, as expected:

   ?- test(a,b).
   HERE?- test(x,y).

The reason is clear: The Prolog term ?-(test(x,y)) is the one that
immediately precedes point, and pressing C-M-b jumps precisely over it.

However, when both of these terms are commented, as in:

   %?- test(a,b).
   %?- test(x,y).HERE

then pressing C-M-b skips over both of them, and, depending on what
appears before these lines, may move away even further than that.

This is completely unexpected and unusual in Emacs modes: In other modes
I tested (C, Lisp etc.), C-M-b and C-M-f behave very consistently, also
if the syntactic elements they usually jump over arise in comments.

In the case of Prolog though, it is particularly unfortunate because of
tools like ediprolog that encourage users to embed Prolog terms as
queries in comments, and pressing C-M-b with the expectation to move to
the beginning of such terms will in general move point to a position
that is much further away, and from where it is not easy to get back.

Please consider reinstating the navigation that was previously
available, or making the current navigation work better with comments.

Thank you and all the best,
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

> This is completely unexpected and unusual in Emacs modes: In other modes
> I tested (C, Lisp etc.), C-M-b and C-M-f behave very consistently, also
> if the syntactic elements they usually jump over arise in comments.

Actually this is a manifestation of a problem that's plagued Emacs
navigation for ever: try

    % foo)HERE

and hit C-M-b (replace the "%" by the comment-starter of the mode you
happen to be using).

But indeed, in the present case it's worse because the comment actually
does contain a complete "sexp", it's just that with infix operators, we
have to find the "previous" in order to decide that we've seen the end.

Also I can't explain why

   % Hello,
   % there.

   it_is_a_fact.

   %?- test(a,b).HERE

would jump back all the way to the beginning.  I'd expect it to stop
at

   HERE%?- test(a,b).

or nearby (since the preceding token is a "." which should stop the movement).

> making the current navigation work better with comments.

Indeed.


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> But indeed, in the present case it's worse because the comment actually
> does contain a complete "sexp", it's just that with infix operators, we
> have to find the "previous" in order to decide that we've seen the
> end.

Another regression, which may also be affected by how the mode currently
finds the end of terms, also affects indentation outside comments. When
writing Prolog code, situations like the following are common: Suppose I
am in the middle of defining a predicate, with point at "HERE":


   serious_predicate :-
           goal_1,
           goal_2,
           something_complicated,
           HERE

at this point, we decide that we better define something_complicated/0
right now, before even bothering with the rest of the clause at hand.

So we leave the current rule unfinished and instead add:

   serious_predicate :-
           goal_1,
           goal_2,
           something_complicated,

   something_complicated :-HERE

When we now press RET, we get:

   serious_predicate :-
           goal_1,
           goal_2,
           something_complicated,

           something_complicated :-
                   HERE

That's OK, and Stefan Bruda's mode does that too.

But now the point: We now need a way to tell the mode to not bother (for
now) with anything prior to the new rule at hand. I.e., we now want to
focus on the definition of something_complicated/0, and need a way to
tell the mode to not bother with the still incomplete definition of
serious_predicate/0.

In Stefan Bruda's mode, there is a straight-forward and intuitive way to
do this at this point:

   C-p C-a M-\ C-n TAB

which gives us, with Stefan Bruda's original mode:


   serious_predicate :-
           goal_1,
           goal_2,
           something_complicated,

   something_complicated :-
           HERE

and we are ready to complete the definition of something_complicated/0.
Indentation will work as expected at this point, not taking into account
the prior, intentionally still unfinished clause of serious_predicate/0:


   serious_predicate :-
           goal_1,
           goal_2,
           something_complicated,

   something_complicated :-
           goal_3,
           goal_4.


In contrast, we get with the current git version, as a result of the
above key sequence in the exact same situation:


   serious_predicate :-
           goal_1,
           goal_2,
           something_complicated,

   something_complicated :-
                   HERE

which is indenting too far in this specific situation. Moreover, when we
now insert the exact same goals, we get:

   serious_predicate :-
           goal_1,
           goal_2,
           something_complicated,

   something_complicated :-
                   goal_3,
                   goal_4.


From what I understand, the current git version parses this as if it
meant:

   serious_predicate :-
           goal_1,
           goal_2,
           something_complicated,
           something_complicated :-
                   goal_3,
                   goal_4.

But this is not valid Prolog syntax, because:

   ?- current_op(X, Y, (:-)).
   X = 1200,
   Y = fx ;
   X = 1200,
   Y = xfx.

This means that any arguments of the infix operator (:-)/2 must be of
*smaller precedence* than (:-)/2 itself (i.e., 1200).

Therefore, the above program is not syntactically valid, and it should
be treated for what it actually is: Two independent rules, with one of
them still unfinished, and only the second one mattering for indentation
in this situation, which previously also worked exactly this way.

If possible, please also take this situation into account.

Thank you and all the best,
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

>    serious_predicate :-
>            goal_1,
>            goal_2,
>            something_complicated,
>            something_complicated :-
>                    goal_3,
>                    goal_4.

> But this is not valid Prolog syntax,

Exactly.  We could try and coerce Prolog into interpreting it the way
you suggest, but I think we're better off telling people to add
a terminating "." in the previous rule, even that rule is not yet finished.

> Therefore, the above program is not syntactically valid, and it should
> be treated for what it actually is: Two independent rules, with one of
> them still unfinished, and only the second one mattering for indentation
> in this situation, which previously also worked exactly this way.

There's no way to know that this is "what it actually is".

And even if it is, the mis-indentation can be useful, in case the user
didn't realize that she forgot to close the previous rule.

IOW, in this particular case, the ideal behavior depends on what's
inside the user's head, so I don't think we should spend much time
trying to fit any particular expected state of mind of the user.


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> We could try and coerce Prolog into interpreting it the way you
> suggest,

This was the default behaviour of this mode, up until the point these
regressions were introduced. All it takes is to re-introduce the flag to
disable the, in my view, currently in many ways broken new indentation
code, and use the previous behaviour of Stefan Bruda's original mode,
which was also the default when this mode was included in GNU Emacs.

> but I think we're better off telling people to add a terminating "."
> in the previous rule, even that rule is not yet finished.

This requirement would make Prolog mode highly unusual even within
Emacs. Please try it yourself, with different languages, to see that the
way I am suggesting is very common also in other modes. In all following
examples, even though the previous form is not yet complicated, it is
completely clear what is intended here, because the user *explicitly*
re-indented the new beginning form in such a way that it is clear that a
new defun/proc/whatever is meant to start.

In the following examples, when the new defun/proc/etc. starts, the mode
still tries to indent according to the still open construct. But, with
just a few key presses that, critically, require *no changes whatsoever*
to the construct that is still open, the indentation is adjusted (please
see my previous mail for the exact key presses, used analogously):

Emacs Lisp:

   (defun test ()
     (if (test)
         (

   (defun complicated ()
     (test)
     (if (further)
         (etc.

C/C++:

   int test () {
     test();
     more(

   int complicated() {
     test();
     further(a, b);
   }

Tcl/Tk:

   proc test {} {
       test
       more

   proc complicated {} {
       test
       if {


and now the current Prolog mode, in contrast to all preceding examples:


   test :-
       test,
       more

       complicated :-
           further,

with *no way* to manually and explicitly adjust the indentation to the
level that is clearly intended in this case (i.e., to the left), because
interpreting it any other way makes absolutely no sense.

> There's no way to know that this is "what it actually is".
>
> And even if it is, the mis-indentation can be useful, in case the user
> didn't realize that she forgot to close the previous rule.

In the case I outline, the user tries, in vain, to *explicitly* and
manually set a different indentation level, so that the layout matches
the user's intention at this point. Stefan Bruda's mode allows this
explicit interaction, and the current Emacs version falls short in this
respect, instead insisting on an indentation that makes no sense. I
agree that this my be useful when pressing RET the first time, but no
longer after the user explicitly re-indented and is *clearly* asking for
a different indentation level.

> IOW, in this particular case, the ideal behavior depends on what's
> inside the user's head, so I don't think we should spend much time
> trying to fit any particular expected state of mind of the user.

Again, please reconsider the example interaction I posted: By explicitly
removing all horizontal whitespace (M-\) to adjust the indentation
level, the user is giving a very clear sign that this is the indentation
level we wish at this point.

Thank you and all the best,
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

> This was the default behaviour of this mode, up until the point these
> regressions were introduced.  All it takes is to re-introduce the flag to
> disable the, in my view, currently in many ways broken new indentation
> code, and use the previous behaviour of Stefan Bruda's original mode,
> which was also the default when this mode was included in GNU Emacs.

OK, let's turn things around, indeed: we revert to Stefan Bruda's
indentation code, you're handed responsibility over that code, and I get
to send you the bug reports about all the regressions this will introduce.


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> OK, let's turn things around, indeed: we revert to Stefan Bruda's
> indentation code, you're handed responsibility over that code, and I get
> to send you the bug reports about all the regressions this will introduce.

This would be a good improvement in most respects I consider important,
thank you! I will treat all issues you and others send to me with great
care and attention, and work on correcting all regressions and issues I
can. I have a lot of experience editing Prolog code with Emacs and can
spot regressions quickly. I reported these issues in the hope that you
can modify the SMIE-based indentation to be more useful in practical
situations. Seeing that is not the case in general, please do restore
the original behaviour, or at least enable it by default via a flag.

Thank you and all the best,
Markus

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

> I reported these issues in the hope that you can modify the SMIE-based
> indentation to be more useful in practical situations.

Right, and I've been doing that so far.  But you can't expect this to
lead to a 100% bug-for-bug reproduction of the old behavior.
Some changes are real regressions, while others (like this one) are much
more debatable.

The debatable ones, not only may need a config var, but more importantly,
have much lower priority.  IOW patches welcome.

> please do restore the original behaviour, or at least enable it by
> default via a flag.

Re-adding the old code and providing a "use-smie" flag would just offer
the user the choice between 2 half-broken options while adding more
maintenance work in the long run.
This said, if you really need it, Stefan Bruda's mode still works in
Emacs-25, AFAIK.


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

> or, depending on the empty lines before this fact, even things like:
>
>    HERE
>
>
>    test(a, b).

[...]

> (2) When I set `prolog-electric-dot-flag' to t, and enter, at
> (point-min) of an empty Prolog mode buffer from "emacs -Q -nw":
>
>    RET RET RET t e s t .
>
> then I see that the first line of the buffer is briefly highlighted,
> apparently in connection with parsing. This may be connected to the
> previous issue, and I hope that fixing (1) also fixes this.

I believe I've just fixed these two (which were both the same
underlying problem) in emacs-25.


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> (2) When I set `prolog-electric-dot-flag' to t, and enter, at
>> (point-min) of an empty Prolog mode buffer from "emacs -Q -nw":
>>
>>    RET RET RET t e s t .
>>
>> then I see that the first line of the buffer is briefly highlighted,
>> apparently in connection with parsing. This may be connected to the
>> previous issue, and I hope that fixing (1) also fixes this.
>
> I believe I've just fixed these two (which were both the same
> underlying problem) in emacs-25.

I've just lightly skimmed this thread, but this was the final message
here, so I'm going to go ahead and guess that the problems discussed
were fixed, and I'm closing this bug report.  If there's anything more
to be worked on here, please send a mail to the debbugs address and
we'll reopen the bug report.  (Or perhaps open a new report for any
remaining issues.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Stefan Monnier]

> I believe I found a solution for this one.  Please test the new code in
> master.

Have you had a chance to test the new code?


        Stefan

bug#21526: 24.5; prolog-mode: broken indentation for if-then-else construct

[Markus Triska]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> Have you had a chance to test the new code?

Yes, I have reported all issues I found in the bugtracker. Please have a
look. I will CC you in future mails if you do not get them otherwise.

Thank you and all the best!
Markus