Add CompletionStage implementation for deferred

[cosineblast]

This provides an implementation and tests for all CompletionStage methods for deferreds using potemkin.

[KingMob]

OK, but rename the param from f for Future to something like cs for CompletionStage.

[KingMob]

Might as well just make the existing calls use potemkin directly, if they're swappable. As macros, they have no performance penalty, but we don't need to keep these around. The utils ns is undocumented, so nobody should be counting on it.

[KingMob]

I assume this is to prevent NPEs from being thrown later in another thread?

You mostly have the right idea, but there's a bad practice here. You're not capturing the values before inserting them.

In this particular case, it doesn't matter, but only because you used the original form exactly once. Had you inserted value more than once, it could be evaluated multiple times, which you rarely want. (The str call doesn't count, because it won't re-evaluate the argument.)

You should get into the habit of capturing, to ensure a form is evaluates only once, like:

(defmacro ^:no-doc assert-some
  "Throws NullPointerException if any of the aruments is null."
  [& values]
  `(do
     ~@(for [value values]
         `(let [value# ~value]
            (when (nil? value#)
              (throw (NullPointerException. ~(str value " was null"))))))))

If I had to use value again, by using value#, I'd be reusing the already-evaluated value, instead of potentially re-evaluating.

Also, be sure trailing parens are all on the final line.

[cosineblast]

Thank you for the advice, taking note on that. I placed these NPE assertions since the documentation for CompletionStage states:

Additionally, while arguments [...] of type T for methods accepting them may be null, passing a null value for any other parameter will result in a NullPointerException being thrown.

And about the trailing parens, I'm checking some clojure style tool before writing my next line of clojure, sorry for this slip

[KingMob]

Well, attempting to use a null will also results in an NPE being thrown, so in single-threaded scenarios, you might not care to do anything special with the arg, and just wait for it to blow up anyway.

In multi-threaded scenarios, however, it can be harder to debug when the NPE occurs on a different thread, so checking and throwing here is fine.

---

The best summary of the community style is at https://guide.clojure.style/. Ironically, Manifold predates a lot of that, so don't take style hints from Manifold 😄

[KingMob]

I took a closer look at this, and realized that you may have had it right the first time, if we tighten up what we accept.

The default macro behavior should always be to capture with a let, since you don't know if the macro will be passed a symbol or a form, but in this case, assert-some is only ever used 1) by us, and 2) with fn param symbols, so we can restrict it and throw a compile-time error if the macro is passed a form instead. And in that case, we can skip the let, since there's no need to capture.

It would look like this:

(defmacro ^:no-doc assert-some
  "Throws NullPointerException if any of the arguments is nil. Only works on symbols.

   Usage:
   (defn foo [x y z]
     (assert-some x y z)
     ...)"
  [& values]
  (when (not-every? symbol? values)
    (throw (IllegalArgumentException. "assert-some only accepts symbols")))
  `(do ~@(for [value values]
           `(when (nil? ~value)
              (throw (NullPointerException. ~(str value " was null")))))))

Finally, can you move assert-some to the utils ns?

[KingMob]

Looks good, but it needs some changes.

Naming

For starters, the param names are a bit confusing. operator makes me think of +, *, etc. Let's just call it f, which is the standard clojure name for a function.

Ditto for this as a param. I don't like it (even though I know it's already used here and there in Manifold). It doesn't technically refer to the object it's helping, nor the helper fn itself. If it's a deferred, it should be marked as such. (And in that case, chain' is preferred over chain.)

Typically, deferred params are called d. When you have two, I'd suggest d1 and d2.

Minor notes

If classes like java.util.function.Function, BiFunction, etc. are already imported, they don't need to be fully-qualified.

[KingMob]

Can you explain your use of success-deferred and flatten-deferred? I'm trying to understand why you're unwrapping or rewrapping return values.

My guess is your "operators" are returning a deferred or CompletionStage, and then future-with wraps those in another deferred, is that right?

[KingMob]

I'd like to nail down the design first, but before we commit, it may be better to turn async-for and async-for-dual into macros, FYI

[cosineblast]

Thank you for all this feedback!

[KingMob]

Any news? I'm looking forward to this!

[cosineblast]

I believe I will have covered the PR review changes by march 26

[KingMob]

Sounds good. I have to fly the 27th, and then it's my birthday the 28th, so I probably won't get back to it until late next week.

[cosineblast]

Also, happy birthday

[KingMob]

Did Github show it was my bday? On Tue Mar 28, 2023, 09:14 AM GMT, figurantpp ***@***.***> wrote: Also, happy birthday — Reply to this email directly, view it on GitHub <#225 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHHB5KHSPMPYR7B7ESQZFTW6KTX7ANCNFSM6AAAAAAVW26BIU> . You are receiving this because your review was requested.Message ID: ***@***.***>

[cosineblast]

Sounds good. I have to fly the 27th, and then it's my birthday the 28th, so I probably won't get back to it until late next week.

You commented about it last week

[KingMob]

Sounds good. I have to fly the 27th, and then it's my birthday the 28th, so I probably won't get back to it until late next week.

You commented about it last week

Oh, uhm, yes. Sorry, the last few weeks have been a bit of a blur. I was prepping for a podcast interview, and I've forgotten everything else going on.

[KingMob]

EDIT: I checked cljdoc.org, and ^:no-doc isn't needed here, since private fns aren't documented.

We probably need to add some metadata to the new fns. ^:no-doc should be added, since I don't want these fns to appear in the public documentation, and ^:private should be propagated in the macros to the async versions.

I can take care of it if you like, but if you really want to, it would look something like this for def-async-for:

(defmacro ^:no-doc def-async-for
  "Defines a CompletionStage async version of the function associated with
   the given symbol."
  [fn-name]
  (let [async-name (symbol (str (name fn-name) "-async"))
        orig-private (:private (meta (resolve fn-name)))]
    `(defn ~(vary-meta async-name assoc :private orig-private :no-doc true)
       ([d# f#]
        (~async-name d# f# (or (ex/executor) (ex/execute-pool))))
       ([d# f# executor#]
        (~fn-name (onto d# executor#) f#)))))

It uses resolve to get the Var that the fn-name's symbol points to, then retrieves the metadata off that var. Then it attaches that to the symbol in the new defn. If you run (set! *print-meta* true) and use macroexpand-1 (or macroexpand), you can see it.

[KingMob]

So, it looks pretty good! I have some suggestions for slight improvements, but nothing that's a showstopper. If you're sick of this PR, we can merge it, and I'll apply my own suggestions. If you want to keep at it, I won't stop you, though.

How do you want to be thanked in the changelog? I usually put people's names in, but I can leave it at your username if you prefer. Whatever you want.

[cosineblast]

I checked cljdoc.org, and ^:no-doc isn't needed here, since private fns aren't documented.

Considering the def-async-for functions are only being used for the CompletionStage implementation functions, why would replacing defn with defn- in the expansion not work well?

If you're sick of this PR, we can merge it, and I'll apply my own suggestions. If you want to keep at it, I won't stop you, though.

I have some free time during the following few days, so I can keep working on this PR for the time being, I can also learn something from the suggestions, so I think it's worth the shot.

And in regards to naming, I go by Renan Ribeiro, and would rather have that in the changelog, thanks for asking!

[KingMob]

I checked cljdoc.org, and ^:no-doc isn't needed here, since private fns aren't documented.

Considering the def-async-for functions are only being used for the CompletionStage implementation functions, why would replacing defn with defn- in the expansion not work well?

It will work just fine. Capturing the metadata of the original var has the slight advantage that it will ensure the original and the async version are always...in sync. 😄 So if we ever wanted to make some public, but not others, the macro would support that.

Realistically, though, all those fns will be either public or private, so altering the macro to use defn- is perfectly fine, and 100x easier to understand/debug.

If you're sick of this PR, we can merge it, and I'll apply my own suggestions. If you want to keep at it, I won't stop you, though.

I have some free time during the following few days, so I can keep working on this PR for the time being, I can also learn something from the suggestions, so I think it's worth the shot.

And in regards to naming, I go by Renan Ribeiro, and would rather have that in the changelog, thanks for asking!

You got it!

I just approved it, assuming the only thing left is to update the macro to use defn-.

[cosineblast]

Yep, it's all done now