DevTools: [JsDocValidator] Be smarter about function name and JSDoc detection

R=aandrey, sergeyv
NOTRY=true

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

git-svn-id: svn://svn.chromium.org/blink/trunk@175648 bbb929c8-8fbe-4397-9dbb-9b2b20218538

third_party/WebKit/Source/devtools/scripts/jsdoc-validator/hashes

-2b7a3211b34c415d5dd8b0fe6e4cb9e3b46365de62c11059e600059a4bf2f864 src
-8690d7b1e6486f73ee34ff076af215901e944d17293dc9f039d014d2f94b5804 jsdoc-validator.jar
+b5c1c0e9e16e027abae9f603ad6ff9228fe3b634be5b170ff9c267e8a3c54573 src
+ac033f55138d8536bd031ca2a1f0cfc703ffa80dbfd24b2bd34dbfda1b90ff5c jsdoc-validator.jar

third_party/WebKit/Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/AstUtil.java

@@ -24,21 +24,23 @@ public class AstUtil {
         }
 
         AstNode parentNode = functionNode.getParent();
-        if (parentNode == null) {
-            return null;
-        }
-
-        switch (parentNode.getType()) {
-        case Token.COLON:
-            return ((ObjectProperty) parentNode).getLeft();
-        case Token.ASSIGN:
-            Assignment assignment = (Assignment) parentNode;
-            if (assignment.getRight() == functionNode) {
-                return assignment.getLeft();
+        while (parentNode != null) {
+            switch (parentNode.getType()) {
+            case Token.COLON:
+                return ((ObjectProperty) parentNode).getLeft();
+            case Token.ASSIGN:
+                Assignment assignment = (Assignment) parentNode;
+                if (assignment.getRight() == functionNode &&
+                        assignment.getLeft().getType() == Token.NAME) {
+                    return assignment.getLeft();
+                }
+                parentNode = assignment.getParent();
+                break;
+            case Token.VAR:
+                return ((VariableInitializer) parentNode).getTarget();
+            default:
+                return null;
             }
-            break;
-        case Token.VAR:
-            return ((VariableInitializer) parentNode).getTarget();
         }
 
         return null;
@@ -76,21 +78,23 @@ public class AstUtil {
 
         // reader.onloadend = function() {...}
         AstNode parentNode = functionNode.getParent();
-        if (parentNode == null) {
-            return null;
-        }
-
-        switch (parentNode.getType()) {
-        case Token.COLON:
-            return ((ObjectProperty) parentNode).getLeft().getJsDocNode();
-        case Token.ASSIGN:
-            Assignment assignment = (Assignment) parentNode;
-            if (assignment.getRight() == functionNode) {
-                return assignment.getJsDocNode();
+        while (parentNode != null) {
+            switch (parentNode.getType()) {
+            case Token.COLON:
+                return ((ObjectProperty) parentNode).getLeft().getJsDocNode();
+            case Token.ASSIGN:
+                Assignment assignment = (Assignment) parentNode;
+                if (assignment.getJsDocNode() != null) {
+                    return assignment.getJsDocNode();
+                } else {
+                    parentNode = assignment.getParent();
+                }
+                break;
+            case Token.VAR:
+                return parentNode.getParent().getJsDocNode();
+            default:
+                return null;
             }
-            break;
-        case Token.VAR:
-            return parentNode.getParent().getJsDocNode();
         }
 
         return null;

third_party/WebKit/Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/ContextTrackingChecker.java

@@ -27,8 +27,7 @@ abstract class ContextTrackingChecker {
         return state.getContext();
     }
 
-    protected boolean hasAnnotationTag(FunctionNode node, String tagName) {
-        Comment comment = AstUtil.getJsDocNode(node);
+    protected boolean hasAnnotationTag(Comment comment, String tagName) {
         return comment != null &&
                 Pattern.matches("(?s).*@" + tagName + "\\b.*", getContext().getNodeText(comment));
     }

third_party/WebKit/Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/ContextTrackingValidationCheck.java

@@ -88,14 +88,16 @@ public class ContextTrackingValidationCheck extends ValidationCheck {
         AstNode nameNode = AstUtil.getFunctionNameNode(node);
 
         // It can be a type declaration: /** @constructor */ function MyType() {...}.
-        String functionName = getNodeText(nameNode);
-        boolean isConstructor =
-                functionName != null && rememberTypeRecordIfNeeded(functionName, jsDocNode);
+        // Or even /** @constructor */ window.Foo.Bar = function() {...}.
+        String functionName = nameNode == null ? null : getNodeText(nameNode);
+        boolean isConstructor = rememberTypeRecordIfNeeded(functionName, jsDocNode);
+
         TypeRecord parentType = state.getCurrentFunctionRecord() == null
                 ? state.getCurrentTypeRecord()
                 : null;
         state.pushFunctionRecord(new FunctionRecord(
                 node,
+                AstUtil.getJsDocNode(node),
                 functionName,
                 isConstructor,
                 getReturnType(jsDocNode),
@@ -162,6 +164,9 @@ public class ContextTrackingValidationCheck extends ValidationCheck {
 
     private boolean rememberTypeRecordIfNeeded(String typeName, Comment jsDocNode) {
         String jsDoc = getNodeText(jsDocNode);
+        if (typeName == null) {
+            return isConstructor(jsDoc) || isInterface(jsDoc);
+        }
         if (!isConstructor(jsDoc) && !isInterface(jsDoc)) {
             return false;
         }

third_party/WebKit/Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java

@@ -190,23 +190,29 @@ public final class FunctionReceiverChecker extends ContextTrackingChecker {
 
     private void checkThisAnnotation(FunctionRecord function) {
         AstNode functionNameNode = AstUtil.getFunctionNameNode(function.functionNode);
-        if (functionNameNode == null) {
+        if (functionNameNode == null && function.jsDocNode == null) {
+            // Do not check anonymous functions without a JSDoc.
             return;
         }
-        boolean hasThisAnnotation = hasAnnotationTag(function.functionNode, "this");
+        AstNode errorTargetNode =
+                functionNameNode == null ? function.jsDocNode : functionNameNode;
+        if (errorTargetNode == null) {
+            errorTargetNode = function.functionNode;
+        }
+        boolean hasThisAnnotation = hasAnnotationTag(function.jsDocNode, "this");
         if (hasThisAnnotation == functionReferencesThis(function)) {
             return;
         }
         if (hasThisAnnotation) {
             if (!function.isTopLevelFunction()) {
                 reportErrorAtNodeStart(
-                        functionNameNode,
+                        errorTargetNode,
                         "@this annotation found for function not referencing 'this'");
             }
             return;
         } else {
             reportErrorAtNodeStart(
-                    functionNameNode,
+                    errorTargetNode,
                     "@this annotation is required for functions referencing 'this'");
         }
     }
@@ -237,7 +243,7 @@ public final class FunctionReceiverChecker extends ContextTrackingChecker {
     private void processFunctionUsesAsArgument(
             FunctionRecord function, Set<SymbolicArgument> argumentUses) {
         if (argumentUses == null ||
-                hasAnnotationTag(function.functionNode, "suppressReceiverCheck")) {
+                hasAnnotationTag(function.jsDocNode, "suppressReceiverCheck")) {
             return;
         }
 

third_party/WebKit/Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionRecord.java

 package org.chromium.devtools.jsdoc.checks;
 
+import com.google.javascript.rhino.head.ast.Comment;
 import com.google.javascript.rhino.head.ast.FunctionNode;
 
 public class FunctionRecord {
     final FunctionNode functionNode;
+    final Comment jsDocNode;
     final String name;
     final boolean isConstructor;
     final String returnType;
     final TypeRecord enclosingType;
     final FunctionRecord enclosingFunctionRecord;
 
-    public FunctionRecord(FunctionNode functionNode, String name, boolean isConstructor,
-            String returnType, TypeRecord parentType, FunctionRecord enclosingFunctionRecord) {
+    public FunctionRecord(FunctionNode functionNode, Comment jsDocNode, String name,
+            boolean isConstructor, String returnType, TypeRecord parentType,
+            FunctionRecord enclosingFunctionRecord) {
         this.functionNode = functionNode;
+        this.jsDocNode = jsDocNode;
         this.name = name;
         this.isConstructor = isConstructor;
         this.returnType = returnType;

third_party/WebKit/Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/MethodAnnotationChecker.java

@@ -17,10 +17,10 @@ import java.util.regex.Pattern;
 public final class MethodAnnotationChecker extends ContextTrackingChecker {
 
     private static final Pattern PARAM_PATTERN =
-            Pattern.compile("@param\\s+(\\{.+\\}\\s+)?([^\\s]+).*$", Pattern.MULTILINE);
+            Pattern.compile("^[^@\n]*@param\\s+(\\{.+\\}\\s+)?([^\\s]+).*$", Pattern.MULTILINE);
 
     private static final Pattern INVALID_RETURN_PATTERN =
-            Pattern.compile("@return(?:s.*|\\s+[^{]*)$", Pattern.MULTILINE);
+            Pattern.compile("^[^@\n]*(@)return(?:s.*|\\s+[^{]*)$", Pattern.MULTILINE);
 
     private final Set<FunctionRecord> valueReturningFunctions = new HashSet<>();
     private final Set<FunctionRecord> throwingFunctions = new HashSet<>();
@@ -187,7 +187,7 @@ public final class MethodAnnotationChecker extends ContextTrackingChecker {
             return -1;
         }
         Matcher m = INVALID_RETURN_PATTERN.matcher(jsDoc);
-        return m.find() ? m.start() : -1;
+        return m.find() ? m.start(1) : -1;
     }
 
     private static AstNode getFunctionNameNode(FunctionNode functionNode) {

third_party/WebKit/Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/ProtoFollowsExtendsChecker.java

@@ -161,23 +161,29 @@ public final class ProtoFollowsExtendsChecker extends ContextTrackingChecker {
         }
         typesWithAssignedProto.add(currentType);
         String value = state.getNodeText(node.getRight());
-        if (!AstUtil.isPrototypeName(value)) {
+        boolean isNullPrototype = "null".equals(value);
+        if (!isNullPrototype && !AstUtil.isPrototypeName(value)) {
             reportErrorAtNodeStart(
                     node.getRight(), "__proto__ value is not a prototype");
             return;
         }
-        String superType = AstUtil.getTypeNameFromPrototype(value);
+        String superType = isNullPrototype ? "null" : AstUtil.getTypeNameFromPrototype(value);
         if (type.isInterface) {
             reportErrorAtNodeStart(node.getLeft(), String.format(
                     "__proto__ defined for interface %s", type.typeName));
             return;
         } else {
-            if (type.extendedTypes.isEmpty()) {
+            if (!isNullPrototype && type.extendedTypes.isEmpty()) {
                 reportErrorAtNodeStart(node.getRight(), String.format(
                         "No @extends annotation for %s extending %s", type.typeName, superType));
                 return;
             }
         }
+
+        if (isNullPrototype) {
+            return;
+        }
+
         // FIXME: Should we check that there is only one @extend-ed type
         // for the non-interface |type|? Closure is supposed to do this anyway...
         InheritanceEntry entry = type.getFirstExtendedType();

third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/function.js

@@ -84,6 +84,23 @@ TypeOne.prototype = {
 
     _privateMethod: function() // OK - non-public method.
     {
+        var obj = {};
+
+        /**
+         * @constructor
+         * @param {number} val
+         */
+        obj["a"] = obj["b"] = function(val) { // OK - constructor
+            this.foo = val;
+        }
+
+        /**
+         * @param {number} val
+         */
+        obj["c"] = obj["d"] = function(val) { // ERROR - no @this
+            this.foo = val;
+        }
+
         return 1;
     },