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?

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.

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.

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.

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?