guicho271828 / trivia

Pattern Matcher Compatible with Optima
Other
332 stars 22 forks source link

The (structure ...) example on the wiki spews warnings #65

Open sjl opened 7 years ago

sjl commented 7 years ago

I'm trying to use (structure ...) patterns but keep getting lots of warnings at macroexpansion time. I tried just running the bare example in the wiki and still get them:

><((°> rlwrap sbcl

[SBCL] CL-USER> (ql:quickload 'trivia)
To load "trivia":
  Load 1 ASDF system:
    trivia
; Loading "trivia"
(TRIVIA)

[SBCL] CL-USER> (use-package :trivia)
T

[SBCL] CL-USER> (defstruct foo bar baz)
FOO

[SBCL] CL-USER> (defvar *x* (make-foo :bar 0 :baz 1))
*X*

[SBCL] CL-USER> (match *x*
  ((foo :bar a :baz b) ;; make-instance style
   (values a b))
  ((foo (bar a) (baz b)) ;; with-slots style
   (values a b))
  ((foo bar baz) ;; slot name
   (values bar baz)))
WARNING:
   Calling #<FUNCTION FOO-BAR> failed, but not by program-error (TYPE-ERROR).
WARNING:
   Calling #<FUNCTION FOO-BAZ> failed, but not by program-error (TYPE-ERROR).
WARNING:
   Calling #<FUNCTION FOO-BAR> failed, but not by program-error (TYPE-ERROR).
WARNING:
   Calling #<FUNCTION FOO-BAZ> failed, but not by program-error (TYPE-ERROR).
WARNING:
   Calling #<FUNCTION FOO-BAR> failed, but not by program-error (TYPE-ERROR).
WARNING:
   Calling #<FUNCTION FOO-BAZ> failed, but not by program-error (TYPE-ERROR).

0
1

SBCL on OS X, if it matters. I'm using the latest Quicklisp dist.

guicho271828 commented 7 years ago

um, yes. this is expected but Im not decided if I should remove these warning messages. For normal accessor functions they are safe to ignore, but it tells that the function is called and you should be careful if your functions have any side effect.

In the recent updates, structure pattern calls the candidate functions each by each with a dumb argument and check if it fails with program error, or it succeeds, or it fails with other errors.

This check is used to decide which function is the accessor function among several functions, with an assumption that an accessor function should have a single argument and otherwise fails by program-error.

For example, imagine if there are two functions person-car (implicitly defined by defstruct person) and car (a constructor function taking several arguments, which create a new car instance). In this case person-car should be selected as an accessor function.

But there is also another style where reader functions are given without person- prefix, e.g., (defclass person ((car :reader car))). To maximize the accessor detection ability, I added the above facility. It currently has several methods to see if the function is unary, including: looking up the function-lambda-expression; effective-slot-reader-function; etc.

The reason of above warning is to tell the user that the function could be called, in case the reader function has some unexpected side effect, which is imaginable. So if that is not the case, you can just ignore the warning. I will also update the warning message so that it gives more information.

sjl commented 7 years ago

Is there a way I can match structures without Trivia calling arbitrarily-named functions at macroexpansion time and hoping they do what it expects based on their names? I'm fine with the something more verbose, because I can just wrap it in a defpattern. But I'd really rather not rely on magic function-name guessing to hopefully do the right thing.

I tried the with-accessors style because I figured it would trust that I had given it the actual accessor names, but that seemed to do the same thing if I recall correctly (on my phone right now).

On Nov 26, 2016, at 04:56, Masataro Asai notifications@github.com wrote:

um, yes. this is expected but Im not decided if I should remove these warning messages. For normal accessor functions they are safe to ignore, but it tells that the function is called and you should be careful if your functions have any side effect.

In the recent updates, structure pattern calls the candidate functions each by each with a dumb argument and check if it fails with program error, or it succeeds, or it fails with other errors.

This check is used to decide which function is the accessor function among several functions, with an assumption that an accessor function should have a single argument and otherwise fails by program-error.

For example, imagine if there are two functions person-car (implicitly defined by defstruct person) and car (a constructor function taking several arguments, which create a new car instance). In this case person-car should be selected as an accessor function.

But there is also another style where reader functions are given without person- prefix, e.g., (defclass person ((car :reader car))). To maximize the accessor detection ability, I added the above facility. It currently has several methods to see if the function is unary, including: looking up the function-lambda-expression; effective-slot-reader-function; etc.

The reason of above warning is to tell the user that the function could be called, in case the reader function has some unexpected side effect, which is imaginable. So if that is not the case, you can just ignore the warning. I will also update the warning message so that it gives more information.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

guicho271828 commented 7 years ago

it does not search through all functions in the lisp image. It checks only 3 functions, namely <class>-<name>, <class><name>, <name>. Most usecases of defstruct falls in the first case. The second case is for compatibility to Optima where <class> can be an arbitrary conc-name, e.g.

(match (make-instance 'foo :bar 2)
  ((foo- bar)
    (pring bar)))

The third case is for the defclass + accessor case. So there are only 3 candidate functions.

guicho271828 commented 7 years ago

I also understood the need for specifying particular function naming scheme. Perhaps through compiler option (I mean special variable) or by adding a specialized version of the structure pattern.

sjl commented 7 years ago

I get that it's only three functions, but that's still three too many for me.

To get a project into Quicklisp, it must build without warnings. I just checked and these warnings do show up when building with (ql:quickload ... :verbose t). I'm unsure if STYLE-WARNINGs count, but I suspect they do since that post says "any warnings". So this would make it impossible to get a project into Quicklisp that uses (structure ...) matching, which is obviously not good.

But please don't just remove the warnings. Having a pattern match call functions at macroexpansion time is extremely unintuitive, and folks need to know it's happening if it's going to happen. Someday someone is going to have a class where one of class-slot, classslot, and slot is a function with side effects, and it's going to be really hard to track down how this function is getting called at compile/macroexpansion time even with the errors. E.g., if I've got a slot called log in some structure, and a log function that logs objects to disk.

My ideal UI for matching structures would look something like this:

For structures with custom :conc-names, I'd use something like:

This mirrors the defstruct syntax. It's verbose, but you can wrap it in a defpattern. Maybe remove the extra level of list nesting that defstruct needs but this wouldn't need.

Maybe I can write this myself as a defpattern, I haven't tried yet.

This would obviously not be compatible with Optima. In a perfect world I'd much rather have a separate :trivia+optima-compatible package with all the magic stuff necessary to be a drop-in replacement for Optima for those who need it. That way the main Trivia API could be kept nice and clean, with all the hacks for Optima compatibility separated out.

sjl commented 7 years ago

A quick proof-of-concept for my sketched-out API seems to work, so I can just use this and be happy if you decide to keep the macroexpansion-time function-calling stuff:

(defun struct% (name vars-and-accessors)
  (with-gensyms (instance)
    `(guard1
      (,instance :type ,name) (typep ,instance ',name)
      ,@(iterate (for (var accessor) :in vars-and-accessors)
                 (collect `(,accessor ,instance))
                 (collect `(guard1 ,var t))))))

(defun find-accessors (name-and-options slots-and-vars)
  (destructuring-bind
      (name &key (conc-name (symb name '-)))
      name-and-options
    (iterate (for (slot var) :in slots-and-vars)
             (for accessor = (symb conc-name slot))
             (collect (list var accessor)))))

(defpattern struct (name-and-options &rest slots)
  (let ((name-and-options (ensure-list name-and-options)))
    (struct% (first name-and-options)
             (when slots
               (etypecase (first slots)
                 (keyword (find-accessors name-and-options (subdivide slots 2)))
                 (symbol (find-accessors name-and-options (mapcar #'list slots slots)))
                 (cons slots))))))
fare commented 7 years ago

Note that you can use the MOP (portably via closer-mop) to find the name of accessors and keywords for given slots; or you can use slot-value — however, the CLHS does not guarantee that this works on structures (as defined by DEFSTRUCT), and I haven't tested how portable that would be and/or whether closer-mop tries to abstract over implementations that would fail to provide MOP access to structures (which would be a good extension to closer-mop, if not done already).

guicho271828 commented 7 years ago

Oh I forgot to note ( and the fact itself ) that it is able to disable this new scheme through (defvar *arity-check-by-test-call* t) --- so (setf *arity-check-by-test-call* nil) suffice.

guicho271828 commented 7 years ago

your idea on (structure (<struct> (:conc-name <c>)) <slot> ...) sounds good, and perhaps I can implement it without affecting the compatibility.

guicho271828 commented 7 years ago

Also I would convert style-warning to simple-condition. That would not cause quicklisp to reject the project while still printing the message.

guicho271828 commented 7 years ago

regarding trivia.optima-compat --- perhaps its time to move to version 2?

guicho271828 commented 7 years ago

At least I made the style warning more verbose. a5a091b

guicho271828 commented 4 years ago

At 54ddc5c7c8915e3f486ca57df34997007b3809c9 , the style warning is shown only once after the library is loaded. This should reduce the annoyance in the most common use cases.