bug#32496: 27.0.50; Strange indentation when ruby-align-chained-calls is t

bug#32496: 27.0.50; Strange indentation when ruby-align-chained-calls is t

[Artur Malabarba]

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

1. (setq ruby-align-chained-calls t)
2. (setq ruby-use-smie t)
3. Open a file in ruby-mode, insert the following and indent it

----------
some_variable.where.not(x: nil)
             .where(y: 2)
----------

Expected behaviour: Nothing would happen, the code is already properly
indented.

What actually happens: The code gets indented as follows

----------
some_variable.where.not(x: nil)
                   .where(y: 2)
----------

Note that this is conflicts with the indentation enforced by rubocop.
Artur

[-- Attachment #2: Type: text/html, Size: 780 bytes --]

bug#32496: 27.0.50; Strange indentation when ruby-align-chained-calls is t

[Dmitry Gutov]

On 8/22/18 2:36 PM, Artur Malabarba wrote:
> 1. (setq ruby-align-chained-calls t)
> 2. (setq ruby-use-smie t)
> 3. Open a file in ruby-mode, insert the following and indent it
> 
> ----------
> some_variable.where.not(x: nil)
>               .where(y: 2)
> ----------
> 
> Expected behaviour: Nothing would happen, the code is already properly
> indented.
> 
> What actually happens: The code gets indented as follows
> 
> ----------
> some_variable.where.not(x: nil)
>                     .where(y: 2)
> ----------
> 
> Note that this is conflicts with the indentation enforced by rubocop.

I'd like to point out that this is exactly the behavior Bozhidar asked 
for, back when this variable was introduced. See:

http://lists.gnu.org/archive/html/emacs-devel/2014-01/msg01802.html

and in particular the Example 1 in the referenced comment:

https://github.com/rubocop-hq/ruby-style-guide/pull/176#issuecomment-18664622

So we even have a test (ruby-align-chained-calls) that check that the 
alignment is do to the last dot, and not to the first one.

bug#32496: 27.0.50; Strange indentation when ruby-align-chained-calls is t

[Artur Malabarba]

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

IIUC, bozhidar was requesting that the dots be aligned to the dot above (as
opposed to being indented by only 2 spaces). He didn't say what should
happen if one of the lines has multiple dots in it.

The linked github comment does explicitly request the aligning to the last
dot, but it's the only comment that requests that on a very long discussion
that was largely focused on a different topic (whether or not to use
trailing dots).

Bozhidar, do you have an opinion on this?

Artur


On Wed, 22 Aug 2018 at 09:50, Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 8/22/18 2:36 PM, Artur Malabarba wrote:
> > 1. (setq ruby-align-chained-calls t)
> > 2. (setq ruby-use-smie t)
> > 3. Open a file in ruby-mode, insert the following and indent it
> >
> > ----------
> > some_variable.where.not(x: nil)
> >               .where(y: 2)
> > ----------
> >
> > Expected behaviour: Nothing would happen, the code is already properly
> > indented.
> >
> > What actually happens: The code gets indented as follows
> >
> > ----------
> > some_variable.where.not(x: nil)
> >                     .where(y: 2)
> > ----------
> >
> > Note that this is conflicts with the indentation enforced by rubocop.
>
> I'd like to point out that this is exactly the behavior Bozhidar asked
> for, back when this variable was introduced. See:
>
> http://lists.gnu.org/archive/html/emacs-devel/2014-01/msg01802.html
>
> and in particular the Example 1 in the referenced comment:
>
>
> https://github.com/rubocop-hq/ruby-style-guide/pull/176#issuecomment-18664622
>
> So we even have a test (ruby-align-chained-calls) that check that the
> alignment is do to the last dot, and not to the first one.
>

[-- Attachment #2: Type: text/html, Size: 2524 bytes --]

bug#32496: 27.0.50; Strange indentation when ruby-align-chained-calls is t

[Bozhidar Batsov]

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

Sorry for the radio silence - I've been super busy lately.

It's hard for me to understand the indentation in the examples in the email
(as it seems the same to me). Very simply put - the idea is to align
multi-line chained calls on the `.`, as opposed to just nest them under the
root receiver as we'd normally do.

I think Dmitry implemented this great and it's behaving just as it's
supposed to be behaving. Perhaps you misunderstood how this was supposed to
behave? What's the indentation you expected?

On Sun, 28 Oct 2018 at 00:22, Artur Malabarba <bruce.connor.am@gmail.com>
wrote:

> IIUC, bozhidar was requesting that the dots be aligned to the dot above
> (as opposed to being indented by only 2 spaces). He didn't say what should
> happen if one of the lines has multiple dots in it.
>
> The linked github comment does explicitly request the aligning to the last
> dot, but it's the only comment that requests that on a very long discussion
> that was largely focused on a different topic (whether or not to use
> trailing dots).
>
> Bozhidar, do you have an opinion on this?
>
> Artur
>
>
> On Wed, 22 Aug 2018 at 09:50, Dmitry Gutov <dgutov@yandex.ru> wrote:
>
>> On 8/22/18 2:36 PM, Artur Malabarba wrote:
>> > 1. (setq ruby-align-chained-calls t)
>> > 2. (setq ruby-use-smie t)
>> > 3. Open a file in ruby-mode, insert the following and indent it
>> >
>> > ----------
>> > some_variable.where.not(x: nil)
>> >               .where(y: 2)
>> > ----------
>> >
>> > Expected behaviour: Nothing would happen, the code is already properly
>> > indented.
>> >
>> > What actually happens: The code gets indented as follows
>> >
>> > ----------
>> > some_variable.where.not(x: nil)
>> >                     .where(y: 2)
>> > ----------
>> >
>> > Note that this is conflicts with the indentation enforced by rubocop.
>>
>> I'd like to point out that this is exactly the behavior Bozhidar asked
>> for, back when this variable was introduced. See:
>>
>> http://lists.gnu.org/archive/html/emacs-devel/2014-01/msg01802.html
>>
>> and in particular the Example 1 in the referenced comment:
>>
>>
>> https://github.com/rubocop-hq/ruby-style-guide/pull/176#issuecomment-18664622
>>
>> So we even have a test (ruby-align-chained-calls) that check that the
>> alignment is do to the last dot, and not to the first one.
>>
>

[-- Attachment #2: Type: text/html, Size: 3474 bytes --]

bug#32496: 27.0.50; Strange indentation when ruby-align-chained-calls is t

[Wendel Scardua]

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

I was about to open a bug about this same issue. Seeing this thread, there
seems to have been some misunderstanding between the parts.

Bhozidar thought it was about indenting on the dot vs indenting according
to the previous line, and in that aspect the feature is working correctly.

But when there are multiple dots on a line, their style guide enforces
indenting on the *first* dot, not the last, while (setq
ruby-align-chained-calls t) - supposedly based on it - enforces the latter.

[-- Attachment #2: Type: text/html, Size: 576 bytes --]

bug#32496: 27.0.50; Strange indentation when ruby-align-chained-calls is t

[Lars Ingebrigtsen]

Bozhidar Batsov <bozhidar@batsov.com> writes:

> Sorry for the radio silence - I've been super busy lately. 
>
> It's hard for me to understand the indentation in the examples in the email
> (as it seems the same to me). Very simply put - the idea is to align multi-line
> chained calls on the `.`, as opposed to just nest them under the root receiver
> as we'd normally do.
>
> I think Dmitry implemented this great and it's behaving just as it's supposed
> to be behaving. Perhaps you misunderstood how this was supposed to
> behave? What's the indentation you expected? 

(I'm going through old bug reports that unfortunately weren't resolved
at the time.)

The examples were in HTML mail, so they were difficult to understand.

Emacs (with (setq ruby-align-chained-calls t)) currently aligns like
this:

some_variable.where
             .not(x: nil)
             .where(y: 2)

Which is correct.  However, when there's a mixture of keeping things on
one line and breaking, we get this:

some_variable.where.not(x: nil)
                   .where(y: 2)

I think the bug reporter wants:

some_variable.where.not(x: nil)
             .where(y: 2)

I.e., align multiline chained calls on the first dot, not the last?

(I don't know Ruby, so I have no opinion here.)

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

bug#32496: 27.0.50; Strange indentation when ruby-align-chained-calls is t

[Bozhidar Batsov]

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

Ah, I finally understood the issue at hand! It's really hard to discuss indentation problems in e-mail. :D

Yeah, I can confirm there's a bug when using (setq ruby-align-chained-calls t) in this example:

some_variable.where.not(x: nil)
                         .where(y: 2)

The two `where`s should be lined up, but currently the second `where` is lined up with the `not`.

some_variable.where.not(x: nil)
                                    .where(y: 2)

I can also confirm that the first behavior is what RuboCop (Ruby's popular linter/formatter) expects, when configured for aligned chained method calls. 

On Wed, Sep 1, 2021, at 12:53 PM, Lars Ingebrigtsen wrote:
> Bozhidar Batsov <bozhidar@batsov.com> writes:
> 
> > Sorry for the radio silence - I've been super busy lately. 
> >
> > It's hard for me to understand the indentation in the examples in the email
> > (as it seems the same to me). Very simply put - the idea is to align multi-line
> > chained calls on the `.`, as opposed to just nest them under the root receiver
> > as we'd normally do.
> >
> > I think Dmitry implemented this great and it's behaving just as it's supposed
> > to be behaving. Perhaps you misunderstood how this was supposed to
> > behave? What's the indentation you expected? 
> 
> (I'm going through old bug reports that unfortunately weren't resolved
> at the time.)
> 
> The examples were in HTML mail, so they were difficult to understand.
> 
> Emacs (with (setq ruby-align-chained-calls t)) currently aligns like
> this:
> 
> some_variable.where
>              .not(x: nil)
>              .where(y: 2)
> 
> Which is correct.  However, when there's a mixture of keeping things on
> one line and breaking, we get this:
> 
> some_variable.where.not(x: nil)
>                    .where(y: 2)
> 
> I think the bug reporter wants:
> 
> some_variable.where.not(x: nil)
>              .where(y: 2)
> 
> I.e., align multiline chained calls on the first dot, not the last?
> 
> (I don't know Ruby, so I have no opinion here.)
> 
> -- 
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no
> 

[-- Attachment #2: Type: text/html, Size: 3655 bytes --]

bug#32496: 27.0.50; Strange indentation when ruby-align-chained-calls is t

[Lars Ingebrigtsen]

"Bozhidar Batsov" <bozhidar@batsov.dev> writes:

> Ah, I finally understood the issue at hand! It's really hard to discuss
> indentation problems in e-mail. :D
>
> Yeah, I can confirm there's a bug when using (setq ruby-align-chained-calls t)
> in this example:
>
> some_variable.where.not(x: nil)
>                    .where(y: 2)
>
> The two `where`s should be lined up, but currently the second `where` is
> lined up with the `not`.

So this is coming from:

    ('(:before . ".")
     (if (smie-rule-sibling-p)
         (and ruby-align-chained-calls 0)
       (smie-backward-sexp ".")
       (cons 'column (+ (current-column)
                        ruby-indent-level))))

In the aligned case, I think we should back up to the first "." in the
chain and use that as the column?  But my SMIE-fu is pretty much
non-existent, so I've added Stefan to the CCs.

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

bug#32496: 27.0.50; Strange indentation when ruby-align-chained-calls is t

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen [2021-09-02 08:55:50] wrote:

> "Bozhidar Batsov" <bozhidar@batsov.dev> writes:
>
>> Ah, I finally understood the issue at hand! It's really hard to discuss
>> indentation problems in e-mail. :D
>>
>> Yeah, I can confirm there's a bug when using (setq ruby-align-chained-calls t)
>> in this example:
>>
>> some_variable.where.not(x: nil)
>>                    .where(y: 2)
>>
>> The two `where`s should be lined up, but currently the second `where` is
>> lined up with the `not`.
>
> So this is coming from:
>
>     ('(:before . ".")
>      (if (smie-rule-sibling-p)
>          (and ruby-align-chained-calls 0)
>        (smie-backward-sexp ".")
>        (cons 'column (+ (current-column)
>                         ruby-indent-level))))
>
> In the aligned case, I think we should back up to the first "." in the
> chain and use that as the column?  But my SMIE-fu is pretty much
> non-existent, so I've added Stefan to the CCs.

You could try something like

    diff --git a/lisp/progmodes/ruby-mode.el b/lisp/progmodes/ruby-mode.el
    index c09f007a5ee..c681800f6a7 100644
    --- a/lisp/progmodes/ruby-mode.el
    +++ b/lisp/progmodes/ruby-mode.el
    @@ -640,7 +640,15 @@ ruby-smie-rules
         ('(:before . "do") (ruby-smie--indent-to-stmt))
         ('(:before . ".")
          (if (smie-rule-sibling-p)
    -         (and ruby-align-chained-calls 0)
    +         (when ruby-align-chained-calls
    +           (while
    +               (let ((pos (point))
    +                     (parent (smie-backward-sexp ".")))
    +                 (if (not (equal (nth 2 parent) "."))
    +                     (progn (goto-char pos) nil)
    +                   (goto-char (nth 1 parent))
    +                   (not (smie-rule-bolp)))))
    +           (cons 'column (current-column)))
            (smie-backward-sexp ".")
            (cons 'column (+ (current-column)
                             ruby-indent-level))))
    @@ -826,13 +834,6 @@ ruby--electric-indent-p
     
     ;; FIXME: Remove this?  It's unused here, but some redefinitions of
     ;; `ruby-calculate-indent' in user init files still call it.
    -(defun ruby-current-indentation ()
    -  "Return the indentation level of current line."
    -  (save-excursion
    -    (beginning-of-line)
    -    (back-to-indentation)
    -    (current-column)))
    -
     (defun ruby-indent-line (&optional ignored)
       "Correct the indentation of the current Ruby line."
       (interactive)

But please add corresponding regression tests,


        Stefan

bug#32496: 27.0.50; Strange indentation when ruby-align-chained-calls is t

[Lars Ingebrigtsen]

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

> You could try something like

[...]

> But please add corresponding regression tests,

Thanks; seems to work perfectly.  (And I've added some more tests, but
this one was already being tested, I found out afterwards.)

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