clj-commons / manifold

A compatibility layer for event-driven abstractions
1.02k stars 106 forks source link

manifold.stream creates dependency cycles #5

Closed cursive-ide closed 9 years ago

cursive-ide commented 9 years ago

I'm not sure if this is technically a bug or not, but I thought it warranted discussion. Cursive includes functionality to check for dependency cycles in namespaces, and will produce an error when the user tries to load a namespace involved in a cycle, the assumption being that they just created it inadvertently. For a while there, the entire transitive closure of dependencies of the file being loaded was calculated and cycles anywhere in the graph were warned about. I've since fixed this, but while that was the case several users reported problems with manifold.stream. This is used from manifold.stream.core, .queue, .seq, .deferred, .async but also requires them itself later in the file. This will cause problems for people wanting to work on the manifold source with Cursive, and seems weird - can it be fixed?

ztellman commented 9 years ago

Just calling (use 'manifold.stream) works. I can break the cycle by pulling out certain functions into a manifold.stream.core namespace and them importing them into the top-level manifold.stream namesapce (this is what I've done in a number of my other libraries), but this seemed like a reasonable workaround that Clojure was okay with.

I guess my question is: if this works fine with Clojure proper, shouldn't Cursive be okay with it? I don't want to preclude the use of a popular IDE, but neither do I want to be unnecessarily constrained by it.

cursive-ide commented 9 years ago

Sure, it definitely works. And it's an open question whether Cursive should accept everything that Clojure can do, or whether it should try to be more opinionated. In this case, I'm not sure what criteria I would use to determine that this particular case is ok. Cursive indexes everything so it knows that manifold.stream has a dependency on e.g. manifold.stream.core, and that the inverse dependency also exists.

I'm actually not sure under which circumstances Clojure will give an error due to a dependency cycle - from a brief look at the code in clojure.core it looks like load will throw a cyclic dependency but require will not, which seems strange. I've definitely heard people who ought to know what they're talking about say that Clojure doesn't allow dependency cycles but this appears to not be the case.

cursive-ide commented 9 years ago

BTW see here for an excellent explanation by Stephen Gilardi of why the Manifold case works. I'll modify the Cursive validation to take this into account.

ztellman commented 9 years ago

Thanks for chasing this down.

cursive-ide commented 9 years ago

BTW it looks like this might actually be considered a problem as of 1.7 alpha 5: https://groups.google.com/d/msg/clojure/jj87-4yVlWI/OjigPR3tIEcJ

neverfox commented 8 years ago

I'm running into this again now that I've upgraded to Cursive 1.2.1 and IDEA 2016.1 (Clojure 1.8). The message is Dependency cycle: time.clj -> time.clj. I'll cross-post to Cursive.

ztellman commented 8 years ago

I think what I'm doing in manifold.time is valid, but let me know if Colin feels differently.

neverfox commented 8 years ago

Will do. Looks like someone already reported it over there.

cursive-ide commented 8 years ago

I would say it's valid but tricky :-) I think this is a Cursive bug, I've worked around it for now and I'll think about a better solution.

neverfox commented 8 years ago

@cursive-ide Colin, is the workaround something one can do for themselves by how the code is written? As it is now, this is a showstopper on all of my manifold-based code.

cursive-ide commented 8 years ago

@neverfox There's a workaround in cursive-ide/cursive#1298: Disabling "Load out of date file dependencies transitively" eliminates the error.

neverfox commented 8 years ago

@cursive-ide Missed that sentence. Thanks. Are there any negative side-effects to disabling that?

cursive-ide commented 8 years ago

@neverfox I'll comment over there since this is Cursive related, not manifold related.