bug#20625: 25.0.50; doc of `define-error` is incorrect

bug#20625: 25.0.50; doc of `define-error` is incorrect

[Drew Adams]

From `C-h f':

 (define-error NAME MESSAGE &optional PARENT)

 Define NAME as a new error signal.
 MESSAGE is a string that will be output to the echo area if such an error
 is signaled without being caught by a `condition-case'.
 PARENT is either a signal or a list of signals from which it inherits.
 Defaults to `error'.

No, MESSAGE is not necessarily a string. It is a required argument, but
the code explicitly makes use of the case where it is nil.  And existing
code delivered with Emacs (e.g. bookmark.el) makes use of a nil MESSAGE.
That case should be documented.

The doc in the manual has the same bug (node Error Symbols).  MESSAGE is
*not* necessarily a string.




In GNU Emacs 25.0.50.1 (i686-pc-mingw32)
 of 2014-10-20 on LEG570
Bzr revision: 118168 rgm@gnu.org-20141020195941-icp42t8ttcnud09g
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --enable-checking=yes,glyphs CPPFLAGS=-DGLYPH_DEBUG=1'

bug#20625: 25.0.50; doc of `define-error` is incorrect

[Eli Zaretskii]

> Date: Thu, 21 May 2015 14:04:31 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> 
> >From `C-h f':
> 
>  (define-error NAME MESSAGE &optional PARENT)
> 
>  Define NAME as a new error signal.
>  MESSAGE is a string that will be output to the echo area if such an error
>  is signaled without being caught by a `condition-case'.
>  PARENT is either a signal or a list of signals from which it inherits.
>  Defaults to `error'.
> 
> No, MESSAGE is not necessarily a string. It is a required argument, but
> the code explicitly makes use of the case where it is nil.  And existing
> code delivered with Emacs (e.g. bookmark.el) makes use of a nil MESSAGE.
> That case should be documented.

AFAICS, users of this property will yield strange messages if MESSAGE
is nil.  So shouldn't we at least advise that it's a string?

Also, what exactly is the purpose of bookmark.el's using nil there?

bug#20625: 25.0.50; doc of `define-error` is incorrect

[Lars Ingebrigtsen]

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

>>From `C-h f':
>
>  (define-error NAME MESSAGE &optional PARENT)
>
>  Define NAME as a new error signal.
>  MESSAGE is a string that will be output to the echo area if such an error
>  is signaled without being caught by a `condition-case'.
>  PARENT is either a signal or a list of signals from which it inherits.
>  Defaults to `error'.
>
> No, MESSAGE is not necessarily a string. It is a required argument, but
> the code explicitly makes use of the case where it is nil.  And existing
> code delivered with Emacs (e.g. bookmark.el) makes use of a nil MESSAGE.
> That case should be documented.

The function doesn't error out if MESSAGE isn't a string, but it still
shouldn't be.  So I don't think that's a bug.

Feel free to submit a new bug report for bookmark.el not supplying a
string.

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

bug#20625: 25.0.50; doc of `define-error` is incorrect

[Drew Adams]

> > No, MESSAGE is not necessarily a string. It is a required argument, but
> > the code explicitly makes use of the case where it is nil.  And existing
> > code delivered with Emacs (e.g. bookmark.el) makes use of a nil MESSAGE.
> > That case should be documented.
> 
> The function doesn't error out if MESSAGE isn't a string, but it still
> shouldn't be.  So I don't think that's a bug.
> 
> Feel free to submit a new bug report for bookmark.el not supplying a
> string.

I think it is too bad that `define-error', which was intended to
be a friendly wrapper around using `error-conditions' directly,
will no longer do all that the latter does.  But so be it.

Bug #23408 reported, for bookmark.el.  Users who wrote
similar code, expecting `define-error' to be like using
`error-conditions', will need to likewise change their
code.

bug#20625: 25.0.50; doc of `define-error` is incorrect

[Drew Adams]

> > MESSAGE is not necessarily a string. It is a requiredargument,
> > but the code explicitly makes use of the case where it is nil.
> > And existing code delivered with Emacs (e.g. bookmark.el) makes
> > use of a nil MESSAGE.  That case should be documented.
> 
> AFAICS, users of this property will yield strange messages if
> MESSAGE is nil.  So shouldn't we at least advise that it's a string?
> 
> Also, what exactly is the purpose of bookmark.el's using nil there?

I'm no expert in this, and I wasn't the one who changed bookmark.el
(in 24.4) to use `define-error'.

But it seems that what is meant is that `bookmark-errors' is being
declared to be an error condition, but with no definition: no
message and no parent (other than the default, `error').

This is the Emacs 24.3 code:

(put 'bookmark-error-no-filename
     'error-conditions
     '(error bookmark-errors bookmark-error-no-filename))
(put 'bookmark-error-no-filename
     'error-message
     "Bookmark has no associated file (or directory)")

And this is the 24.4+ code:

(define-error 'bookmark-errors nil)
(define-error 'bookmark-error-no-filename
  "Bookmark has no associated file (or directory)"
  'bookmark-errors)

In both cases, `bookmark-errors' is presumably being declared
as an error condition, but it is not defined in any way.  It
is presumably there to provide a more general category than
`bookmark-error-no-filename'.  That makes sense to me.

If my interpretation is correct, then something like that
explanation should be added to the doc, I think.  Before
`define-error', things were clear enough with the doc for
property `error-conditions'.  It seems that the doc for
`define-error' should cover the same cases.

I believe that means that with nil MESSAGE, property
`error-conditions' is applied (using MESSAGE), and property
`error-message' is not (absent or nil value).

In fact, I think that the right change would be not only
to say what nil MESSAGE means but to make MESSAGE an
&optional parameter.

bug#20625: 25.0.50; doc of `define-error` is incorrect

[Eli Zaretskii]

> Date: Sat, 23 May 2015 07:48:46 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 20625@debbugs.gnu.org
> 
> > > MESSAGE is not necessarily a string. It is a requiredargument,
> > > but the code explicitly makes use of the case where it is nil.
> > > And existing code delivered with Emacs (e.g. bookmark.el) makes
> > > use of a nil MESSAGE.  That case should be documented.
> > 
> > AFAICS, users of this property will yield strange messages if
> > MESSAGE is nil.  So shouldn't we at least advise that it's a string?
> > 
> > Also, what exactly is the purpose of bookmark.el's using nil there?
> 
> I'm no expert in this, and I wasn't the one who changed bookmark.el
> (in 24.4) to use `define-error'.
> 
> But it seems that what is meant is that `bookmark-errors' is being
> declared to be an error condition, but with no definition: no
> message and no parent (other than the default, `error').
> 
> This is the Emacs 24.3 code:
> 
> (put 'bookmark-error-no-filename
>      'error-conditions
>      '(error bookmark-errors bookmark-error-no-filename))
> (put 'bookmark-error-no-filename
>      'error-message
>      "Bookmark has no associated file (or directory)")
> 
> And this is the 24.4+ code:
> 
> (define-error 'bookmark-errors nil)
> (define-error 'bookmark-error-no-filename
>   "Bookmark has no associated file (or directory)"
>   'bookmark-errors)
> 
> In both cases, `bookmark-errors' is presumably being declared
> as an error condition, but it is not defined in any way.  It
> is presumably there to provide a more general category than
> `bookmark-error-no-filename'.  That makes sense to me.

I've grepped all the uses of 'error-message property in the Emacs
sources, and I only see uses that expect the value to be a string.

> If my interpretation is correct, then something like that
> explanation should be added to the doc, I think.  Before
> `define-error', things were clear enough with the doc for
> property `error-conditions'.  It seems that the doc for
> `define-error' should cover the same cases.
> 
> I believe that means that with nil MESSAGE, property
> `error-conditions' is applied (using MESSAGE), and property
> `error-message' is not (absent or nil value).
> 
> In fact, I think that the right change would be not only
> to say what nil MESSAGE means but to make MESSAGE an
> &optional parameter.

I cannot describe in the docs something I don't understand.  I hope
someone who will would either fix the docs or post here what it means
for that property to have a nil value, and then I can put that in the
docs.

bug#20625: 25.0.50; doc of `define-error` is incorrect

[Stefan Monnier]

> In fact, I think that the right change would be not only
> to say what nil MESSAGE means but to make MESSAGE an
> &optional parameter.

I disagree.  The bookmark.el code is just a historical accident.


        Stefan

bug#20625: 25.0.50; doc of `define-error` is incorrect

[Drew Adams]

> > In fact, I think that the right change would be not only
> > to say what nil MESSAGE means but to make MESSAGE an
> > &optional parameter.
> 
> I disagree.  The bookmark.el code is just a historical accident.

Then please remove it.  Either this is an intended feature
or it is not.  If you don't want `define-error` to provide
exactly the same thing that you can get by using property
`error-conditions' directly, then leave its definition and
doc as they are, but change the `bookmark.el' code back to
using `error-conditions' directly, so any code that expects
error condition `bookmark-errors' to be available will not
be surprised.  Or remove it from bookmark.el altogether,
if you don't care about such backward compatibility.  Or
provide a MESSAGE arg for the definition of `bookmark-errors'.

One way or another, the doc and behavior should match, and
bookmark.el should be made coherent with both.

bug#20625: 25.0.50; doc of `define-error` is incorrect

[Drew Adams]

> I've grepped all the uses of 'error-message property in the Emacs
> sources, and I only see uses that expect the value to be a string.

What can I say, beyond what I said?  I don't think that is really
relevant.  A nil value means (I think) precisely that an error
condition is declared that has no associated message string.  The
fact that no code makes use of such an error condition doesn't
mean anything significant, IMO.

IIUC, the use of `bookmark-errors' with a nil value in bookmark.el
is precisely the same as its use of `bookmark-errors' with
`error-condition' prior to Emacs 24.4.  It declares, even if it
does not define an error message for, an error condition that
is more general than `bookmark-error-no-filename'.

It tells programmers who might define their own bookmark errors
to have them, like `bookmark-error-no-filename', inherit from
condition `bookmark-errors'.  That seems reasonable to me.  But
of course it is not necessary - users can just have their new
bookmark errors inherit from `error'.

Now you could argue that the presence of `bookmark-errors' in
bookmark.el prior to Emacs 24.4 was a mistake.  That is the
same argument as saying that its presence now is a mistake.
But if you suppose that it is a good idea to predefine a general
error class for bookmark code, then I don't see why doing so
using `define-error' is any less meaningful/reasonable than
doing so using property `error-conditions' directly.

> I cannot describe in the docs something I don't understand.  I hope
> someone who will would either fix the docs or post here what it
> means for that property to have a nil value, and then I can put
> that in the docs.

I tried to explain the meaning, as I understand it.  Perhaps
the person who changed the Emacs 24.3 bookmark.el code to use
`define-error', or the person who introduced `define-error' to
Emacs 24.4, can confirm what I said or otherwise weigh in here.

Meanwhile, perhaps you could consider mentioning that the
MESSAGE value can be nil.  (This doc bug would remain open, as
long as what nil means is not described, but at least the type
of MESSAGE would not be incorrectly specified).  And perhaps
you could consider making parameter MESSAGE optional (since,
in effect, it is).