clj-suitable icon indicating copy to clipboard operation
clj-suitable copied to clipboard

Suitable is running side-effects in -> chains for pure ClojureScript (non-interop)

Open tekacs opened this issue 4 years ago • 8 comments

Thanks so much for the amazing library!

I just got it set up for the first time -- and I'm finding that in -> chains, I'm seeing side-effects being evaluated even when the results are pure ClojureScript.

I've included a video below that demonstrates this by simply using (-> (println "Hello") (println "Hello")).

My deps look like this -- and I'm using latest stable Calva (v2.0.232):

{nrepl/nrepl       {:mvn/version "0.8.3"}
 cider/cider-nrepl {:mvn/version "0.26.0"}
 org.rksm/suitable {:mvn/version "0.4.1"}}

https://user-images.githubusercontent.com/63247/148339931-3897c0f2-813e-46d6-9b14-2dc4294fc528.mp4

tekacs avatar Jan 06 '22 06:01 tekacs

I think there are always side-effects (or rather evaluations) when dynamic completion is enabled https://github.com/clojure-emacs/clj-suitable#beware-the-side-effects

bbatsov avatar Jan 06 '22 07:01 bbatsov

@bbatsov from the linked section:

Moreover, also chains of methods and properties will be evaluated upon completion requests. So for example, asking for completions for the code / cursor position (-> js/some-object (.deleteAllMyFilesAndStartAWar) .|) will evaluate the JavaScript code some-object.deleteAllMyFilesAndStartAWar()!

This only applies to JavaSript interop code, i.e. JavaScript methods and properties. Pure ClojureScript is not inspected or evaluated.

To be clear I fully expect side-effects to execute when requesting a completion in interop position, so in the cases of (-> (println "Hello") .|) or (.| something), as suggested in the example above.

It seems in contravention to the section you linked however (see above) that dynamic evaluation would happen when using a chain of 'pure ClojureScript'. I imagine that Calva is requesting completions and the .someJavaScriptFunction completions are being... requested, but to be clear the aim here (and the implementation that the linked section suggests) would be to only trigger when specifically requesting an interop completion.

In the example I gave, I was using (-> (println "Hello") (prin|)) -- none of this is requesting an interop completion and I would rather that suitable do as its docs suggest and not try to make a dynamic suggestion here.

tekacs avatar Jan 06 '22 08:01 tekacs

I guess I should have read the docs first. :D Probably @rksm can shed more light on this.

bbatsov avatar Jan 06 '22 12:01 bbatsov

What appears to be wrong in your snippet is that it has top-level side-effects: the side-effects are performed at compile-time.

Even the clojurescript compiler itself would perform those printlns.

So it's a pattern that is best avoided regardless of clj-suitable; one also can see it discouraged in JVM clojure, etc.

So instead, you can either wrap the -> in a defn, or maybe in a comment form where I also wouldn't expect the side-effects to be performed.

Let us know if you see a difference.

Cheers - V

vemv avatar Jan 06 '22 14:01 vemv

Hey @vemv sorry that's my fault for cropping the screenshot -- it is inside a (comment block (the top of it is off the top of the video because it had other things in it.

In fact in the video you can see the trailing ), which is the comment block closing -- it's definitely not top-level!

tekacs avatar Jan 06 '22 18:01 tekacs

Ah thanks!

That's interesting/alarming.

Could you confirm that the same unfortunate effect also happens if wrapping the -> under a defn?


(I do have an idea of what could be going on: a specific interaction between Suitable and https://github.com/alexander-yakushev/compliment/)

vemv avatar Jan 06 '22 21:01 vemv

@vemv I just tested it and it does indeed happen instead defns too!

tekacs avatar Jan 08 '22 01:01 tekacs

Thanks. Aside from an interaction between suitable and compliment, it could be a simple bug in whatever code tries to distinguish between interop and non-interop code.

I tried to follow a bit what's going on ns suitable.js-completions but it's bit of a dense ns that would need a closer inspection.

If you'd generously be able to check it out and do some experiments / deftests, that would be most welcome.

(I've done some maintainance for Suitable but I don't use it day-to-day; new contributions are quite needed)

vemv avatar Jan 08 '22 17:01 vemv

I'll give a shot to this one.

So far I've observed that if the first member is a variable, no side-effects are triggered when hitting TAB here:

image

However, if it's a constant, side-effects are triggered when hitting TAB here:

image

...maybe it's purely coincidence, as one cannot run the code in the first screenshot without causing an exception.

vemv avatar Jul 22 '23 18:07 vemv