Handle property definition by {cr|Object}.defineProperty() in compiler pass

BUG=393873
R=dbeam@chromium.org,tbreisacher@chromium.org
TEST=third_party/closure_compiler/runner/how_to_test_compiler_pass.md

Review URL: https://codereview.chromium.org/460163002

Cr-Commit-Position: refs/heads/master@{#289535}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@289535 0039d316-1c4b-4281-b951-d872f2087c98

third_party/closure_compiler/README.chromium

@@ -18,6 +18,7 @@ Local modifications:
   compiler to "IDE mode" (single-file checks, doesn't stop on first error).
 - Chrome-specific coding conventions to understand cr.addSingletonGetter().
 - third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java
-  Added pass to handle Chrome-specific namespace creation with cr.define()
+  Added pass to handle namespace definition with cr.define() and property
+  definition with {cr|Object}.defineProperty().
   See third_party/closure_compiler/runner/how_to_test_compiler_pass.md for
   testing instructions on this pass.

third_party/closure_compiler/compiler_customization_test.py

@@ -6,7 +6,8 @@
 import os
 import unittest
 
-from checker import Checker, FileCache, Flattener
+from checker import Checker
+from processor import FileCache, Processor
 
 
 CR_FILE = os.path.join("..", "..", "ui", "webui", "resources", "js", "cr.js")
@@ -18,22 +19,32 @@ def rel_to_abs(rel_path):
 
 
 class CompilerCustomizationTest(unittest.TestCase):
-  _CR_DEFINE_DEFINITION = Flattener(rel_to_abs(CR_FILE)).contents
+  _CR_DEFINE_DEFINITION = Processor(rel_to_abs(CR_FILE)).contents
 
   def setUp(self):
     self._checker = Checker()
 
-  def _runCheckerTest(self, source_code, expected_error):
+  def _runChecker(self, source_code):
     file_path = "/script.js"
     FileCache._cache[file_path] = source_code
-    _, output = self._checker.check(file_path)
+    return self._checker.check(file_path)
+
+  def _runCheckerTestExpectError(self, source_code, expected_error):
+    _, output = self._runChecker(source_code)
 
     self.assertTrue(expected_error in output,
         msg="Expected chunk: \n%s\n\nOutput:\n%s\n" % (
             expected_error, output))
 
+  def _runCheckerTestExpectSuccess(self, source_code):
+    return_code, output = self._runChecker(source_code)
+
+    self.assertTrue(return_code == 0,
+        msg="Expected success, got return code %d\n\nOutput:\n%s\n" % (
+            return_code, output))
+
   def testGetInstance(self):
-    self._runCheckerTest(source_code="""
+    self._runCheckerTestExpectError("""
 var cr = {
   /** @param {!Function} ctor */
   addSingletonGetter: function(ctor) {
@@ -51,12 +62,11 @@ function Class() {
 
 cr.addSingletonGetter(Class);
 Class.getInstance().needsNumber("wrong type");
-""",
-      expected_error="ERROR - actual parameter 1 of Class.needsNumber does "
-                     "not match formal parameter")
+""", "ERROR - actual parameter 1 of Class.needsNumber does not match formal "
+        "parameter")
 
   def testCrDefineFunctionDefinition(self):
-    self._runCheckerTest(source_code=self._CR_DEFINE_DEFINITION + """
+    self._runCheckerTestExpectError(self._CR_DEFINE_DEFINITION + """
 cr.define('a.b.c', function() {
   /** @param {number} num */
   function internalName(num) {}
@@ -67,11 +77,11 @@ cr.define('a.b.c', function() {
 });
 
 a.b.c.needsNumber("wrong type");
-""", expected_error="ERROR - actual parameter 1 of a.b.c.needsNumber does "
-                    "not match formal parameter")
+""", "ERROR - actual parameter 1 of a.b.c.needsNumber does not match formal "
+        "parameter")
 
   def testCrDefineFunctionAssignment(self):
-    self._runCheckerTest(source_code=self._CR_DEFINE_DEFINITION + """
+    self._runCheckerTestExpectError(self._CR_DEFINE_DEFINITION + """
 cr.define('a.b.c', function() {
   /** @param {number} num */
   var internalName = function(num) {};
@@ -82,11 +92,11 @@ cr.define('a.b.c', function() {
 });
 
 a.b.c.needsNumber("wrong type");
-""", expected_error="ERROR - actual parameter 1 of a.b.c.needsNumber does "
-                    "not match formal parameter")
+""", "ERROR - actual parameter 1 of a.b.c.needsNumber does not match formal "
+        "parameter")
 
   def testCrDefineConstructorDefinitionPrototypeMethod(self):
-    self._runCheckerTest(source_code=self._CR_DEFINE_DEFINITION + """
+    self._runCheckerTestExpectError(self._CR_DEFINE_DEFINITION + """
 cr.define('a.b.c', function() {
   /** @constructor */
   function ClassInternalName() {}
@@ -102,11 +112,11 @@ cr.define('a.b.c', function() {
 });
 
 new a.b.c.ClassExternalName().method("wrong type");
-""", expected_error="ERROR - actual parameter 1 of a.b.c.ClassExternalName."
-                    "prototype.method does not match formal parameter")
+""", "ERROR - actual parameter 1 of a.b.c.ClassExternalName.prototype.method "
+        "does not match formal parameter")
 
   def testCrDefineConstructorAssignmentPrototypeMethod(self):
-    self._runCheckerTest(source_code=self._CR_DEFINE_DEFINITION + """
+    self._runCheckerTestExpectError(self._CR_DEFINE_DEFINITION + """
 cr.define('a.b.c', function() {
   /** @constructor */
   var ClassInternalName = function() {};
@@ -122,11 +132,11 @@ cr.define('a.b.c', function() {
 });
 
 new a.b.c.ClassExternalName().method("wrong type");
-""", expected_error="ERROR - actual parameter 1 of a.b.c.ClassExternalName."
-                    "prototype.method does not match formal parameter")
+""", "ERROR - actual parameter 1 of a.b.c.ClassExternalName.prototype.method "
+        "does not match formal parameter")
 
   def testCrDefineEnum(self):
-    self._runCheckerTest(source_code=self._CR_DEFINE_DEFINITION + """
+    self._runCheckerTestExpectError(self._CR_DEFINE_DEFINITION + """
 cr.define('a.b.c', function() {
   /** @enum {string} */
   var internalNameForEnum = {key: 'wrong_type'};
@@ -140,8 +150,42 @@ cr.define('a.b.c', function() {
 function needsNumber(num) {}
 
 needsNumber(a.b.c.exportedEnum.key);
-""", expected_error="ERROR - actual parameter 1 of needsNumber does not "
-                    "match formal parameter")
+""", "ERROR - actual parameter 1 of needsNumber does not match formal "
+        "parameter")
+
+  def testObjectDefineProperty(self):
+    self._runCheckerTestExpectSuccess("""
+/** @constructor */
+function Class() {}
+
+Object.defineProperty(Class.prototype, 'myProperty', {});
+
+alert(new Class().myProperty);
+""")
+
+  def testCrDefineProperty(self):
+    self._runCheckerTestExpectSuccess(self._CR_DEFINE_DEFINITION + """
+/** @constructor */
+function Class() {}
+
+cr.defineProperty(Class.prototype, 'myProperty', cr.PropertyKind.JS);
+
+alert(new Class().myProperty);
+""")
+
+  def testCrDefinePropertyTypeChecking(self):
+    self._runCheckerTestExpectError(self._CR_DEFINE_DEFINITION + """
+/** @constructor */
+function Class() {}
+
+cr.defineProperty(Class.prototype, 'booleanProp', cr.PropertyKind.BOOL_ATTR);
+
+/** @param {number} num */
+function needsNumber(num) {}
+
+needsNumber(new Class().booleanProp);
+""", "ERROR - actual parameter 1 of needsNumber does not match formal "
+        "parameter")
 
 
 if __name__ == "__main__":

third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java

@@ -6,7 +6,10 @@ package com.google.javascript.jscomp;
 
 import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
 import com.google.javascript.rhino.IR;
+import com.google.javascript.rhino.JSDocInfoBuilder;
+import com.google.javascript.rhino.JSTypeExpression;
 import com.google.javascript.rhino.Node;
+import com.google.javascript.rhino.Token;
 
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -14,73 +17,25 @@ import java.util.List;
 import java.util.Map;
 
 /**
- * Compiler pass for Chrome-specific needs. Right now it allows the compiler to check types with
- * methods defined inside Chrome namespaces.
+ * Compiler pass for Chrome-specific needs. It handles the following Chrome JS features:
+ * <ul>
+ * <li>namespace declaration using {@code cr.define()},
+ * <li>unquoted property declaration using {@code {cr|Object}.defineProperty()}.
+ * </ul>
  *
- * <p>The namespaces in Chrome JS are declared as follows:
- * <pre>
- * cr.define('my.namespace.name', function() {
- *   /** @param {number} arg
- *   function internalStaticMethod(arg) {}
- *
- *   /** @constructor
- *   function InternalClass() {}
- *
- *   InternalClass.prototype = {
- *     /** @param {number} arg
- *     method: function(arg) {
- *       internalStaticMethod(arg);  // let's demonstrate the change of local names after our pass
- *     }
- *   };
- *
- *   return {
- *     externalStaticMethod: internalStaticMethod,
- *     ExternalClass: InternalClass
- *   }
- * });
- * </pre>
- *
- * <p>Outside of cr.define() statement the function can be called like this:
- * {@code my.namespace.name.externalStaticMethod(42);}.
- *
- * <p>We need to check the types of arguments and return values of such functions. However, the
- * function is assigned to its namespace dictionary only at run-time and the original Closure
- * Compiler isn't smart enough to predict behavior in that case. Therefore, we need to modify the
- * AST before any compiler checks. That's how we modify it to tell the compiler what's going on:
- *
- * <pre>
- * var my = my || {};
- * my.namespace = my.namespace || {};
- * my.namespace.name = my.namespace.name || {};
- *
- * cr.define('my.namespace.name', function() {
- *   /** @param {number} arg
- *   my.namespace.name.externalStaticMethod(arg) {}
- *
- *   /** @constructor
- *   my.namespace.name.ExternalClass = function() {};
- *
- *   my.namespace.name.ExternalClass.prototype = {
- *     /** @param {number} arg
- *     method: function(arg) {
- *       my.namespace.name.externalStaticMethod(arg);  // see, it has been changed
- *     }
- *   };
- *
- *   return {
- *     externalStaticMethod: my.namespace.name.externalStaticMethod,
- *     ExternalClass: my.namespace.name.ExternalClass
- *   }
- * });
- * </pre>
+ * <p>For the details, see tests inside ChromePassTest.java.
  */
 public class ChromePass extends AbstractPostOrderCallback implements CompilerPass {
     final AbstractCompiler compiler;
 
     private static final String CR_DEFINE = "cr.define";
 
-    private static final String CR_DEFINE_COMMON_EXPLANATION = "It should be called like this: " +
-            "cr.define('name.space', function() '{ ... return {Export: Internal}; }');";
+    private static final String OBJECT_DEFINE_PROPERTY = "Object.defineProperty";
+
+    private static final String CR_DEFINE_PROPERTY = "cr.defineProperty";
+
+    private static final String CR_DEFINE_COMMON_EXPLANATION = "It should be called like this:"
+            + " cr.define('name.space', function() '{ ... return {Export: Internal}; }');";
 
     static final DiagnosticType CR_DEFINE_WRONG_NUMBER_OF_ARGUMENTS =
             DiagnosticType.error("JSC_CR_DEFINE_WRONG_NUMBER_OF_ARGUMENTS",
@@ -96,8 +51,13 @@ public class ChromePass extends AbstractPostOrderCallback implements CompilerPas
 
     static final DiagnosticType CR_DEFINE_INVALID_RETURN_IN_FUNCTION =
             DiagnosticType.error("JSC_CR_DEFINE_INVALID_RETURN_IN_SECOND_ARGUMENT",
-                    "Function passed as second argument of cr.define() should return the " +
-                    "dictionary in its last statement. " + CR_DEFINE_COMMON_EXPLANATION);
+                    "Function passed as second argument of cr.define() should return the"
+                    + " dictionary in its last statement. " + CR_DEFINE_COMMON_EXPLANATION);
+
+    static final DiagnosticType CR_DEFINE_PROPERTY_INVALID_PROPERTY_KIND =
+            DiagnosticType.error("JSC_CR_DEFINE_PROPERTY_INVALID_PROPERTY_KIND",
+                    "Invalid cr.PropertyKind passed to cr.defineProperty(): expected ATTR,"
+                    + " BOOL_ATTR or JS, found \"{0}\".");
 
     public ChromePass(AbstractCompiler compiler) {
         this.compiler = compiler;
@@ -115,10 +75,58 @@ public class ChromePass extends AbstractPostOrderCallback implements CompilerPas
             if (callee.matchesQualifiedName(CR_DEFINE)) {
                 visitNamespaceDefinition(node, parent);
                 compiler.reportCodeChange();
+            } else if (callee.matchesQualifiedName(OBJECT_DEFINE_PROPERTY) ||
+                    callee.matchesQualifiedName(CR_DEFINE_PROPERTY)) {
+                visitPropertyDefinition(node, parent);
+                compiler.reportCodeChange();
             }
         }
     }
 
+    private void visitPropertyDefinition(Node call, Node parent) {
+        Node callee = call.getFirstChild();
+        String target = call.getChildAtIndex(1).getQualifiedName();
+        if (callee.matchesQualifiedName(CR_DEFINE_PROPERTY) && !target.endsWith(".prototype")) {
+            target += ".prototype";
+        }
+
+        Node property = call.getChildAtIndex(2);
+
+        Node getPropNode = NodeUtil.newQualifiedNameNode(compiler.getCodingConvention(),
+                target + "." + property.getString()).srcrefTree(call);
+
+        if (callee.matchesQualifiedName(CR_DEFINE_PROPERTY)) {
+            setJsDocWithType(getPropNode, getTypeByCrPropertyKind(call.getChildAtIndex(3)));
+        } else {
+            setJsDocWithType(getPropNode, new Node(Token.QMARK));
+        }
+
+        Node definitionNode = IR.exprResult(getPropNode).srcref(parent);
+
+        parent.getParent().addChildAfter(definitionNode, parent);
+    }
+
+    private Node getTypeByCrPropertyKind(Node propertyKind) {
+        if (propertyKind.matchesQualifiedName("cr.PropertyKind.ATTR")) {
+            return IR.string("string");
+        }
+        if (propertyKind.matchesQualifiedName("cr.PropertyKind.BOOL_ATTR")) {
+            return IR.string("boolean");
+        }
+        if (propertyKind.matchesQualifiedName("cr.PropertyKind.JS")) {
+            return new Node(Token.QMARK);
+        }
+        compiler.report(JSError.make(propertyKind, CR_DEFINE_PROPERTY_INVALID_PROPERTY_KIND,
+                propertyKind.getQualifiedName()));
+        return null;
+    }
+
+    private void setJsDocWithType(Node target, Node type) {
+        JSDocInfoBuilder builder = new JSDocInfoBuilder(false);
+        builder.recordType(new JSTypeExpression(type, ""));
+        target.setJSDocInfo(builder.build(target));
+    }
+
     private void visitNamespaceDefinition(Node crDefineCallNode, Node parent) {
         if (crDefineCallNode.getChildCount() != 3) {
             compiler.report(JSError.make(crDefineCallNode, CR_DEFINE_WRONG_NUMBER_OF_ARGUMENTS));
@@ -141,14 +149,14 @@ public class ChromePass extends AbstractPostOrderCallback implements CompilerPas
             parent.getParent().addChildBefore(n, parent);
         }
 
-        Node returnNode, functionBlock, objectLit;
         if (!function.isFunction()) {
             compiler.report(JSError.make(namespaceArg, CR_DEFINE_INVALID_SECOND_ARGUMENT));
             return;
         }
 
-        if ((functionBlock = function.getLastChild()) == null ||
-                (returnNode = functionBlock.getLastChild()) == null ||
+        Node returnNode, objectLit;
+        Node functionBlock = function.getLastChild();
+        if ((returnNode = functionBlock.getLastChild()) == null ||
                 !returnNode.isReturn() ||
                 (objectLit = returnNode.getFirstChild()) == null ||
                 !objectLit.isObjectLit()) {
@@ -210,9 +218,9 @@ public class ChromePass extends AbstractPostOrderCallback implements CompilerPas
     }
 
     private class RenameInternalsToExternalsCallback extends AbstractPostOrderCallback {
-        final private String namespaceName;
-        final private Map<String, String> exports;
-        final private Node namespaceBlock;
+        private final String namespaceName;
+        private final Map<String, String> exports;
+        private final Node namespaceBlock;
 
         public RenameInternalsToExternalsCallback(String namespaceName,
                 Map<String, String> exports, Node namespaceBlock) {
@@ -223,10 +231,6 @@ public class ChromePass extends AbstractPostOrderCallback implements CompilerPas
 
         @Override
         public void visit(NodeTraversal t, Node n, Node parent) {
-            if (n == null) {
-                return;
-            }
-
             if (n.isFunction() && parent == this.namespaceBlock &&
                     this.exports.containsKey(n.getFirstChild().getString())) {
                 // It's a top-level function/constructor definition.
@@ -245,8 +249,9 @@ public class ChromePass extends AbstractPostOrderCallback implements CompilerPas
                 // externalName.
                 Node functionTree = n.cloneTree();
                 Node exprResult = IR.exprResult(
-                        IR.assign(buildQualifiedName(n.getFirstChild()), functionTree));
-                NodeUtil.setDebugInformation(exprResult, n, n.getFirstChild().getString());
+                            IR.assign(buildQualifiedName(n.getFirstChild()), functionTree).srcref(n)
+                        ).srcref(n);
+
                 if (n.getJSDocInfo() != null) {
                     exprResult.getFirstChild().setJSDocInfo(n.getJSDocInfo());
                     functionTree.removeProp(Node.JSDOC_INFO_PROP);
@@ -266,8 +271,9 @@ public class ChromePass extends AbstractPostOrderCallback implements CompilerPas
                         //   my.namespace.name.enum = { 'one': 1, 'two': 2 };
                         Node varContent = n.removeFirstChild();
                         Node exprResult = IR.exprResult(
-                                IR.assign(buildQualifiedName(n), varContent));
-                        NodeUtil.setDebugInformation(exprResult, parent, n.getString());
+                                    IR.assign(buildQualifiedName(n), varContent).srcref(parent)
+                                ).srcref(parent);
+
                         if (parent.getJSDocInfo() != null) {
                             exprResult.getFirstChild().setJSDocInfo(parent.getJSDocInfo().clone());
                         }
@@ -294,9 +300,8 @@ public class ChromePass extends AbstractPostOrderCallback implements CompilerPas
         private Node buildQualifiedName(Node internalName) {
             String externalName = this.exports.get(internalName.getString());
             return NodeUtil.newQualifiedNameNode(compiler.getCodingConvention(),
-                                                 this.namespaceName + "." + externalName,
-                                                 internalName,
-                                                 internalName.getString());
+                    this.namespaceName + "." + externalName).srcrefTree(internalName);
         }
     }
+
 }

third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java

@@ -184,4 +184,55 @@ public class ChromePassTest extends CompilerTestCase {
             null, ChromePass.CR_DEFINE_INVALID_RETURN_IN_FUNCTION);
     }
 
+    public void testObjectDefinePropertyDefinesUnquotedProperty() throws Exception {
+        test(
+            "Object.defineProperty(a.b, 'c', {});",
+            "Object.defineProperty(a.b, 'c', {});\n" +
+            "/** @type {?} */\n" +
+            "a.b.c;");
+    }
+
+    public void testCrDefinePropertyDefinesUnquotedPropertyWithStringTypeForPropertyKindAttr()
+            throws Exception {
+        test(
+            "cr.defineProperty(a.prototype, 'c', cr.PropertyKind.ATTR);",
+            "cr.defineProperty(a.prototype, 'c', cr.PropertyKind.ATTR);\n" +
+            "/** @type {string} */\n" +
+            "a.prototype.c;");
+    }
+
+    public void testCrDefinePropertyDefinesUnquotedPropertyWithBooleanTypeForPropertyKindBoolAttr()
+            throws Exception {
+        test(
+            "cr.defineProperty(a.prototype, 'c', cr.PropertyKind.BOOL_ATTR);",
+            "cr.defineProperty(a.prototype, 'c', cr.PropertyKind.BOOL_ATTR);\n" +
+            "/** @type {boolean} */\n" +
+            "a.prototype.c;");
+    }
+
+    public void testCrDefinePropertyDefinesUnquotedPropertyWithAnyTypeForPropertyKindJs()
+            throws Exception {
+        test(
+            "cr.defineProperty(a.prototype, 'c', cr.PropertyKind.JS);",
+            "cr.defineProperty(a.prototype, 'c', cr.PropertyKind.JS);\n" +
+            "/** @type {?} */\n" +
+            "a.prototype.c;");
+    }
+
+    public void testCrDefinePropertyDefinesUnquotedPropertyOnPrototypeWhenFunctionIsPassed()
+            throws Exception {
+        test(
+            "cr.defineProperty(a, 'c', cr.PropertyKind.JS);",
+            "cr.defineProperty(a, 'c', cr.PropertyKind.JS);\n" +
+            "/** @type {?} */\n" +
+            "a.prototype.c;");
+    }
+
+    public void testCrDefinePropertyInvalidPropertyKind()
+            throws Exception {
+        test(
+            "cr.defineProperty(a.b, 'c', cr.PropertyKind.INEXISTENT_KIND);",
+            null, ChromePass.CR_DEFINE_PROPERTY_INVALID_PROPERTY_KIND);
+    }
+
 }

ui/webui/resources/js/cr.js

@@ -75,17 +75,20 @@ var cr = function() {
     /**
      * Plain old JS property where the backing data is stored as a "private"
      * field on the object.
+     * Use for properties of any type. Type will not be checked.
      */
     JS: 'js',
 
     /**
      * The property backing data is stored as an attribute on an element.
+     * Use only for properties of type {string}.
      */
     ATTR: 'attr',
 
     /**
      * The property backing data is stored as an attribute on an element. If the
      * element has the attribute then the value is true.
+     * Use only for properties of type {boolean}.
      */
     BOOL_ATTR: 'boolAttr'
   };