Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add: (-let) Bind vars to EIEIO object slots with &obj #251

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alphapapa
Copy link

This extends -let to bind to EIEIO object slots with the &obj symbol.

@alphapapa
Copy link
Author

alphapapa commented Nov 13, 2017

The test fails on Emacs <25, but I can't figure out why. I ran emacs24 -q on my system, opened up the EIEIO info page, and copied the example straight out of it to the scratch buffer:

(require 'eieio)

(defclass record () ; No superclasses
       ((name :initarg :name
              :initform ""
              :type string
              :custom string
              :documentation "The name of a person.")
        (birthday :initarg :birthday
                  :initform "Jan 1, 1970"
                  :custom string
                  :type string
                  :documentation "The person's birthday.")
        (phone :initarg :phone
               :initform ""
               :documentation "Phone number."))
       "A single record for tracking people I know.")

(defmethod call-record ((rec record) &optional scriptname)
       "Dial the phone for the record REC.
     Execute the program SCRIPTNAME to dial the phone."
       (message "Dialing the phone for %s"  (oref rec name))
       (shell-command (concat (or scriptname "dialphone.sh")
                              " "
                              (oref rec phone))))

(setq rec (record :name "Eric" :birthday "June" :phone "555-5555"))

I evaluated each sexp, but when I got to the last one, I get the same kind of error:

Debugger entered--Lisp error: (invalid-slot-name "#<record :name>" "Eric")
  signal(invalid-slot-name ("#<record :name>" "Eric"))
  #[(object slot-name operation &optional new-value) "\302\303\304�!	D\"\207" [object slot-name signal invalid-slot-name eieio-object-name] 4 "Method invoked when an attempt to access a slot in OBJECT fails.\nSLOT-NAME is the name of the failed slot, OPERATION is the type of access\nthat was requested, and optional NEW-VALUE is the value that was desired\nto be set.\n\nThis method is called from `oref', `oset', and other functions which\ndirectly reference slots in EIEIO objects."]([object record :name "" "Jan 1, 1970" ""] "Eric" oset :birthday)
  apply(#[(object slot-name operation &optional new-value) "\302\303\304�!	D\"\207" [object slot-name signal invalid-slot-name eieio-object-name] 4 "Method invoked when an attempt to access a slot in OBJECT fails.\nSLOT-NAME is the name of the failed slot, OPERATION is the type of access\nthat was requested, and optional NEW-VALUE is the value that was desired\nto be set.\n\nThis method is called from `oref', `oset', and other functions which\ndirectly reference slots in EIEIO objects."] ([object record :name "" "Jan 1, 1970" ""] "Eric" oset :birthday))
  slot-missing([object record :name "" "Jan 1, 1970" ""] "Eric" oset :birthday)
  #[(obj slots) "\304\216�\305H	B�\n\2057�\306�\305H\n@\"\211�\204%�\307�\n@\310\nA@$\210\202-�\311��\nA@#\210)\nAA\211�\204\f�\312)\207" [obj eieio--scoped-class-stack slots rn ((byte-code "�\210�A�\301\207" [eieio--scoped-class-stack nil] 1)) 1 eieio-initarg-to-attribute slot-missing oset eieio-oset nil] 6 "Set slots of OBJ with SLOTS which is a list of name/value pairs.\nCalled from the constructor routine."]([object record :name "" "Jan 1, 1970" ""] ("Eric" :birthday "June" :phone "555-5555"))
  apply(#[(obj slots) "\304\216�\305H	B�\n\2057�\306�\305H\n@\"\211�\204%�\307�\n@\310\nA@$\210\202-�\311��\nA@#\210)\nAA\211�\204\f�\312)\207" [obj eieio--scoped-class-stack slots rn ((byte-code "�\210�A�\301\207" [eieio--scoped-class-stack nil] 1)) 1 eieio-initarg-to-attribute slot-missing oset eieio-oset nil] 6 "Set slots of OBJ with SLOTS which is a list of name/value pairs.\nCalled from the constructor routine."] ([object record :name "" "Jan 1, 1970" ""] ("Eric" :birthday "June" :phone "555-5555")))
  shared-initialize([object record :name "" "Jan 1, 1970" ""] ("Eric" :birthday "June" :phone "555-5555"))
  #[(this &optional slots) "�\306H\307N\211�\310H�	\311H�\n\203/�\312�@!\211��@=\204%�\313�\n@\f#\210)\nA��A�\202��+\314�
\"\207" [this this-class slot defaults dflt slots 1 eieio-class-definition 5 6 eieio-default-eval-maybe eieio-oset shared-initialize] 5 "Construct the new object THIS based on SLOTS.\nSLOTS is a tagged list where odd numbered elements are tags, and\neven numbered elements are the values to store in the tagged slot.\nIf you overload the `initialize-instance', there you will need to\ncall `shared-initialize' yourself, or you can call `call-next-method'\nto have this constructor called automatically.  If these steps are\nnot taken, then new objects of your class will not have their values\ndynamically set from SLOTS."]([object record :name "" "Jan 1, 1970" ""] ("Eric" :birthday "June" :phone "555-5555"))
  apply(#[(this &optional slots) "�\306H\307N\211�\310H�	\311H�\n\203/�\312�@!\211��@=\204%�\313�\n@\f#\210)\nA��A�\202��+\314�
\"\207" [this this-class slot defaults dflt slots 1 eieio-class-definition 5 6 eieio-default-eval-maybe eieio-oset shared-initialize] 5 "Construct the new object THIS based on SLOTS.\nSLOTS is a tagged list where odd numbered elements are tags, and\neven numbered elements are the values to store in the tagged slot.\nIf you overload the `initialize-instance', there you will need to\ncall `shared-initialize' yourself, or you can call `call-next-method'\nto have this constructor called automatically.  If these steps are\nnot taken, then new objects of your class will not have their values\ndynamically set from SLOTS."] ([object record :name "" "Jan 1, 1970" ""] ("Eric" :birthday "June" :phone "555-5555")))
  initialize-instance([object record :name "" "Jan 1, 1970" ""] ("Eric" :birthday "June" :phone "555-5555"))
  eieio-default-superclass(record :name "Eric" :birthday "June" :phone "555-5555")
  apply(eieio-default-superclass (record :name "Eric" :birthday "June" :phone "555-5555"))
  eieio-generic-call(constructor (record :name "Eric" :birthday "June" :phone "555-5555"))
  constructor(record :name "Eric" :birthday "June" :phone "555-5555")
  apply(constructor record :name ("Eric" :birthday "June" :phone "555-5555"))
  record(:name "Eric" :birthday "June" :phone "555-5555")
  (setq rec (record :name "Eric" :birthday "June" :phone "555-5555"))
  eval((setq rec (record :name "Eric" :birthday "June" :phone "555-5555")) nil)
  eval-last-sexp-1(nil)
  eval-last-sexp(nil)
  call-interactively(eval-last-sexp nil nil)
  command-execute(eval-last-sexp)

This seems bizarre, since I'm using the example straight out of the manual, and it works in Emacs 25. I googled around but couldn't find anything, except a few similar reports of this error with Emacs 24. I didn't see any solutions presented, except in each case to upgrade the related third-party packages, so I don't know what is going on.

@alphapapa
Copy link
Author

alphapapa commented Nov 13, 2017

It seems that make-instance works, but I still have no explanation as to why the constructor doesn't work.

I also noticed that pcase-let supports an eieio type, like:

(pcase-let (((eieio slot2 color slot1) (progn
                                         (defclass -let-test-class ()
                                           ((slot1 :initarg :slot1)
                                            (slot2 :initarg :slot2)
                                            (color :initarg :color)))
                                         (-let-test-class :slot1 1 :slot2 2 :color "red"))))
  (list slot1 slot2 color))  ; => '(1 2 "red")

It has the advantage that it doesn't require manually specifying the name of variables, just the slot names (but you can specify them if you choose). So I guess this PR isn't as useful as it would be without pcase-let in Emacs 25. But it would still be useful in Emacs 24...and some users might like to not have to resort to pcase. So I'll fix the test...

@Fuco1
Copy link
Collaborator

Fuco1 commented Nov 13, 2017

There's the simplified binding issue somewhere where you also wouldn't have to specify the slot names and they would derive automatically. If we implement that it will be incldued for all the matchers so the syntax will be basically the same.

@alphapapa
Copy link
Author

alphapapa commented Nov 13, 2017

@Fuco1 That would probably be good, although the user would have to be careful in case a slot name shadowed a variable he had already bound.

The tests are passing now on Emacs 24 and 25. The test on Emacs 23 fails with Lisp nesting exceeds max-lisp-eval-depth, but I'm guessing we'll have to excuse that.

@Fuco1
Copy link
Collaborator

Fuco1 commented Nov 13, 2017

Yea, we can put some conditions around emacs 23 code, it's supported in backward-compatible way but new features don't have to work there. We need to make sure the features working only on newer emacsen are documented as such, so that people who really care about supporting e23 will avoid them.

@alphapapa
Copy link
Author

Ok. I'm not sure how to do that, so I'll let you handle it, or if you tell me how you want it done, I'll see what I can do.

@Fuco1
Copy link
Collaborator

Fuco1 commented Nov 13, 2017

@alphapapa It should be enough to just wrap it in a condition. We have this in the examples.el

(unless (version< emacs-version "24")
    (defexamples -rpartial
      (funcall (-rpartial '- 5) 8) => 3
      (funcall (-rpartial '- 5 2) 10) => 3)

    (defexamples -juxt
      (funcall (-juxt '+ '-) 3 5) => '(8 -2)
      (-map (-juxt 'identity 'square) '(1 2 3)) => '((1 1) (2 4) (3 9)))

    (defexamples -compose
      (funcall (-compose 'square '+) 2 3) => (square (+ 2 3))
      (funcall (-compose 'identity 'square) 3) => (square 3)
      (funcall (-compose 'square 'identity) 3) => (square 3)
      (funcall (-compose (-compose 'not 'even?) 'square) 3) => (funcall (-compose 'not (-compose 'even? 'square)) 3)))

So something like that. Tweak the version as appropriate.

@Fuco1
Copy link
Collaborator

Fuco1 commented Nov 13, 2017

And about the documentation, just put it in the docstring, something like "This only works with emacs xx.x or newer", should be enough.

@alphapapa
Copy link
Author

alphapapa commented Nov 13, 2017

Ok, the test only runs on Emacs >23 now, and I added a note to the docstring about that.

However, now when I run create-docs.sh, none of the documentation files are updated. I even deleted README.md, and the script doesn't recreate it. Well, one file is updated: dash.info. However, it no longer has the updated docstring, rather a bunch of changes consisting of differently wrapped lines throughout the file. I have no explanation. :/

@alphapapa
Copy link
Author

I just noticed that the test was already failing on Emacs 23. See https://travis-ci.org/magnars/dash.el/jobs/294156579. So I don't know if this commit works on Emacs 23, because the tests don't get that far.

So, two problems:

  1. create-docs.sh mysteriously doesn't update the files with the changed docstring.
  2. The tests were already failing on Emacs 23.

@phst
Copy link
Contributor

phst commented Nov 13, 2017

It has the advantage that it doesn't require manually specifying the name of variables, just the slot names (but you can specify them if you choose). So I guess this PR isn't as useful as it would be without pcase-let in Emacs 25. But it would still be useful in Emacs 24

The same is possible using with-slots, which I think has been part of Emacs/EIEIO for a very long time.

@alphapapa
Copy link
Author

@phst Thanks! I did not know about that. There are so many great things in Emacs, it's easy to be unaware of them for years. :)

So I guess that further diminishes the usefulness of this PR. But at the same time, having it in -let means that users could potentially have a single -let form instead of a with-slots or pcase-let inside of a regular let or -let.

@Fuco1
Copy link
Collaborator

Fuco1 commented Jul 19, 2018

I think this could be valuable if we could also destructure defstruct the same way as objects, after all, they are pretty similar. And I hate using those super long accessor functions like your-defstruct-your-attribute.

We could treat them both as objects and use a bit of runtime check to decide what to do.

@alphapapa are you still interested in implementing this?

@alphapapa
Copy link
Author

I think this could be valuable if we could also destructure defstruct the same way as objects, after all, they are pretty similar. And I hate using those super long accessor functions like your-defstruct-your-attribute.

pcase-let supports destructuring defstructs.

@alphapapa are you still interested in implementing this?

Not at this time, sorry. I think it would be nice if -let supported everything pcase-let does, so I think there's value in this idea, but I'm working on other things right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants