diff --git a/src/com/google/javascript/jscomp/PolymerClassRewriter.java b/src/com/google/javascript/jscomp/PolymerClassRewriter.java index 4c0e94c1e4f..83b880bf985 100644 --- a/src/com/google/javascript/jscomp/PolymerClassRewriter.java +++ b/src/com/google/javascript/jscomp/PolymerClassRewriter.java @@ -31,7 +31,6 @@ import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Token; import java.util.ArrayList; -import java.util.Collection; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; @@ -128,7 +127,7 @@ void rewritePolymerCall( ImmutableList readOnlyProps = parseReadOnlyProperties(cls, block); ImmutableList attributeReflectedProps = parseAttributeReflectedProperties(cls); - addInterfaceExterns(cls, readOnlyProps, attributeReflectedProps); + createExportsAndExterns(cls, readOnlyProps, attributeReflectedProps); removePropertyDocs(objLit, PolymerClassDefinition.DefinitionType.ObjectLiteral); Node statements = block.removeChildren(); @@ -218,7 +217,7 @@ void rewritePolymerClassDeclaration( ImmutableList readOnlyProps = parseReadOnlyProperties(cls, block); ImmutableList attributeReflectedProps = parseAttributeReflectedProperties(cls); - addInterfaceExterns(cls, readOnlyProps, attributeReflectedProps); + createExportsAndExterns(cls, readOnlyProps, attributeReflectedProps); // If an external interface is required, mark the class as implementing it if (polymerExportPolicy == PolymerExportPolicy.EXPORT_ALL @@ -426,19 +425,6 @@ private void appendPropertiesToBlock( } } - /** Appends all of the given methods to the given block. */ - private void appendMethodsToBlock( - final Collection methods, Node block, String basePath) { - for (MemberDefinition method : methods) { - Node propertyNode = IR.exprResult( - NodeUtil.newQName(compiler, basePath + method.name.getString())); - propertyNode.useSourceInfoIfMissingFromForTree(method.name); - JSDocInfoBuilder info = JSDocInfoBuilder.maybeCopyFrom(method.info); - propertyNode.getFirstChild().setJSDocInfo(info.build()); - block.addChildToBack(propertyNode); - } - } - /** Remove all JSDocs from properties of a class definition */ private void removePropertyDocs( final Node objLit, PolymerClassDefinition.DefinitionType defType) { @@ -539,7 +525,7 @@ private Node makeReadOnlySetter(String propName, String qualifiedPath) { JSDocInfoBuilder info = new JSDocInfoBuilder(true); // This is overriding a generated function which was added to the interface in - // {@code addInterfaceExterns}. + // {@code createExportsAndExterns}. info.recordOverride(); exprResNode.getFirstChild().setJSDocInfo(info.build()); @@ -547,12 +533,25 @@ private Node makeReadOnlySetter(String propName, String qualifiedPath) { } /** - * Adds an interface for the given ClassDefinition to externs. This allows generated setter - * functions for read-only properties to avoid renaming altogether. + * Create exports and externs to protect element properties and methods from renaming and dead + * code removal. * - * @see https://www.polymer-project.org/0.8/docs/devguide/properties.html#read-only + *

Since Polymer templates, observers, and computed properties rely on string references to + * element properties and methods, and because we don't have a way to update those references + * reliably, we instead export or extern them. + * + *

For properties, we create a new interface called {@code PolymerInterface}, add + * all element properties to it, mark that the element class {@code @implements} this interface, + * and add the interface to the Closure externs. The specific set of properties we add to this + * interface is determined by the value of {@code polymerExportPolicy}. + * + *

For methods, when {@code polymerExportPolicy = EXPORT_ALL}, we instead generate {@code + * goog.exportProperty} calls. This is preferable to using the externs, since it can result in + * better minified code (since compiled code can still use the minified name), avoids unnecessary + * {@code missingOverride} checks, and avoids needing to keep the signatures of the element class + * and the generated interface synchronized (previously the source of a bug). */ - private void addInterfaceExterns( + private void createExportsAndExterns( final PolymerClassDefinition cls, List readOnlyProps, List attributeReflectedProps) { @@ -591,7 +590,14 @@ private void addInterfaceExterns( for (MemberDefinition method : cls.methods) { uniqueMethods.put(method.name.getString(), method); } - appendMethodsToBlock(uniqueMethods.values(), block, interfaceBasePath); + for (MemberDefinition method : uniqueMethods.values()) { + addExportPropertyCall( + cls.target.getQualifiedName() + ".prototype", + method.name.getString(), + cls.target.getQualifiedName() + ".prototype." + method.name.getString(), + method.value, + cls.definition); + } } else if (polymerVersion == 1) { // For Polymer 1, all declared properties are non-renameable @@ -634,6 +640,47 @@ private void addInterfaceExterns( compiler.reportChangeToEnclosingScope(stmts); } + /** + * Insert a call to {@code goog.exportProperty} (or equivalent function depending on current + * coding convention). + * + *

Note this function is very similar to {@link GenerateExports#addExportPropertyCall}. + * + * @param object Object whose static property is being exported (e.g. @{code MyClass.prototype}). + * @param publicName Unobfuscated name to export (e.g. @{code "myMethod"}). + * @param symbol Object the name should point to (e.g. @{code MyClass.prototype.myMethod}). + * @param source The context node to use for source info. + * @param insertAfter The call will be inserted as the next sibling of this node (or as close as + * possible). + */ + private void addExportPropertyCall( + String object, String publicName, String symbol, Node source, Node insertAfter) { + CodingConvention convention = compiler.getCodingConvention(); + + // Generate the `goog.exportProperty(object, publicName, symbol);` call. + String exportPropertyFunction = convention.getExportPropertyFunction(); + Node call = + IR.call( + NodeUtil.newQName( + compiler, exportPropertyFunction, + source, exportPropertyFunction), + NodeUtil.newQName( + compiler, object, + source, exportPropertyFunction), + IR.string(publicName), + NodeUtil.newQName( + compiler, symbol, + source, exportPropertyFunction)); + Node expression = IR.exprResult(call).useSourceInfoIfMissingFromForTree(source); + + // Walk up until we find a statement we can insert after. + while (!NodeUtil.isStatementBlock(insertAfter.getParent())) { + insertAfter = insertAfter.getParent(); + } + insertAfter.getParent().addChildAfter(expression, insertAfter); + compiler.reportChangeToEnclosingScope(expression); + } + /** * @return The name of the generated extern interface which the element implements. */ diff --git a/test/com/google/javascript/jscomp/IntegrationTest.java b/test/com/google/javascript/jscomp/IntegrationTest.java index 88c86c02a0a..382a5d18249 100644 --- a/test/com/google/javascript/jscomp/IntegrationTest.java +++ b/test/com/google/javascript/jscomp/IntegrationTest.java @@ -55,6 +55,12 @@ public final class IntegrationTest extends IntegrationTestCase { private static final String CLOSURE_COMPILED = "var COMPILED = true; var goog$exportSymbol = function() {};"; + private static final String EXPORT_PROPERTY_DEF = + lines( + "goog.exportProperty = function(object, publicName, symbol) {", + " object[publicName] = symbol;", + "};"); + @Test public void testStaticMemberClass() { CompilerOptions options = createCompilerOptions(); @@ -1034,18 +1040,20 @@ public void testPolymerExportPolicyExportAll() { options.polymerExportPolicy = PolymerExportPolicy.EXPORT_ALL; Compiler compiler = - compile(options, - lines( - "class FooElement extends PolymerElement {", - " static get properties() {", - " return {", - " longUnusedProperty: String,", - " }", - " }", - " longUnusedMethod() {", - " return this.longUnusedProperty;", - " }", - "}")); + compile( + options, + lines( + EXPORT_PROPERTY_DEF, + "class FooElement extends PolymerElement {", + " static get properties() {", + " return {", + " longUnusedProperty: String,", + " }", + " }", + " longUnusedMethod() {", + " return this.longUnusedProperty;", + " }", + "}")); String source = compiler.getCurrentJsSource(); // If we see these identifiers anywhere in the output source, we know that we successfully @@ -1074,6 +1082,7 @@ public void testPolymerPropertyDeclarationsWithConstructor() { compile( options, lines( + EXPORT_PROPERTY_DEF, "class FooElement extends PolymerElement {", " constructor() {", " super();", diff --git a/test/com/google/javascript/jscomp/PolymerPassTest.java b/test/com/google/javascript/jscomp/PolymerPassTest.java index 19f9518b5a5..24173105002 100644 --- a/test/com/google/javascript/jscomp/PolymerPassTest.java +++ b/test/com/google/javascript/jscomp/PolymerPassTest.java @@ -121,6 +121,12 @@ public class PolymerPassTest extends CompilerTestCase { " */", "$jscomp.reflectObject = function (type, object) { return object; };"); + private static final String EXPORT_PROPERTY_DEF = + lines( + "goog.exportProperty = function(object, publicName, symbol) {", + " object[publicName] = symbol;", + "};"); + private int polymerVersion = 1; private PolymerExportPolicy polymerExportPolicy = PolymerExportPolicy.LEGACY; private boolean propertyRenamingEnabled = false; @@ -3549,18 +3555,42 @@ public void testObjectReflectionAddedToConfigProperties3() { "XElement.prototype.thingToDo;")); } + @Test + public void textExportsMethodsFromClassBasedElement() { + polymerExportPolicy = PolymerExportPolicy.EXPORT_ALL; + test( + 2, + lines( + EXPORT_PROPERTY_DEF, + "class TestElement extends PolymerElement {", + " method1() {}", + " method2() {}", + "}"), + lines( + EXPORT_PROPERTY_DEF, + "/** @implements {PolymerTestElementInterface} */", + "class TestElement extends PolymerElement {", + " method1() {}", + " method2() {}", + "}", + "goog.exportProperty(TestElement.prototype, \"method2\",", + " TestElement.prototype.method2);", + "goog.exportProperty(TestElement.prototype, \"method1\",", + " TestElement.prototype.method1);")); + } + /** - * When --polymer_export_policy=EXPORT_ALL, the PolymerPass will add all methods of an element to - * that element's generated interface (which is injected into the externs), including methods - * inherited from Polymer Behaviors. Ensure that each method is included on the interface only - * once, even when it is implemented in multiple places. + * When --polymer_export_policy=EXPORT_ALL, the PolymerPass will export all methods of an element + * including methods inherited from Polymer Behaviors. Ensure that each method is included on the + * interface only once, even when it is implemented in multiple places. */ @Test - public void testExportAllOnlyExportsEachMethodOnce() { + public void textExportsUniqueMethodsFromLegacyElementAndBehaviors() { polymerExportPolicy = PolymerExportPolicy.EXPORT_ALL; - - String js = + test( + 2, lines( + EXPORT_PROPERTY_DEF, "/** @polymerBehavior */", "const Behavior1 = {", " onAll: function() {},", @@ -3576,18 +3606,43 @@ public void testExportAllOnlyExportsEachMethodOnce() { " behaviors: [Behavior1, Behavior2],", " onAll() {},", " onElement: function() {},", - "});"); - - String newExterns = + "});"), lines( - EXTERNS, - "/** @interface */ var PolymerTestElementElementInterface=function(){};", - "PolymerTestElementElementInterface.prototype.onAll;", - "PolymerTestElementElementInterface.prototype.onBehavior1;", - "PolymerTestElementElementInterface.prototype.onBehavior2;", - "PolymerTestElementElementInterface.prototype.onElement"); - - testExternChanges(EXTERNS, js, newExterns); + EXPORT_PROPERTY_DEF, + "/** @nocollapse @polymerBehavior */", + "const Behavior1 = {", + " /** @suppress {checkTypes,globalThis,visibility} */ onAll: function() {},", + " /** @suppress {checkTypes,globalThis,visibility} */ onBehavior1: function() {}", + "};", + "/** @nocollapse @polymerBehavior */", + "const Behavior2 = {", + " /** @suppress {checkTypes,globalThis,visibility} */ onAll: function() {},", + " /** @suppress {checkTypes,globalThis,visibility} */ onBehavior2: function() {}", + "};", + "/**", + " * @constructor", + " * @extends {PolymerElement}", + " * @implements {PolymerTestElementElementInterface}", + " */", + "var TestElementElement = function() {};", + "/** @suppress {unusedPrivateMembers} */", + "TestElementElement.prototype.onBehavior1 = function() {};", + "/** @suppress {unusedPrivateMembers} */", + "TestElementElement.prototype.onBehavior2 = function() {};", + "Polymer(/** @lends {TestElementElement.prototype} */ {", + " is: \"test-element\",", + " behaviors: [Behavior1, Behavior2],", + " /** @this {TestElementElement} */ onAll() {},", + " /** @this {TestElementElement} */ onElement: function() {}", + "});", + "goog.exportProperty(TestElementElement.prototype, \"onElement\",", + " TestElementElement.prototype.onElement);", + "goog.exportProperty(TestElementElement.prototype, \"onBehavior2\",", + " TestElementElement.prototype.onBehavior2);", + "goog.exportProperty(TestElementElement.prototype, \"onBehavior1\",", + " TestElementElement.prototype.onBehavior1);", + "goog.exportProperty(TestElementElement.prototype, \"onAll\",", + " TestElementElement.prototype.onAll);")); } @Override