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

core proxy fn impl #818

Closed
wants to merge 3 commits into from
Closed

Conversation

ikappaki
Copy link
Contributor

Hi,

could you please consider an attempt to implement the core proxy fn. It partially fixes #425 .

In addition to interfaces, the fn can implement/override fn of any python class (abstract or not), because classes in python can have method that throw the not-implemented exception or they are abstract.

There is an oddity when implementing the interfaces that basilisp demands method_arityN fn to be defined. We thus defined them as dummy nil fns.

Multiarity interface fns are defined as a single function with a case form that switches to the right arity based on the number of arguments supplied to the method.

I understand that a better implementation might be possible in pure python, but thought to give it a go in the lisp. Feel free to thoroughly scrutinize the implementation and suggest alternatives.

Thanks

@ikappaki ikappaki force-pushed the feat/proxy branch 2 times, most recently from 03612fd to 6316d9a Compare January 22, 2024 07:38
fname (first f)
fargs (second f)
body `(do ~@(drop 2 f))]
[[(.replace (str fname) "-" "_") (with-meta `(fn ~(into ['this] fargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[[(.replace (str fname) "-" "_") (with-meta `(fn ~(into ['this] fargs)
[[(munge fname) (with-meta `(fn ~(into ['this] fargs)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

Comment on lines 7173 to 7191
;; Implementation of variadic functions for Basilisp
;; interfaces necessitates the inclusion of
;; additional arity method signatures, such as
;; method-name_arityN, to be registered for proper
;; functionality, even if their implementations are
;; nil.
fname-fndummy-pairs
(for [spec (rest f)]
(let [fargs (first spec)
argc (count fargs)
variadic? (try
(.index fargs '&)
(catch python/ValueError _
nil))
arity (if variadic?
"_rest"
(if (= argc 0) 0 (inc argc)))]
[(str "_" (first f) "_arity" arity) nil]))]
(into [fname-fn-pair] fname-fndummy-pairs))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why you implemented these as dummy methods rather than just implementing each arity directly? The generated Python for multi-arity functions already does this. It uses a pure Python version of the case strategy you use above but ultimately calls a function rather than putting the code inline.

I'm wondering if you wouldn't get a lot of this logic for free if you just rewrote the multi-arity methods directly as multi-arity fns with this prepended to the argument vectors.

My concern is basically that we're going to have dummy methods to appease the ABC, but those methods do exist and can be called but won't work like they would for any other implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still not clear how the multi-arity fns are implement internally and as such is difficult for me to understanding why both the dispatch (the 'case' fn) and the individual arity fns are needed.

I've provided an example at the end of this command how the macro expansion is done with my latest update

With the latest update, I populate all multi, _multi_arity0, _multi_arity2 and _multi_arity_rest fn defintion, where previously I was only providing the first and the rest were nil. Though there is no calling one another here, it is more like providing a fn definition twice, one for multi and another for the corresponding arity fn (e.g. _multi_arity2). Unfortunately I can't (apply ._multi_arity2 (first args) (rest args)) from within the multi fn so as to call the arity fn, since apply'ing to a method doesn't seem to work.

I've also tried to set the multi method to a multiarity fn, but it still complains that _multi_arity0 etc are not defined.

Perhaps you can guide me with an example how you think the type fn dictionary should be better structured, if there is a better alternative?

Thanks

example

(proxy [ITestProxyMultiVariadic] []
  (multi
    ([] 0)
    ([atm]
     (swap! atm inc)
     @atm)
    ([one two three & more] [one two three more])))

macro expands to

((python/type
  "proxy--ITestProxyMultiVariadic_14748"
  (python/tuple [ITestProxyMultiVariadic])
  (basilisp.core/lisp->py
   {"multi"
    (basilisp.core/fn
      [& args_14749]
      (basilisp.core/case
          (basilisp.core/count args_14749)
          1
          (basilisp.core/apply (basilisp.core/fn [this] 0) args_14749)
          2
          (basilisp.core/apply
           (basilisp.core/fn
             [this atm]
             (swap! atm inc)
             (basilisp.core/deref atm))
           args_14749)
          (if
              (basilisp.core/<= (basilisp.core/count args_14749) 3)
              (throw
               (basilisp.core/ex-info
                "Arguments passed to function does not match the variadic fn specification"
                {:args-spec-expected '[one two three & more],
                 :args-provided (basilisp.core/drop 1 args_14749)}))
              (basilisp.core/apply
               (basilisp.core/fn
                 [this one two three & more]
                 [one two three more])
               args_14749)))),
    "_multi_arity0" (basilisp.core/fn [this] 0),
    "_multi_arity2"
    (basilisp.core/fn
      [this atm]
      (swap! atm inc)
      (basilisp.core/deref atm)),
    "_multi_arity_rest"
    (basilisp.core/fn
      [this one two three & more]
      [one two three more])})))

;; (if-not (variadic? fn-args)
;; (throw exception)
;; (do variadic-body))
~(apply list 'case (list 'count vargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
~(apply list 'case (list 'count vargs)
~(apply list `case `(count ~vargs)

Does this work? I don't think we want to use normal quotes for names that will refer to known vars.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, thanks, updated.

@ikappaki
Copy link
Contributor Author

Hi @chrisrink10 ,

I've re-implemented the logic using let bindings, in which case the functions can be shared in the multi-arity setup, plus as you hinted there is no need to implemented my own switch fn, but can assign the multiarity fn directly to it which simplifies the logic a lot.

The new macro expansion for

(proxy [ITestProxySingleArg ITestProxyMultiVariadic] []
              (arg-simple [arg] arg)
              (multi-a
                ([] 0)
                ([one] one)
                ([one two three & more] [one two three more])))

now looks like

(basilisp.core/let
 [arg-simple_6493
  (basilisp.core/fn arg-simple [this arg] arg)
  _multi-a_arity0_6494
  (basilisp.core/fn _multi-a_arity0 [this] 0)
  _multi-a_arity2_6495
  (basilisp.core/fn _multi-a_arity2 [this one] one)
  _multi-a_arity_rest_6496
  (basilisp.core/fn
   _multi-a_arity_rest
   [this one two three & more]
   [one two three more])
  multi-a_6497
  (basilisp.core/fn
   multi-a
   ([this] (_multi-a_arity0_6494 this))
   ([this one] (_multi-a_arity2_6495 this one))
   ([this one two three & more]
    (basilisp.core/apply
     _multi-a_arity_rest_6496
     this
     one
     two
     three
     more)))]
 ((python/type
   "proxy--ITestProxySingleArg_6492"
   (python/tuple [ITestProxySingleArg ITestProxyMultiVariadic])
   (basilisp.core/lisp->py
    {"_multi_a_arity_rest" _multi-a_arity_rest_6496,
     "multi_a" multi-a_6497,
     "_multi_a_arity2" _multi-a_arity2_6495,
     "_multi_a_arity0" _multi-a_arity0_6494,
     "arg_simple" arg-simple_6493}))))

could you please have another look?

Thanks

@ikappaki
Copy link
Contributor Author

Closing this, as Chris prefers to handle it directly in the compiler

from @chrisrink10 in #895

I also have 2 practical concerns. First is that this relies on proxies, which are as yet unimplemented. I know you made an attempt to do so in Basilisp (as #818), but it is my preference to actually do this work in the compiler using a special form since I don't prefer to spread the arity-mapping logic across core and the compiler (to the extent that is possible)

@ikappaki ikappaki closed this Aug 18, 2024
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.

Proxies
2 participants