ClojureScript

referred namespace is shadowed by argument name in whitespace mode

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Environment:
    cljs 1934

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))
  1. cljs-631-20131030.diff
    30/Oct/13 2:12 PM
    2 kB
    Travis Thieman
  2. shadow.tar.gz
    21/Oct/13 1:41 PM
    3.48 MB
    George Fraser
  3. shadow-with-let.tar.gz
    21/Oct/13 6:42 PM
    3.48 MB
    George Fraser

Activity

David Nolen made changes -
Field Original Value New Value
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))
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.

{code}
(ns bar)
(defn barFunction [x] (+ x 1))

(ns foo
  (:require [bar :refer [barFunction]]))
(defn ^:export fooFunction [bar]
  (barFunction bar))
{code}
Hide
David Nolen added a comment -

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!

Show
David Nolen added a comment - 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!
Hide
George Fraser added a comment -

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

Show
George Fraser added a comment - > 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.
Hide
George Fraser added a comment -

Updated with additional variation of error.

Show
George Fraser added a comment - Updated with additional variation of error.
George Fraser made changes -
Attachment shadow-with-let.tar.gz [ 12353 ]
Hide
David Nolen added a comment -

Thanks will look into it.

Show
David Nolen added a comment - Thanks will look into it.
David Nolen made changes -
Assignee David Nolen [ dnolen ]
Hide
Travis Thieman added a comment -

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?

Show
Travis Thieman added a comment - 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?
Hide
George Fraser added a comment -

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.

Show
George Fraser added a comment - 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.
Hide
Travis Thieman added a comment - - edited

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);
Show
Travis Thieman added a comment - - edited 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);
Hide
George Fraser added a comment -

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?

Show
George Fraser added a comment - 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?
Hide
Travis Thieman added a comment -

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

Show
Travis Thieman added a comment - My output is from main.js. I'm not sure what else to try here, do you have any ideas for me?
Hide
George Fraser added a comment -

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

Show
George Fraser added a comment - 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
Hide
Travis Thieman added a comment - - edited

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.

Show
Travis Thieman added a comment - - edited 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.
Hide
Travis Thieman added a comment - - edited

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.

Show
Travis Thieman added a comment - - edited 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.
Hide
George Fraser added a comment -

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.

Show
George Fraser added a comment - 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.
Hide
David Nolen added a comment - - edited

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

Show
David Nolen added a comment - - edited 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).
Hide
Travis Thieman added a comment - - edited

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.

Show
Travis Thieman added a comment - - edited 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.
Hide
Travis Thieman added a comment -

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.

Show
Travis Thieman added a comment - 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.
Travis Thieman made changes -
Attachment cljs-631-20131030.diff [ 12415 ]
Hide
Travis Thieman added a comment - - edited

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
Show
Travis Thieman added a comment - - edited 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
Hide
David Nolen added a comment -

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

Show
David Nolen added a comment - Looks good. George can you confirm this patch works for you? Thanks!
Hide
George Fraser added a comment -

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

Show
George Fraser added a comment - Confirmed to work on my machine and the squeaky-clean server I set up for testing. Thanks guys!
David Nolen made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Resolved [ 5 ]
David Nolen made changes -
Status Resolved [ 5 ] Closed [ 6 ]

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: