<< Back to previous view

[CLJS-631] referred namespace is shadowed by argument name in whitespace mode Created: 21/Oct/13  Updated: 30/Oct/13  Resolved: 30/Oct/13

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

Type: Defect Priority: Major
Reporter: George Fraser Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None
Environment:

cljs 1934


Attachments: File cljs-631-20131030.diff     GZip Archive shadow.tar.gz     GZip Archive shadow-with-let.tar.gz    

 Description   

Under whitespace mode compilation, the argument named "bar" shadows the namespace "bar" in the compiled JS. Interestingly, this problem only occurs consistently when you compile clean; incremental compilation sometimes fixes it.

See attached project for example.

(ns bar)
(defn barFunction [x] (+ x 1))

(ns foo
  (:require [bar :refer [barFunction]]))
(defn ^:export fooFunction [bar]
  (barFunction bar))


 Comments   
Comment by David Nolen [ 21/Oct/13 6:30 PM ]

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!

Comment by George Fraser [ 21/Oct/13 6:41 PM ]

> 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.

Comment by George Fraser [ 21/Oct/13 6:42 PM ]

Updated with additional variation of error.

Comment by David Nolen [ 22/Oct/13 9:27 AM ]

Thanks will look into it.

Comment by Travis Thieman [ 28/Oct/13 10:43 AM ]

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?

Comment by George Fraser [ 28/Oct/13 1:07 PM ]

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.

Comment by Travis Thieman [ 28/Oct/13 4:26 PM ]

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);
Comment by George Fraser [ 28/Oct/13 5:20 PM ]

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?

Comment by Travis Thieman [ 29/Oct/13 6:37 PM ]

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

Comment by George Fraser [ 29/Oct/13 7:34 PM ]

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

Comment by Travis Thieman [ 30/Oct/13 10:22 AM ]

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.

Comment by Travis Thieman [ 30/Oct/13 12:36 PM ]

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.

Comment by George Fraser [ 30/Oct/13 12:43 PM ]

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.

Comment by David Nolen [ 30/Oct/13 12:43 PM ]

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).

Comment by Travis Thieman [ 30/Oct/13 12:46 PM ]

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.

Comment by Travis Thieman [ 30/Oct/13 2:12 PM ]

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.

Comment by Travis Thieman [ 30/Oct/13 2:57 PM ]

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
Comment by David Nolen [ 30/Oct/13 3:19 PM ]

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

Comment by George Fraser [ 30/Oct/13 6:16 PM ]

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

Comment by David Nolen [ 30/Oct/13 7:41 PM ]

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

Generated at Thu Aug 21 05:09:59 CDT 2014 using JIRA 4.4#649-r158309.