bug#35771: [PATCH] Customization type of recentf-max-saved-items

[Basil L. Contovounesios]

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

>> I don't know whether this has been discussed before, but it seems more
>> suited for emacs-devel or its own bug report, rather than the current
>> one.
>
> Well, it certainly also applies to this bug report, I think,
> which is purportedly about the "Customization _type_ of
> recentf-max-saved-items".

Sure, but it applies also to all other user options of a similar nature,
and concerns a potential change in general Elisp conventions, so I would
rather it were discussed on its own and with other people included, and
any conclusions reported as wishlists and/or documented.

>> >> The customization type of recentf-max-saved-items is currently defined
>> >> as integer, which does not include nil in its domain.  However, setting
>> >> this variable to nil is supported in the code and also documented.
>> >>
>> >> This patch changes the customization type to explicitly allow for the
>> >> variable to be set to nil through the Customization interface and
>> >> similar.  (Please note that I copied the type from save-place-limit in
>> >> order to be consistent.)
>> >
>> > (I'm looking at Emacs 26.2, so apologies if the Emacs 27
>> > code has already fixed this.)
>> >
>> > The code should also be changed to do one of the following:
>> >
>> > 1. Use `abs' when the variable value is used.
>> 
>> I disagree.  It does not make sense for a size to be set to a negative
>> number without indication that this is a supported value; it is clearly
>> a user error to do so.  Silently interpreting negative numbers as their
>> absolute value further restricts any future modifications to the
>> interpretation of this user option.  The programmer should neither be
>> punished for such user errors, nor have to spoon-feed the user.
>> 
>> If there is ambiguity as to whether an integral user option can take a
>> negative value, the simplest and IMO best solution is to make the
>> documentation clearer, not to try to outsmart the user.
>
> I agree that #1 is not the best way to go (#2 is).  But #1
> is certainly better than allowing a negative value to 
> percolate through the code.

No-one is letting a negative value percolate through the code.

> (Not that a negative value would be disastrous in this case.  For this
> particular bug it's not a big deal.  But see, again, the Subject line
> - why not fix it right?

This bug report is about fixing the custom :type to include nil as an
accepted value, which the submitted patch fixes in a way suitable for
inclusion in emacs-26.  I would rather we dealt with one issue at a
time.

>> > 2. Use `restricted-sexp' in the defcustom :type, to require
>> >    the value to be a non-negative (or possibly a positive?)
>> >    integer (or nil).
>> >
>> > I'm guessing there are additional places in Emacs code
>> > where :type 'integer is used but a non-negative integer is
>> > really needed/appropriate.  It would be good to improve
>> > those :type specs as well.
>> >
>> > (We might also want to consider adding `natnum' or
>> > `nonnegative-integer', `positive-integer' and
>> > `negative-integer' as possible :type values.)
>> 
>> I'd welcome a natnum type.
>> 
>> > But it is simple to use `restricted-sexp' to control such
>> > things.  And not only would that improve the behavior for
>> > users; it would also, by way of model, encourage users to
>> > use `restricted-sexp' in their own code.
>> 
>> I don't see why restricted-sexp should be encouraged.  It is far simpler
>> to use and harder to abuse combinations of predefined simple types.
>> Besides, not everyone uses the Customize interface.
>
> There is no alternative, when the type you want to express
> is not available as any "combination of predefined simple
> types".

Hence my welcoming of a new simple type natnum.  But in this case there
is nothing wrong with using the integer type.

> Use of `restricted-sexp' should be encouraged when it's
> _needed_, and that's when the type is not currently as
> restrictive as it should be AND there is no simpler way
> to define the more accurate type.
>
> That's the point.  What we have today is not people
> avoiding/discouraging use of `restricted-sexp' in
> favor of just-as-useful, just-as-accurate, but simpler
> :type specs.  We have people not using `restricted-sexp'
> out of ignorance of it, or perhaps out of laziness.
> (That's my guess, until convinced otherwise.)

FWIW, I am neither ignorant of it, nor too lazy to use it; rather, in my
limited experience, I have yet to author or review a case where it is
truly "needed", this report included.

Existing precedent says the integer type is fine even when dealing with
nonnegative sizes.  If you prefer to use a more accurate natnum type in
these cases, which I sympathise with, then please submit a feature
request for this, if one does not already exist.  I think it is wrong to
start using restricted-sexp to emulate a natnum type in an ad hoc way.

> As for "not everyone uses Customize" - that's irrelevant
> here.  This is about those who do use it, obviously.
> It's about the :type spec of a defcustom.

It is not irrelevant.  First, authors cannot rely on the custom :type
alone to tell users what qualifies as an expected value; the docstring
must also contain this information.  This encourages writing better
docstrings and is how users may know not to set recentf-max-saved-items
to a negative number, regardless of whether its custom :type is integer
or natnum.  Emacs customisations have worked fine like this until now.

Second, application code must work correctly regardless of the custom
:type.  Since Elisp is not a strongly typed language, the programmer can
only assume that the user has understood the docstring and hasn't set
the user option to a silly value.

> More accurate defcustoms, using more appropriate :type
> specs, and sometimes using :set etc., is something we
> should encourage.  Customize and defcustoms could use
> more love by Emacs developers, not less.

As long as "more love" means smarter, not more, use, then I agree.

>> > Emacs-Lisp code delivered with Emacs is not a great model
>> > in this respect.  It rarely uses `restricted-sexp' - at
>> > least it uses it much less than it could/should (IMHO).
>> 
>> IMO, that's one of the many things that makes Emacs a great and fun
>> model - the freedom from having to fight a (easily badly specified) type
>> system.  Custom types should be as simple and declarative as possible.
>> Anything else should be reserved for exceptional cases.
>
> No idea what you're saying there.  On the face of it
> it sounds like an argument for using only :type 'sexp,
> or perhaps an argument for not using defcustom at all.
> I think we probably disagree about 90% here (but I
> would glad to learn that I'm wrong about that guess).

I meant neither of those things.  What I meant is KISS: a good-enough
but simple custom :type is far better than a more accurate but more
complicated one, especially in the context of something as trivial as a
natnum.  There is no need to encourage complicated (and very easily
buggy) logic in defcustoms when a simpler solution will do.

> Using an accurate :type spec doesn't limit/hurt users.
> It helps them.  Likewise, using a widget edit field
> that provides completion when appropriate etc.

Agreed.

>> > More generally, the distributed Emacs code is pretty
>> > "lazy" when it comes to providing defcustom definitions -
>> > few :tag descriptions, overly general :type specs, etc.
>> >
>> > E.g.,
>> >
>> > (defcustom recentf-max-saved-items 20
>> >   "Maximum number of recently used file names to save.
>> > `nil' means save the whole list.
>> > See command `recentf-save-list'."
>> >   :group 'recentf
>> >   :type '(choice
>> >           (restricted-sexp
>> >            :tag "Save no more than this many file names"
>> >            :match-alternatives
>> >            ((lambda (x) (and (natnump x) (not (zerop x)))))
>> >            :value ignore)
>> >           (const :tag "Save all file names" nil)))
>> 
>> FWIW, my vote is against both trying to overspecify custom types, and
>> using restricted-sexp for such simple examples.  If a particular type
>> such as natnum keeps cropping up, TRT is to add that type, not emulate
>> and duplicate it each time as a restricted-sexp.  If you agree, and
>> there is no existing bug report for this, please submit one.
>
> "Overspecify"?  "Trying to overspecify"?  Please elaborate.
> The value should be a natural number (or perhaps a positive
> integer), no?  How is specifying that exactly overspecifying?
> Specifying `integer' is underspecifying.  You have that
> exactly backward.

No, I'm voting against the general notion of trying to specify more than
is necessary, just because we can.

> Why shouldn't users be helped to provide the most appropriate
> values?  Why did you say you would welcome a :type of `natnum',
> if you argue for unrestricted typing?  Why prefer using
> `natnum' to `integer' - or even to `sexp' - for such a value,
> in that case?

I welcome a natnum type because it is simple, informative, and
declarative and can be used universally.  Elisp already has unrestricted
typing, so I don't pretend otherwise.

> I don't object to adding a `natnum' :type - I suggested it.
> But I also don't have a problem with expressing the same
> thing even if we don't have such a type.  I think it's silly
> to _not_ specify such behavior, and to just use `integer' (or
> `sexp') simply because we don't have a `natnum'.  That makes
> no sense to me.

Then please submit a report for that, if one does not already exist.

> Do I think we should use `restricted-sexp' even when a
> simpler type spec is available to accomplish the same
> thing?  Of course not.

Agreed.

Just one opinion,

-- 
Basil