ClojureScript

Details

Type:
Defect

Status:
Closed

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

David Nolen
added a comment - 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!

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:

Travis Thieman
added a comment - 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?

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

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.

Travis Thieman
added a comment - 30/Oct/13 10:22 AM - 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.

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.

Travis Thieman
added a comment - 30/Oct/13 12:36 PM - 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.

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

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

David Nolen
added a comment - 30/Oct/13 12:43 PM - 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).

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.

Travis Thieman
added a comment - 30/Oct/13 12:46 PM - 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.

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
added a comment - 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.