search

atlas-engineer / nhooks

MIT License
18 stars 5 forks source link

CCL support? #6

Closed Ambrevar closed 2 years ago

Ambrevar commented 2 years ago

Serapeum contrib hooks have support for CCL since the ftype is declaimed.

With the `nhooks' approach, it does not work anymore as highlighted here: https://github.com/atlas-engineer/nhooks/pull/5#issuecomment-1015349890

Any idea, anyone?

kchanqvq commented 2 years ago

Serapeum contrib hooks have support for CCL since the ftype is declaimed.

Are you sure they work as intended? The problem demonstrated in https://github.com/atlas-engineer/nhooks/pull/5#issuecomment-1015358826 will probably be still there, i.e. the function definition for the handlers is effectively unchecked, e.g. it will happily let you define a handler (lambda (x) (* x 2)) as (function (string) string).

Ambrevar commented 2 years ago

It should work for functions with well-defined types.

kchanqvq commented 2 years ago

I don't see any case that will be caught by the previous implementation but not by the current one. Can you give an example? declaim won't let CCL check the function body. It will check at call site but that is not very useful -- for make-handler-* in almost every case it is still a n-op.

> (declaim (ftype (function ((function (string) string)) t) foof))
> (compile nil (lambda () (foof (lambda (x) (1+ x))))) ; no error

It won't check even if you pass a function with defined type (basically, no higher order checking)

> (declaim (ftype (function ((function (number) number)) t) foof))
> (declaim (ftype (function (string) string) c1))
> (compile nil (lambda () (foof #'c1))) ; no error
Ambrevar commented 2 years ago

If a function has a declaimed type, then some code (the call site) adds this function to a hook, the previous implementation should catch the error with CCL.

It only works with named functions with declaimed types indeed.

Am I getting this wrong?

kchanqvq commented 2 years ago

If a function has a declaimed type, then some code (the call site) adds this function to a hook, the previous implementation should catch the error with CCL. It only works with named functions with declaimed types indeed. Am I getting this wrong?

Are you sure that will happen? See my last example above:

> (declaim (ftype (function ((function (number) number)) t) foof))
> (declaim (ftype (function (string) string) c1))
> (compile nil (lambda () (foof #'c1))) ; no error

foof is basically make-handler-* and c1 is a handler function with the wrong type. It doesn't seem to work. I guess CCL doesn't check higher-order functions properly.

Ambrevar commented 2 years ago

After some investigation, I found out that call-site type checking is indeed only done on "simple" types, in particular it does not work when the argument is a function.

Well, then in a way it's good news, means that nhooks is strictly superior to serapeum/contrib/hooks!

Feel free to close this issue.