bug#21701: 25.0.50; ert explainer for equal can't handle negative numbers (work in 24.5)

bug#21701: 25.0.50; ert explainer for equal can't handle negative numbers (work in 24.5)

[Anders Lindgren]

[-- Attachment #1: Type: text/plain, Size: 3695 bytes --]

Steps to repeat, eval the following and run `ert'.

(ert-deftest bad-equal ()
  (should (equal 23 -50)))

Instead of a normal Ert error, the following is reported:

A bad-equal
    aborted

When placing the cursor on the error and press "d" (to run the test case in
the debugger), the following call stack is presented:

  apply(debug (error (wrong-type-argument characterp -50)))
  ert--run-test-debugger([cl-struct-ert--test-execution-info...
  #[128 "\301\300\x02\"\207" [[cl-struct-ert--test-execution-info ...
  format("?%c" -50)
  ert--explain-format-atom(-50)
  ert--explain-equal-rec(23 -50)
  ert--explain-equal(23 -50)
  apply(ert--explain-equal (23 -50))
  (list :explanation (apply -explainer- args-1))

Clearly, the code is trying to print -50 as a character.

This worked in Emacs 24.5, where Ert reported:

F bad-equal
    (ert-test-failed
     ((should
       (equal 23 -50))
      :form
      (equal 23 -50)
      :value nil :explanation
      (different-atoms
       (23 "#x17" "?\x17")
       (-50 "#x3fffffffffffffce"))))


Sincerely,
    Anders Lindgren


In GNU Emacs 25.0.50.158 (x86_64-apple-darwin14.5.0, NS appkit-1348.17
Version 10.10.5 (Build 14F27))
 of 2015-10-16
Repository revision: ff4798b8b493ba1ec51dcb1c59a11824865124b8
Windowing system distributor 'Apple', version 10.3.1348
Configured using:
 'configure --with-ns --without-dbus'

Configured features:
ACL ZLIB TOOLKIT_SCROLL_BARS NS

Important settings:
  value of $LC_CTYPE: UTF-8
  locale-coding-system: utf-8-unix

Major mode: ERT-Results

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
bad-equal
Ran 1 tests, 0 results were as expected, 1 unexpected
Running test bad-equal...
Entering debugger...

Load-path shadows:
None found.

Features:
(shadow sort gnus-util mail-extr emacsbug message dired format-spec
rfc822 mml mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util help-fns mail-prsvr mail-utils pp cl-extra help-mode cl-macs gv
ert find-func ewoc easymenu debug cl-loaddefs pcase cl-lib time-date
mule-util tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel ns-win term/common-win tool-bar dnd fontset image
regexp-opt fringe tabulated-list newcomment elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow timer select scroll-bar
mouse jit-lock font-lock syntax facemenu font-core frame cl-generic cham
georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese charscript case-table epa-hook
jka-cmpr-hook help simple abbrev minibuffer cl-preloaded nadvice
loaddefs button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote cocoa ns multi-tty
make-network-process emacs)

Memory information:
((conses 16 89635 5963)
 (symbols 48 19436 0)
 (miscs 40 78 159)
 (strings 32 15385 4052)
 (string-bytes 1 463936)
 (vectors 16 12257)
 (vector-slots 8 415700 5035)
 (floats 8 162 183)
 (intervals 56 271 6)
 (buffers 976 14))

[-- Attachment #2: Type: text/html, Size: 4110 bytes --]

bug#21701: cl-typecase broken (was 25.0.50; ert explainer for equal can't handle negative numbers)

[Anders Lindgren]

[-- Attachment #1: Type: text/plain, Size: 268 bytes --]

I just realised that the underlying problem is a change to `cl-typecase'.
It treats -50 as a character.

(cl-typecase -50
  (character "A character")
  (fixnum "A fixnum")
  (t "Something else"))

Emacs 25 returns "A character" and emacs 24 "A fixnum".

    -- Anders

[-- Attachment #2: Type: text/html, Size: 477 bytes --]

bug#21701: Fwd: cl-typecase broken (was 25.0.50; ert explainer for equal can't handle negative numbers)

[Stefan Monnier]

> I just realised that the underlying problem is a change to `cl-typecase'.
> It treats -50 as a character.
>
> (cl-typecase -50
>   (character "A character")
>   (fixnum "A fixnum")
>   (t "Something else"))
>
> Emacs 25 returns "A character" and emacs 24 "A fixnum".

I installed the patch below which should fix this.


        Stefan


diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index 09d2d3f..c8aad3a 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -2885,7 +2885,7 @@ cl--macroexp-fboundp
 (put 'real 'cl-deftype-satisfies #'numberp)
 (put 'fixnum 'cl-deftype-satisfies #'integerp)
 (put 'base-char 'cl-deftype-satisfies #'characterp)
-(put 'character 'cl-deftype-satisfies #'integerp)
+(put 'character 'cl-deftype-satisfies #'natnump)
 
 
 ;;;###autoload

bug#21701: Fwd: cl-typecase broken (was 25.0.50; ert explainer for equal can't handle negative numbers)

[Anders Lindgren]

[-- Attachment #1: Type: text/plain, Size: 1846 bytes --]

Hi,

This should solve the immediate problem with negative numbers.

However, I gave this some though and realised that there is still a problem
with large numbers. For example:

(cl-typecase (+ (max-char) 1)
  (character "A character")
  (fixnum "A fixnum")
  (t "Something else"))

Returns "A character".

However, "(format "%c" (+ (max-char) 1))" raises the error
"(wrong-type-argument characterp 4194304)".

The question is if `cl-typecase', `format', and `characterp' should have
the same definition on what a character is. If not, then ERT must be
modified to handle this, e.g. by using `base-char' rather than `character'.

Personally, I would perfer if `character' would mean the same thing in all
contexts. I would suggest that we restore the old meaning of `character',
drop `base-char', and add a new type class, say `key-event', that could
include things like ?\M-\C-x.

    -- Anders

On Fri, Dec 4, 2015 at 2:30 PM, Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > I just realised that the underlying problem is a change to `cl-typecase'.
> > It treats -50 as a character.
> >
> > (cl-typecase -50
> >   (character "A character")
> >   (fixnum "A fixnum")
> >   (t "Something else"))
> >
> > Emacs 25 returns "A character" and emacs 24 "A fixnum".
>
> I installed the patch below which should fix this.
>
>
>         Stefan
>
>
> diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
> index 09d2d3f..c8aad3a 100644
> --- a/lisp/emacs-lisp/cl-macs.el
> +++ b/lisp/emacs-lisp/cl-macs.el
> @@ -2885,7 +2885,7 @@ cl--macroexp-fboundp
>  (put 'real 'cl-deftype-satisfies #'numberp)
>  (put 'fixnum 'cl-deftype-satisfies #'integerp)
>  (put 'base-char 'cl-deftype-satisfies #'characterp)
> -(put 'character 'cl-deftype-satisfies #'integerp)
> +(put 'character 'cl-deftype-satisfies #'natnump)
>
>
>  ;;;###autoload
>

[-- Attachment #2: Type: text/html, Size: 2758 bytes --]

bug#21701: Fwd: cl-typecase broken (was 25.0.50; ert explainer for equal can't handle negative numbers)

[Stefan Monnier]

> However, "(format "%c" (+ (max-char) 1))" raises the error

> The question is if `cl-typecase', `format', and `characterp' should have
> the same definition on what a character is.

characterp corresponds to `base-char', at least if we want cl-*
functions to follow the Common-Lisp semantics.

> If not, then ERT must be modified to handle this, e.g. by using
> `base-char' rather than `character'.

That's indeed what should be done if ERT needs this to be a plain
character that can inserted in a string.  Common-Lisp's `character'
includes not just characters but also "characters with modifiers" such
as ?\M-\H-é, which can't appear in a string and are rejected by
`characterp'.


        Stefan

bug#21701: Fwd: cl-typecase broken (was 25.0.50; ert explainer for equal can't handle negative numbers)

[Stefan Monnier]

> That's indeed what should be done if ERT needs this to be a plain
> character that can inserted in a string.  Common-Lisp's `character'
> includes not just characters but also "characters with modifiers" such
> as ?\M-\H-é, which can't appear in a string and are rejected by
> `characterp'.

I installed a patch which makes ERT use pcase over cl-typecase.
In most cases it doesn't make a big difference, but in a few spots, it
is cleaner because a subsequent cl-destructuring-bind can be merged into
it (and it got rid of those places where we used (member :foo)
as a type to just check equality, which is rather inefficient).


        Stefan

bug#21701: Fwd: cl-typecase broken (was 25.0.50; ert explainer for equal can't handle negative numbers)

[Anders Lindgren]

[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]

Hi!

I assumed that `characterp' and the `character' type class was connected in
Common Lisp. If they aren't I guess the current system makes sense.

I downloaded the ert changes and ran all my local tests and I haven't seen
any problems.

Thanks!

    -- Anders

On Fri, Dec 4, 2015 at 7:17 PM, Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > That's indeed what should be done if ERT needs this to be a plain
> > character that can inserted in a string.  Common-Lisp's `character'
> > includes not just characters but also "characters with modifiers" such
> > as ?\M-\H-é, which can't appear in a string and are rejected by
> > `characterp'.
>
> I installed a patch which makes ERT use pcase over cl-typecase.
> In most cases it doesn't make a big difference, but in a few spots, it
> is cleaner because a subsequent cl-destructuring-bind can be merged into
> it (and it got rid of those places where we used (member :foo)
> as a type to just check equality, which is rather inefficient).
>
>
>         Stefan
>

[-- Attachment #2: Type: text/html, Size: 1609 bytes --]

bug#21701: Fwd: cl-typecase broken (was 25.0.50; ert explainer for equal can't handle negative numbers)

[Drew Adams]

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]

A reasonable guess.It might make sense to add a note to the Elisp manual, in node `Character Type' or node `Character Codes' (where we talk about `characterp'), or both, to point out that Emacs Lisp characters are not the same as Common Lisp chars.

 

I assumed that `characterp' and the `character' type class was connected in Common Lisp. If they aren't I guess the current system makes sense.

[-- Attachment #2: Type: text/html, Size: 2491 bytes --]

bug#21701: Fwd: cl-typecase broken (was 25.0.50; ert explainer for equal can't handle negative numbers)

[Stefan Monnier]

> I assumed that `characterp' and the `character' type class was connected in
> Common Lisp.

They are.  The CLHS says:

          (characterp object) ==  (typep object 'character)

In Elisp that would translate to

          (cl-characterp object) ==  (cl-typep object 'character)

But cl-lib does not define cl-characterp and Elisp defines characterp
differently than Common-Lisp.

> If they aren't I guess the current system makes sense.

I think the current system can't make complete sense, sadly, because of
such subtle incompatibility between Elisp and Common Lisp.


        Stefan

bug#21701: Fwd: cl-typecase broken (was 25.0.50; ert explainer for equal can't handle negative numbers)

[Drew Adams]

> (cl-characterp object) ==  (cl-typep object 'character)
> 
> But cl-lib does not define cl-characterp

Perhaps it should.  And then the doc should point out
that in Emacs Lisp, `cl-characterp' != `characterp'.

bug#21701: Fwd: cl-typecase broken (was 25.0.50; ert explainer for equal can't handle negative numbers)

[Glenn Morris]

Stefan Monnier wrote:

> I installed a patch which makes ERT use pcase over cl-typecase.

This causes test failures.

http://hydra.nixos.org/build/28477383
http://hydra.nixos.org/build/28477383/log/raw


   FAILED  ert-test-equal-including-properties
   FAILED  ert-test-explain-equal
   FAILED  ert-test-explain-equal-improper-list
   FAILED  ert-test-explain-equal-string-properties
   FAILED  ert-test-plist-difference-explanation
   FAILED  ert-test-run-tests-interactively
   FAILED  ert-test-run-tests-interactively-2

bug#21701: Fwd: cl-typecase broken (was 25.0.50; ert explainer for equal can't handle negative numbers)

[Anders Lindgren]

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

Hi,

Two of the problems can be fixed with the attached patch.

The third, `ert-test-record-backtrace' in `ert-tests.el' seems to compare
backtrace output -- I haven't looked it as I have no idea what it is
supposed to test.

    -- Anders

On Fri, Dec 4, 2015 at 11:58 PM, Glenn Morris <rgm@gnu.org> wrote:

> Stefan Monnier wrote:
>
> > I installed a patch which makes ERT use pcase over cl-typecase.
>
> This causes test failures.
>
> http://hydra.nixos.org/build/28477383
> http://hydra.nixos.org/build/28477383/log/raw
>
>
>    FAILED  ert-test-equal-including-properties
>    FAILED  ert-test-explain-equal
>    FAILED  ert-test-explain-equal-improper-list
>    FAILED  ert-test-explain-equal-string-properties
>    FAILED  ert-test-plist-difference-explanation
>    FAILED  ert-test-run-tests-interactively
>    FAILED  ert-test-run-tests-interactively-2
>

[-- Attachment #1.2: Type: text/html, Size: 1492 bytes --]

[-- Attachment #2: ert.diff --]
[-- Type: text/plain, Size: 1005 bytes --]

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index a75b23b..02ae41b 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -515,7 +515,7 @@ ert--explain-equal-rec
                   for xi = (ert--explain-equal-rec ai bi)
                   do (when xi (cl-return `(array-elt ,i ,xi)))
                   finally (cl-assert (equal a b) t))))
-      ((pred atomp)
+      ((pred atom)
        (if (not (equal a b))
            (if (and (symbolp a) (symbolp b) (string= a b))
                `(different-symbols-with-the-same-name ,a ,b)
@@ -1071,7 +1071,7 @@ ert--insert-human-readable-selector
                      (make-symbol "<unnamed test>")))
                   (`(,operator . ,operands)
                    (pcase operator
-                     ((or 'eql 'and 'not 'or)
+                     ((or 'member 'eql 'and 'not 'or)
                       `(,operator ,@(mapcar #'rec operands)))
                      ((or 'tag 'satisfies)
                       selector))))))