Lifting all buffer restrictions in indentation functions

Lifting all buffer restrictions in indentation functions

[Eli Zaretskii]

Hi, Emacs.

The issue I raise here is a spin-off from discussing the changes on
the widen-less branch, but I think it is largely independent and has
more broad effect, so IMO it should be discussed separately.

The widen-less branch proposes to have indent-according-to-mode,
indent-for-tab-command, and indent-region call 'widen' before calling
indent-line-function.  This call is unconditional, so for example the
following

  (save-excursion
    (narrow-to-region START END)
    (indent-for-tab-command))

will not do what the caller expects, because indent-line-function will
not run restricted to the region bounds, but will instead be able to
access the whole buffer.

The rationale for this change seems to be twofold:

  . it is TRT for indentation operations
  . MMM and similar features need that, and will apply the restriction
    as appropriate before calling the mode-specific indentation
    function

I'm worried by widening unconditionally, because some strange mode
could need to run its indentation function restricted, and the
indentation function itself might not have enough context to narrow by
itself.

IOW, widening unconditionally seems to invalidate potentially
legitimate uses of this functionality, so I wonder whether we should
have some "fire escape", or maybe condition this widening only on MMM
and similar modes being active.

I'd like to hear opinions about this.  If I'm the only one who is
bothered by this, then I will defer to Stefan's and Dmitry's opinions.

TIA

Re: Lifting all buffer restrictions in indentation functions

[Stefan Monnier]

>   (save-excursion
          ^^^^^^^^^
          restriction
>     (narrow-to-region START END)
>     (indent-for-tab-command))

While such code might exist somewhere [tho most likely calling
indent-according-to-mode instead], the reasons why I think
breaking such code is the right thing are:
- it's very rare
- it should be easy to change this code so it works with the new (and
  the old) convention, by replacing (indent-according-to-mode) with
  (funcall indent-line-function).
- the reverse problem (i.e. where the restriction is currently obeyed
  while it shouldn't) is much more common.

IOW the change could introduce breakage, but in my opinion, it will
correct many more situations than it breaks, and those that it breaks
are easy to fix.


        Stefan

Re: Lifting all buffer restrictions in indentation functions

[Robert Weiner]

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

On Fri, Dec 8, 2017 at 11:51 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> Hi, Emacs.
>
> The widen-less branch proposes to have indent-according-to-mode,
> indent-for-tab-command, and indent-region call 'widen' before calling
> indent-line-function.  This call is unconditional,


​You don't say in your message but I would assume that
a save-restriction is used automatically within these
tab commands so that after the indent command, the
restriction is restored and there is no need for callers
to wrap indent functions within a save-restriction.

So after these two calls:

> ​​
>
> ​​
>     (narrow-to-region START END)
> ​​
>     (indent-for-tab-command)


​the narrowed region would be nearly the same (except
for changes made by the tab command).  Is that correct?

If that is the case, then this seems much less problematic
than if it is not and I would defer to Stefan's thinking.

Bob

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

Re: Lifting all buffer restrictions in indentation functions

[Stefan Monnier]

> ​You don't say in your message but I would assume that
> a save-restriction is used automatically within these
> tab commands so that after the indent command, the
> restriction is restored and there is no need for callers
> to wrap indent functions within a save-restriction.

Yes, of course.


        Stefan

Re: Lifting all buffer restrictions in indentation functions

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 08 Dec 2017 12:03:45 -0500
> 
> While such code might exist somewhere [tho most likely calling
> indent-according-to-mode instead], the reasons why I think
> breaking such code is the right thing are:
> - it's very rare
> - it should be easy to change this code so it works with the new (and
>   the old) convention, by replacing (indent-according-to-mode) with
>   (funcall indent-line-function).
> - the reverse problem (i.e. where the restriction is currently obeyed
>   while it shouldn't) is much more common.
> 
> IOW the change could introduce breakage, but in my opinion, it will
> correct many more situations than it breaks, and those that it breaks
> are easy to fix.

To clarify: I'm not only afraid of breaking some existing code, I'm
also worried by disabling any _future_ code which could call these
functions in a narrowed buffer.  This is AFAIU a legitimate usage, so
unconditionally disallowing it sounds gross.

And I don't think calling indent-line-function directly instead is the
solution, because indent-according-to-mode, indent-for-tab-command
etc. are not just trivial wrappers around the call to
indent-line-function, they have additional functionality (otherwise
why do we have them at all?).

Re: Lifting all buffer restrictions in indentation functions

[Stefan Monnier]

> And I don't think calling indent-line-function directly instead is the
> solution, because indent-according-to-mode, indent-for-tab-command
> etc. are not just trivial wrappers around the call to
> indent-line-function, they have additional functionality (otherwise
> why do we have them at all?).

I can't imagine a scenario where you'd use indent-for-tab-command in
that context.  As for indent-according-to-mode, its only difference with
calling indent-line-function is for pseudo-indentation functions
(indent-relative and friends) where it has an ad-hoc hack to "try and do
the right thing", but it's far from clear what would be The Right Thing
to do in such a narrowed scenario anyway (call indent-line-function
directly, use the ad-hoc hack, or do yet something else).

Most cases of calling indent-line-function or indent-according-to-mode
when indent-line-function is one of those pseudo-indentation functions
don't really do the right thing anyway.

So it sounds highly hypothetical, and it's not clear if would be more
often right or more often wrong in those hypothetical cases.


        Stefan

Re: Lifting all buffer restrictions in indentation functions

[Stephen Leake]

Eli Zaretskii <eliz@gnu.org> writes:

> Hi, Emacs.
>
> The issue I raise here is a spin-off from discussing the changes on
> the widen-less branch, but I think it is largely independent and has
> more broad effect, so IMO it should be discussed separately.
>
> The widen-less branch proposes to have indent-according-to-mode,
> indent-for-tab-command, and indent-region call 'widen' before calling
> indent-line-function.  This call is unconditional, so for example the
> following
>
>   (save-excursion
>     (narrow-to-region START END)
>     (indent-for-tab-command))
>
> will not do what the caller expects, because indent-line-function will
> not run restricted to the region bounds, but will instead be able to
> access the whole buffer.
>
> The rationale for this change seems to be twofold:
>
>   . it is TRT for indentation operations
>   . MMM and similar features need that, and will apply the restriction
>     as appropriate before calling the mode-specific indentation
>     function
>
> I'm worried by widening unconditionally, because some strange mode
> could need to run its indentation function restricted, and the
> indentation function itself might not have enough context to narrow by
> itself.
>
> IOW, widening unconditionally seems to invalidate potentially
> legitimate uses of this functionality, so I wonder whether we should
> have some "fire escape", or maybe condition this widening only on MMM
> and similar modes being active.
>
> I'd like to hear opinions about this.  If I'm the only one who is
> bothered by this, then I will defer to Stefan's and Dmitry's opinions.
>
> TIA

As I understand it, what MMM will do in these functions is change the
buffer bounds to the current chunk.

Since MMM needs to change the buffer bounds, why can't it just disregard
the current bounds, instead of relying on the caller to widen first?

-- 
-- Stephe

Re: Lifting all buffer restrictions in indentation functions

[Dmitry Gutov]

On 12/9/17 12:40 AM, Stephen Leake wrote:

> Since MMM needs to change the buffer bounds, why can't it just disregard
> the current bounds, instead of relying on the caller to widen first?

I does. But it'll have to rely on indent-line-function, subsequently, 
not calling widen.

So some other piece of code that runs earlier, has to call widen 
instead. That's not for MMM's benefit, but for the common user who 
narrows interactively.

Re: Lifting all buffer restrictions in indentation functions

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 08 Dec 2017 15:47:44 -0500
> 
> So it sounds highly hypothetical, and it's not clear if would be more
> often right or more often wrong in those hypothetical cases.

It _is_ hypothetical.  The question that bothers me is specifically
whether we should flatly disallow those hypothetical cases, just
because we "cannot imagine" them.  My experience with Emacs
development is that everything I thought was unimaginable someone
somewhere imagined it and wanted it to be possible, as long as that
made sense in their particular application.

Re: Lifting all buffer restrictions in indentation functions

[Stefan Monnier]

> It _is_ hypothetical.  The question that bothers me is specifically
> whether we should flatly disallow those hypothetical cases, just
> because we "cannot imagine" them.

They can still call indent-line-function, and the extra work that
indent-according-to-mode does isn't that much, so if it's *really*
indispensable to do "all that indent-according-to-mode does except
widen", then you can do it with something like

    (let* ((ilf indent-line-function)
           (res (list (point-min) (point-max)))
           (indent-line-function
            (lambda (&rest args)
              (apply #'narrow-to-region res)
              (apply ilf args))))
      (indent-according-to-mode))


-- Stefan