Re: bizarre byte-compile issue, possibly due to EIEIO

Re: bizarre byte-compile issue, possibly due to EIEIO

[Ted Zlatanov]

On Fri, 01 Apr 2011 15:29:38 -0500 Ted Zlatanov <tzz@lifelogs.com> wrote: 

TZ> In other words, the registry.elc file is breaking the tests.  I can't
TZ> figure out what's wrong, but it seems EIEIO-related.  If I try to
TZ> edebug, that evaluates the problematic methods (e.g. `registry-lookup')
TZ> and then they don't exhibit the problem.

TZ> Using "(eval-when-compile (require 'registry))" instead of 
TZ> "(require 'registry)" doesn't help.  All the registry.el ERT tests pass.

(CC to ding mailing list; see prior messages on emacs-devel for context)

I've been able to work around it with:

(let ((load-suffixes '(".el")))
  (require 'registry))

in gnus-registry.el, but that makes the registry.el functionality really
slow and generates this byte-compile warning:

In end of data:
gnus-registry.el:910:1:Warning: the following functions are not known to be
    defined: registry-db, eieio-persistent-read, registry-size,
    registry-prune, eieio-persistent-save, registry-delete, registry-insert,
    registry-lookup-secondary-value, registry-lookup-secondary,
    registry-lookup

So I hope someone can help me out :)

Ted

Re: bizarre byte-compile issue, possibly due to EIEIO

[Lars Magne Ingebrigtsen]

Ted Zlatanov <tzz@lifelogs.com> writes:

> So I hope someone can help me out :)

If it's EIEIO-related, perhaps they can help you out?  EIEIO is a
mystery to me...

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: bizarre byte-compile issue, possibly due to EIEIO

[David Engster]

Ted Zlatanov writes:
> On Fri, 01 Apr 2011 15:29:38 -0500 Ted Zlatanov <tzz@lifelogs.com> wrote: 
>
> TZ> In other words, the registry.elc file is breaking the tests.  I can't
> TZ> figure out what's wrong, but it seems EIEIO-related.  If I try to
> TZ> edebug, that evaluates the problematic methods (e.g. `registry-lookup')
> TZ> and then they don't exhibit the problem.
>
> TZ> Using "(eval-when-compile (require 'registry))" instead of 
> TZ> "(require 'registry)" doesn't help.  All the registry.el ERT tests pass.
>
> (CC to ding mailing list; see prior messages on emacs-devel for context)
>
> I've been able to work around it with:
>
> (let ((load-suffixes '(".el")))
>   (require 'registry))

[...]

> So I hope someone can help me out :)

I can't really explain anything, but maybe I can at least shift the
blame. ;-)

If I rewrite your registry-lookup function to use 'mapcar' instead of
'loop', the tests work as expected:

(defmethod registry-lookup ((db registry-db) keys)
  "Search for KEYS in the registry-db THIS.
Returns a alist of the key followed by the entry in a list, not a cons cell."
  (let ((data (oref db :data)))
    (delq nil
	  (mapcar
	   (lambda (k)
	     (when (gethash k data)
	       (list k (gethash k data))))
	   keys))))

-David

Re: bizarre byte-compile issue, possibly due to EIEIO

[David Engster]

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

David Engster writes:
> Ted Zlatanov writes:
>> On Fri, 01 Apr 2011 15:29:38 -0500 Ted Zlatanov <tzz@lifelogs.com> wrote: 
>>
>> TZ> In other words, the registry.elc file is breaking the tests.  I can't
>
>> TZ> figure out what's wrong, but it seems EIEIO-related.  If I try to
>> TZ> edebug, that evaluates the problematic methods (e.g. `registry-lookup')
>> TZ> and then they don't exhibit the problem.
>>
>> TZ> Using "(eval-when-compile (require 'registry))" instead of 
>> TZ> "(require 'registry)" doesn't help.  All the registry.el ERT tests pass.
>
> I can't really explain anything, but maybe I can at least shift the
> blame. ;-)

> If I rewrite your registry-lookup function to use 'mapcar' instead of
> 'loop', the tests work as expected:

[...]

I tested a little bit more. I created a short test case which I attached
to this mail. Run it by doing

emacs --batch -L .  -l bc-test -f bc-test

If everything works, you should get

Method with mapcar: (("20" "foo 20") ("30" "foo 30") ("40" "foo 40"))
Method with loop: (("20" "foo 20") ("30" "foo 30") ("40" "foo 40"))

Here's what I observe:

* The 'mapcar' method always works.
* If you don't byte-compile, the 'loop' method will also work.
* If you byte-compile, the 'loop' method will fail with Emacs24 *before*
  the lexbind merge. After the lexbind merge, it works as expected.

Note that the :initform evaluation will not work with the EIEIO version
which ships with Emacs23. I remember some discussions regarding this
issue; it's probably best to use an explicit constructor. I included it
in the test case (you have to uncomment it). When using the class w/
constructor, you can also compile it under Emacs23, and the 'loop'
method will then also fail. The funny thing is that Emacs24 *after* the
lexbind merge cannot run that byte-compiled code from Emacs23; is that
to be expected?

-David


[-- Attachment #2: bc-test.el --]
[-- Type: application/emacs-lisp, Size: 1552 bytes --]

Re: bizarre byte-compile issue, possibly due to EIEIO

[Ted Zlatanov]

On Sun, 03 Apr 2011 11:34:42 +0200 David Engster <deng@randomsample.de> wrote: 

DE> Here's what I observe:

DE> * The 'mapcar' method always works.
DE> * If you don't byte-compile, the 'loop' method will also work.
DE> * If you byte-compile, the 'loop' method will fail with Emacs24 *before*
DE>   the lexbind merge. After the lexbind merge, it works as expected.

Of course, I found and reported all of this right before the lexbind
merge.  Argh.

I took your mapcar version and used it.  Everything works now, and will
work in older Emacs versions too.

DE> Note that the :initform evaluation will not work with the EIEIO version
DE> which ships with Emacs23. I remember some discussions regarding this
DE> issue; it's probably best to use an explicit constructor. I included it
DE> in the test case (you have to uncomment it). When using the class w/
DE> constructor, you can also compile it under Emacs23, and the 'loop'
DE> method will then also fail. 

When I use the explicit constructor (commented out right now), many
tests fail.  Do you want to take a look at that?  It's the only
remaining merge blocker.

My changes are in the new "tzz-gnus-registry-rewrite" branch.  I won't
merge it until we're sure it works in Emacs 23 and 24.  I'll write the
manual changes and ChangeLog at that time too.

DE> The funny thing is that Emacs24 *after* the lexbind merge cannot run
DE> that byte-compiled code from Emacs23; is that to be expected?

I would assume major versions introduce binary incompatibilities, so I
wouldn't worry about that.

Ted

Re: bizarre byte-compile issue, possibly due to EIEIO

[Ted Zlatanov]

On Sat, 02 Apr 2011 22:34:33 +0200 Lars Magne Ingebrigtsen <larsi@gnus.org> wrote: 

LMI> Ted Zlatanov <tzz@lifelogs.com> writes:
>> So I hope someone can help me out :)

LMI> If it's EIEIO-related, perhaps they can help you out?  EIEIO is a
LMI> mystery to me...

I think it's a problem with the `loop' macro actually but can't isolate
it in a separate test.

Ted

Re: bizarre byte-compile issue, possibly due to EIEIO

[David Engster]

Ted Zlatanov writes:
> On Sat, 02 Apr 2011 22:34:33 +0200 Lars Magne Ingebrigtsen <larsi@gnus.org> wrote: 
>
> LMI> Ted Zlatanov <tzz@lifelogs.com> writes:
>>> So I hope someone can help me out :)
>
> LMI> If it's EIEIO-related, perhaps they can help you out?  EIEIO is a
> LMI> mystery to me...
>
> I think it's a problem with the `loop' macro actually but can't isolate
> it in a separate test.

I wasn't able to reproduce the problem without using 'defmethod', so I'm
guessing it's at least related to EIEIO (and maybe also responsible for
the bug). Though it seems to work fine after the lexbind merge, I'm
guessing this is only fixed by side-effect, so there could very well
still a bug be lurking somewhere. I know zero about the byte-compiler,
so I have no idea were to start debugging this.

-David

Re: bizarre byte-compile issue, possibly due to EIEIO

[David Engster]

Ted Zlatanov writes:
> On Sun, 03 Apr 2011 11:34:42 +0200 David Engster <deng@randomsample.de> wrote: 
>
> DE> Here's what I observe:
>
> DE> * The 'mapcar' method always works.
> DE> * If you don't byte-compile, the 'loop' method will also work.
> DE> * If you byte-compile, the 'loop' method will fail with Emacs24 *before*
> DE>   the lexbind merge. After the lexbind merge, it works as expected.
>
> Of course, I found and reported all of this right before the lexbind
> merge.  Argh.

Yes, but we can't be sure the "real bug" has been fixed here...

> DE> Note that the :initform evaluation will not work with the EIEIO version
> DE> which ships with Emacs23. I remember some discussions regarding this
> DE> issue; it's probably best to use an explicit constructor. I included it
> DE> in the test case (you have to uncomment it). When using the class w/
> DE> constructor, you can also compile it under Emacs23, and the 'loop'
> DE> method will then also fail. 
>
> When I use the explicit constructor (commented out right now), many
> tests fail.  Do you want to take a look at that?  It's the only
> remaining merge blocker.

I wasn't thinking. The constructor has to check if there was an initarg
provided for 'data', otherwise it will overwrite it. Hence you have to
use something like

 (defmethod initialize-instance :after ((this registry-db) slots)
   "Set value of data slot of THIS after initialization."
   ;; 'data' will already be set if read from file, so don't overwrite it.
   (with-slots (data tracker) this
     (unless (member :data slots)
       (setq data (make-hash-table :size 10000 :rehash-size 2.0 :test 'equal)))
     (unless (member :tracker slots)
       (setq tracker (make-hash-table :size 100 :rehash-size 2.0)))))

This is also why you shouldn't set the other slot values here.

BTW, you can use 

            :type hash-table

for 'tracker' and 'data'. It will then use 'hash-table-p' for checking
the provided argument.

> DE> The funny thing is that Emacs24 *after* the lexbind merge cannot run
> DE> that byte-compiled code from Emacs23; is that to be expected?
>
> I would assume major versions introduce binary incompatibilities, so I
> wouldn't worry about that.

The docs say: "In general, any version of Emacs can run byte-compiled
code produced by recent earlier versions of Emacs, but the reverse is
not true."

So I'm guessing this is one exception from this general rule. :-)

-David

Re: bizarre byte-compile issue, possibly due to EIEIO

[Stefan Monnier]

> method will then also fail. The funny thing is that Emacs24 *after* the
> lexbind merge cannot run that byte-compiled code from Emacs23; is that
> to be expected?

Whether it's expected, I don't know because I don't know where the problem
comes from.  But it's not desirable: we like to preserve compatibility
with older .elc files and we made no decision to introduce such
incompatibility recently, so: it's bug.


        Stefan

Re: bizarre byte-compile issue, possibly due to EIEIO

[Ted Zlatanov]

On Mon, 04 Apr 2011 17:34:10 +0200 David Engster <deng@randomsample.de> wrote: 

DE> I wasn't thinking. The constructor has to check if there was an initarg
DE> provided for 'data', otherwise it will overwrite it. Hence you have to
DE> use something like

DE>  (defmethod initialize-instance :after ((this registry-db) slots)
DE>    "Set value of data slot of THIS after initialization."
DE>    ;; 'data' will already be set if read from file, so don't overwrite it.
DE>    (with-slots (data tracker) this
DE>      (unless (member :data slots)
DE>        (setq data (make-hash-table :size 10000 :rehash-size 2.0 :test 'equal)))
DE>      (unless (member :tracker slots)
DE>        (setq tracker (make-hash-table :size 100 :rehash-size 2.0)))))

DE> This is also why you shouldn't set the other slot values here.

DE> BTW, you can use 

DE>             :type hash-table

DE> for 'tracker' and 'data'. It will then use 'hash-table-p' for checking
DE> the provided argument.

I took your advice on both the constructor and the type.  The updates
plus some bug fixes are pushed.  I'll test it a bit and if it works OK
will merge the "tzz-gnus-registry-rewrite" branch into "master".

Thanks!
Ted

Re: bizarre byte-compile issue, possibly due to EIEIO

[Ted Zlatanov]

On Mon, 04 Apr 2011 17:34:10 +0200 David Engster <deng@randomsample.de> wrote: 

DE> Ted Zlatanov writes:
>> On Sun, 03 Apr 2011 11:34:42 +0200 David Engster <deng@randomsample.de> wrote: 
>> 
DE> Here's what I observe:
>> 
DE> * The 'mapcar' method always works.
DE> * If you don't byte-compile, the 'loop' method will also work.
DE> * If you byte-compile, the 'loop' method will fail with Emacs24 *before*
DE> the lexbind merge. After the lexbind merge, it works as expected.
>> 
>> Of course, I found and reported all of this right before the lexbind
>> merge.  Argh.

DE> Yes, but we can't be sure the "real bug" has been fixed here...

I added a `registry-lookup-breaks-before-lexbind' method to registry.el
in my branch and it's in the tests.  So you can check that:

emacs -batch -L . -l ert.el -l gnus-registry.el -f ert-run-tests-batch-and-exit

fails before the lexbind merge, and minus registry.elc:

rm registry.elc
emacs -batch -L . -l ert.el -l gnus-registry.el -f ert-run-tests-batch-and-exit

it succeeds.  Both work after the lexbind merge.

You're right, it smells like some interaction of the EIEIO defmethod and
the loop macro.  I don't know how much incentive we have to fix it,
though, without a good isolated test case.  I tried and couldn't find a
smoking gun.

Ted

Re: bizarre byte-compile issue, possibly due to EIEIO

[David Engster]

Ted Zlatanov writes:
> On Mon, 04 Apr 2011 17:34:10 +0200 David Engster <deng@randomsample.de> wrote: 
>
> DE> I wasn't thinking. The constructor has to check if there was an initarg
> DE> provided for 'data', otherwise it will overwrite it. Hence you have to
> DE> use something like
>
> DE>  (defmethod initialize-instance :after ((this registry-db) slots)
> DE>    "Set value of data slot of THIS after initialization."
> DE>    ;; 'data' will already be set if read from file, so don't overwrite it.
> DE>    (with-slots (data tracker) this
> DE>      (unless (member :data slots)
> DE>        (setq data (make-hash-table :size 10000 :rehash-size 2.0 :test 'equal)))
> DE>      (unless (member :tracker slots)
> DE>        (setq tracker (make-hash-table :size 100 :rehash-size 2.0)))))
>
> DE> This is also why you shouldn't set the other slot values here.

[...]

> I took your advice on both the constructor and the type.  The updates
> plus some bug fixes are pushed.  I'll test it a bit and if it works OK
> will merge the "tzz-gnus-registry-rewrite" branch into "master".

I saw that you now set every slot value in the :after method. This isn't
necessary; :initform is absolutely save to use for symbols. The only
thing that changed in EIEIO is for the case where :initform is a
function which has to be evaluated, which is only working correctly in
newer versions.

-David

Re: bizarre byte-compile issue, possibly due to EIEIO

[Ted Zlatanov]

On Tue, 05 Apr 2011 18:38:23 +0200 David Engster <deng@randomsample.de> wrote: 

DE> I saw that you now set every slot value in the :after method. This isn't
DE> necessary; :initform is absolutely save to use for symbols. The only
DE> thing that changed in EIEIO is for the case where :initform is a
DE> function which has to be evaluated, which is only working correctly in
DE> newer versions.

I thought it was more consistent to set all the slot values in one
place.  It is more verbose though...  I'm 50-50 on it, do you see any
issues with the method other than being more verbose?

Ted

Re: bizarre byte-compile issue, possibly due to EIEIO

[Ted Zlatanov]

On Tue, 05 Apr 2011 11:14:53 -0500 Ted Zlatanov <tzz@lifelogs.com> wrote: 

TZ> I added a `registry-lookup-breaks-before-lexbind' method to registry.el
TZ> in my branch and it's in the tests.  So you can check that:

TZ> emacs -batch -L . -l ert.el -l gnus-registry.el -f ert-run-tests-batch-and-exit

TZ> fails before the lexbind merge, and minus registry.elc:

TZ> rm registry.elc
TZ> emacs -batch -L . -l ert.el -l gnus-registry.el -f ert-run-tests-batch-and-exit

TZ> it succeeds.  Both work after the lexbind merge.

This is in the master branch of the Gnus repository now, so it should
get merged soon-ish into Emacs.

Ted

Re: bizarre byte-compile issue, possibly due to EIEIO

[David Engster]

Ted Zlatanov writes:
> On Tue, 05 Apr 2011 18:38:23 +0200 David Engster <deng@randomsample.de> wrote: 
>
> DE> I saw that you now set every slot value in the :after method. This isn't
> DE> necessary; :initform is absolutely save to use for symbols. The only
> DE> thing that changed in EIEIO is for the case where :initform is a
> DE> function which has to be evaluated, which is only working correctly in
> DE> newer versions.
>
> I thought it was more consistent to set all the slot values in one
> place.  It is more verbose though...  I'm 50-50 on it, do you see any
> issues with the method other than being more verbose?

I just wanted to make clear that aside from function evaluations there's
no issue with using :initform. I think :initform is better for
documentation purposes (it will be shown as "default" in
describe-function, although that seems to be currently broken in
Emacs24), and it's also better in case you want to allow users to
customize objects using eieio-customize. Otherwise, it's more a matter
of style. I think constructors should be used to create somewhat
"dynamical" objects.

-David

Re: bizarre byte-compile issue, possibly due to EIEIO

[Ted Zlatanov]

On Tue, 05 Apr 2011 20:31:37 +0200 David Engster <deng@randomsample.de> wrote: 

DE> Ted Zlatanov writes:
>> On Tue, 05 Apr 2011 18:38:23 +0200 David Engster <deng@randomsample.de> wrote: 
>> 
DE> I saw that you now set every slot value in the :after method. This isn't
DE> necessary; :initform is absolutely save to use for symbols. The only
DE> thing that changed in EIEIO is for the case where :initform is a
DE> function which has to be evaluated, which is only working correctly in
DE> newer versions.
>> 
>> I thought it was more consistent to set all the slot values in one
>> place.  It is more verbose though...  I'm 50-50 on it, do you see any
>> issues with the method other than being more verbose?

DE> I just wanted to make clear that aside from function evaluations there's
DE> no issue with using :initform. I think :initform is better for
DE> documentation purposes (it will be shown as "default" in
DE> describe-function, although that seems to be currently broken in
DE> Emacs24), and it's also better in case you want to allow users to
DE> customize objects using eieio-customize. Otherwise, it's more a matter
DE> of style. I think constructors should be used to create somewhat
DE> "dynamical" objects.

OK, I've put the initforms back.  Thanks for the help!

Ted