dash.el icon indicating copy to clipboard operation
dash.el copied to clipboard

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

Open alphapapa opened this issue 8 years ago • 14 comments

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

alphapapa avatar Nov 13 '17 07:11 alphapapa

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 "\210A\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 "\210A\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)\nAA\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)\nAA\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 avatar Nov 13 '17 07:11 alphapapa

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...

alphapapa avatar Nov 13 '17 08:11 alphapapa

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.

Fuco1 avatar Nov 13 '17 09:11 Fuco1

@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.

alphapapa avatar Nov 13 '17 11:11 alphapapa

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.

Fuco1 avatar Nov 13 '17 11:11 Fuco1

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.

alphapapa avatar Nov 13 '17 11:11 alphapapa

@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 avatar Nov 13 '17 11:11 Fuco1

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

Fuco1 avatar Nov 13 '17 11:11 Fuco1

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 avatar Nov 13 '17 12:11 alphapapa

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.

alphapapa avatar Nov 13 '17 12:11 alphapapa

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.

phst avatar Nov 13 '17 21:11 phst

@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.

alphapapa avatar Nov 14 '17 16:11 alphapapa

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?

Fuco1 avatar Jul 19 '18 12:07 Fuco1

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.

alphapapa avatar Jul 19 '18 22:07 alphapapa