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

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

[Clément Pit-Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 769 bytes --]

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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Noam Postavsky]

Clément Pit-Claudel <clement.pitclaudel@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.

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

[Michael Heerdegen]

Noam Postavsky <npostavs@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.

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

[Noam Postavsky]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Noam Postavsky <npostavs@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.

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

[Michael Heerdegen]

Noam Postavsky <npostavs@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.

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

[Clément Pit-Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 674 bytes --]



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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Clément Pit-Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 531 bytes --]

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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Noam Postavsky]

Clément Pit-Claudel <clement.pitclaudel@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.

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

[Clément Pit-Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 1071 bytes --]

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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Noam Postavsky]

Clément Pit-Claudel <clement.pitclaudel@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.

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

[Clément Pit-Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 1114 bytes --]

On 2018-06-05 19:26, Noam Postavsky wrote:
> Clément Pit-Claudel <clement.pitclaudel@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 :)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Michael Heerdegen]

Noam Postavsky <npostavs@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.

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

[Noam Postavsky]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Noam Postavsky <npostavs@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.

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

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@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