Possible ClojureScript compiler issue...

[John Szakmeister]

I've been using ClojureScript rather successfully for a year now, but
ran into an interesting issue recently.

I've discovered that js->clj isn't quite working as expected. After
quite a bit of tearing things down and apart, I discovered it was one
of the keys in some JSON data coming back from the server that looks
like:

"Bi": 8

And the resultant error would be:

No protocol method IEncodeClojure.-js->clj defined for type
object: [object Object]

I don't know why this particular sequence caused a problem, but I
ended trying to vary things a bit just to see what happens. If I use:

"Bi": 0

I don't see the issue. Any non-zero value though does seem to bring
the problem about for this sequence of letters though. Similarly, I
tried varying the two letter sequence, and discovered two others with
similar problems: Ai, and ba. The latter brought about a different
error:

[object Object] is not ISeqable

And the pattern with the value needing to be non-zero follows. If the
value associated with the key is 0, then I don't see a problem.

As I wasn't seeing this issue before, I backpedaled my version of the
ClojureScript compiler and eventually found that 1.9.56 didn't seem to
sufffer from the problem. After checking out ClojureScript, and
bisecting more I found that commit
bcf60ce194e5292fbc5c4b2d89dfc5a7b886b94c is when the behavior changed
from behaving correctly to behaving badly, but I think it's a false
positive.

I then created a much smaller test case, with no other libraries
involved. This time it's a different set of values that are broken:

* "Ta" with a non-zero value gives: [object Object] is not ISeqable

* "Ub" with a non-zero value gives: No protocol method
IEmptyableCollection.-empty defined for type object: [object Object]

* "Wb" with a non-zero value gives: No protocol method
IEncodeClojure.-js->clj defined for type object: [object Object]

If I back up to before the previously mentioned commit, I can still
cause the problem, though the letter sequences change a bit.

Also, this only appears to happen with advanced optimizations. With
no optimizations or with whitespace optimizations, there's no issues.
So it smells like a strange interaction between the ClojureScript
runtime and the compiler.

Has anyone seen an issue like this before? Does anyone have any ideas
what is going on? We have these short keys in our data, and it'd be
pretty painful to make them something else (especially since the names
are meaningful to those in the know, despite them being cryptic--they
refer to bit settings on various motors).

I'd be happy to file a ticket, if that's what needs to get done too.

Thanks!

-John

PS Here's the code I used to hunt down problematic keys:

(def alphabet [\a \b \c \d \e \f \g \h \i \j \k \l \m \n \o \p \q \r \s \t \u
\v \w \x \y \z \A \B \C \D \E \F \G \H \I \J \K \L \M \N \O \P
\Q \R \S \T \U \V \W \X \Y \Z])

(enable-console-print!)

(defn ^:export main-page []
(println"Starting...")
(doseq [c1 alphabet
c2 alphabet
i (range 256)]
(let [s (str "{\"" c1 c2 "\": " i "}")]
(try
;; (prn :parse-s (js/JSON.parse s))
;; (prn :js-clj-parse (js->clj (js/JSON.parse s)))
(js->clj (js/JSON.parse s))
(catch js/Error e
(println "didn't work" s (str e))))
))
(println "Done!"))

Then you can observe the results from the JS console.

[David Nolen]

This issue is somewhat to be expected if you're going to use `js->clj`. This issue has nothing to do with ClojureScript compiler versions - you just got lucky before. Google Closure will collapse properties, but some of these collapsed properties are going to be used to determine protocol membership. That's it.

I suggest just avoiding `js->clj` and using your own simple helper for recursively converting JSON into Clojure values. Changing the (admittedly questionable) behavior of `js->clj` will only lead to more breakage.

HTH,

David

--
You received this message because you are subscribed to the Google
Groups "Clojure" group.
To post to this group, send email to clo...@googlegroups.com
Note that posts from new members are moderated - please be patient with your first post.
To unsubscribe from this group, send email to
clojure+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/clojure?hl=en
---
You received this message because you are subscribed to the Google Groups "Clojure" group.
To unsubscribe from this group and stop receiving emails from it, send an email to clojure+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[John Szakmeister]

On Sat, Oct 15, 2016 at 2:49 PM, David Nolen <dnolen...@gmail.com> wrote:
> This issue is somewhat to be expected if you're going to use `js->clj`. This
> issue has nothing to do with ClojureScript compiler versions - you just got
> lucky before. Google Closure will collapse properties, but some of these
> collapsed properties are going to be used to determine protocol membership.
> That's it.

Wow. I did not that expect that at all. It makes sense, but it's unfortunate.

> I suggest just avoiding `js->clj` and using your own simple helper for
> recursively converting JSON into Clojure values. Changing the (admittedly
> questionable) behavior of `js->clj` will only lead to more breakage.

I'll definitely look at alternatives. It'd be nice if js->clj had
documentation on this shortcoming though, and perhaps pointers to
better alternatives.

Thanks for the help David!

-John

[Antonin Hildebrand]

Unfortunately, this problem is not specific to `js->clj` only.

I believe in general under :advanced optimizations, any object which was created or modified by a code which 

was not subject of the same closure compiler optimization pass could exhibit similar problems when used with ClojureScript core functions like `satisfies?`.

That also includes working with external data (your case), and working with objects created/modified by adding properties by string names.

To illustrate, I created a screenshot of cljs type instance with two protocols, to see the internals in dev mode:

https://dl.dropboxusercontent.com/u/559047/cljs-protocol-internals-01.png

In the selected text I highlighted part of generated code for `satisfies?` call.

ClojureScript adds/checks $cljs prefixed properties to objects. These internal properties get renamed by closure compiler.

So any object which happens to have one of those renamed names independently added as their property will potentially confuse functions like `satisfies?`.

Possible solutions I see:

1. use string names for clojurescript internal properties, and avoid clashes by using "unique-enough" prefix even in advanced mode - still not safe solution, but would minimize clash chances

or 

2. start tracking which objects belong to cljs land, have one "unique-enough" string name as a marker, clojurescript functions like satisfies? would check this marker before proceeding further. Still dirty, one could clobber cljs properties by modifying a cljs-land-object from unaware Javascript code. And this would probably change behaviour of some existing code.

or

3. use prototypal inheritance and "hide" all ClojureScript internal properties in a new link in the prototype chain, plain javascript objects would miss this link so it could be easily detected, properties would not clash even if they got same names. ClojureScript functions like satisfies? would properly

walk the chain and read properties from proper link which belongs only to ClojureScript. Ale we would not need any special "magic" marker - the Javascript type of the link in prototype would safely identify it.

I believe this would be correct solution. But I guess, this would be too dramatic change in ClojureScript internals and might break a lot of other things I currently don't understand. Also performance could get a hit.

Better ideas, anyone? :-)

ps. don't use :advanced mode when programming atomic reactors in ClojureScript ;-p

[David Nolen]

It's true that there other scenarios where you can encounter this but it gets reported infrequently enough that I don't think these kinds of property name collisions are common.

Still it has come up before and I think the simplest solution least likely to adversely affect performance is to test for a def'onced unique object instead of `true`.

David

--

[Thomas Heller]

FWIW I investigated the check with "true" and a sentinel value and found them to both have a small performance impact over just checking for a true-ish property.

http://dev.clojure.org/jira/browse/CLJS-1658

The impact is really small so it might be worth the trade-off.

/thomas

clojure+u...@googlegroups.com

For more options, visit this group at
http://groups.google.com/group/clojure?hl=en
---
You received this message because you are subscribed to the Google Groups "Clojure" group.

To unsubscribe from this group and stop receiving emails from it, send an email to clojure+u...@googlegroups.com.

[Antonin Hildebrand]

I think one reason why these issues are reported infrequently is because it is really hard to track them down. I can imagine people disable :advanced optimizations or restructure their code instead of hunting down the real cause. John went really far to isolate/demonstrate the issue.

Thanks for posting that sentinel idea, Thomas. It looks really simple. I think instead of sentinel object we could use some large random integer which could have similar performance profile as checking for boolean.

I would add another possible idea:

4. we could alter google closure compiler with ClojureScript-aware renaming logic, it would preserve prefix $cljs prefix when renaming properties[1]. Also in ClojureScript compiler we would rename all cljs-related properties to use this convention.

Pros:

1. This would decrease a chance of clashes. When someone runs into it - it would be quite self-describing that "$cljs" named property is involved.

Cons:

1. This would lead to slightly bigger generated code (all the $cljs prefixes which would previously disappear).

2. The bigger downside IMO is that we would have to maintain our own fork of Closure Compiler. Which is not that scary with git. It is pretty easy to automate rebasing of a few patch-commits on top of arbitrary complex foreign repo.

[1] https://github.com/google/closure-compiler/blob/5616a66e40c5dc6ec9be9beacfc0ea20d47bbcfc/src/com/google/javascript/jscomp/RenameProperties.java#L284

[Thomas Heller]

Yeah the issue can be quite confusing since the error produced may complain about protocols you are not even calling anywhere. I've run into it several times when the code ended up checking protocols on js/window. Since that has all the munged global variables closure produces just about anything can result in a false positive there.

As for the sentinel, there is no notable difference to which sentinel is used. Strings, Numbers, boolean, object ... doesn't really matter. object with ===/identical? check seemed the simplest without a chance for a false positive (any number, string, boolean can in theory still clash).

4) would not be acceptable to me as there are quite a lot of "$cljs..." properties and increasing the code size for such a tiny benefit isn't really justified IMHO.

While this issue can be very confusing you will hardly ever run into it when following best practices. As David suggested using a custom js->clj here would prevent the issue and is probably the best course of action regardless.

/thomas

[John Szakmeister]

On Tue, Oct 18, 2016 at 2:59 AM, Thomas Heller <th.h...@gmail.com> wrote:
[snip]

> While this issue can be very confusing you will hardly ever run into it when
> following best practices. As David suggested using a custom js->clj here
> would prevent the issue and is probably the best course of action
> regardless.

Which best practices? Is there a good place to read about them? I've
not seen anything that would have steered me away from this problem.
In fact, I've seen quite the opposite: js->clj appears to be *the* way
to convert from JavaScript data structures to ClojureScript ones. :-(

FWIW, I did end up putting something together that was able to do what
I needed, but it could have easily gone a different direction.
js->clj was being called in a library that I'm using (cljs-ajax), and
it, fortunately, had a knob that I could turn to just get the raw json
back out without running anything through js->clj. Had the knob been
missing, I think the solution would have been much more painful as I'd
either have to fork and maintain a copy of the library, migrate to a
different library, or write my own to replace it with. :-(

I also like the sentinel idea. I hope some version of your patch is
incorporated.

-John

[Thomas Heller]

Don't think there is a best practice for your case in particular.

The issue is that js->cli is built on top of protocols to allow converting custom JS types to CLJS types. Which makes it extensible for the price of checking protocols. In your case you are converting JSON which cannot have custom types, so a custom converter only checking for the very few possible JSON types would "fix" your problem (and would probably be a lot faster).

The case can be made that cljs-ajax should not be using js->clj when converting JSON, maybe even add a json->clj to cljs.core.

The sentinel is the "safest" solution but impacts the performance of everyone, so we should be doing more benchmarks on more platforms before deciding anything. Benchmarks and Votes on the Jira Issue would help to push this along.

Cheers,

/thomas

[Matching Socks]

A reliable "implements?" would be better than a fast-and-sometimes-wrong "implements?".

With that in mind, have you tried a distinct sentinel object, as opposed to true? 

[Gary Trakhman]

If you're parsing raw json streams/strings, I think transit claims to be a 30x perf improvement over js/JSON.parse+js->clj: http://swannodette.github.io/2014/07/26/transit-clojurescript

On Tue, Oct 18, 2016 at 6:41 AM Matching Socks <phill...@gmail.com> wrote:

A reliable "implements?" would be better than a fast-and-sometimes-wrong "implements?".

With that in mind, have you tried a distinct sentinel object, as opposed to true? 

[John Szakmeister]

Yes, you could do that, but it could also do Bad Things. Namely if
you have strings that match some of the format, it could be
misinterpreted as Transit data rather than JSON, so I don't consider
it a particularly useful solution either. It just moves where the
problem can happen. :-( If there was a way to so "ignore the transit
extensions", that would change things though.

-John

[Gary Trakhman]

Just a quick glance makes it look like handlers can be overridden, but I haven't tried this and I don't think it's documented anywhere: https://github.com/cognitect/transit-cljs/blob/master/src/cognitect/transit.cljs#L109

[John Szakmeister]

On Tue, Oct 18, 2016 at 5:59 AM, Thomas Heller <th.h...@gmail.com> wrote:
> Don't think there is a best practice for your case in particular.

Okay. Earlier in the thread you said "while this issue can be very

confusing you will hardly ever run into it when following best

practices." So it seemed to me that perhaps I missing something
important, or didn't know about a best practice that would have saved
me. I think the issue here is a bit different though. There's
nothing that would have saved short of knowing that js->clj has this
issue lurking in the background. :-)

> The issue is that js->cli is built on top of protocols to allow converting
> custom JS types to CLJS types. Which makes it extensible for the price of
> checking protocols. In your case you are converting JSON which cannot have
> custom types, so a custom converter only checking for the very few possible
> JSON types would "fix" your problem (and would probably be a lot faster).

Yep. OTOH, I don't really need the performance and js->clj would have
been just as nice to use, if it worked correctly. :-)

> The case can be made that cljs-ajax should not be using js->clj when
> converting JSON, maybe even add a json->clj to cljs.core.

I think having json->clj would be a nice option. Though for
cljs-ajax, we probably need to roll our own since the api wouldn't be
available in older ClojureScript versions.

> The sentinel is the "safest" solution but impacts the performance of
> everyone, so we should be doing more benchmarks on more platforms before
> deciding anything. Benchmarks and Votes on the Jira Issue would help to push
> this along.

True, but I think the correctness argument here is pretty valuable.
Again, much of what I see out there is the js->clj is the way to
convert data, and having this problem lurking seems like setting folks
up for failure. I'm pretty persistent when it comes to
troubleshooting this stuff, but I could see others being awfully
frustrated by the result. So I hope that some version of the sentinel
patch makes it in. :-)

-John

[David Nolen]

This issue is fixed in master now thanks to Thomas Heller. The performance hit is negligible. 

Thank you for the report.

David

-John

--
You received this message because you are subscribed to the Google
Groups "Clojure" group.
To post to this group, send email to clo...@googlegroups.com
Note that posts from new members are moderated - please be patient with your first post.
To unsubscribe from this group, send email to

clojure+unsubscribe@googlegroups.com

For more options, visit this group at
http://groups.google.com/group/clojure?hl=en
---
You received this message because you are subscribed to the Google Groups "Clojure" group.

To unsubscribe from this group and stop receiving emails from it, send an email to clojure+unsubscribe@googlegroups.com.

[John Szakmeister]

Fantastic! Thank you!

-John

>> clojure+u...@googlegroups.com

>> For more options, visit this group at
>> http://groups.google.com/group/clojure?hl=en
>> ---
>> You received this message because you are subscribed to the Google Groups
>> "Clojure" group.
>> To unsubscribe from this group and stop receiving emails from it, send an

>> email to clojure+u...@googlegroups.com.

>> For more options, visit https://groups.google.com/d/optout.
>
>

> --
> You received this message because you are subscribed to the Google
> Groups "Clojure" group.
> To post to this group, send email to clo...@googlegroups.com
> Note that posts from new members are moderated - please be patient with your
> first post.
> To unsubscribe from this group, send email to

> clojure+u...@googlegroups.com

> For more options, visit this group at
> http://groups.google.com/group/clojure?hl=en
> ---
> You received this message because you are subscribed to the Google Groups
> "Clojure" group.
> To unsubscribe from this group and stop receiving emails from it, send an

> email to clojure+u...@googlegroups.com.