This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
2017-02-23
Channels
- # aws-lambda (1)
- # beginners (11)
- # boot (456)
- # cider (3)
- # cljsrn (7)
- # clojure (340)
- # clojure-berlin (6)
- # clojure-dev (207)
- # clojure-germany (12)
- # clojure-greece (3)
- # clojure-italy (3)
- # clojure-russia (12)
- # clojure-spec (42)
- # clojure-uk (29)
- # clojured (7)
- # clojurescript (125)
- # datascript (1)
- # datomic (47)
- # defnpodcast (4)
- # emacs (30)
- # events (7)
- # hoplon (13)
- # instaparse (64)
- # jobs (13)
- # jobs-discuss (1)
- # lein-figwheel (1)
- # leiningen (10)
- # luminus (1)
- # lumo (14)
- # off-topic (10)
- # om (16)
- # om-next (3)
- # onyx (1)
- # pedestal (3)
- # protorepl (5)
- # re-frame (17)
- # reagent (66)
- # ring (1)
- # ring-swagger (13)
- # spacemacs (12)
- # specter (4)
- # untangled (272)
- # vim (4)
- # yada (24)
odd, I get new reflection warnings in some go block where there were none before the upgrade. The symbols are hinted before the go block and it seems that metadata is somehow lost later.
the new core.async release contains some changes to how the local environment crosses in to the go macro code, and also changes to how try/catches are handled. so a first step to narrow things down might be to add and intermediate let inside the go block (but outside the try/catch) that rebinds and hints
-> https://github.com/mpenet/alia/blob/master/modules/alia-async/src/qbits/alia/async.clj#L102-L139
Reflection warning, qbits/alia/async.clj:109:19 - reference to field getAvailableWithoutFetching can't be resolved.
Reflection warning, qbits/alia/async.clj:109:19 - reference to field next can't be resolved.
Reflection warning, qbits/alia/async.clj:109:19 - reference to field isFullyFetched can't be resolved.
Reflection warning, qbits/alia/async.clj:109:19 - reference to field fetchMoreResults can't be resolved.
no warnings on master. you can test it with lein modules install && lein test (but you need cassandra running heh)
it looks like the core.async version on master is one or two versions old, have you tried the intervening versions?
I see 'Reflection warning, clojure/tools/reader/impl/commons.clj:112:31 - call to method endsWith can't be resolved (target class is unknown).' on master with each version up to the latest (assuming I am change versions correctly, not sure how this project works)
i am just running the tests at the moment to test things, with the commands mentioned earlier
for the relfection to show up you need to update core.async in alia-async, bump the version in alia-all projects.clj and then lein install tho
$ java -cp `lein classpath` clojure.main -e "(do (set! *warn-on-reflection* true) (require 'qbits.alia.async))"
Reflection warning, clojure/tools/reader/impl/commons.clj:112:31 - call to method endsWith can't be resolved (target class is unknown).
$
I change alia-async's project.clj and the change is reflected in git diff, but lein deps :tree is still showing the old version
this branch shows the issue, you can just launch a repl from alia-async without changing anything
I guess it's prolly related to ASYNC-155, but I am not really familiar with this code tbh
there are asm version diffs between tools analyzer and cassandra-driver-core, but I doubt it has any effect
another odd one:
(defn foo [x]
(let [y nil]
(a/go
(loop []
(let [t (.getTime ^Date x)]
(println t))))))
Slimmed down case without all cassandra mess. It's probably related to the other issue I encountered. /cc @alexmiller
explodes here https://github.com/clojure/core.async/blob/master/src/main/clojure/clojure/core/async/impl/ioc_macros.clj#L1099
(require '[clojure.core.async :as async])
(set! *warn-on-reflection* true)
(defn f []
(let [a "foo"]
(.substring a 1 2)
(async/go
(loop []
(.substring a 1 2)
(async/<! nil)
(recur)))))
anything that regressed and involves a local crossing over from an outer env into a go block is likely my fault
@bronsa, looks like a nil check is missing here: https://github.com/clojure/core.async/blame/master/src/main/clojure/clojure/core/async/impl/ioc_macros.clj#L1098 should be something like (some-> lb .hasJavaClass)
. But maybe it's just a surface issue
I gathered all the repro we have here: https://github.com/mpenet/async-bug/blob/master/src/async_bug/core.clj
compiler.java returns true for (.hasJavaClass lb)
when a a lb is bound to a nil literal
I was going with the assumption that getJavaClass
would return Class
rather than Class U Nil
if hasJavaClass
returned true, that doesn't seem to be the case
@mpenet yeah I'm quite sure useful
by amalloy has a similar usage that might be similary broken
let's see what @alexmiller thinks about it
preserving type hints has to be the greatest source of incidental complexity while doing lexical/AST transformations in clojure
the reflection bug doesn't seem like a regression caused by ASYNC-138 strictly speaking, but an existing bug similar to ASYNC-155 that the transformation done by ASYNC-138 simply made evident more often
repro that doesn't involve ASYNC-138 and causes a reflection warning on the previous version of core.async too:
(defn foo []
(a/go
(let [^String a (identity "foo")]
(loop []
(.substring a 1 2)
(a/<! nil)
(recur)))))
the first reflection warning I got this morning didn't trigger with previous releases tho
yeah @mpenet ASYNC-138 causes code to be rewritten in a way that triggers that pre-existing bug
the bug is pre-existing, we're just hitting it now because of the code rewrite from ASYNC-138
I've carded the reflection warning in http://dev.clojure.org/jira/browse/ASYNC-187
the state machine emits differently based on whether a path contains a parking call or not
@tbaldridge OT: do you ever regret using that state monadic style for ioc-macros or do you think it was the best approach still? Asking because I find it very hard to follow every time I'm debugging something
@bronsa absolutely, if I had it to do over again, i'd replace it with some sort of mutable writer object, or dynamic vars.
dynamic vars have worked fine in the few tools.analyzer passes that required cross-node state to be backtracked
@bronsa is http://dev.clojure.org/jira/browse/ASYNC-187 fixed by http://dev.clojure.org/jira/browse/ASYNC-186 patch?
@alexmiller what do you think about https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L2005-L2011 ?
I saw your comments earlier. don’t know.
I wouldn’t gate on that changing any time soon
seems like most uses of getJavaClass() guard against null as a possible return
even if the has returns true
so doesn’t seem accidental
the question is: if nil returned false for hasJavaClass(), what is the impact. but that would require more work to understand.
confirmed, the reflection is gone on alia-async with the patch (I didn't apply the other one tho)
@tbaldridge could you give http://dev.clojure.org/jira/secure/attachment/16516/0001-ASYNC-187-preserve-type-info-of-locals.patch a look, I'm not completely sure I got the pop-binding
bits right
AFAIU it should be right, as we're pushing n times via (all (map let-binding-to-ssa bindings))
, then popping them n times, pushing once again via the extra push-alter-binding that I added and then we only need to pop once
👍 looks good. I'm reading the code the same way
and yeah, now that we have tools analyzer, we should look at replacing the monad stuff at some point.
agreed
yeah I kindof hacked in t.a when it first came out, and there's a lot of overlap there. We could make several bits of the IOC macro be t.a. passes (maybe you've already done that.
I'm getting an itch to try and rewrite the core.async frontend as a t.a. pass at some point
eh, the simpler we can make the ioc code the less edge-cases we have.
yeah, and at some point I'd like to refactor the code to not use atomic object cells. That not only hurts boxing, but there is no reason to keep them atomic, they could be volatile instead.
altho i predict a rewrite would break tons of stuff for a while, so not sure if it's worth it
I guess it could be an experimental additional frontend, no need the 2 frontends can't coexist
that's kindof my thing with type hinting as well. It's enough of a pain to do type hinting in go-blocks that it makes me refactor code out of a go scope and into a function...so win-win?!?
@tbaldridge heh, "it's a feature not a bug" :)
@alexmiller can we expect a new core.async release soon'ish? (wondering if I should just revert some upgrades I did, or wait until a new version comes out)
@alexmiller don't hate me
Well thx for the heads up
Release is already out there
I think we've all learned a lesson here today
i don't think there's any difference between the patch you pushed and the one in the ticket
well, you should use the patch that's on the ticket now rather than the one that's on master 😬 the one that you pushed works but isn't technically correct
the technical details is that it doesn't include the pop-binding stuff i was talking with tbaldridge above
which by accident still fixes the issue and doesn't cause any bugs that I can think of
Make a new ticket and a new patch as pennance