<< Back to previous view

[CLJS-509] Spurious warning about symbol not being a protocol Created: 21/May/13  Updated: 08/Sep/13  Resolved: 08/Sep/13

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Praki Prakash Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

Clojure 1.5.1, ClojureScript, cljsbuild 0.3.2


Attachments: File CLJS-509-clean-up-after-parse-ns.diff     File foo.cljs     File fubar.cljs    

 Description   

"lein cljsbuild" generates "Symbol X is not a protocol" warning. The following code snippet reproduces the issue. Renaming "my.foo" namespace to "foo" compiles with no warning.

;; file:: foo.cljs
(ns my.foo)

(defprotocol IFoo
(bar[this]))

;; file: fubar.cljs
(ns fubar
(:require
[my.foo :as foo]))

(deftype FuBar []
foo/IFoo
(bar [this]))

cljs output:
WARNING: Symbol foo/IFoo is not a protocol at line 5 src/fubar.cljs



 Comments   
Comment by Colin Jones [ 11/Jun/13 11:17 AM ]

We're seeing this warning as well.

The code path that produces this warning appears to imply that the var could be found via `cljs.analyzer/resolve-existing-var`, but just didn't have the :protocol-symbol key attached. At first I suspected that .indexOf here should be .lastIndexOf: https://github.com/clojure/clojurescript/blob/61dee726a6878539f9c07b1eb49abc69e5c3cc21/src/clj/cljs/analyzer.clj#L172-L191

cljs.user=> (cljs.analyzer/resolve-existing-var (cljs.repl.rhino/repl-env) 'foo.bar.Baz)
{:ns foo, :name foo/bar.Baz}

But even if that's the case (and I'm not really sure it is), it seems like the my.foo/IFoo case ought to take the first path in resolve-var's cond, so it's probably unrelated.

There are no discernible problems at runtime, only when (auto-)building. I wonder whether this is limited to code reloading scenarios like the lein-cljsbuild auto-builder. Protocols in JVM clojure certainly have lots of code-reloading caveats around them.

I only dug into it a bit before needing to give up for now, but hopefully this helps.

Comment by David Nolen [ 12/Jun/13 2:36 AM ]

Colin thanks for the additional information. Will try to find some time to look into it.

Comment by Matjaz Gregoric [ 12/Jun/13 4:38 PM ]

The reason for these warnings is that under some circumstances, some of the files that don't require compilation (as determined by cljs.compiler/requires-compilation?), don't get properly analyzed.

Normally when analyzing a file, the analyzer will also analyze all of the namespaces that the current file/namespace depends on (using cljs.analyzer/analyze-deps). The analyzer tries to avoid analyzing the same namespace twice by checking whether the namespace is already present in cljs.analyzer/namespaces, see: https://github.com/clojure/clojurescript/blob/f80956d90f455810be140cfec1632f55254385a5/src/clj/cljs/analyzer.clj#L615

When trying to compile a file that doesn't require compilation, cljs.compiler/compile-file delegates to cljs.compiler/parse-ns, which uses cljs.analyzer/analyze to parse the ns form. Once it reaches the ns form, it stops further analysis of the file. Analyzing the ns form has the side effect of populating cljs.analyzer/namespaces with the basic namespace info (https://github.com/clojure/clojurescript/blob/f80956d90f455810be140cfec1632f55254385a5/src/clj/cljs/analyzer.clj#L710-L721), preventing the analyzer from subsequently fully analyzing the file in case another file that gets compiled depends on it.

In the example above, the warning will get printed when compiling foo.cljs and fubar.cljs for the second time, after fubar.cljs has changed since first compilation, but foo.cljs hasn't. It will only happen if compile-file is called on foo.cljs before fubar.cljs. Since foo.cljs doesn't require compilation, only its ns form will get analyzed, and basic namespace info put into cljs.analyzer/namespaces. When compile-file gets called on fubar.cljs later, the analyzer will see that fubar.cljs depends on foo.cljs, but will not go on to analyze it since 'foo.cljs will already be present in cljs.analyzer/namespaces.

I'm attaching a patch that tries to reverse the change that parse-ns makes to cljs.analyzer/namespaces. It is the simplest patch I could come up with, although it would probably be more appropriate to not use cljs.analyzer/analyze to parse the namespace form in parse-ns at all, but instead create a separate function (cljs.analyzer/analyze-file-ns ( ? )) whose only purpose would be to gather file's namespace info, without changing the analyzer's state.

Comment by Julian Birch [ 22/Aug/13 9:58 AM ]

This cost me a few hours the other day. I tried #clojure as well, but no-one was aware of it.

Crosslink: https://github.com/yogthos/cljs-ajax/issues/20

Comment by David Nolen [ 25/Aug/13 11:04 PM ]

I would prefer a patch that does as Matjaz suggests - don't call analyze and instead provide a separate function just for parsing namespace forms.

Comment by David Nolen [ 04/Sep/13 11:37 PM ]

While it's always annoying to add yet another dynamic var isn't this best solved by binding such a dynamic var such that analyze-ns doesn't emit any warnings or side effects namespaces?

Another option would be restore the previous namespaces value after ns analysis?

Comment by Jonas Enlund [ 05/Sep/13 2:01 AM ]

Here's how I'm able to reproduce this issue:

jonas@ThinkPad:~/dev/clojure/cljs-509$ lein clean && lein cljsbuild clean
Deleting files generated by lein-cljsbuild.
jonas@ThinkPad:~/dev/clojure/cljs-509$ tree
.
├── doc
│   └── intro.md
├── LICENSE
├── project.clj
├── README.md
├── resources
├── src
│   └── cljs_509
│       └── core.clj
├── src-cljs
│   └── cljs_509
│       ├── foo.cljs
│       └── fubar.cljs
└── test
    └── cljs_509
        └── core_test.clj

8 directories, 8 files
jonas@ThinkPad:~/dev/clojure/cljs-509$ lein cljsbuild once
Compiling ClojureScript.
Compiling "main.js" from ["src-cljs"]...
Successfully compiled "main.js" in 8.121183885 seconds.
jonas@ThinkPad:~/dev/clojure/cljs-509$ touch src-cljs/cljs_509/fubar.cljs 
jonas@ThinkPad:~/dev/clojure/cljs-509$ lein cljsbuild once
Compiling ClojureScript.
Compiling "main.js" from ["src-cljs"]...
WARNING: Symbol foo/IFoo is not a protocol at line 4 src-cljs/cljs_509/fubar.cljs
Successfully compiled "main.js" in 5.346990248 seconds.
jonas@ThinkPad:~/dev/clojure/cljs-509$

As you can see, the warning appears when I 'touch' fubar.cljs which forces recompilation.

Comment by David Nolen [ 07/Sep/13 8:51 PM ]

Can everyone experiencing this bug please try this branch - http://github.com/clojure/clojurescript/tree/509-protocol-warn. Fixing this bug also made me look at bit closer at the build process, lein cljsbuild auto builds when not using optimizations should be considerably faster (10X) as a result - we no longer analyze files that haven't changed.

Comment by Thomas Heller [ 08/Sep/13 1:41 AM ]

I have not seen the protocol warning in a while, so I can't say anything to wether that is fixed or not.

I tried the branch for the optimization alone and while it works it is coupled to the behavior of lein-cljsbuild in that "incremental" builds only work when the increments happen in the same JVM instance. You can't build incrementally if you kill the build VM and restart it, at least my build tool couldn't.

lein-cljsbuild deletes the :output-to directory whenever it is launched therefore forcing a compile/analyze of everything, if that delete doesn't happen something is missing and the compilation fails cause cljs.core is not known. Not exactly sure whats missing but if :output-to is deleted whenever a new build VM is started everything works fine.

Comment by David Nolen [ 08/Sep/13 1:50 AM ]

The protocol warning problem is definitely real, w/o the fix you will get it with the minimal project that Jonas describes - unpleasant and surprising. As far as incremental builds - I'm not sure what we can about the different JVM "problem". Also I can't recreate your issue with lein cljsbuild.

Comment by Praki Prakash [ 08/Sep/13 11:50 AM ]

The protocol warnings have gone away with this branch.

However, I don't see any improvements in "lein cljsbuild auto". My code still takes about 13 seconds to do an incremental compilation. I believe it used to be ~1 sec earlier with another auto build script I was using. Not sure if "lein cljsbuild" was ever faster than 13 seconds as I started using it only recently.

Comment by David Nolen [ 08/Sep/13 12:00 PM ]

The speed of lein cljsbuild auto is completely dependent on whether you're using Closure as part of the step. With :optimizations :none, a simple file with a few definitions now takes ~0.04s to recompile on my Macbook Air 1.7ghz after the first build.

Comment by David Nolen [ 08/Sep/13 5:20 PM ]

fixed http://github.com/clojure/clojurescript/commit/5a038ffc1c0cba0f0d7f16efe23f0e95cb711518

Generated at Fri Sep 19 19:08:56 CDT 2014 using JIRA 4.4#649-r158309.