bug#17234: 24.3.50; overlay priority : cons cells make an error in ediff

bug#17234: 24.3.50; overlay priority : cons cells make an error in ediff

[Nicolas Richard]

Steps to reproduce from -Q :
M-x ediff-regions-wordwise RET
RET ; selects scratch buf
RET ; ditto
C-SPC C-n C-M-c ; select first line of scratch buffer
C-n C-SPC C-n C-M-c ; select second line of scratch buffer
n ; go to next difference

That makes an error
> Wrong type argument: number-or-marker-p, (nil . 100)
in function ediff-highest-priority, which comes from commit
> cdb3fff3f588caeed50cbb5b64c09bce0a0b31e3
> Author: Stefan Monnier <monnier@iro.umontreal.ca>
> Date:   Sun Mar 23 18:30:47 2014 -0400

>     * lisp/simple.el (redisplay-highlight-region-function): Increase priority of
>     overlay to make sure boundaries are visible.
>     * src/buffer.c (struct sortvec): Add field `spriority'.
>     (compare_overlays): Use it.
>     (sort_overlays): Set it.
where the notion of the "priority" of an overlay was changed : it can
now be a cons cell (which holds a priority and a secondary priority).
The changelog mentions bug#15899 but I saw nothing in that (very long)
thread mentionning the fix.

At (info "(elisp) Overlay Properties") however it is said that the
priority "should be a non-negative integer".

Also, and that's where the error comes from, ediff-init.el relies on the
priority being either nil or an integer, in function
ediff-highest-priority. For this I can suggest a fix (see patch below)
but I don't know what lispref should say about the change.

In GNU Emacs 24.3.50.6 (i686-pc-linux-gnu, GTK+ Version 2.24.20)
 of 2014-04-09 on LDLC-portable

	Modified   lisp/ChangeLog
diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index 54ac144..3ed0195 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,8 @@
+2014-04-10  Nicolas Richard  <theonewiththeevillook@yahoo.fr>
+
+	* vc/ediff-init.el (ediff-highest-priority): Don't make an error
+	if overlay priority is a cons.
+
 2014-04-09  Dmitry Gutov  <dgutov@yandex.ru>
 
 	* progmodes/ruby-mode.el (ruby-font-lock-keywords): Highlight more
	Modified   lisp/vc/ediff-init.el
diff --git a/lisp/vc/ediff-init.el b/lisp/vc/ediff-init.el
index 000fdb9..110dc63 100644
--- a/lisp/vc/ediff-init.el
+++ b/lisp/vc/ediff-init.el
@@ -1352,7 +1352,12 @@ this variable represents.")
 			       (null (ediff-overlay-get ovr 'ediff))
 			       (null (ediff-overlay-get ovr 'ediff-diff-num)))
 			  ;; use the overlay priority or 0
-			  (or (ediff-overlay-get ovr 'priority) 0)
+			  (let ((priority (ediff-overlay-get ovr 'priority)))
+                            (cond ((integerp priority)
+                                   priority)
+                                  ((consp priority)
+                                   (or (car priority) (cdr priority)))
+                                  (t 0)))
 			0))
 		    ovr-list))))))))
 
-- 
Nico.

bug#17234: 24.3.50; overlay priority : cons cells make an error in ediff

[Stefan Monnier]

> That makes an error
>> Wrong type argument: number-or-marker-p, (nil . 100)
> in function ediff-highest-priority, which comes from commit

Indeed, thanks.  Ediff's handling of overlay priorities is a good
example of what's wrong with overlay priorities, adding code to
make sure its overlays are "on top", then adding yet more code to ensure
its other overlays are "even more on top".  And then yet more code to
try and defeat the other packages which might be taking part in this
race to the highest priority.

So, I just threw it out, which should solve those problem.


        Stefan

bug#17234: 24.3.50; overlay priority : cons cells make an error in ediff

[Glenn Morris]

Nicolas Richard wrote:

> [...] the notion of the "priority" of an overlay was changed : it can
> now be a cons cell (which holds a priority and a secondary priority).
[...]
> At (info "(elisp) Overlay Properties") however it is said that the
> priority "should be a non-negative integer".


What about the documentation/NEWS aspect?

bug#17234: 24.3.50; overlay priority : cons cells make an error in ediff

[Stefan Monnier]

>> [...] the notion of the "priority" of an overlay was changed : it can
>> now be a cons cell (which holds a priority and a secondary priority).
> [...]
>> At (info "(elisp) Overlay Properties") however it is said that the
>> priority "should be a non-negative integer".
> What about the documentation/NEWS aspect?

The new feature was introduced specifically for the region handling, and
I'm not yet convinced I'm satisfied with it, so I'd rather not
document it for now.


        Stefan

bug#17234: 24.3.50; overlay priority : cons cells make an error in ediff

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Date: Fri, 11 Apr 2014 08:30:33 -0400
> Cc: 17234@debbugs.gnu.org
> 
> >> [...] the notion of the "priority" of an overlay was changed : it can
> >> now be a cons cell (which holds a priority and a secondary priority).
> > [...]
> >> At (info "(elisp) Overlay Properties") however it is said that the
> >> priority "should be a non-negative integer".
> > What about the documentation/NEWS aspect?
> 
> The new feature was introduced specifically for the region handling, and
> I'm not yet convinced I'm satisfied with it, so I'd rather not
> document it for now.

Then how can we expect authors of other Lisp packages to be able to
fix their code so it works with Emacs 24.4 and later?  We must say
_something_ in NEWS.

bug#17234: 24.3.50; overlay priority : cons cells make an error in ediff

[Stefan Monnier]

> Then how can we expect authors of other Lisp packages to be able to
> fix their code so it works with Emacs 24.4 and later?  We must say
> _something_ in NEWS.

It has always been the case that `priority' could be any value.

Any non-number value was treated as nil by the display engine, whereas
now some cons values are treated as something else.

But ediff's bug could already be triggered in Emacs<24.4 by a package
installing an overlay with a `priority' that's a cons cell.


        Stefan

bug#17234: 24.3.50; overlay priority : cons cells make an error in ediff

[Glenn Morris]

Stefan Monnier wrote:

> It has always been the case that `priority' could be any value.

But the only documented values were and are nil and positive integers.
I really doubt anyone was intentionally using anything else to mean "nil".

> Any non-number value was treated as nil by the display engine, whereas
> now some cons values are treated as something else.

So even if your previous point was correct, it's still an incompatible
change. A cons cell can no longer be used to mean "no explicit priority".

> But ediff's bug could already be triggered in Emacs<24.4 by a package
> installing an overlay with a `priority' that's a cons cell.

Since we never (?) had any such reports, I conclude nobody did that.

But I imagine ediff was not the only package trying to get the priority
of an overlay and do something with the answer (however misguided you
think that might be).

bug#17234: 24.3.50; overlay priority : cons cells make an error in ediff

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rgm@gnu.org,  17234@debbugs.gnu.org
> Date: Fri, 11 Apr 2014 11:56:24 -0400
> 
> > Then how can we expect authors of other Lisp packages to be able to
> > fix their code so it works with Emacs 24.4 and later?  We must say
> > _something_ in NEWS.
> 
> It has always been the case that `priority' could be any value.
> 
> Any non-number value was treated as nil by the display engine, whereas
> now some cons values are treated as something else.
> 
> But ediff's bug could already be triggered in Emacs<24.4 by a package
> installing an overlay with a `priority' that's a cons cell.

Sorry, I don't see how all this removes the need to inform Lisp
programmers of the change of a long-time traditional behavior.

I don't understand the reason(s) for hiding this information.  If you
don't want to divulge some internal details, the information can be
conveyed in a purely functional manner ("don't do XXX; instead, do
YYY" or "don't assume that overlay priority is either an integer or
nil) that doesn't require to describe implementation details.  But I
don't see how can we say nothing about this incompatible change.

bug#17234: 24.3.50; overlay priority : cons cells make an error in ediff

[Stefan Monnier]

>> Any non-number value was treated as nil by the display engine, whereas
>> now some cons values are treated as something else.
> So even if your previous point was correct, it's still an incompatible
> change. A cons cell can no longer be used to mean "no explicit priority".

Indeed.  Tho as you said:

  I really doubt anyone was intentionally using anything else to mean "nil".

So I think we're safe on this side of the backward incompatibility.

> But I imagine ediff was not the only package trying to get the priority
> of an overlay and do something with the answer (however misguided you
> think that might be).

Hard to tell.  Not sure what we can do about it, tho.


        Stefan

bug#17234: 24.3.50; overlay priority : cons cells make an error in ediff

[Stefan Monnier]

> "don't assume that overlay priority is either an integer or nil"

Feel free to put that in.


        Stefan

bug#17234: 24.3.50; overlay priority : cons cells make an error in ediff

[Glenn Morris]

Stefan Monnier wrote:

>> But I imagine ediff was not the only package trying to get the priority
>> of an overlay and do something with the answer (however misguided you
>> think that might be).
>
> Hard to tell.

One minute of searching suggests that:

extent-at in lucid.el
ps-generate-postscript-with-faces1 in ps-def.el

will have problems now.

It would seem suprising if there are three files in Emacs that this
causes problems for, and nothing external.

> Not sure what we can do about it, tho.

The least you can do is make an "incompatible change" NEWS entry.
Ideally it would it explain what do if you still want to get the
priority of an overlay as a number.

bug#17234: 24.3.50; overlay priority : cons cells make an error in ediff

[Stefan Monnier]

> extent-at in lucid.el
> ps-generate-postscript-with-faces1 in ps-def.el

Indeed.  And htmlize also.

> The least you can do is make an "incompatible change" NEWS entry.

Not sure which part is incompatible.

> Ideally it would it explain what to do if you still want to get the
> priority of an overlay as a number.

You don't, really.  But I've added an argument `sorted' to overlays-at
which seems to provide the information that was really needed in the
above cases.


        Stefan

bug#17234: 24.3.50; overlay priority : cons cells make an error in ediff

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rgm@gnu.org,  17234@debbugs.gnu.org
> Date: Fri, 11 Apr 2014 16:07:47 -0400
> 
> > "don't assume that overlay priority is either an integer or nil"
> 
> Feel free to put that in.

Done.