GNU bug report logs - #31715
cl-incf and cl-decf error out when passed a nil-valued variable as 'offset'

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 31715 in the body.
You can then email your comments to 31715 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
To: bug-gnu-emacs <at> gnu.org
Subject: cl-incf and cl-decf error out when passed a nil-valued variable as
 'offset'
Date: Mon, 4 Jun 2018 16:06:55 -0400

[Message part 1 (text/plain, inline)]

Hi all,

The following works:
  (let ((x 1)) (cl-incf x nil))

… but following raises "setq: Wrong type argument: number-or-marker-p, nil":
  (let ((x 1) (y nil)) (cl-incf x y))

… yet the docs say this, which suggests that both should work:
  (cl-incf PLACE &optional X)
  Increment PLACE by X (1 by default).

The issue comes from the expansion of cl-incf:

    (defmacro cl-incf (place &optional x) …
      (if (symbolp place)
          (list 'setq place (if x (list '+ place x) (list '1+ place)))
        (list 'cl-callf '+ place (or x 1))))

Shouldn't that `if x' check be quoted?  Same for the second branch of the if (shouldn't the `(or x 1)' part be quoted, too?)

cl-decf has the same issue.  Am I missing something?

Clément.

[signature.asc (application/pgp-signature, attachment)]

Message #8 received at 31715 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
Cc: 31715 <at> debbugs.gnu.org
Subject: Re: bug#31715: cl-incf and cl-decf error out when passed a nil-valued
 variable as 'offset'
Date: Mon, 04 Jun 2018 18:58:01 -0400

Clément Pit-Claudel <clement.pitclaudel <at> live.com> writes:

> The following works:
>   (let ((x 1)) (cl-incf x nil))
>
> … but following raises "setq: Wrong type argument: number-or-marker-p, nil":
>   (let ((x 1) (y nil)) (cl-incf x y))
>
> … yet the docs say this, which suggests that both should work:
>   (cl-incf PLACE &optional X)
>   Increment PLACE by X (1 by default).

X is an optional macro parameter, so the "optionalness" applies at
compile time.

> The issue comes from the expansion of cl-incf:
>
>     (defmacro cl-incf (place &optional x) …
>       (if (symbolp place)
>           (list 'setq place (if x (list '+ place x) (list '1+ place)))
>         (list 'cl-callf '+ place (or x 1))))
>
> Shouldn't that `if x' check be quoted?  Same for the second branch of the if (shouldn't the `(or x 1)' part be quoted, too?)

I think that would approximately double the cost of cl-incf in the
simple case.  And since you would expect cl-incf to be used in loops a
lot, that seems like a bad idea.

Message #11 received at 31715 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: Clément Pit-Claudel <clement.pitclaudel <at> live.com>,
 31715 <at> debbugs.gnu.org
Subject: Re: bug#31715: cl-incf and cl-decf error out when passed a nil-valued
 variable as 'offset'
Date: Tue, 05 Jun 2018 01:43:21 +0200

Noam Postavsky <npostavs <at> gmail.com> writes:

> X is an optional macro parameter, so the "optionalness" applies at
> compile time.

Are you sure we always treat optional macro parameters like this?

> I think that would approximately double the cost of cl-incf in the
> simple case [...]

We could drop the optimization in case an X expression is specified,
resulting in one additional `or' call:

(defmacro cl-incf (place &optional x)
  (if (and (symbolp place) (not x))
      (list 'setq place (list '1+ place))
    (list 'cl-callf '+ place (or x 1))))

Would that be significantly slower than the current definition?


Michael.

Message #14 received at 31715 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Clément Pit-Claudel <clement.pitclaudel <at> live.com>,
 31715 <at> debbugs.gnu.org
Subject: Re: bug#31715: cl-incf and cl-decf error out when passed a nil-valued
 variable as 'offset'
Date: Mon, 04 Jun 2018 20:12:33 -0400

Michael Heerdegen <michael_heerdegen <at> web.de> writes:

> Noam Postavsky <npostavs <at> gmail.com> writes:
>
>> X is an optional macro parameter, so the "optionalness" applies at
>> compile time.
>
> Are you sure we always treat optional macro parameters like this?

Do DOCSTRING and DECL count?

    (defun NAME ARGLIST &optional DOCSTRING DECL &rest BODY)

Since they're obviously not evaluated, it doesn't quite feel the same.
I couldn't find any other cases of optional parameters in core macros.

There is the special form defvar's INITVALUE, but that is treated even
more specially, since it distinguishes nil from omitted.

>> I think that would approximately double the cost of cl-incf in the
>> simple case [...]

Actually, checking again, I seem to be wrong about this; the compiler
knows to elide the extra test if the increment is a constant expression,
so it would only add runtime when incrementing by a computed amount.

Message #17 received at 31715 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: Clément Pit-Claudel <clement.pitclaudel <at> live.com>,
 31715 <at> debbugs.gnu.org
Subject: Re: bug#31715: cl-incf and cl-decf error out when passed a nil-valued
 variable as 'offset'
Date: Tue, 05 Jun 2018 02:40:07 +0200

Noam Postavsky <npostavs <at> gmail.com> writes:

> Do DOCSTRING and DECL count?
>
>     (defun NAME ARGLIST &optional DOCSTRING DECL &rest BODY)
>
> Since they're obviously not evaluated, it doesn't quite feel the same.

Yes, not really.

It's not that super clear.  If we keep the current definition (though I
would rather like to see this fixed if possible), maybe consider to add
something like "If X is specified, it should be an expression that
should evaluate to a number".


Michael.

Message #20 received at 31715 <at> debbugs.gnu.org (full text, mbox):

From: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 31715 <at> debbugs.gnu.org
Subject: Re: bug#31715: cl-incf and cl-decf error out when passed a nil-valued
 variable as 'offset'
Date: Tue, 5 Jun 2018 11:03:01 -0400

[Message part 1 (text/plain, inline)]

On 2018-06-04 18:58, Noam Postavsky wrote:
> X is an optional macro parameter, so the "optionalness" applies at
> compile time.

I think I see what you mean, but I'm not entirely convinced (in part because the docstring doesn't say so, and in part because it doesn't seem worth it to break referential transparency: if we accept nil, we should also accept a variable that evaluates to nil).

> I think that would approximately double the cost of cl-incf in the
> simple case.  And since you would expect cl-incf to be used in loops a
> lot, that seems like a bad idea.

I think we could still optimize the case in which we get an explicit nil.

Clément.

[signature.asc (application/pgp-signature, attachment)]

Message #23 received at 31715 <at> debbugs.gnu.org (full text, mbox):

From: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
To: Noam Postavsky <npostavs <at> gmail.com>,
 Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: 31715 <at> debbugs.gnu.org
Subject: Re: bug#31715: cl-incf and cl-decf error out when passed a nil-valued
 variable as 'offset'
Date: Tue, 5 Jun 2018 11:19:22 -0400

[Message part 1 (text/plain, inline)]

On 2018-06-04 20:12, Noam Postavsky wrote:
> I couldn't find any other cases of optional parameters in core macros.

I'm not sure which macros count as core macros.  In cl-lib itself there's cl-assert, which seems to behave the same as cl-incf.

In the rest of Emacs there are lots of other examples.  Many of them (for example semantic-find-tags-by-name or calendar-increment-month) seem to work when passed a nil-valued variable, but many others behave like cl-incf (for example gnus-summary-article-score).

Clément.

[signature.asc (application/pgp-signature, attachment)]

Message #26 received at 31715 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, 31715 <at> debbugs.gnu.org
Subject: Re: bug#31715: cl-incf and cl-decf error out when passed a nil-valued
 variable as 'offset'
Date: Tue, 05 Jun 2018 18:53:29 -0400

Clément Pit-Claudel <clement.pitclaudel <at> live.com> writes:

> On 2018-06-04 18:58, Noam Postavsky wrote:
>> X is an optional macro parameter, so the "optionalness" applies at
>> compile time.
>
> I think I see what you mean, but I'm not entirely convinced (in part
> because the docstring doesn't say so, and in part because it doesn't
> seem worth it to break referential transparency: if we accept nil, we
> should also accept a variable that evaluates to nil).

Hmm, not sure if referential transparency makes much sense for macros.

Both SBCL and CLisp throw an error for (let ((x 1) (y nil)) (incf x y)),
although I can't see anything in the Common Lisp spec to decide either
way.  E.g., cltl2 says:

    If delta is not supplied, then the number in place is changed by 1.

https://www.cs.cmu.edu/Groups/AI/html/cltl/clm/node125.html

>> I think that would approximately double the cost of cl-incf in the
>> simple case.  And since you would expect cl-incf to be used in loops a
>> lot, that seems like a bad idea.
>
> I think we could still optimize the case in which we get an explicit nil.

As I wrote earlier, I was wrong about the optimization thing anyway (I
initially looked at the macro expansion output instead of the byte
compiler output).

> In the rest of Emacs there are lots of other examples.  Many of them
> (for example semantic-find-tags-by-name or calendar-increment-month)
> seem to work when passed a nil-valued variable, but many others behave
> like cl-incf (for example gnus-summary-article-score).

I don't think those are great examples of macros to emulate.
semantic-find-tags-by-name and gnus-summary-article-score look like they
should be (inline?) functions instead of macros.
calendar-increment-month doesn't use make-symbol for proper temp
variable hygiene.

Message #29 received at 31715 <at> debbugs.gnu.org (full text, mbox):

From: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, 31715 <at> debbugs.gnu.org
Subject: Re: bug#31715: cl-incf and cl-decf error out when passed a nil-valued
 variable as 'offset'
Date: Tue, 5 Jun 2018 19:01:56 -0400

[Message part 1 (text/plain, inline)]

On 2018-06-05 18:53, Noam Postavsky wrote:
> Both SBCL and CLisp throw an error for (let ((x 1) (y nil)) (incf x y)),
> although I can't see anything in the Common Lisp spec to decide either
> way.  E.g., cltl2 says:
> 
>     If delta is not supplied, then the number in place is changed by 1.
> 
> https://www.cs.cmu.edu/Groups/AI/html/cltl/clm/node125.html

Thanks for looking up the spec and testing other implementations.  I'm not sure what to make of the result with SBCL and CLisp, since (incf x nil) also fails in both of them (whereas it works for us, since we can't distinguish nil and unspecified).

>> In the rest of Emacs there are lots of other examples.  Many of them
>> (for example semantic-find-tags-by-name or calendar-increment-month)
>> seem to work when passed a nil-valued variable, but many others behave
>> like cl-incf (for example gnus-summary-article-score).
> 
> I don't think those are great examples of macros to emulate.

Agreed, I was just collecting other examples, both in support and against my point.

Clément.

[signature.asc (application/pgp-signature, attachment)]

Message #32 received at 31715 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, 31715 <at> debbugs.gnu.org
Subject: Re: bug#31715: cl-incf and cl-decf error out when passed a nil-valued
 variable as 'offset'
Date: Tue, 05 Jun 2018 19:26:51 -0400

Clément Pit-Claudel <clement.pitclaudel <at> live.com> writes:

> I'm not sure what to make of the result with SBCL and CLisp, since
> (incf x nil) also fails in both of them (whereas it works for us,
> since we can't distinguish nil and unspecified).

Oh, huh, I didn't think to check that case.  Maybe we should just change
cl-incf to disintguish between nil and unspecified then.

(defmacro cl-incf (place &rest args)
  "Increment PLACE by X (1 by default).
PLACE may be a symbol, or any generalized variable allowed by `setf'.
The return value is the incremented value of PLACE.

\(fn PLACE &optional X)"
  (declare (debug (place &optional form)))
  (let* ((got-x (= (length args) 1))
         (x (car args)))
    (if (symbolp place)
        (list 'setq place (if got-x (list '+ place x) (list '1+ place)))
      (list 'cl-callf '+ place (if got-x x 1)))))

>>> In the rest of Emacs there are lots of other examples.  Many of them
>>> (for example semantic-find-tags-by-name or calendar-increment-month)
>>> seem to work when passed a nil-valued variable, but many others behave
>>> like cl-incf (for example gnus-summary-article-score).
>> 
>> I don't think those are great examples of macros to emulate.
>
> Agreed, I was just collecting other examples, both in support and against my point.

Yeah, I just meant we can't really use those examples either to support
or argue against your point.

Message #35 received at 31715 <at> debbugs.gnu.org (full text, mbox):

From: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, 31715 <at> debbugs.gnu.org
Subject: Re: bug#31715: cl-incf and cl-decf error out when passed a nil-valued
 variable as 'offset'
Date: Tue, 5 Jun 2018 19:36:41 -0400

[Message part 1 (text/plain, inline)]

On 2018-06-05 19:26, Noam Postavsky wrote:
> Clément Pit-Claudel <clement.pitclaudel <at> live.com> writes:
> 
>> I'm not sure what to make of the result with SBCL and CLisp, since
>> (incf x nil) also fails in both of them (whereas it works for us,
>> since we can't distinguish nil and unspecified).
> 
> Oh, huh, I didn't think to check that case.  Maybe we should just change
> cl-incf to disintguish between nil and unspecified then.

Hmm, neat trick! Wouldn't that be backwards-incompatible, though?
Plus, it's not common for ELisp functions to distinguish between unspecified and nil, and changing cl-incf that way would make it less convenient to wrap in other macros.  If it doesn't had runtime costs, I'd be in favor of going the other route, and making sure that (cl-incf x y) adds 1 even when y is bound to nil.

>>> I don't think those are great examples of macros to emulate.
>>
>> Agreed, I was just collecting other examples, both in support and against my point.
> 
> Yeah, I just meant we can't really use those examples either to support
> or argue against your point.

OK :)

[signature.asc (application/pgp-signature, attachment)]

Message #38 received at 31715 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: Clément Pit-Claudel <clement.pitclaudel <at> live.com>,
 31715 <at> debbugs.gnu.org
Subject: Re: bug#31715: cl-incf and cl-decf error out when passed a nil-valued
 variable as 'offset'
Date: Wed, 06 Jun 2018 02:32:18 +0200

Noam Postavsky <npostavs <at> gmail.com> writes:

> Oh, huh, I didn't think to check that case.  Maybe we should just change
> cl-incf to disintguish between nil and unspecified then.
>
> (defmacro cl-incf (place &rest args)
>   "Increment PLACE by X (1 by default).
> PLACE may be a symbol, or any generalized variable allowed by `setf'.
> The return value is the incremented value of PLACE.
>
> \(fn PLACE &optional X)"
>   (declare (debug (place &optional form)))
>   (let* ((got-x (= (length args) 1))
>          (x (car args)))
>     (if (symbolp place)
>         (list 'setq place (if got-x (list '+ place x) (list '1+ place)))
>       (list 'cl-callf '+ place (if got-x x 1)))))

That's quite hackish, just to make (cl-incf x nil) error, which is a
backward incompatible change with no real gain.  Wouldn't we even lose
the byte compiler barfing for something like (incf x 1 2)?


Michael.

Message #41 received at 31715 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Clément Pit-Claudel <clement.pitclaudel <at> live.com>,
 31715 <at> debbugs.gnu.org
Subject: Re: bug#31715: cl-incf and cl-decf error out when passed a nil-valued
 variable as 'offset'
Date: Tue, 05 Jun 2018 20:37:45 -0400

Michael Heerdegen <michael_heerdegen <at> web.de> writes:

> Noam Postavsky <npostavs <at> gmail.com> writes:
>
>> Oh, huh, I didn't think to check that case.  Maybe we should just change
>> cl-incf to disintguish between nil and unspecified then.

> That's quite hackish, just to make (cl-incf x nil) error, which is a
> backward incompatible change with no real gain.

But it's more compatible with other CL implementations!  (yeah, maybe
not worth it)

> Wouldn't we even lose the byte compiler barfing for something like
> (incf x 1 2)?

I didn't bother putting in the check for extra arguments, but of course
it could be done if we decide to go this way.

Message #46 received at 31715 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Clément Pit-Claudel <clement.pitclaudel <at> live.com>,
 Noam Postavsky <npostavs <at> gmail.com>, 31715 <at> debbugs.gnu.org
Subject: Re: bug#31715: cl-incf and cl-decf error out when passed a
 nil-valued variable as 'offset'
Date: Thu, 21 Apr 2022 15:11:13 +0200

Michael Heerdegen <michael_heerdegen <at> web.de> writes:

> It's not that super clear.  If we keep the current definition (though I
> would rather like to see this fixed if possible), maybe consider to add
> something like "If X is specified, it should be an expression that
> should evaluate to a number".

Reading this bug thread, different tweaks were suggested, but they all
seem to have various disadvantages.  So I think the conclusion here is
that doing this documentation clarification is the safest thing, so I've
now done so in Emacs 29.

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.