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

General-purpose hooks and functional config #483

Closed
wants to merge 40 commits into from
Closed

Conversation

Ambrevar
Copy link
Member

@Ambrevar Ambrevar commented Nov 5, 2019

Work in progress, not done yet.

So I've addressed many of the points mentioned in #419.
Those hooks are based off Serapeum's hooks. They fix most (all?) issues in #383.

The biggest issue I'm facing at the moment is type-checking: I'd like to check that the handlers match the type specified in the the hook object, but it seems impossible to do in Common Lisp:

https://old.reddit.com/r/Common_Lisp/comments/ds4tpy/programmable_and_compiletime_function_typechecking/?

This is a general issue: how do we validate the type of a variable / slot that's supposed to host a function?

I've implemented some form of global, ahead-of-time hooks that can be bound to any symbol / object. It's a very naive implementation, I don't know if that would suit everyone's needs.

This implementation makes some parts of Serapeum's hooks irrelevant, such as run-hook-with-args and derivatives.

@ruricolist @scymtym What do you think?

@Ambrevar
Copy link
Member Author

Ambrevar commented Nov 5, 2019

Left to do:

@Ambrevar
Copy link
Member Author

Ambrevar commented Nov 5, 2019

@ruricolist In ruricolist/serapeum#41 (comment) you mentioned a trick to check the arity of a function. I'm not sure I understand how that would work in practice, can you explain?

@ruricolist
Copy link

The idea I had was that when you add the function to the hook, you get the arity of the hook and then (at runtime) compile a lambda that calls the function with that arity.

Here's what it looks like with SBCL:

(defun x (y z) (list y z))

(compile nil '(lambda (a b c) (x a b c)))
; caught WARNING:
;   The function X is called with three arguments, but wants exactly two.

(let ((fn (lambda (y z) (list y z))))
  (compile nil `(lambda (a b c) (funcall ,fn a b c))))
; caught WARNING:
;   The function (LAMBDA (Y Z)) is called with three arguments, but wants exactly two.

Clozure signals a warning for the global definition, but unfortunately not for the local definition.

@Ambrevar
Copy link
Member Author

Ambrevar commented Nov 6, 2019 via email

@Ambrevar
Copy link
Member Author

Ambrevar commented Nov 6, 2019 via email

@jmercouris
Copy link
Member

It occurred to me that we could use a macro called set-default or something which expands into a hook that runs after initialization, I’m much more of the opinion that compile time checking and slots is not important, the slot set function can do the checking. This is much more towards proper encapsulation of OO data structure anyways. I know this is not strictly related to only the contents of this pull request, but I can’t think of anywhere else it belongs.

@Ambrevar
Copy link
Member Author

Ambrevar commented Nov 7, 2019 via email

@Ambrevar
Copy link
Member Author

Ambrevar commented Nov 7, 2019

Great news! I've figured a way to use macros to generate typed hooks and typed handlers, and now add-hook will report a warning if called over the wrong combination of hook / handler.

The only drawback: The types must be declared before use.

In practice, it looks like this:

(define-hook-type string->string (function (string) string)) ; Need to be done only once for this type.

;; Then
(defvar *my-hook* (make-instance 'hook-string->string))

(defun my-settings1 ()
  (add-hook *foo* (make-handler-string->string (lambda (s) (str:concat "foo-" s)) :name 'foo)

(defun my-settings2 ()
  (add-hook *foo* (make-handler-string->string (lambda (s) 17) :name 'foo)))

; caught WARNING:
;   Derived type of (LAMBDA (S) :IN MY-SETTINGS2) is
;     (FUNCTION (T) (VALUES (INTEGER 17 17) &OPTIONAL)),
;   conflicting with its asserted type
;     (FUNCTION (STRING) (VALUES STRING &REST T)).

What do you think? @ruricolist ?

@Ambrevar
Copy link
Member Author

Ambrevar commented Nov 7, 2019

In particular, the above approach may supersede @ruricolist arity-checking technique since it non only checks the arity but also the argument and return value types.

@jmercouris
Copy link
Member

Is there anything yet blocking us from merging this in?

@Ambrevar
Copy link
Member Author

Ambrevar commented Dec 17, 2019 via email

@Ambrevar
Copy link
Member Author

I think that a facility to list known hooks would be very valuable to ensure
that there aren't duplicate hooks (as a user). It would also serve as feedback
(as a user), that a hook has been successfully added (or deleted).

I think you mixed up the terms here. The hook is the value that holds a list
of handlers. So it's the handler that is added to the hook.

Note that handlers cannot be duplicated with my typed-hook implementation.

My question was about listing hooks, not handlers.

I think make-instance suffices more than enough. In my opinion, there is no
checking we have to do upon creation that would justify creating a function.

Actually there is checking that could be done:

  • :combination must be a function.
  • handlers must be handlers.

Perhaps I don't understand, but couldn't one disable a handler by deleting it?
Would the point of disabling and re-enabling handlers be to keep some state in
the handler perhaps without getting rid of it? If so, then I think the user
could just as easily keep a reference to it if they really need it.

Keeping track of the handler has at least 2 use cases:

  • It makes it easy to toggle handlers on/off for testing, say, when a handler
    misbehaves.
  • A handler may be a closure and thus hold internal state. Thus if the closure
    is deleted from the hook, there is no way to recover it later.

The user could very well keep track of the handlers themselves, but then all
users interested in this feature
would have to re-implement something that this library could have provided.

If you don't do it, some user will do it eventually :-) We can make the API as
strict as we want, but people will make a library or copy paste a snippet for
sure that adds make-handler. Better that we add it, than it get's copy pasted
into thousands of init files :-D

It's Common Lisp, un-exported symbols can always be accessed with ::, so there
won't be any copy-paste anyways.

The question was whether we tell the user "this is OK to use" or not.

I see no problem with just making it run-hook. We don't say
funcall-with-args, we simply say funcall. Since there is no distinction here
in our codebase, we do not need it. Thoughts?

run-hook and run-hook-with-args are generic functions, so I cannot change
their arguments. In particular, I cannot pass extra arguments to run-hook
which is what I would like to do here for typed hooks.

Some options:

  • Change Serapeum's default.
  • Stick to run-hook-with-args.

I'm not sure I understand this question. I may be missing too much context. Can
you please explain it? Edit: just to be clear, I've also read your subsequent
follow-up, and I still don't quite understand.

When run, a hook may return a value.
But the call site may want to make the distinction between the return value of
the handlers and what is returned when the hook has no handlers.

Besides, for some combination we want to adopt a specific behaviour.
See the example with the load-hook: if it has no handler, it should return the
URL unchanged.

Does that make more sense?

@jmercouris
Copy link
Member

I think you mixed up the terms here. The hook is the value that holds a list
of handlers. So it's the handler that is added to the hook.

You are correct, I had mixed up the terms in my head for that paragraph for some reason. I still think a function to list hooks would be useful as part of the discoverability and interactivity of the platform. I imagine we could bind a command like list-hooks or something so that people can search for them.

Note that handlers cannot be duplicated with my typed-hook implementation.

How so? Could I not add two lambda handlers that do the same thing?

I think make-instance suffices more than enough. In my opinion, there is no
checking we have to do upon creation that would justify creating a function.

Actually there is checking that could be done:

  • :combination must be a function.
  • handlers must be handlers.

I think we should allow people to shoot themselves in the foot. I have consistently been against adding in more checking of user valid input. The reason is:

  1. It adds overhead to making changes
  2. It "freezes" the code at a specific point

It is too soon to do either of those things. Checking types, at this point, for most users, simply does not add as much value as a new feature. In other words, for the time being, I believe our efforts are better spent elsewhere.

The user could very well keep track of the handlers themselves, but then all
users interested in this feature
would have to re-implement something that this library could have provided.

I guess I don't see the purpose here exactly, but if you think it would be useful instead of having every user keep a reference to whatever they are testing, why not!

The question was whether we tell the user "this is OK to use" or not.

Ah, yes, export the symbol.

Some options:

  • Change Serapeum's default.
  • Stick to run-hook-with-args.

I should think that Serapeum should change, @ruricolist ?

When run, a hook may return a value.
But the call site may want to make the distinction between the return value of
the handlers and what is returned when the hook has no handlers.

Besides, for some combination we want to adopt a specific behaviour.
See the example with the load-hook: if it has no handler, it should return the
URL unchanged.

Does that make more sense?

That makes sense now, I believe. I however, do not have any ideas... I will think about it :-)

@Ambrevar
Copy link
Member Author

Yes listing the hooks would be useful, but the question is how do we list them?
It's a general problem: how do we list variables in a programming language?

  • What's the name of the hook? If we don't have a name, then the listing is
    mostly meaningless.
  • How do we store the list? How do we make sure we are not missing hooks?
  • Can we remove hooks?

How so? Could I not add two lambda handlers that do the same thing?

See the docstrings, the tests and this thread. Lambdas are only accepted with
the extra name argument, so they can be compared (see the equals function).

I think we should allow people to shoot themselves in the foot.

I think you are talking about a different thing, which is a different problem.
Users still can call make-instance anytime. The question here is whether we
add helper functions (constructors) to provide some extra checks and shorter code.

Ah, yes, export the symbol.

I'm leaning towards not exporting it. The main intent of this library is to expose
typed hooks after all, which are much more powerful than untyped hooks.

@jmercouris
Copy link
Member

See the docstrings, the tests and this thread. Lambdas are only accepted with
the extra name argument, so they can be compared (see the equals function).

Yes, I saw, but I mean two lambdas, two names, do the same thing. I don't believe this can be prevented :-D

I think we should allow people to shoot themselves in the foot.

I think you are talking about a different thing, which is a different problem.
Users still can call make-instance anytime. The question here is whether we
add helper functions (constructors) to provide some extra checks and shorter code.

I'm not talking about a different thing. I am referencing a different thing, but is the same idea here. I don't believe constructors with extra checks are necessary.

Ah, yes, export the symbol.

I'm leaning towards not exporting it. The main intent of this library is to expose
typed hooks after all, which are much more powerful than untyped hooks.

Well...as you wish :-D I don't have a very strong opinion on this.

@ruricolist
Copy link

I've changed the signature of serapeum:run-hooks to allow arguments.

Is there anything else you need from me for this?

@Ambrevar
Copy link
Member Author

@ruricolist: I think you meant run-hook :)
Your change looks good, but haven't you broken backward compatibility in the process?

@Ambrevar
Copy link
Member Author

@vindarel

Either catch the absence of handlers early, or throw a condition?

An empty hook is a valid hook. run-hook should not fail for empty hooks. See Emacs for instance.

yes disable-hook is valuable.

It is, but the question is about handlers: do we need to add a disable-handler function, or add a parameter to disable-hook?

@Ambrevar
Copy link
Member Author

@vindarel

I do, generally I like this style for its shortness and the encapsulation. And is it not needed? In make-hook you check that a name is given. The argument for a constructor?

Hooks don't have names. Did you mean something else?

(And what about even more precise constructors, like (make-string->string-hook …) for the most used ones?)

Indeed, this is what I had in mind: generate constructors in the define-hook-type macro.

@vindarel
Copy link
Contributor

Hooks don't have names. Did you mean something else?

I mixed up with handlers and make-handler.

@Ambrevar
Copy link
Member Author

I believe this is ready to merge. I'll merge tomorrow if there is no objection.

@jmercouris
Copy link
Member

No objection, please feel free to merge!

@jmercouris
Copy link
Member

May I merge this in on your behalf?

@Ambrevar
Copy link
Member Author

Ambrevar commented Dec 27, 2019 via email

@jmercouris
Copy link
Member

OK, thank you! I was just asking because I want to do quite a few file operations (renaming base.lisp to start.lisp etc) in preparation for our new work and I want the merges to be nice and easy!

@Ambrevar
Copy link
Member Author

Merged.

@Ambrevar Ambrevar closed this Dec 29, 2019
@jmercouris jmercouris deleted the wip-hooks branch June 18, 2020 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants