tools.namespace's tracker doesn't handle namespaces of same name across dialects

[magnars]

Here's a new one for me:

clojure.lang.ExceptionInfo: Circular dependency between quiescent.core and quiescent.core
 at clojure.core$ex_info.invoke (core.clj:4593)
    mranderson046.toolsnamespace.v0v3v0_alpha1.clojure.tools.namespace.dependency.MapDependencyGraph.depend (dependency.cljc:89)
    mranderson046.toolsnamespace.v0v3v0_alpha1.clojure.tools.namespace.track$add_deps$fn__50653$fn__50655.invoke (track.cljc:26)

Full stack: https://gist.github.com/magnars/43702192d9d8108f98eb

The namespace for quiescent.core in clj is simply:

(ns quiescent.core)

The namespace for quiescent.core in cljs is:

(ns quiescent.core
  (:require [cljsjs.react])
  (:require-macros [quiescent.core :refer [react-method]]))

[benedekfazekas]

this is basically toolsnamespace 0.3.0-alpha1 complaining I don't think it is mranderson specific. we rather need to poke around the ns.tracker I guess:

mranderson046.toolsnamespace.v0v3v0_alpha1.clojure.tools.namespace.file$add_files.invoke (file.clj:81)
    refactor_nrepl.ns.tracker$build_tracker.invoke (tracker.clj:11)
    refactor_nrepl.find.find_macros$find_macro.invoke (find_macros.clj:209)
    refactor_nrepl.find.find_symbol$find_symbol$fn__55197.invoke (find_symbol.clj:254)

thx for the report @magnars

[expez]

I'm not sure if this is problem with our usage or an actual bug in the implementation of tools.namespace.

I'm reading TNS-35 to mean that clojurescript support isn't working correctly yet. Is that correct, @stuartsierra?

[stuartsierra]

tools.namespace supports .cljs and .cljc as of 0.3.0-alpha1. TNS-35 should have been closed with that release — I just closed it.

But tools.namespace can't handle the case of two files defining the same namespace. I added TNS-38 to track this issue. This might be difficult to fix in the near-term, since tools.namespace assumes a one-to-one correspondence between namespaces and files.

[stuartsierra]

Even with a fix for TNS-38, there may be other complications here.

refactor-nrepl.ns.tracker uses clojure.tools.namespace.file/add-files on all source files regardless of extension: .clj, .cljs, or .cljc.

Since there may be multiple files for the same namespace with different extensions, there is no way to know which one will be stored in :clojure.tools.namespace.file/filemap, used in refactor-nrepl.ns.tracker/project-files-in-topo-order.

An even more complicated, though unlikely, scenario: Since refactor-nrepl.ns.tracker merges the dependency graphs of Clojure and ClojureScript sources, it could potentially find cyclic dependencies between Clojure and ClojureScript code which do not exist in either language separately.

Mixed Clojure/ClojureScript projects open up all kinds of complications for tooling. I'm just starting to discover them for tools.namespace; I expect there will be more.

[expez]

@stuartsierra We use the tracker, among other things, in rename-file-or-dir to find the dependent namespaces which need to be updated after a source file is moved. The only way to get this to work, in the general case, is to put all variants of source files in the tracker.

It would be great if this could be made to work reliably. For now, I'll limit the scope of the tracker we're building to only .clj files whenever possible.

[stuartsierra]

I've fixed TNS-38 which resolves the specific case of :require-macros from a .cljs file to a .clj file. This is included in tools.namespace release 0.3.0-alpha2.

[benedekfazekas]

@stuartsierra thanks, will give a try to 0.3.0-alpha2

[expez]

@stuartsierra we use TNS to find dependent files, e.g. when one file is moved we need to update the ns forms of dependent files to require the right thing and fix up fully-qualified references.

With this change, we'll now report success to the user, but fail to update one of the affected files when the user performs a rename-file refactoring. To me this is a regression, as I'd much rather have the op blow up than perform a partial refactoring and report success.

[benedekfazekas]

I am just about publishing a new snapshot with upgraded tns? (so @magnars can retest) @expez do you prefer this to be reverted?

[expez]

Yes, I think it's better to fail in this edge-case than perform a partial refactoring and tell the user all is OK.

[stuartsierra]

Well, that's why -alpha releases are useful. :) I will reconsider
TNS-38 in light of this feedback. Thanks for testing the alphas and
reporting.

This discussion is leading me to think that deps-from-ns-decl should
ignore :require-macros completely, because it crosses the “platform”
boundary between ClojureScript and Clojure. That would be a different
way of resolving TNS-38 from the solution in -alpha2, but it still
wouldn’t help for this use case.

Regardless of how tools.namespace ends up handling :require-macros,
it probably won’t be able to provide the perfect answer for the use
cases here, because it only tracks dependencies among namespaces, not
among files. tools.namespace has no way to represent a dependency
between two files which both declare the same namespace.

add-files on two different files with the same namespace is
undefined. Even detecting that as an error would require adding more
state to the tracker, which is already too complicated.

It might be possible to deal with this problem using separate trackers
for Clojure and ClojureScript, but I haven’t tested that scenario.
Even then, there’s still no way to accurately represent the fact that
foo.cljs depends on foo.clj.

I have some vague ideas for a redesigned tools.namespace which might
be able to handle these cases, but no concrete implementation ideas
right now.

[benedekfazekas]

@expez care to pop into the gitter room wrt this issue?

[expez]

Rum is also affected by this: tonsky/rum#54

[benedekfazekas]

what about going with the temporary fix @expez?

[expez]

what about going with the temporary fix @expez?

I still don't think that's a good idea.

I'll have time to write our own tracker early next year, unless this is solved upstream by then.

[benedekfazekas]

okay. does this blocks 2.0 tho?

[expez]

okay. does this blocks 2.0 tho?

No, I don't think that's warranted. Fixing this won't create any API incompatibilities. Moving from the tools.namespace tracker to our own tracker is just an internal change.

[benedekfazekas]

fair point wrt API change. on the other hand it completely blocks users to use refactor-nrepl if they use certain deps in their project. but I appreciate that you don't wanna go with the temporary fix.

[benedekfazekas]

will try to have a look on the TNS issue

[magnars]

It blocks most of my usage, as one example.

On Mon, Dec 14, 2015 at 2:40 PM Benedek Fazekas [email protected]
wrote:

fair point wrt API change. on the other hand it completely blocks users to
use refactor-nrepl if they use certain deps in their project. but I
appreciate that you don't wanna go with the temporary fix.

—
Reply to this email directly or view it on GitHub
#127 (comment)
.

[benedekfazekas]

any ways we could perhaps enhance the temporary fix? (crazy things like warn when rename-file is used or completely disable it for projects where we detect multiple platform namespaces (we can figure this out without the tracker i guess)) again, I appreciate that you don't wanna go with a temp fix which actually breaks a feature really badly but on the other hand this is a serious blocker for many users.

[expez]

First priority now is getting 2.0.0 out the door so people don't have to depend on snapshots anymore.

Once that is done I'll tackle this. That means any temporary fix will only have a lifetime of around 2-3 weeks. Furthermore, the lifetime of the temp-fix overlaps with the holiday when people are doing less refactoring than usual.

There's also the issue of teaching people about the tempfix (double prefix means force? new setting?). Most likely a permanent solution is out by the time people have 1) decided to upgrade 2) learned about the workaround.

Since people haven't been bumping this thread with "+1" at all, I'm also thinking this doesn't affect a lot of users, yet.

[bilus]

+1 It affects anyone using Om I believe.

I'm using 2.0.0-SNAPSHOT

clojure.lang.ExceptionInfo: Circular dependency between om.dom and om.dom {:reason :mranderson046.toolsnamespace.v0v3v0-alpha1.clojure.tools.namespace.dependency/circular-dependency, :node om.dom, :dependency om.dom}�[0m

error in process filter: Error in nrepl-refactor:       

[expez]

@stuartsierra Any plans on solving this in the immediate future (say in the next 2-3 weeks)? I started working on a file-based tracker (instead of a namespace based one), but I'd obviously love to offload this work to TNS :)

[benedekfazekas]

@expez: @stuartsierra just pushed a fix for TNS-38 along the lines of his previous comment. Are you happy with go with this (at least temporarily) as soon as an alpha is released?

[magnars]

The fix for TNS-38 looks good to me at least. And I'd be thrilled to use clj-refactor again too.

[stuartsierra]

Just released tools.namespace 0.3.0-alpha3.

This contains the TNS-38 fix described above,
which ignores :require-macros and :use-macros completely. This
effectively isolates the dependency graph for each "platform"
(Clojure or ClojureScript).

[benedekfazekas]

upgraded TNS dependency and released new snapshot to clojars.

@magnars @bilus pls test if you are ok with latest snapshot.

I keep the issue open because as expected cljs->clj (via :require-macros) is not tracked so this is a temporary fix with some regression for now.

[magnars]

@benedekfazekas Thanks! 😄 Would that be 2.0.0-SNAPSHOT or 2.1.0-alpha1?

[benedekfazekas]

the snapshot
On 6 Jan 2016 7:39 am, "Magnar Sveen" [email protected] wrote:

@benedekfazekas https://github.com/benedekfazekas Thanks! [image:
😄] Would that be 2.0.0-SNAPSHOT or 2.1.0-alpha1?

—
Reply to this email directly or view it on GitHub
#127 (comment)
.

[magnars]

@benedekfazekas That solved the issue for me. It was nice seeing the find-usages window again. 😎

[benedekfazekas]

Nice one! :)
On 6 Jan 2016 9:04 am, "Magnar Sveen" [email protected] wrote:

@benedekfazekas https://github.com/benedekfazekas That solved the issue
for me. It was nice seeing the find-usages window again. [image:
😎]

—
Reply to this email directly or view it on GitHub
#127 (comment)
.

[magnars]

Time to close this?

[benedekfazekas]

yup, I think. there is a branch by @expez with a more ambitious solution with a new tracker in the works so that won't go forgotten.