Re: master a7c65fc666: Allow nil value for filter-buffer-substring-function

Re: master a7c65fc666: Allow nil value for filter-buffer-substring-function

[Robert Pluim]

>>>>> On Mon, 19 Sep 2022 10:57:14 -0400 (EDT), Stefan Kangas <stefankangas@gmail.com> said:


    Stefan> diff --git a/lisp/simple.el b/lisp/simple.el
    Stefan> index 1b9bf9fa6d..40df5695c3 100644
    Stefan> --- a/lisp/simple.el
    Stefan> +++ b/lisp/simple.el
    Stefan> @@ -5363,7 +5363,10 @@ These commands include \\[set-mark-command] and \\[start-kbd-macro]."
    Stefan>  The function is called with the same 3 arguments (BEG END DELETE)
    Stefan>  that `filter-buffer-substring' received.  It should return the
    Stefan>  buffer substring between BEG and END, after filtering.  If DELETE is
    Stefan> -non-nil, it should delete the text between BEG and END from the buffer.")
    Stefan> +non-nil, it should delete the text between BEG and END from the buffer.
    Stefan> +
    Stefan> +The default value is `buffer-substring--filter', and nil means
    Stefan> +the same as the default.")

Ick. If `buffer-substring--filter' is the default function to call, it
should not have the '--' internal function marker.

Robert
-- 

Re: master a7c65fc666: Allow nil value for filter-buffer-substring-function

[Stefan Kangas]

Robert Pluim <rpluim@gmail.com> writes:

>     Stefan> +The default value is `buffer-substring--filter', and nil means
>     Stefan> +the same as the default.")
>
> Ick. If `buffer-substring--filter' is the default function to call, it
> should not have the '--' internal function marker.

AFAICT, it's been named that way since it was first introduced in 2013.

Is it worth renaming?

Re: master a7c65fc666: Allow nil value for filter-buffer-substring-function

[Stefan Monnier]

> Ick. If `buffer-substring--filter' is the default function to call, it
> should not have the '--' internal function marker.

Why?  You're *not* supposed to call `buffer-substring--filter'.
Instead you're supposed to call the value contained in
`filter-buffer-substring-function`, whichever it is.  (and if you're
modifying this variable, then you're supposed to modify it with
`add-function` and your function will then receive the "previous"
function to call).


        Stefan

Re: master a7c65fc666: Allow nil value for filter-buffer-substring-function

[Robert Pluim]

>>>>> On Mon, 19 Sep 2022 16:37:04 -0400, Stefan Monnier <monnier@iro.umontreal.ca> said:

    >> Ick. If `buffer-substring--filter' is the default function to call, it
    >> should not have the '--' internal function marker.

    Stefan> Why?  You're *not* supposed to call `buffer-substring--filter'.
    Stefan> Instead you're supposed to call the value contained in
    Stefan> `filter-buffer-substring-function`, whichever it is.  (and if you're
    Stefan> modifying this variable, then you're supposed to modify it with
    Stefan> `add-function` and your function will then receive the "previous"
    Stefan> function to call).

Itʼs not obvious to me that youʼre supposed to use `add-function'. And
even if it were, having '--' appear in the public api just feels
wrong. Oh well.

Robert
-- 

Re: master a7c65fc666: Allow nil value for filter-buffer-substring-function

[Stefan Monnier]

Robert Pluim [2022-09-20 10:08:19] wrote:
>>>>>> On Mon, 19 Sep 2022 16:37:04 -0400, Stefan Monnier <monnier@iro.umontreal.ca> said:
>
>     >> Ick. If `buffer-substring--filter' is the default function to call, it
>     >> should not have the '--' internal function marker.
>
>     Stefan> Why?  You're *not* supposed to call `buffer-substring--filter'.
>     Stefan> Instead you're supposed to call the value contained in
>     Stefan> `filter-buffer-substring-function`, whichever it is.  (and if you're
>     Stefan> modifying this variable, then you're supposed to modify it with
>     Stefan> `add-function` and your function will then receive the "previous"
>     Stefan> function to call).
>
> Itʼs not obvious to me that youʼre supposed to use `add-function'. And
> even if it were, having '--' appear in the public api just feels
> wrong. Oh well.

It's not the public API.
The public API is to call the value that's stored in the variable,
whatever that value is.

Look at it this way.  The choice is between:

    (defvar foo-function (lambda () ...blabla...))

vs

    (defvar foo-function #'foo--default)
    (defun foo--default ()
      ...blabla...)

In both cases the actual function is "internal".  The second version
just gives a name to the function because it's more convenient, hiding
a bit more of the internals, and making it possible to use `C-h o` to
jump to the definition of the function.

The "--" version makes it *possible* to call that function directly, but
it still *shouldn't* be called directly: it's still very much internal.


        Stefan

Re: master a7c65fc666: Allow nil value for filter-buffer-substring-function

[Stefan Monnier]

>     Allow nil value for filter-buffer-substring-function
>
>     * lisp/simple.el
>     (filter-buffer-substring): Support a nil value to be more resilient.
>     (filter-buffer-substring-function): Doc fix; improve and update for
>     above change.

Why?  It's been that way "forever" already and new code should not put
a nil in there since it'll break uses of `add-function`.


        Stefan

Re: master a7c65fc666: Allow nil value for filter-buffer-substring-function

[Stefan Kangas]

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

> >     * lisp/simple.el
> >     (filter-buffer-substring): Support a nil value to be more resilient.
> >     (filter-buffer-substring-function): Doc fix; improve and update for
> >     above change.
>
> Why?  It's been that way "forever" already and new code should not put
> a nil in there since it'll break uses of `add-function`.

This came up because I set 'filter-buffer-substring-function' to
something bad, and had a hard time figuring out what to set it back
to.  It would have helped if the default value was in the docstring,
but allowing nil would also have been useful.  So I did both those
changes.

Note that before this patch, you had to go to the variable's
definition to find out how to set things back.  But then, you just
broke killing by being a bad so you have to type it in manually.  nil
is just handy to allow.  (BTW, is it still true that some
distributions don't even install the .el files by default?)

I don't see a good reason not to accept nil, though we don't need to
document it if we don't want to encourage its (ab)use.

Re: master a7c65fc666: Allow nil value for filter-buffer-substring-function

[Stefan Monnier]

> I don't see a good reason not to accept nil, though we don't need to

As I said, it breaks `add-function`.  If the var's value is nil

    (add-function :before filter-buffer-substring-function #'ignore)

will lead to errors, even though it should be harmless (as well as
useless, of course).


        Stefan

Re: master a7c65fc666: Allow nil value for filter-buffer-substring-function

[Stefan Kangas]

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

> As I said, it breaks `add-function`.  If the var's value is nil
>
>     (add-function :before filter-buffer-substring-function #'ignore)
>
> will lead to errors, even though it should be harmless (as well as
> useless, of course).

OK, I'll revert the change.

RE: [External] : Re: master a7c65fc666: Allow nil value for filter-buffer-substring-function

[Drew Adams]

> > Allow nil value for filter-buffer-substring-function
> >	
> > * lisp/simple.el
> > (filter-buffer-substring): Support a nil value to be
> > more resilient.
> > (filter-buffer-substring-function): Doc fix; improve
> > and update for above change.
> 
> Why?  It's been that way "forever" already and new
> code should not put a nil in there since it'll break
> uses of `add-function`.

Not following this thread, and at the risk of
being booed or ignored, I'll chime in here...

"It's been that way 'forever'" is a legitimate
(but not strong) argument.  The rest is not, IMO.

I don't think that, in general, variables whose
names end in `-function' should be disallowed a
non-function value - in particular a value of nil.

Which values such a var can take shouldn't follow
some iron-clad rule.  They should be defined case
by case.  The point of the `-function' convention
is to indicate that the value can be, often is,
or typically is, a function, not to require that
it always be a function.

The argument that allowing a nil value for such
a variable "breaks" the use of `add-function' is
no more than a re-assertion that the variable
shouldn't have a nil value.  It's not a _reason_
why it shouldn't have a nil value.  It really
just repeats the claim.

And in particular, the use of function `ignore'
is not the same thing as the use of nil to mean
"NO function" or "NOT a function" - not at all.

No good argument exists for a blanket rule that
a var whose value can be a function shouldn't
also be allowed other, non-function values - in
particular the value nil.  At least we've seen
no good argument so far.

Arguments that such a proscription lets code
blindly apply the value as a function, or blindly
advise it, are misplaced, IMO.  One use case's
"handy" is another use case's "obstacle" or
"obfuscation".

(eq var 'ignore) or (eq var #'ignore) is more,
not less, problematic than (null var).

You can certainly use (when var (add-function...))
instead of just (add-function...).  Or instead of
`apply' or `funcall'.  Not a problem -- or if you
think it is then please show the problem.

I wrote this about the same question, in thread
"[PATCH] Let bookmark handlers do everything,
including display the destination", 2022-09-05:

___

 A `nil' value does matter - it's not the same
 as calling a function unconditionally, even
 when the function is `ignore'.

 E.g., my version of `bookmark-default-handler'
 has this code, which doesn't raise the frame if
 there's no display going on (a bookmark need
 not display anything):

 (when bmkp-jump-display-function
   (save-current-buffer
     (funcall bmkp-jump-display-function (current-buffer)))
   (raise-frame))

 Sure, I could change (when XXXX ...) to
 (when (eq XXXX 'ignore) ...), but that wouldn't
 really improve anything, IMO.

 And I test the display function in some handlers,
 for conditional handling/dispatch.  E.g., my
 handler for Dired bookmarks:

 (defun bmkp-jump-dired (bookmark)
   "..."
   (let ((dir
          (bookmark-prop-get bookmark 'dired-directory))
         (mfiles
          (bookmark-prop-get bookmark 'dired-marked))
         (switches
          (bookmark-prop-get bookmark 'dired-switches))
         (subdirs
          (bookmark-prop-get bookmark 'dired-subdirs))
         (hidden-dirs
          (bookmark-prop-get bookmark 'dired-hidden-dirs)))
     (case bmkp-jump-display-function
       ((nil bmkp--pop-to-buffer-same-window display-buffer)
        (dired dir switches))
       ((bmkp-select-buffer-other-window
         pop-to-buffer
         switch-to-buffer-other-window)
        (dired-other-window dir switches))
       (t (dired dir switches)))
     (let ((inhibit-read-only  t))
       (dired-insert-old-subdirs subdirs)
       (dired-mark-remembered 
        (mapcar (lexical-let ((dd  dir))
                  (lambda (mf)
                    (cons (expand-file-name mf dd) 42)))
                mfiles))
       (save-excursion
         (dolist (dir  hidden-dirs)
           (when (dired-goto-subdir dir)
             (dired-hide-subdir 1)))))
     (let ((pos  (bookmark-get-position bookmark)))
       (when pos (goto-char pos)))))

 Obviously, testing for functional equality isn't
 possible.  But testing a function symbol lets a
 particular kind of bookmark jump differently
 depending on current context/settings.  

 > > Indeed :-(

 Not so obvious to me.  Fine as a general guideline.
 Not clear that it's a real win (no loss) here.

Re: [External] : Re: master a7c65fc666: Allow nil value for filter-buffer-substring-function

[Michael Heerdegen]

Drew Adams <drew.adams@oracle.com> writes:

> The argument that allowing a nil value for such
> a variable "breaks" the use of `add-function' is
> no more than a re-assertion that the variable
> shouldn't have a nil value.

I don't think this is true.  You are missing an, or even the, important
point IMO.

> And in particular, the use of function `ignore'
> is not the same thing as the use of nil to mean
> "NO function" or "NOT a function" - not at all.
>
> No good argument exists for a blanket rule that
> a var whose value can be a function shouldn't
> also be allowed other, non-function values - in
> particular the value nil.  At least we've seen
> no good argument so far.

> (eq var 'ignore) or (eq var #'ignore) is more,
> not less, problematic than (null var).
>
> You can certainly use (when var (add-function...))
> instead of just (add-function...).  Or instead of
> `apply' or `funcall'.  Not a problem -- or if you
> think it is then please show the problem.

I don't think testing the value directly for equivalence to some given
value is not an important use case:

I think the idea behind this treatment is: we want to allow different
modes/features, plus the user and/or buffer local configurations etc, to
push their behavior changes into that variable at the same time (!),
namely by advising it, by wrapping different layers around the value.
That's only possible if the value is always guaranteed to be a function
- so this is a requirement per se to allow this kind of approach, not an
excuse to disallow non-function values.

In this scenario, plain (equal var some-value) tests are quite useless
because you don't know which modes and features or whatever already
installed their modifications into the value (changing it).

Michael.

Re: [External] : Re: master a7c65fc666: Allow nil value for filter-buffer-substring-function

[Emanuel Berg]

Michael Heerdegen wrote:

>> The argument that allowing a nil value for such a variable
>> "breaks" the use of `add-function' is no more than
>> a re-assertion that the variable shouldn't have
>> a nil value.
>
> I don't think this is true. You are missing an, or even the,
> important point IMO.

When fixing something breaks something else this implies
poor modularization.

But as for what to do, well obviously what it breaks must be
fixed as well and everything else connected to it in the
same way.

And interestingly, fixing such issues can be a, or even the,
method to achieve modularization.

-- 
underground experts united
https://dataswamp.org/~incal

RE: [External] : Re: master a7c65fc666: Allow nil value for filter-buffer-substring-function

[Drew Adams]

> > The argument that allowing a nil value for such
> > a variable "breaks" the use of `add-function' is
> > no more than a re-assertion that the variable
> > shouldn't have a nil value.
> 
> I don't think this is true.  You are missing an,
> or even the, important point IMO.

I don't think that point was given.  My point was
that an argument that nil should be disallowed for
variables whose names end in `-function' is wrong
(misguided).  It may be reasonable for variable
`filter-buffer-substring-function', and for some
other so-named variables.  But as a general rule
it's misguided, IMO.
 
> > And in particular, the use of function `ignore'
> > is not the same thing as the use of nil to mean
> > "NO function" or "NOT a function" - not at all.
> >
> > No good argument exists for a blanket rule that
                                ^^^^^^^^^^^^^^
> > a var whose value can be a function shouldn't
> > also be allowed other, non-function values - in
> > particular the value nil.  At least we've seen
> > no good argument so far.
> 
> > (eq var 'ignore) or (eq var #'ignore) is more,
> > not less, problematic than (null var).
> >
> > You can certainly use (when var (add-function...))
> > instead of just (add-function...).  Or instead of
> > `apply' or `funcall'.  Not a problem -- or if you
> > think it is then please show the problem.
> 
> I don't think testing the value directly for equivalence
> to some given value is not an important use case:

You don't think it's _not_ an important use case?
Or was that a typo?  I'm guessing the latter,
based on the rest of what you say.

In any case, it may not be important to you, and
it may not be general enough for you or for
everything.  But it can be an important use case
for some code - for some `-function' vars, IMO.

I see no reason that a convention for naming vars
`-function' should be limited to variables whose
value can _only_ be a function.

I'm speaking to the blanket assertion that a
variable whose name ends in `-function', per our
convention, must be expected to only have a
function as its value.  I'm not speaking about
`filter-buffer-substring-function' specifically.

I disagree that that's what this naming is, or
should be, about.  Certainly any so-named var
whose value _must_ always be a function should
have that called out in its doc, and preferably
the code should support/enforce that requirement.
(_Does_ the doc call it out for this var?)

But from there to proclaim that all vars with
names that end in `-function' must only have
function values, I think is inappropriate, and
unnecessary.

It's a naming convention - to help human readers.
It's not a programming declaration - something
that controls code (e.g. byte-compilation), and
it shouldn't be.  (We _could_ have a declaration
for that, if it's helpful/needed.  Or we can
just communicate such things to humans in a doc
string.)

> I think the idea behind this treatment is: we want to allow different
> modes/features, plus the user and/or buffer local configurations etc, to
> push their behavior changes into that variable at the same time (!),
                                   ^^^^^^^^^^^^^

If you mean only `filter-buffer-substring-function'
then I have no objection.  I wasn't speaking about
that variable, or any particular variable.  I was
speaking to a general "rule" about variables with
such a name suffix.

I see now that I could have been clearer about that.
But I did say:

  I don't think that, in general, variables whose
                      ^^^^^^^^^^
  names end in `-function' should be disallowed a
  non-function value - in particular a value of nil.

I was talking about what the naming convention, in
itself, should be expected to mean.  I don't think
it should be expected that all such vars must have
only function values.

> namely by advising it, by wrapping different layers around the value.
> That's only possible if the value is always guaranteed to be a function
> - so this is a requirement per se to allow this kind of approach, not an
> excuse to disallow non-function values.

If you mean that only for this variable, I have no
problem with it.  For instance, I depend on the
same kind of thing you mention, in my code, wrt
`isearch-filter-predicate'.

Of course some vars should be required to have only
function values. And they (too) can benefit from
applying the `-function' naming convention to them.
But they shouldn't "own" (appropriate, pwn) that
convention.

> In this scenario, plain (equal var some-value) tests are quite useless
> because you don't know which modes and features or whatever already
> installed their modifications into the value (changing it).

Of course.

Speaking of which - I think it's a mistake BTW for
function `advice--p' to be "internal".  It's fine
that its implementation might change.  But it's a
fine function for user code to use.  There's no real
alternative, other than directly using the code that
implements it.  (Likewise for `advice--car' etc.)

Re: [External] : Re: master a7c65fc666: Allow nil value for filter-buffer-substring-function

[Michael Heerdegen]

Drew Adams <drew.adams@oracle.com> writes:

> I don't think that point was given.  My point was
> that an argument that nil should be disallowed for
> variables whose names end in `-function' is wrong
> (misguided).  It may be reasonable for variable
> `filter-buffer-substring-function', and for some
> other so-named variables.  But as a general rule
> it's misguided, IMO.

Is that written somewhere, or did only say someone that we have that
rule, or people should, if possible, follow that rule?

In (info "(elisp) Coding Conventions") I read

   • If the purpose of a variable is to store a single function, give it
     a name that ends in ‘-function’.  If the purpose of a variable is
     to store a list of functions (i.e., the variable is a hook), please
     follow the naming conventions for hooks.  *Note Hooks::.

It doesn't tell anything about the other direction (name -> restriction
to certain values).

In the Emacs Elisp sources I find over hundred `defvar's of symbols
named "*-function" that default to nil.  Doesn't seem to be a very
strict thing.

Maybe a good rule would be "[now that we have nadvice] please think about
whether someone or somecode might want to advice that binding before
allowing arbitrary values for *-function named variables".  Maybe it
would a good idea to spell that hint out in the manual if it isn't yet
(didn't check).

> > I don't think testing the value directly for equivalence
> > to some given value is not an important use case:
>
> You don't think it's _not_ an important use case?
> Or was that a typo?  I'm guessing the latter,
> based on the rest of what you say.

Yes, thanks for the correction.

> In any case, it may not be important to you, and
> it may not be general enough for you or for
> everything.  But it can be an important use case
> for some code - for some `-function' vars, IMO.
>
> I see no reason that a convention for naming vars
> `-function' should be limited to variables whose
> value can _only_ be a function.

I agree it should not be a strict must.  More a guideline to follow when
no reasons speak against it.  There are cases where other value types
and a *-function name fit.

> (_Does_ the doc call it out for this var?)

Yes.  Implicitly, but clearly.

> Speaking of which - I think it's a mistake BTW for
> function `advice--p' to be "internal".  It's fine
> that its implementation might change.  But it's a
> fine function for user code to use.  There's no real
> alternative, other than directly using the code that
> implements it.  (Likewise for `advice--car' etc.)

Dunno if `advice--p' has non-internal use cases.  We have some related
"public" functions in nadvice that might suffice: `advice-mapc' and
`advice-member-p'.

Michael.

RE: [External] : Re: master a7c65fc666: Allow nil value for filter-buffer-substring-function

[Drew Adams]

> > I don't think that point was given.  My point was
> > that an argument that nil should be disallowed for
> > variables whose names end in `-function' is wrong
> > (misguided).  It may be reasonable for variable
> > `filter-buffer-substring-function', and for some
> > other so-named variables.  But as a general rule
> > it's misguided, IMO.
> 	
> Is that written somewhere, or did only say someone that we have that
> rule, or people should, if possible, follow that rule?

I don't know of any rule/convention that says
that vars named `*-function' must never have
a non-function value.  (I would argue against
any such rule, in any case.)

> In (info "(elisp) Coding Conventions") I read
> 
>    • If the purpose of a variable is to store a single function, give it
>      a name that ends in ‘-function’.  If the purpose of a variable is
>      to store a list of functions (i.e., the variable is a hook), please
>      follow the naming conventions for hooks.  *Note Hooks::.
> 
> It doesn't tell anything about the other direction (name -> restriction
> to certain values).

Right.

> In the Emacs Elisp sources I find over hundred `defvar's of symbols
> named "*-function" that default to nil.  Doesn't seem to be a very
> strict thing.

Right.

That's maybe one reason to not now claim a rule
against that.  On the other hand, that doesn't
mean that maybe some of those vars shouldn't
have `ignore' as their default value - nothing
wrong with that.

My points are (1) nil is at least sometimes not
an unreasonable value, (2) there shouldn't be,
at least just with this naming convention, any
restriction to have only a function value.

> Maybe a good rule would be "[now that we have nadvice] please think about
> whether someone or somecode might want to advice that binding before
> allowing arbitrary values for *-function named variables".  Maybe it
> would a good idea to spell that hint out in the manual if it isn't yet
> (didn't check).

I'm OK with that.  Except NOT "for *-function
named variables".  IMO the advice to users here
should be to advise them to either try (by code)
to control the value, to not let it be a
non-function, or to document that the value
_needs_ to be a function.

And this is the case _regardless_ of the var
name.  Don't ever rely on a variable name for
something as important as a need for the value
to satisfy some property.  At least _document_
that need clearly.  And maybe try to code some
control/check, to enforce that need.

> > In any case, it may not be important to you, and
> > it may not be general enough for you or for
> > everything.  But it can be an important use case
> > for some code - for some `-function' vars, IMO.
> >
> > I see no reason that a convention for naming vars
> > `-function' should be limited to variables whose
> > value can _only_ be a function.
> 
> I agree it should not be a strict must.  More a guideline to follow when
> no reasons speak against it.  There are cases where other value types
> and a *-function name fit.

We shouldn't count on the variable name for a
lot.  If the value needs to satisfy a condition
(being a function is just one such possibility)
then that condition should be spelled out in the
doc, and maybe some code should try to enforce it.

BTW, this is one reason that I've argued in the
past for the ability to add a :type declaration
to a defvar, and not just to a defcustom.

A :type declaration gives you a lot of control
over the value, and it communicates the kind of
value expected/needed to users/coders.

I've also argued for better, tighter :type specs.
Both in vanilla Emacs code, if needed, and as
advice/recommendation for users.  You can do a
lot with a `restricted-sexp' type spec.

Unfortunately, both my proposal/patch to allow
:type etc. for defvar and my recommendation to
not discourage `restricted-sexp' were summarily
dismissed.  But give it another decade or two...

> > Speaking of which - I think it's a mistake BTW for
> > function `advice--p' to be "internal".  It's fine
> > that its implementation might change.  But it's a
> > fine function for user code to use.  There's no real
> > alternative, other than directly using the code that
> > implements it.  (Likewise for `advice--car' etc.)
> 
> Dunno if `advice--p' has non-internal use cases.  We have some related
> "public" functions in nadvice that might suffice: `advice-mapc' and
> `advice-member-p'.

I use it in two of my libraries, help-fns+.el
and isearch+.el.  Likewise other `advice--'
functions.  Those are extensions of standard
libraries help-fns.el and isearch.el.  But
isearch.el doesn't use such functions at all.

isearch+.el lets users interactively add &
remove advice to `isearch-filter-predicate':

https://www.emacswiki.org/emacs/DynamicIsearchFiltering

There's little, if any, reason for Emacs to
declare this or that Lisp code as "internal".

Why?  Because Free Software.

It's fine to let Emacs users know that
function XYZ might be more inclined to be
modified in the future than most functions.

There's no good reason to do that by way of
a naming convention.  And in particular, it's
unproductive to label XYZ as "internal".
(The use of the "--" convention is relatively
recent.  It's an unnecessary obstacle.)

Just one opinion.

Re: [External] : Re: master a7c65fc666: Allow nil value for filter-buffer-substring-function

[Stefan Monnier]

> Maybe a good rule would be "[now that we have nadvice] please think about
> whether someone or somecode might want to advice that binding before
> allowing arbitrary values for *-function named variables".  Maybe it
> would a good idea to spell that hint out in the manual

Agreed.

> if it isn't yet (didn't check).

I don't think it is, no.


        Stefan