Debugging SMIE for sh-script.el

Debugging SMIE for sh-script.el

[Kévin Le Gouguec]

Hello,

I am having a hard time understanding the mechanics of SMIE in the
context of sh-script.el; for starters, I am stumped trying to debug a
call to smie-indent-calculate.

Take this two-line shell snippet:

#+begin_src sh
${foo}/bar --baz \
      --quux
#+end_src

With point on "--quux", evaluate (smie-indent-calculate): this yields 6.
Looking at this function, I see that it simply funcalls everything in
smie-indent-functions, which in a shell buffer is
(sh-smie--indent-continuation t).

Instrumenting sh-smie--indent-continuation with C-M-x, I see that we
first go into the catch-all branch of the cond (";; Just make sure a
line-continuation is indented deeper."), which let-binds "indent" to:

#+begin_src elisp
(let ((sh-indent-after-continuation nil))
  (smie-indent-calculate))
#+end_src

Stepping through this recursive call with Edebug, I see that we go into
the first branch of sh-smie--indent-continuation's opening cond, which
returns nil.  Continuing into the parent call of
sh-smie--indent-continuation… "indent" is set to 6 for some reason??

I struggle to understand whether I am missing something, or whether this
binding is bogus.  On the one hand, run-hook-wrapped makes no promises
as to its return value when all functions return nil; on the other hand,
"indent" gets consistently assigned to whatever column "/bar" begins at
(e.g. 6 here, or 9 with "${foobar}/bar"), so there is a method to the
madness…

What am I missing?

Re: Debugging SMIE for sh-script.el

[Nikolay Kudryavtsev]

SMIE funcalls smie-indent-functions until one of them returns non-nil. 
If we do smie-config-show-intent on your line we get:

Rules used: :list-intro "" -> nil

This means that our line has been indented by the default logic of 
smie-indent-exps. If we go through it we'll see that it's trying to 
align with the first arg of sexp which it thinks is "/bar". So in effect 
it's treating your expression as if it was:

({foo} /bar

            --baz \ --quux)

P. S. Incidentally I've spent like the entire last week writing SMIE 
indentation logic for some proprietary language with a really terrible 
syntax, and I'm not even close to getting it done, so that's how I know 
this stuff.

Re: Debugging SMIE for sh-script.el

[Kévin Le Gouguec]

Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com> writes:

> SMIE funcalls smie-indent-functions until one of them returns
> non-nil. If we do smie-config-show-intent on your line we get:
>
> Rules used: :list-intro "" -> nil
>
> This means that our line has been indented by the default logic of
> smie-indent-exps. If we go through it we'll see that it's trying to
> align with the first arg of sexp which it thinks is "/bar". So in
> effect it's treating your expression as if it was:
>
> ({foo} /bar
>
>            --baz \ --quux)

Thanks, smie-config-show-intent looks like a neat debugging tool, I'm
sure it will help me understand how to achieve what I want (cf. commit
6392bc37a, which was reverted because of bug#50320).


I'm still at a loss regarding why this snippet returns 6, despite
sh-smie--indent-continuation returning nil:

#+begin_src elisp
(let ((sh-indent-after-continuation nil))
  (smie-indent-calculate))
#+end_src

This is not necessarily an SMIE-related issue; it feels like there is
something more fundamental I don't understand about what's going on.
This specific question is what prompted my message to help-gnu-emacs.

Re: Debugging SMIE for sh-script.el

[Nikolay Kudryavtsev]

It returns 6, because when one of the smie-indent-functions returns nil, 
it does not mean that the line won't be indented, it just means that 
this function in particular(sh-smie--indent-continuation) could not 
indent the line and maybe there's another function further down the 
chain which can, smie-indent-exps that is and that's where 6 comes from.

Re: Debugging SMIE for sh-script.el

[Stefan Monnier via Users list for the GNU Emacs text editor]

> smie-indent-functions, which in a shell buffer is
> (sh-smie--indent-continuation t).

Not that the `t` here is a general feature of hooks which means "use
here the lit stored in the global (i.e. not buffer-local)" variable.
So the hook will include all the functions in the default
value of that variable, i.e.

    (smie-indent-fixindent smie-indent-bob smie-indent-close
     smie-indent-comment smie-indent-comment-continue smie-indent-comment-close
     smie-indent-comment-inside smie-indent-inside-string smie-indent-keyword
     smie-indent-after-keyword smie-indent-empty-line smie-indent-exps)


-- Stefan

Re: Debugging SMIE for sh-script.el

[Stefan Monnier via Users list for the GNU Emacs text editor]

> This means that our line has been indented by the default logic of
> smie-indent-exps. If we go through it we'll see that it's trying to align
> with the first arg of sexp which it thinks is "/bar". So in effect it's
> treating your expression as if it was:
>
> ({foo} /bar
>
>            --baz \ --quux)

Indeed, the problem seems to be in `sh-smie-sh-forward-token` where it
should skip over the whole of "${foo}/bar".


        Stefan

Re: Debugging SMIE for sh-script.el

[Kévin Le Gouguec]

Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com> writes:

>                         maybe there's another function further down
> the chain which can, smie-indent-exps that is and that's where 6 comes
> from.

See my original message.  To paraphrase, with this shell buffer:

#+begin_src sh
${foo}/bar --baz \
      --quux
#+end_src

With point on "--quux", this yields 6:

#+begin_src elisp
(let ((sh-indent-after-continuation nil))
  (smie-indent-calculate))
#+end_src

- smie-indent-calculate runs all functions in smie-indent-functions,
- in a shell buffer, this variable only holds
  (sh-smie--indent-continuation t),
- but this yields nil:

#+begin_src elisp
(let ((sh-indent-after-continuation nil))
  (sh-smie--indent-continuation))
#+end_src

So what gives?  I've tried Edebugging, but couldn't figure out how to
stop at a spot that made things clearer.


PS: aaand I've just read now Stefan's replies, which kill two birds in
one stone explaining both this riddle, and what the trailing "t" means.
That's the thing I was missing.

Re: Debugging SMIE for sh-script.el

[Kévin Le Gouguec]

Stefan Monnier via Users list for the GNU Emacs text editor
<help-gnu-emacs@gnu.org> writes:

> Not that the `t` here is a general feature of hooks which means "use
> here the lit stored in the global (i.e. not buffer-local)" variable.

Ah-ha!  That's what I was missing.  I can't find this feature described
in the docstrings of the run-hook* functions or in add-hook's; have I
missed it?

I do find it mentioned in (info "(elisp) Running Hooks") though.  One
more coin for the "always read the info page" jar :)

Re: Debugging SMIE for sh-script.el

[Kévin Le Gouguec]

Stefan Monnier via Users list for the GNU Emacs text editor
<help-gnu-emacs@gnu.org> writes:

> Indeed, the problem seems to be in `sh-smie-sh-forward-token` where it
> should skip over the whole of "${foo}/bar".

Mmm, I see that with point on "--baz", (smie-backward-sexp) stops at
"/bar" instead of going all the way back to bol.

I'm tracking this down to sh-smie--default-backward-token, which makes
sense: this is where I tentatively added "()" in the call to
(skip-syntax-backward ".w_'") in 6392bc37a.

bug#50320 showed that this causes all paired delimiters (parentheses and
braces) to turn into tokens that smie-backward-sexp can jump to, so this
got misindented:

#+begin_src sh
foo ()
{
bar     # derp
}
#+end_src

If I understand correctly, the progn form in
sh-smie--default-backward-token…

1. skips over chars matching syntax classes ".w_'",
2. skips over backslashes.

I guess TRT would be to add a third step to check whether (char-before
(point)) is a closing delimiter, and if so, skip back to its matching
opener?  With (forward-sexp -1)?  And then assert whether we've hit a $
sign?

No idea whether this makes sense.  I don't mind implementing that and
seeing how it goes, but I'd like to make sure I'm not completely off the
mark…

Re: Debugging SMIE for sh-script.el

[Stefan Monnier via Users list for the GNU Emacs text editor]

> I guess TRT would be to add a third step to check whether (char-before
> (point)) is a closing delimiter, and if so, skip back to its matching
> opener?  With (forward-sexp -1)?  And then assert whether we've hit a $
> sign?

Could be but that could also be dangerous, since we don't want to treat
$(cat food) as a single token.  I think we'd want something a bit less generic.

> No idea whether this makes sense.

The only way to be sure is to think about what *should* be a "token".

Also, the ...-backward-token and ...-forward-token need to be kept in-sync.


        Stefan

Re: Debugging SMIE for sh-script.el

[Kévin Le Gouguec]

Stefan Monnier via Users list for the GNU Emacs text editor
<help-gnu-emacs@gnu.org> writes:

>> I guess TRT would be to add a third step to check whether (char-before
>> (point)) is a closing delimiter, and if so, skip back to its matching
>> opener?  With (forward-sexp -1)?  And then assert whether we've hit a $
>> sign?
>
> Could be but that could also be dangerous, since we don't want to treat
> $(cat food) as a single token.  I think we'd want something a bit less generic.

Not sure why we wouldn't want to treat $(cat food) as a single token?
Do you mean, as opposed to ${cat_food}?  IIUC, I'd like both $() and ${}
to be skipped over for the purposes of indentation:

#+begin_src sh
${top_dir}/foo --bar \
               --baz

$(git rev-parse --show-toplevel)/foo --bar \
                                     --baz
#+end_src

> Also, the ...-backward-token and ...-forward-token need to be kept in-sync.

Gotcha.

Re: Debugging SMIE for sh-script.el

[Stefan Monnier]

> Not sure why we wouldn't want to treat $(cat food) as a single token?

Because tokens are "the atoms" of indentation and I think if we start
to go down this road, it will lead to disappointments in other cases
like

   x="<<$(cat food;
          cat litter)>>"

> Do you mean, as opposed to ${cat_food}?

Yes.

> $(git rev-parse --show-toplevel)/foo --bar \
>                                      --baz

Maybe you're right.  Maybe it's OK to keep $(...) a single token when it
doesn't spread over multiple lines.  It'll prevent `smie-auto-fill` from
inserting a new line inside of it, but other than that it might work
well enough.


        Stefan

Re: Debugging SMIE for sh-script.el

[Nikolay Kudryavtsev]

The proper solution IMHO should be changing sh-smie--indent-continuation 
to not rely on other functions to calculate indent. Something along the 
lines of going (smie-backward-sexp) until you get (smie-rule-bolp), this 
would get you to the start of the parent line and then you can base your 
indentation decisions on it. It's generally a bad idea to touch the 
token grab code, unless you absolutely have to.

Also note, that sh-mode separates default-backward\forward-token 
functions from the actual backward\forward-token functions, with the 
latter handling whatever special cases there are and the default 
functions just doing straight buffer reading.

Re: Debugging SMIE for sh-script.el

[Stefan Monnier via Users list for the GNU Emacs text editor]

> The proper solution IMHO should be changing sh-smie--indent-continuation to
> not rely on other functions to calculate indent. Something along the lines
> of going (smie-backward-sexp) until you get (smie-rule-bolp), this would get
> you to the start of the parent line and then you can base your indentation
> decisions on it.

I'm not sure which "other functions" you're thinking of (obviously
`smie-backward-sexp` and `smie-rule-bolp` aren't included in those
"other functions" that shouldn't be used ;-).

Note that if you start with the loop you describe (which corresponds to
the algorithm used by `smie-indent-exps`) you'll sooner or later
hit cases where the code needs to be refined a bit, as at some point
I think you'll realize that you're reimplementing the whole indentation
code and that you're probably better off calling `smie-indent-calculate`
recursively and call it a day ;-)

> Also note, that sh-mode separates default-backward\forward-token functions
> from the actual backward\forward-token functions, with the latter handling
> whatever special cases there are and the default functions just doing
> straight buffer reading.

The default-backward\forward-token functions are only meant as examples
to help get started (they're often good enough during the first
half-hour of fiddling with a new grammar to get a feel for what might
work).  The actual backward\forward-token functions don't need to
use them or to look like them.


        Stefan

Re: Debugging SMIE for sh-script.el

[Nikolay Kudryavtsev]

> I'm not sure which "other functions" you're thinking of

I was talking about other smie-indent-functions. 
sh-smie--indent-continuation does a wacky thing in that it runs 
(smie-indent-calculate) again while excluding itself and then looks at 
the results and decides "ok, that's good enough".

Re: Debugging SMIE for sh-script.el

[Stefan Monnier via Users list for the GNU Emacs text editor]

> I was talking about other
> smie-indent-functions. sh-smie--indent-continuation does a wacky thing in
> that it runs (smie-indent-calculate) again while excluding itself and then
> looks at the results and decides "ok, that's good enough".

I agree it's a bit wacky (it basically tries to do what
a `add-function :around` would do, but `smie-indent-functions` is
a hook so we're stuck with `add-hook` instead of `add-function`).

But as I just explained if you try and avoid `smie-indent-calculate`
you'll end up reinventing (part of) it.


        Stefan

Re: Debugging SMIE for sh-script.el

[Nikolay Kudryavtsev]

I do understand that, but the default (smie-indent-calculate) cycle 
being inadequate for the current use case is where we started at. I'm 
just saying that since someone has already introduced a custom function 
(sh-smie--indent-continuation) for dealing with this particular use 
case, we might as well implement whatever proper logic is required 
within it.

Actually, I'm not even sure if introducing 
(sh-smie--indent-continuation) was warranted, since we could just have 
implemented a (:after . "\")  rule, but I'm probably missing some 
particular use case or caveat requiring it.

Re: Debugging SMIE for sh-script.el

[Stefan Monnier]

> I do understand that, but the default (smie-indent-calculate) cycle being
> inadequate for the current use case is where we started at.

I don't think so.

At the beginning the case for continuation lines was handled directly to
indent things like

    /usr/bin/emacs -l foo/bar.el   \
                   toto/titi.el

and

    cat food; /usr/bin/emacs -l foo/bar.el   \
                             toto/titi.el

If you remove `sh-smie--indent-continuation` you should see that indeed
the above lines are indented as shown.

Then some users came and said they didn't like this style and wanted
instead continuation lines to be indented without paying attention to
the structure of the code on the logical line (which is spread over
several lines) and just indent the subsequent lines at a fixed offset
from the first line, as in:

    /usr/bin/emacs -l foo/bar.el   \
        toto/titi.el

That's when `sh-smie--indent-continuation` was added (along with the
var `sh-indent-after-continuation`), IIRC.
Additionally, the original scheme (which pays attention to the structure
of the code) was refined to make sure that continuation lines are
indented at least a bit more than the first line,
i.e. instead of

    if grep foo    \
            files; \
    then bar;      \
    else baz;      \
    fi

we wanted to impose something like:

    if grep foo    \
            files; \
        then bar;  \
        else baz;  \
        fi

which is why `sh-smie--indent-continuation` calls
`smie-indent-calculate` first, then looks at the result and if it's less
deep than "baseline indent + offset" then it forces "baseline indent + offset".


        Stefan

Re: Debugging SMIE for sh-script.el

[Nikolay Kudryavtsev]

> indent the subsequent lines at a fixed offset from the first line
The key question here is how do we calculate that offset. Currently it 
is the offset to the start of the second token of the parent line. Kevin 
wants to change it, so that it would be the start of the first token of 
the parent non-continuation line + sh-basic-offset.

There are two sane ways of accomplishing that:

1. Implementing that logic inside sh-smie--indent-continuation. Just so 
we're clear, when I said "the default (smie-indent-calculation) cycle", 
I meant the default smie-indent-functions. There's generally no reason 
to introduce a new function (sh-smie--indent-continuation) into 
smie-indent-functions if the default ones are adequate for the 
job(indenting continuation lines), so that's why I've assumed that the 
default functions are not adequate for the general use case.

2. Getting rid of sh-smie--indent-continuation and implementing its 
logic and the new fixed logic within sh-smie-sh-rules. Maybe this can be 
done without opening any Pandora's boxes.

Re: Debugging SMIE for sh-script.el

[Stefan Monnier]

>> indent the subsequent lines at a fixed offset from the first line
> The key question here is how do we calculate that offset.

No.  The rule "indent the subsequent lines at a fixed offset from the
first line" is the one used when `sh-indent-after-continuation` is set
to `always`, which is a different case than the one under discussion.

> Currently it is the offset to the start of the second token of the
> parent line.

No.  This indentation is the one you get when we use the *other* rule
which tries to indent depending on the structure of the code on the
line.  E.g. you get that when `sh-indent-after-continuation` is nil.

> Kevin wants to change it, so that it would be the start
> of the first token of the parent non-continuation line +
> sh-basic-offset.

We're miscommunicating, then.

Those who want that just need

    (setq sh-indent-after-continuation 'always)

AFAIK this thread is about fixing the indentation from

    $(catfood)/foo bar \
              baz

to

    $(catfood)/foo bar \
                   baz


[ and AFAIK it's a plain bug-fix, not a change in style.  ]


        Stefan

Re: Debugging SMIE for sh-script.el

[Nikolay Kudryavtsev]

Oh, you're right. Seems that I haven't read Kevin's last letter 
carefully enough. Sorry about that.

I also don't understand the discussion of treating $(cat food) as a 
single token, since blocks with the standard openers/closers are 
supposed to be returned by the smie lexer as "" when it runs into them 
from the outside.

Anyway, I still think that even in this case, changing 
sh-smie--indent-continuation is a better idea than touching the token 
grab code. Just to test I've coded an implementation of it 
<https://gist.github.com/sg2002/43bbd454da3254f4d092e2758057c372>, that 
behaves correctly for Kevin's use case. Needs more testing, but seems to 
work ok in general.

Re: Debugging SMIE for sh-script.el

[Stefan Monnier]

> returned by the smie lexer as "" when it runs into them from the outside.

There's no such thing as "the smie lexer":
Every major mode uses the lexer it wants with the behavior it fancies.

If the lexer decides to return nil when bumping into a char with paren
or string syntax then SMIE will treat it as a sign that this should be
handled in the "normal" way.  The main benefit is that paired parens can
then be skipped with things like `forward-sexp` which is much faster
than letting SMIE do the lexing+parsing inside the pair.

But a lexer doesn't have to do that.  An example would be when the
parens are escaped, e.g, in ELisp, `foo\(toto\)bar` is a single symbol
so an SMIE lexer for ELisp should handle it as a single token.


        Stefan

Re: Debugging SMIE for sh-script.el

[Kévin Le Gouguec]

Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com> writes:

> Anyway, I still think that even in this case, changing
> sh-smie--indent-continuation is a better idea than touching the token
> grab code. Just to test I've coded an implementation of it
> <https://gist.github.com/sg2002/43bbd454da3254f4d092e2758057c372>,
> that behaves correctly for Kevin's use case. Needs more testing, but
> seems to work ok in general.

Thanks for taking a stab at fixing that use-case; if I am not mistaken
though, your implementation reverts Dario's good work from bug#44592:
the test case Lars added fails when I run make -C test sh-script-tests.

NB: I'm reporting this only for due diligence, since I'm the one who
caused you to spend time on this issue.  I don't want to waste more of
your time on it; the current state of affairs isn't overly horrible.

I've added this itch to my ever-growing pile; I might scratch it next
time I open a shell script buffer, if no-one beats me to it :)