Fork me on GitHub
#clojure-dev
<
2017-02-23
>
mpenet07:02:11

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.

mpenet07:02:23

the go block inner code is also wrapped in a try/catch btw

hiredman07:02:18

the type hinting happens outside the go block?

mpenet07:02:01

I am trying to get to a repro

hiredman07:02:23

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

mpenet07:02:47

I tried that first 🙂

mpenet07:02:12

I ll double check if it's really a c.async issue first, we also upgraded a dependency

hiredman07:02:21

Ah, interesting

mpenet07:02:00

yep, core async issue

mpenet07:02:13

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.

mpenet07:02:30

(it's not on the latest core.async on github, just locally)

mpenet07:02:08

no warnings on master. you can test it with lein modules install && lein test (but you need cassandra running heh)

hiredman07:02:12

it looks like the core.async version on master is one or two versions old, have you tried the intervening versions?

mpenet07:02:03

good point, I haven't

mpenet07:02:30

the issue is really with the latest one, 0.2.395 is fine

mpenet07:02:30

removing the try doesn't change anything, still reflecting

hiredman07:02:10

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)

mpenet07:02:56

this should be harmless

hiredman07:02:26

how are you loading the code?

mpenet07:02:47

i am just running the tests at the moment to test things, with the commands mentioned earlier

mpenet07:02:03

otherwise you can launch a repl from alia-all

mpenet07:02:24

(the root of the project)

mpenet07:02:36

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

hiredman07:02:47

$  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).
$ 

mpenet07:02:27

since it's a multi module thing

hiredman07:02:01

that is with 0.3.426

mpenet08:02:51

are you sure about that?

mpenet08:02:12

I still get the same warnings. and confirmed version with deps :tree

mpenet08:02:55

ill make a branch with an easy to reproduce alia-async standalone

mpenet08:02:59

less messy

hiredman08:02:02

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

mpenet08:02:48

this branch shows the issue, you can just launch a repl from alia-async without changing anything

hiredman08:02:17

have you looked at resolving all the conflicts in deps :tree?

mpenet08:02:46

not yet no

mpenet08:02:19

seems this could be caused by tools.analyzer & co conflicts, true

mpenet08:02:41

tools.reader actually

hiredman08:02:18

well, the alia-async code on its own doesn't have any conflicts

mpenet08:02:25

but no conflicts there yeah

mpenet08:02:57

if I remove the loop (keeping it's contents tho) the reflection goes away

mpenet08:02:15

try doesn't change anything it seems

mpenet08:02:32

I guess it's prolly related to ASYNC-155, but I am not really familiar with this code tbh

mpenet08:02:57

maybe @bronsa can have a look later

hiredman08:02:18

the loop thing makes me think 155 too

mpenet08:02:26

I can't build a small repro. case tho, annoying

mpenet08:02:09

there are asm version diffs between tools analyzer and cassandra-driver-core, but I doubt it has any effect

mpenet08:02:24

another odd one:

(defn foo [x]
  (let [y nil]
    (a/go
      (loop []
        (let [t (.getTime ^Date x)]
          (println t))))))

mpenet08:02:46

without the (let [y nil] ...) it's fine

mpenet08:02:59

if y is something diff from nil it's ok too

mpenet08:02:09

Slimmed down case without all cassandra mess. It's probably related to the other issue I encountered. /cc @alexmiller

hiredman08:02:04

I find a little repro for the reflection warnings

hiredman08:02:16

(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)))))

hiredman08:02:38

git bisect says the issue is actually with ASYNC-138

mpenet08:02:36

interesting

bronsa08:02:41

I'll take a look at soon as I'm fully awake

bronsa08:02:06

anything that regressed and involves a local crossing over from an outer env into a go block is likely my fault

mpenet08:02:07

@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

bronsa09:02:53

interesting

bronsa09:02:12

compiler.java returns true for (.hasJavaClass lb) when a a lb is bound to a nil literal

bronsa09:02:25

but returns nil for (.getJavaClass lb)

bronsa09:02:39

I'd say it's a quirk/bug in compiler.java, but meh, I'll work around that

bronsa09:02:49

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

bronsa09:02:10

easy workaround tho, looking at the reflection warning now

mpenet09:02:56

yep, quite odd

mpenet09:02:37

makes me wonder if that's used/relied upon in other libs now

mpenet09:02:41

or even core

bronsa09:02:43

@mpenet yeah I'm quite sure useful by amalloy has a similar usage that might be similary broken

mpenet09:02:52

core.typed too

mpenet09:02:10

unverified but I see references of it there

bronsa09:02:04

I'm tempted to make a patch for clojure to fix that

bronsa09:02:14

let's see what @alexmiller thinks about it

mpenet09:02:15

that'd make sense

bronsa09:02:28

but the current behaviour makes no sense to me

bronsa09:02:05

core.async would still needs the fix in place to deal with older versions of clojure

mpenet09:02:43

the reflection warning is a separate issue tho, right?

bronsa09:02:54

I'm looking at that now

bronsa09:02:48

preserving type hints has to be the greatest source of incidental complexity while doing lexical/AST transformations in clojure

bronsa09:02:19

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

bronsa10:02:11

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)))))

mpenet10:02:10

the first reflection warning I got this morning didn't trigger with previous releases tho

bronsa10:02:18

still a regression I guess, will take a look at it anyway

mpenet10:02:29

hopefully it's the same source of problem

bronsa10:02:43

yeah @mpenet ASYNC-138 causes code to be rewritten in a way that triggers that pre-existing bug

bronsa10:02:57

the bug is pre-existing, we're just hitting it now because of the code rewrite from ASYNC-138

bronsa10:02:09

don't have a fix yet

mpenet11:02:01

without the take! no reflection, if you replace it with close! either

bronsa11:02:35

yeah that's expected

mpenet11:02:58

-> rewriting

bronsa11:02:07

the state machine emits differently based on whether a path contains a parking call or not

mpenet11:02:30

anyway, I was trying to understand how it works, but it's not a 5min code read

bronsa11:02:35

there's a missing with-meta somehwere

bronsa11:02:47

fix should be easy

bronsa11:02:53

finding where to fix it, not that much

bronsa11:02:56

i'm on it tho

bronsa11:02:08

@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

mpenet11:02:10

is ioc_macro documented anywhere?

bronsa11:02:12

maybe it's just me tho

tbaldridge12:02:37

@bronsa absolutely, if I had it to do over again, i'd replace it with some sort of mutable writer object, or dynamic vars.

bronsa13:02:02

dynamic vars have worked fine in the few tools.analyzer passes that required cross-node state to be backtracked

bronsa13:02:11

admittedly the scope was much smaller than the core.async emitter

bronsa15:02:44

they're unrelated

bronsa15:02:53

I'm still trying to track down ASYNC-187

bronsa15:02:34

I have a vague understanding of what's happening

bronsa15:02:43

but not 100% sure about the best way to fix it

bronsa15:02:23

it seems intentional but it doesn't make any sense to me

Alex Miller (Clojure team)15:02:45

I saw your comments earlier. don’t know.

bronsa15:02:33

i guess i'll try removing it and see if anything breaks ¯\(ツ)

Alex Miller (Clojure team)15:02:07

I wouldn’t gate on that changing any time soon

Alex Miller (Clojure team)15:02:49

seems like most uses of getJavaClass() guard against null as a possible return

bronsa15:02:00

ah, you're right

Alex Miller (Clojure team)15:02:09

even if the has returns true

bronsa15:02:15

instance.hasJavaClass() && instance.getJavaClass() != null

bronsa15:02:16

yeah I see

Alex Miller (Clojure team)15:02:29

so doesn’t seem accidental

bronsa15:02:41

wonder if it has something to do with Reflector.java quirckness

Alex Miller (Clojure team)15:02:07

the question is: if nil returned false for hasJavaClass(), what is the impact. but that would require more work to understand.

bronsa15:02:37

and for the number of users of Compiler internals, not worth it

bronsa16:02:41

aight, I have a patch for ASYNC-187

bronsa16:02:47

testing on real projectz appreciated :)

mpenet16:02:15

bronsa: i am out, but i ll give it a try tomorrow morning

bronsa16:02:26

👍 thanks

mpenet17:02:06

confirmed, the reflection is gone on alia-async with the patch (I didn't apply the other one tho)

mpenet17:02:17

I ll give it a try with both just to make sure

bronsa17:02:46

alex already merged the other patch into master FYI

mpenet17:02:16

all good with both patches 🙂 thanks!

bronsa16:02:01

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

tbaldridge16:02:27

👍 looks good. I'm reading the code the same way

tbaldridge16:02:55

and yeah, now that we have tools analyzer, we should look at replacing the monad stuff at some point.

bronsa16:02:37

i don't even think we need push-binding stuff for :locals at this point

bronsa16:02:48

because t.a deals with local shadowing

bronsa16:02:59

and I assume that that's what that was there for

tbaldridge16:02:54

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.

bronsa16:02:20

I'm getting an itch to try and rewrite the core.async frontend as a t.a. pass at some point

bronsa16:02:31

yeah.. that ^ :)

bronsa16:02:55

not that anybody would probably care

tbaldridge16:02:13

eh, the simpler we can make the ioc code the less edge-cases we have.

bronsa16:02:15

but we could get some small speed improvements by avoiding some boxing here and there

bronsa16:02:40

also ^ yes

tbaldridge16:02:06

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.

bronsa16:02:11

altho i predict a rewrite would break tons of stuff for a while, so not sure if it's worth it

bronsa16:02:36

existing stability vs simpler code

bronsa16:02:21

I guess it could be an experimental additional frontend, no need the 2 frontends can't coexist

tbaldridge16:02:22

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?!?

bronsa16:02:40

then if it gets stable we could switch it to be the default

bronsa16:02:05

@tbaldridge heh, "it's a feature not a bug" :)

bronsa16:02:22

"we're making you write simpler code by making your complex code not compile"

mpenet17:02:45

@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)

mpenet18:02:02

that was fast, thanks

bronsa18:02:51

you've pushed the wrong patch

bronsa18:02:05

I uploaded one and removed it in 2 minutes hoping nobody would notice

Alex Miller (Clojure team)18:02:18

Well thx for the heads up

bronsa18:02:18

but apparently you were faster than my reuploading

Alex Miller (Clojure team)18:02:32

Release is already out there

mpenet18:02:39

just released 2 libs depending on that version 😄

bronsa18:02:09

mpenet: don't think it should cause any bugs FWIW

Alex Miller (Clojure team)18:02:04

I think we've all learned a lesson here today

bronsa18:02:05

i don't think there's any difference between the patch you pushed and the one in the ticket

bronsa18:02:18

but one is correct, the other happens to work

bronsa18:02:10

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

bronsa18:02:22

if something unrelated to that changes, it might break it

bronsa18:02:41

let's just say the one you pushed has a bug.

bronsa18:02:12

the technical details is that it doesn't include the pop-binding stuff i was talking with tbaldridge above

bronsa18:02:38

which by accident still fixes the issue and doesn't cause any bugs that I can think of

bronsa18:02:41

but still, it's not correct

bronsa18:02:30

i'll make a note to never mutate a patch in place again

Alex Miller (Clojure team)18:02:30

Make a new ticket and a new patch as pennance

bronsa18:02:56

i'm lashing myself too