<< Back to previous view

[CLJS-670] Fix/enhance ^:export to work with properties that will be modified externally Created: 08/Nov/13  Updated: 11/Nov/13  Resolved: 11/Nov/13

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

Type: Enhancement Priority: Major
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None

Attachments: Text File CLJS-670.patch    
Patch: Code

 Description   

Thin line between bug/enhancement here, but whatever...

Given this code:

(ns foo.bar)

(def ^:export baz 42)

(defn ^:export quux [] baz)

Advanced compilation yields code where baz is inlined, and even the property itself disappears. Whatever a downstream user/app/lib does to foo.bar.baz, quux will always return 42. Here's that code:

var d = this;
function f(g, e) {
  var b = g.split("."), a = d;
  b[0] in a || !a.execScript || a.execScript("var " + b[0]);
  for (var c;b.length && (c = b.shift());) {
    b.length || void 0 === e ? a = a[c] ? a[c] : a[c] = {} : a[c] = e;
  }
}
;f("foo.bar.baz", 42);
f("foo.bar.quux", function() {
  return 42;
});

Initially, I thought this was only because baz was a scalar, but the problem remains the same if an exported fn is returned by another fn; if the former is set to a different value downstream, the latter will continue returning the original that it closed over.

Based on this exchange with a GClosure compiler contributor, we need to use the @expose annotation on each exported property, and export the symbol for each intervening "namespace" segment in order to ensure that functions that refer to exported properties will return the same value that downstream users (might) mutate.



 Comments   
Comment by Chas Emerick [ 08/Nov/13 2:50 PM ]

Attached patch that changes the effect of ^:export as described above. With this patch, this input:

(ns foo.bar)

(def ^:export baz 42)

(defn ^:export quux [] baz)

(defn ^:export a-fn [] 88)

(defn ^:export returns-a-fn [] a-fn)

yields this under advanced compilation:

var a = this;
function b(h, f) {
  var d = h.split("."), c = a;
  d[0] in c || !c.execScript || c.execScript("var " + d[0]);
  for (var e;d.length && (e = d.shift());) {
    d.length || void 0 === f ? c = c[e] ? c[e] : c[e] = {} : c[e] = f;
  }
}
;var g = {a:{}};
g.a.baz = 42;
b("foo", g);
b("foo.bar", g.a);
g.a.quux = function() {
  return g.a.baz;
};
b("foo", g);
b("foo.bar", g.a);
g.a.a_fn = function() {
  return 88;
};
b("foo", g);
b("foo.bar", g.a);
g.a.returns_a_fn = function() {
  return g.a.a_fn;
};
b("foo", g);
b("foo.bar", g.a);

No exported properties are inlined, and external modifications to baz or a-fn affect functions that refer to them as expected.

Comment by Chas Emerick [ 08/Nov/13 4:24 PM ]

BTW, please consider that patch a draft. I'd like to work out some way to test this...

Comment by David Nolen [ 09/Nov/13 7:38 PM ]

Approach seems reasonable.

Comment by Chas Emerick [ 09/Nov/13 8:52 PM ]

It's subtly broken, discovered only after using it to build/test some real code. Working on it.

Comment by Chas Emerick [ 11/Nov/13 10:57 AM ]

There are various problems with the attached patch / approach, all rooted in GClosure's @expose:

1. More than preventing the munging/renaming/etc of a property, @expose disables a variety of advanced optimizations and analysis. When applied to functions, this will result in potentially a significant perf hit compared to what CLJS emits now.
2. More irritating than problematic is that a side effect of @expose is that the way we are transliterating CLJS namespaces into JavaScript "namespaces" yields spurious "incomplete alias created for namespace XXX" warnings when @expose is used within the affected namespace (even though projects that use ^:export continue to test well with the proposed patch). This is apparently a known issue.
3. @expose doesn't affect only the annotated property; its don't-rename and don't-optimize policies carry over to any same-named property anywhere in the codebase. e.g. (defn ^:export foo [] ...) in one namespace will cause any other foo vars in all other namespaces to not be renamed, inlined, optimized, etc.

Much of the above is reiterated elsewhere on the web, e.g. https://zooskdev.wordpress.com/2013/11/05/integrating-angularjs-with-a-large-google-closure-codebase/

Stepping back a bit, the objective of ^:export is solely to ensure that the affected var/property retains the name we give it, nothing else. GClosure provides an escape hatch for preventing renaming already — string-based property access — which, when combined with exporting of the "namespace"'s property, would address the problems with the existing ^:export impl.

The big hurdle is that the compiler will need to use string-based property access when touching any ^:export ed var/property. Lifting this information into the compiler environment so it can be looked up is easy; the delicate part will be changing any part of the compiler that emit var/property names to use a helper that distinguishes between locals and vars and uses string-based property access for the latter that are ^:export ed. This seems quite doable, but would be a more significant compiler change than simply twiddling goog.export* calls and such, so I thought I'd solicit feedback before moving forward.

Comment by Chas Emerick [ 11/Nov/13 11:18 AM ]

Per feedback from @dnolen in #clojure, ^:export is not intended to support mutable properties, and the changes necessary to add that support are undesirable.

Workarounds:

  • provide "setters" for any configuration/parameterization users need to do
  • in a testing context, fixtures can look up parameters set on e.g. window ahead of time and either set! or use binding to configure the tests being run, etc.
Generated at Mon Jul 28 05:40:48 CDT 2014 using JIRA 4.4#649-r158309.