ClojureScript

This ticket needs more information. What is the error produced by the example code given above? Does this only affect single segment namespaces? etc. Thanks!

> What is the error produced by the example code given above?

This is the compiled javascript:

foo.fooFunction = function fooFunction(bar) {
  return bar.barFunction.call(null, bar)
};

When you try to run it, bar.barFunction is undefined, because (bar the argument) is shadowing (bar the namespace)

> Does this only affect single segment namespaces?

No, it affects big namespaces in our real codebase. A variant of this error can also be produced by let:

(defn ^:export fooFunctionLet []
  (let [bar 1]
    (barFunction bar)))

Which compiles to:

foo.fooFunctionLet = function fooFunctionLet() {
  var bar = 1;
  return bar.barFunction.call(null, bar)
};

I will update the example project with this variation.

Updated with additional variation of error.

Thanks will look into it.

I am unable to get your shadow project to reliably reproduce the problem, even when compiling clean. Do you have any other methods of reproducing this?

I just re-downloaded it and ran:

rm -rf target/ out/
lein cljsbuild clean; lein cljsbuild once

and the end of main.js is:

goog.provide("bar");
goog.require("cljs.core");
bar.barFunction = function barFunction(x) {
  return x + 1
};
goog.provide("foo");
goog.require("cljs.core");
goog.require("bar");
goog.require("bar");
foo.fooFunction = function fooFunction(bar) {
  return bar.barFunction.call(null, bar)
};
goog.exportSymbol("foo.fooFunction", foo.fooFunction);
foo.fooFunctionLet = function fooFunctionLet() {
  var bar = 1;
  return bar.barFunction.call(null, bar)
};
goog.exportSymbol("foo.fooFunctionLet", foo.fooFunctionLet);

I opened Test.html and there is a console error from the whitespace-mode compiled file.

Is this possibly an issue with other tools? Here's my info:

lein --version
Leiningen 2.3.2 on Java 1.7.0_45 Java HotSpot(TM) 64-Bit Server VM

java -version
java version "1.7.0_45"
Java(TM) SE Runtime Environment (build 1.7.0_45-b18)
Java HotSpot(TM) 64-Bit Server VM (build 24.45-b08, mixed mode)

This is what I'm getting when I compile the shadow-with-let project using the method you described, with `cljsbuild clean`:

goog.provide("bar");
goog.require("cljs.core");
bar.barFunction = function barFunction(x) {
  return x + 1
};
goog.provide("foo");
goog.require("cljs.core");
goog.require("bar");
goog.require("bar");
foo.fooFunction = function fooFunction(bar__$1) {
  return bar.barFunction.call(null, bar__$1)
};
goog.exportSymbol("foo.fooFunction", foo.fooFunction);
foo.fooFunctionLet = function fooFunctionLet() {
  var bar__$1 = 1;
  return bar.barFunction.call(null, bar__$1)
};
goog.exportSymbol("foo.fooFunctionLet", foo.fooFunctionLet);

Mine:

lein --version
Leiningen 2.3.3 on Java 1.7.0_09-icedtea OpenJDK 64-Bit Server VM

Are you looking at main.js, main.min.js, or one of the intermediate files?

My output is from main.js. I'm not sure what else to try here, do you have any ideas for me?

I've made a temporary server on EC2 with everything fresh installed and it reproduces the issue. Email me and I'll send you the login info. [my first name]@fivetran.com

Some initial findings:

1. I do not believe {:optimizations :whitespace} is necessary to reproduce this. When I set optimizations to :none, the before-Closure js in the target directory can still exhibit the problem.

2. I believe the issue affects single-segment and multi-segment namespaces equally.

3. Using the shadow-with-let example, I am able to reliably reproduce the problem by removing the bar.cljs file from the src directory. This causes compilation to throw a warning but still complete. The resulting foo.js is a reproduction of the issue, with bar being shadowed within the let.

Okay, I believe I have determined the problem. Let's first consider a working scenario with two files, bar.cljs and foo.cljs. These are compiled in that order. The order of compilation is important to the bug, which I believe explains why incremental compiliation can sometimes fix it.

1. bar.cljs gets compiled. When its ns form is emitted, compiler/emit :ns adds "bar" to the ns-first-segments atom.
2. foo.cljs gets compiled. When compiler/munge finds the bar variable in the function call, it calls compiler/shadow-depth to figure out whether it is shadowing something. Since bar.cljs already got compiled, shadow-depth sees its ns in the ns-first-segments atom. This causes compiler/shadow-depth to return 1, and then compiler/munge correctly outputs bar__$1 for the variable name.

In the purposefully broken example which reproduces the shadowing error, bar.cljs no longer exists. Even though the bar namespace is required in foo's ns declaration, it never makes it into the ns-first-segments atom. This causes the compiler/shadow-depth call to return 0, so compiler/munge sees no reason to append to the variable name and simply outputs "bar" which shadows the bar ns.

I believe the fix here is to patch compiler/emit :ns to conjoin the namespaces of its requires and requires-macros into the ns-first-segments atom. I am working on that now.

Thanks for taking the time to look at this. It seems that you are implying that it is necessary to delete bar.cljs in order to reproduce the error, which is not true: all it takes is to run

lein cljsbuild clean; lein cljsbuild once

on a system with everything clean-installed like the server I set up.

I think we need to think a bit more about what's going on here before we change anything. It seems to me we could circumvent the issue by making `ns-first-segments` impervious to incremental compilation instead. It may make sense to move this into the analyzer since everything always gets analyzed (but not everything always gets compiled).

George, I believe the issue is hard to reproduce because it will not occur if cljsbuild compiles bar.cljs before compiling foo.cljs. If bar.cljs is removed, you can guarantee that it doesn't get compiled first and therefore guarantee a reproduction of the problem.

Patch: 20131030

This patch uses the analyzer's existing namespaces atom to resolve namespace shadowing conflicts. The compiler's ns-first-segments atom has been removed.

I think this will fix the issue when all files are included and the ns forms are valid. However, the toy example I used where I deleted the bar.cljs file will still be incorrect. Are there any reasons why we would wish to fix that case? It should already throw a warning letting you know your ns form is requiring something it can't find.

closure/build benchmarks through a REPL, as per David Nolen's request. edit: I'm compiling the src/cljs folder.

Before Patch:

cljs.closure> (dotimes [_ 10] (time (build "/repos/clojurescript/src/cljs" {:optimizations :none} true)))
"Elapsed time: 3821.425 msecs"
"Elapsed time: 3261.022 msecs"
"Elapsed time: 2977.293 msecs"
"Elapsed time: 3062.12 msecs"
"Elapsed time: 2964.948 msecs"
"Elapsed time: 3025.59 msecs"
"Elapsed time: 2893.04 msecs"
"Elapsed time: 2946.842 msecs"
"Elapsed time: 3108.782 msecs"
"Elapsed time: 2916.127 msecs"
nil

With Patch:

cljs.closure> (dotimes [_ 10] (time (build "/repos/clojurescript/src/cljs" {:optimizations :none} true)))
"Elapsed time: 5722.203 msecs"
"Elapsed time: 3464.829 msecs"
"Elapsed time: 3160.55 msecs"
"Elapsed time: 3283.244 msecs"
"Elapsed time: 3015.998 msecs"
"Elapsed time: 3023.166 msecs"
"Elapsed time: 2991.538 msecs"
"Elapsed time: 2844.978 msecs"
"Elapsed time: 3101.24 msecs"
"Elapsed time: 2860.506 msecs"
nil

Looks good. George can you confirm this patch works for you? Thanks!

Confirmed to work on my machine and the squeaky-clean server I set up for testing. Thanks guys!

fixed, https://github.com/clojure/clojurescript/commit/c135f06992823ceec85f38877776a7c8bc238b74