bug#38708: [PATCH] Deduplicate flonum and bignum constants in bytecode

[Pip Cet <pipcet@gmail.com>]

On Sat, Dec 28, 2019 at 6:50 PM Mattias Engdegård <mattiase@acm.org> wrote:
> Elisp has a few 'boundedly undefined' parts which would be avoided in a greenfield design but that we are more or less stuck with for the time being, and probably should paint in large letters on the first page of the manual. Such as: don't use eq for numbers; don't mutate literals (strings, conses, vectors); etc.

I don't think we're stuck with those two, and I don't think we should
be making them worse than they already are (in Emacs 26.3).

Sorry, the rest of this got a bit long. I think your change isn't as
trivial as it seems at first glance, and we shouldn't do it for Emacs
27, but it's fine on the master branch and might help us shake out
some bugs (and, strategically, the paradoxical behavior observed with
your patch is a good argument for making eq and eql synonymous).

> In my experience people seem to have little trouble understanding that eq shouldn't be used to compare numbers.

Not my experience.

In fact, anyone who reads
https://www.gnu.org/software/emacs/manual/html_mono/elisp.html#Equality-Predicates
today would come away with the reassuring knowledge that "If object1
and object2 are integers with the same value, they are considered to
be the same object (i.e., eq returns t)."

And there's plenty of code that uses eq to compare numbers, on the
master branch, today.

So whether you learn by experience or by "stable" documentation,
you'll pick up the habit of comparing integers with eq.

> If anything, the introduction of bignums helps driving the point home: up to and including Emacs 26, elisp programmers could get away with eq for integers but not floating-point numbers. Now, the rules are simpler. (Characters still get a free pass, I suppose.)

What are the rules, then? "Use eql, except if you're comparing against
a small integer, when eq is fine, or a character, which might not be
that small but will be fine, or a number you know from studying the
Emacs internals is a fixnum"?

> While we could document a safe range of fixnums for which eq applies, it's probably better not to.

My impression was people were very careful to use eq and EQ whenever
they could prove the numbers were still in fixnum range.

> > Technically, I think code such as
> >
> > (cond ((eq a b) 1) ((not (eq a b)) 2) (t 3))
> >
> > is allowed to return 3. We should attempt not to make changes that
> > make this more likely, though.
>
> Given the state of elisp byte-code optimisation, there is plenty of room for improvements before such semantics become unavoidable. In particular, the committed change does not in any way enable such paradoxical behaviour.

(defun f ()
  (cond
   ((eq 1.0 1.0) 1)
   ((my-not-eq 1.0 1.0) 2)
   (t 3))))

produces 3 here, with your patch and standard byte compilation. That
seems just as paradoxical to me.

(You're right about hash tables).

> > You're right, that is eq's contract. But people do misuse it, and one
> > of the things they wouldn't expect is that optimization results in
> > things becoming non-eq that were eq before optimization; I think the
> > other direction is much less problematic.
>
> Good thing that the change works in that other direction then!

Sorry, I may have been ambiguous: With your patch, (eq 1.0 1.0) is t
without optimization, nil with optimization. That's the direction I
meant.

> >> What would anyone gain from such a restriction? And the change is minor because it's a small thing to do; what I thought looked like an obvious oversight, or one that made more sense back when Elisp didn't have bignums.
> >
> > Well, Elisp has had floats for a while, and bignum constants appear to
> > be nonexistent, so far.
>
> In other words, there is no broken bignum code that can break.

Sorry, that was a response to the second part: the original code makes
just as much sense now as it did in the pre-bignum age. You're right
that folding floats but not bignums is probably a bad idea.

This probably isn't leading anywhere, but I still think this snippet
of code is somewhat realistic and should never output "1 nil". First
we count the number of eq-distinct elements, then we check whether the
elements are eq-distinct.

(defun count-distinct-elements (&rest args)
  (let ((ht (make-hash-table :test 'eq)))
    (dolist (arg args)
      (puthash arg 1 ht))
    (hash-table-count ht)))

(defmacro f (x y)
  `(format "%S %S" (count-distinct-elements ,x ,y) (eq ,x ,y)))

(defun g ()
  (f 1.0 1.0))