DevTools: [JsDocValidator] Make sure function receivers agree with @this annotations

R=sergeyv@chromium.org, vsevik@chromium.org, aandrey, sergeyv, vsevik

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

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

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

-1a87df50daef26f0801f8d044485daffc83f42817310d779eaa9de9aad5b7538 src
-19d5abc0792d134ead5a26d1e7637a15fea112d94cd364ae6f8b67e636378599 jsdoc-validator.jar
+61eae58d8ddc4e962f98143aa6b244663bf52fa311d4a78e94302dfeafa314b8 src
+025d06d67547086a28dea803763983ffbfe8d5069a3cd9bd5d61c6e475bfd38a jsdoc-validator.jar

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

@@ -7,6 +7,8 @@ import com.google.javascript.rhino.head.ast.Comment;
 import com.google.javascript.rhino.head.ast.FunctionNode;
 import com.google.javascript.rhino.head.ast.ObjectProperty;
 
+import org.chromium.devtools.jsdoc.ValidatorContext;
+
 public class AstUtil {
 
     private static final String PROTOTYPE_SUFFIX = ".prototype";
@@ -88,5 +90,10 @@ public class AstUtil {
         return null;
     }
 
+    static boolean hasThisAnnotation(FunctionNode node, ValidatorContext context) {
+        Comment comment = AstUtil.getJsDocNode(node);
+        return comment != null && context.getNodeText(comment).contains("@this");
+    }
+
     private AstUtil() {}
 }

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

@@ -22,4 +22,8 @@ abstract class ContextTrackingChecker {
     protected ValidatorContext getContext() {
         return state.getContext();
     }
+
+    void reportErrorAtNodeStart(AstNode node, String errorText) {
+        getContext().reportErrorInNode(node, 0, errorText);
+    }
 }

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

@@ -32,6 +32,7 @@ public class ContextTrackingValidationCheck extends ValidationCheck {
         registerClient(new ProtoFollowsExtendsChecker());
         registerClient(new RequiredThisAnnotationChecker());
         registerClient(new ReturnAnnotationChecker());
+        registerClient(new FunctionReceiverChecker());
     }
 
     @Override

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

+package org.chromium.devtools.jsdoc.checks;
+
+import com.google.javascript.rhino.head.Token;
+import com.google.javascript.rhino.head.ast.AstNode;
+import com.google.javascript.rhino.head.ast.FunctionCall;
+import com.google.javascript.rhino.head.ast.FunctionNode;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+public final class FunctionReceiverChecker extends ContextTrackingChecker {
+
+    private final Map<String, FunctionRecord> nestedFunctionsByName = new HashMap<>();
+    private final Map<String, Set<CallSite>> callSitesByFunctionName = new HashMap<>();
+    private final Map<String, Set<AstNode>> argumentNodesByName = new HashMap<>();
+
+    @Override
+    void enterNode(AstNode node) {
+        switch (node.getType()) {
+        case Token.CALL:
+            handleCall((FunctionCall) node);
+            break;
+        case Token.FUNCTION:
+            FunctionRecord function = getState().getCurrentFunctionRecord();
+            if (function == null) {
+                break;
+            }
+            if (function.isTopLevelFunction()) {
+                argumentNodesByName.clear();
+            } else {
+                AstNode nameNode = AstUtil.getFunctionNameNode((FunctionNode) node);
+                if (nameNode == null) {
+                    break;
+                }
+                nestedFunctionsByName.put(getContext().getNodeText(nameNode), function);
+            }
+            break;
+        default:
+            break;
+        }
+    }
+
+    private void handleCall(FunctionCall functionCall) {
+        String[] targetParts = getContext().getNodeText(functionCall.getTarget()).split("\\.");
+        String firstPart = targetParts[0];
+        int partCount = targetParts.length;
+        List<String> arguments = new ArrayList<>(functionCall.getArguments().size());
+        for (AstNode argumentNode : functionCall.getArguments()) {
+            String argumentText = getContext().getNodeText(argumentNode);
+            arguments.add(argumentText);
+            getOrCreateSetByKey(argumentNodesByName, argumentText).add(argumentNode);
+        }
+        boolean hasBind = partCount > 1 && "bind".equals(targetParts[partCount - 1]);
+        if (hasBind && partCount == 3 && "this".equals(firstPart) &&
+            !(arguments.size() > 0 && "this".equals(arguments.get(0)))) {
+                reportErrorAtNodeStart(functionCall,
+                        "Member function can only be bound to 'this' as the receiver");
+                return;
+        }
+        if (partCount > 2 || "this".equals(firstPart)) {
+            return;
+        }
+        boolean hasReceiver = hasBind && isReceiverSpecified(arguments);
+        hasReceiver |= (partCount == 2) &&
+                ("call".equals(targetParts[1]) || "apply".equals(targetParts[1])) &&
+                isReceiverSpecified(arguments);
+        getOrCreateSetByKey(callSitesByFunctionName, firstPart)
+                .add(new CallSite(hasReceiver, functionCall));
+    }
+
+    private static <K, T> Set<T> getOrCreateSetByKey(Map<K, Set<T>> map, K key) {
+        Set<T> set = map.get(key);
+        if (set == null) {
+            set = new HashSet<>();
+            map.put(key, set);
+        }
+        return set;
+    }
+
+    private boolean isReceiverSpecified(List<String> arguments) {
+        return arguments.size() > 0 && !"null".equals(arguments.get(0));
+    }
+
+    @Override
+    void leaveNode(AstNode node) {
+        if (node.getType() != Token.FUNCTION) {
+            return;
+        }
+
+        FunctionRecord function = getState().getCurrentFunctionRecord();
+        if (function == null || !function.isTopLevelFunction()) {
+            return;
+        }
+
+        for (FunctionRecord record : nestedFunctionsByName.values()) {
+            processNestedFunction(record);
+        }
+        nestedFunctionsByName.clear();
+        callSitesByFunctionName.clear();
+        argumentNodesByName.clear();
+    }
+
+    private void processNestedFunction(FunctionRecord record) {
+        String name = record.name;
+        Set<AstNode> argumentUsages = argumentNodesByName.get(name);
+        Set<CallSite> callSites = callSitesByFunctionName.get(name);
+        boolean hasThisAnnotation = AstUtil.hasThisAnnotation(record.functionNode, getContext());
+        if (hasThisAnnotation && argumentUsages != null) {
+            for (AstNode argumentNode : argumentUsages) {
+                reportErrorAtNodeStart(argumentNode,
+                        "Function annotated with @this used as argument without " +
+                         "bind(<non-null-receiver>)");
+            }
+        }
+
+        if (callSites == null) {
+            return;
+        }
+        for (CallSite callSite : callSites) {
+            if (hasThisAnnotation == callSite.hasReceiver || record.isConstructor) {
+                continue;
+            }
+            if (callSite.hasReceiver) {
+                reportErrorAtNodeStart(callSite.callNode,
+                        "Receiver specified for a function not annotated with @this");
+            } else {
+                reportErrorAtNodeStart(callSite.callNode,
+                        "Receiver not specified for a function annotated with @this");
+            }
+        }
+    }
+
+    private static class CallSite {
+        boolean hasReceiver;
+        FunctionCall callNode;
+
+        public CallSite(boolean hasReceiver, FunctionCall callNode) {
+            this.hasReceiver = hasReceiver;
+            this.callNode = callNode;
+        }
+    }
+}

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

@@ -99,7 +99,7 @@ public final class ProtoFollowsExtendsChecker extends ContextTrackingChecker {
         if (extendedType == null) {
             return;
         }
-        if (extendedType != null && !IGNORED_SUPER_TYPES.contains(extendedType.superTypeName)) {
+        if (!IGNORED_SUPER_TYPES.contains(extendedType.superTypeName)) {
             functionsMissingSuperCall.add(function);
         }
     }
@@ -113,7 +113,7 @@ public final class ProtoFollowsExtendsChecker extends ContextTrackingChecker {
         if (extendedType == null) {
             return;
         }
-        getContext().reportErrorInNode(AstUtil.getFunctionNameNode(function.functionNode), 0,
+        reportErrorAtNodeStart(AstUtil.getFunctionNameNode(function.functionNode),
                 String.format("Type %s extends %s but does not properly invoke its constructor",
                         function.name, extendedType.superTypeName));
     }
@@ -162,18 +162,18 @@ public final class ProtoFollowsExtendsChecker extends ContextTrackingChecker {
         typesWithAssignedProto.add(currentType);
         String value = state.getNodeText(node.getRight());
         if (!AstUtil.isPrototypeName(value)) {
-            state.getContext().reportErrorInNode(
-                    node.getRight(), 0, "__proto__ value is not a prototype");
+            reportErrorAtNodeStart(
+                    node.getRight(), "__proto__ value is not a prototype");
             return;
         }
         String superType = AstUtil.getTypeNameFromPrototype(value);
         if (type.isInterface) {
-            state.getContext().reportErrorInNode(node.getLeft(), 0, String.format(
+            reportErrorAtNodeStart(node.getLeft(), String.format(
                     "__proto__ defined for interface %s", type.typeName));
             return;
         } else {
             if (type.extendedTypes.isEmpty()) {
-                state.getContext().reportErrorInNode(node.getRight(), 0, String.format(
+                reportErrorAtNodeStart(node.getRight(), String.format(
                         "No @extends annotation for %s extending %s", type.typeName, superType));
                 return;
             }
@@ -183,7 +183,7 @@ public final class ProtoFollowsExtendsChecker extends ContextTrackingChecker {
         InheritanceEntry entry = type.getFirstExtendedType();
         String extendedTypeName = entry.superTypeName;
         if (!superType.equals(extendedTypeName)) {
-            state.getContext().reportErrorInNode(node.getRight(), 0, String.format(
+            reportErrorAtNodeStart(node.getRight(), String.format(
                     "Supertype does not match %s declared in @extends for %s (line %d)",
                     extendedTypeName, type.typeName,
                     state.getContext().getPosition(entry.jsDocNode, entry.offsetInJsDocText).line));
@@ -213,7 +213,7 @@ public final class ProtoFollowsExtendsChecker extends ContextTrackingChecker {
             return;
         }
         if (!type.extendedTypes.isEmpty()) {
-            state.getContext().reportErrorInNode(prototypeValueNode, 0, String.format(
+            reportErrorAtNodeStart(prototypeValueNode, String.format(
                     "@extends found for type %s but its prototype is not an object "
                     + "containing __proto__", AstUtil.getTypeNameFromPrototype(assignedTypeName)));
         }

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

@@ -2,7 +2,6 @@ package org.chromium.devtools.jsdoc.checks;
 
 import com.google.javascript.rhino.head.Token;
 import com.google.javascript.rhino.head.ast.AstNode;
-import com.google.javascript.rhino.head.ast.Comment;
 import com.google.javascript.rhino.head.ast.FunctionNode;
 
 import java.util.HashSet;
@@ -33,20 +32,23 @@ public final class RequiredThisAnnotationChecker extends ContextTrackingChecker
         }
 
         ContextTrackingState state = getState();
-        FunctionRecord record = state.getCurrentFunctionRecord();
-        if (!functionsRequiringThisAnnotation.contains(record)) {
+        FunctionRecord function = state.getCurrentFunctionRecord();
+        FunctionNode functionNode = (FunctionNode) node;
+        if (!functionsRequiringThisAnnotation.contains(function)) {
+            AstNode functionNameNode = AstUtil.getFunctionNameNode(functionNode);
+            if (functionNameNode != null && !function.isTopLevelFunction() &&
+                    AstUtil.hasThisAnnotation(functionNode, getContext())) {
+                reportErrorAtNodeStart(
+                        functionNameNode,
+                        "@this annotation found for function not referencing 'this'");
+            }
             return;
         }
-        FunctionNode functionNode = (FunctionNode) node;
         AstNode functionNameNode = AstUtil.getFunctionNameNode(functionNode);
-        if (functionNameNode != null && shouldAddThisAnnotation(functionNode)) {
-            state.getContext().reportErrorInNode(functionNameNode, 0,
+        if (functionNameNode != null && !AstUtil.hasThisAnnotation(functionNode, getContext())) {
+            reportErrorAtNodeStart(
+                    functionNameNode,
                     "@this annotation is required for functions referencing 'this'");
         }
     }
-
-    private boolean shouldAddThisAnnotation(FunctionNode node) {
-        Comment comment = AstUtil.getJsDocNode(node);
-        return comment == null || !getContext().getNodeText(comment).contains("@this");
-    }
 }

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

@@ -101,7 +101,8 @@ public final class ReturnAnnotationChecker extends ContextTrackingChecker {
 
         if (isReturningFunction) {
             if (!function.hasReturnAnnotation() && isApiFunction) {
-                getContext().reportErrorInNode(functionNameNode, 0,
+                reportErrorAtNodeStart(
+                        functionNameNode,
                         "@return annotation is required for API functions that return value");
             }
         } else {
@@ -110,7 +111,7 @@ public final class ReturnAnnotationChecker extends ContextTrackingChecker {
             if (function.hasReturnAnnotation()
                     && !isInterfaceFunction
                     && !throwingFunctions.contains(function)) {
-                getContext().reportErrorInNode(functionNameNode, 0,
+                reportErrorAtNodeStart(functionNameNode,
                         "@return annotation found, yet function does not return value");
             }
         }

third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/golden.dat

@@ -186,4 +186,68 @@ function badFuncAnnotatedButNoReturnValue() // ERROR - no returned value.
         function badCallbackInMethod() {
                  ^
 
-Total errors: 47
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:153: ERROR - @this annotation found for function not referencing 'this'
+        function callbackNotReferencingThis() {
+                 ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:191: ERROR - Member function can only be bound to 'this' as the receiver
+        var badMemberBinding1 = this.memberTwo.bind(null); // ERROR - Member not bound to |this| receiver.
+                                ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:192: ERROR - Member function can only be bound to 'this' as the receiver
+        var badMemberBinding2 = this.memberTwo.bind(bar); // ERROR - Member not bound to |this| receiver.
+                                ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:217: ERROR - Receiver not specified for a function annotated with @this
+        callbackWithThis(); // ERROR - No receiver.
+        ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:218: ERROR - Receiver not specified for a function annotated with @this
+        callbackWithThis.call(); // ERROR - No receiver.
+        ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:219: ERROR - Receiver not specified for a function annotated with @this
+        callbackWithThis.call(null); // ERROR - No receiver.
+        ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:220: ERROR - Receiver not specified for a function annotated with @this
+        callbackWithThis.apply(); // ERROR - No receiver.
+        ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:221: ERROR - Receiver not specified for a function annotated with @this
+        callbackWithThis.apply(null); // ERROR - No receiver.
+        ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:222: ERROR - Receiver specified for a function not annotated with @this
+        callbackNoThis.call(this); // ERROR - Function has no @this annotation.
+        ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:223: ERROR - Receiver specified for a function not annotated with @this
+        callbackNoThis.call(foo); // ERROR - Function has no @this annotation.
+        ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:224: ERROR - Receiver specified for a function not annotated with @this
+        callbackNoThis.apply(this); // ERROR - Function has no @this annotation.
+        ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:225: ERROR - Receiver specified for a function not annotated with @this
+        callbackNoThis.bind(this); // ERROR - Function has no @this annotation.
+        ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:227: ERROR - Function annotated with @this used as argument without bind(<non-null-receiver>)
+        this.memberTwo(callbackWithThis); // ERROR - Used as argument with no bound receiver.
+                       ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:228: ERROR - Receiver not specified for a function annotated with @this
+        this.memberTwo(callbackWithThis.bind(null, 2)); // ERROR - Used as argument with no bound receiver (null means "no receiver").
+                       ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:229: ERROR - Receiver specified for a function not annotated with @this
+        this.memberTwo(callbackNoThis.bind(this)); // ERROR - Bound to a receiver but has no @this annotation.
+                       ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:230: ERROR - Receiver specified for a function not annotated with @this
+        this.memberTwo(callbackNoThis.bind(foo)); // ERROR - Bound to a receiver but has no @this annotation.
+                       ^
+
+Total errors: 63

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

@@ -146,6 +146,13 @@ TypeThree.prototype = {
         function badCallbackInMethod() {
             this.bar = this.bar + 1; // ERROR - @this not declared.
         }
+
+        /**
+         * @this {TypeOne}
+         */
+        function callbackNotReferencingThis() {
+            return 3; // ERROR - @this for a function not referencing |this|.
+        }
     }
 }
 
@@ -173,3 +180,55 @@ var object = {
         }
     };
 })();
+
+/**
+ * @constructor
+ */
+var ReceiverTest = function() {}
+
+ReceiverTest.prototype = {
+    memberOne: function() {
+        var badMemberBinding1 = this.memberTwo.bind(null); // ERROR - Member not bound to |this| receiver.
+        var badMemberBinding2 = this.memberTwo.bind(bar); // ERROR - Member not bound to |this| receiver.
+        var goodMemberBinding = this.memberTwo.bind(this);
+
+        /** @this {ReceiverTest} */
+        function callbackWithThis()
+        {
+            this.memberTwo();
+        }
+
+        function callbackNoThis()
+        {
+            return 42;
+        }
+
+        callbackWithThis.call(this);
+        callbackWithThis.call(foo);
+        callbackNoThis();
+        callbackNoThis.call(null, 1);
+        callbackNoThis.apply(null, [2]);
+        callbackNoThis.bind(null, 1);
+        this.memberTwo(callbackWithThis.bind(this, 1));
+        this.memberTwo(callbackWithThis.bind(foo, 1));
+        this.memberTwo(callbackNoThis);
+        this.memberTwo(callbackNoThis.bind(null));
+
+        callbackWithThis(); // ERROR - No receiver.
+        callbackWithThis.call(); // ERROR - No receiver.
+        callbackWithThis.call(null); // ERROR - No receiver.
+        callbackWithThis.apply(); // ERROR - No receiver.
+        callbackWithThis.apply(null); // ERROR - No receiver.
+        callbackNoThis.call(this); // ERROR - Function has no @this annotation.
+        callbackNoThis.call(foo); // ERROR - Function has no @this annotation.
+        callbackNoThis.apply(this); // ERROR - Function has no @this annotation.
+        callbackNoThis.bind(this); // ERROR - Function has no @this annotation.
+
+        this.memberTwo(callbackWithThis); // ERROR - Used as argument with no bound receiver.
+        this.memberTwo(callbackWithThis.bind(null, 2)); // ERROR - Used as argument with no bound receiver (null means "no receiver").
+        this.memberTwo(callbackNoThis.bind(this)); // ERROR - Bound to a receiver but has no @this annotation.
+        this.memberTwo(callbackNoThis.bind(foo)); // ERROR - Bound to a receiver but has no @this annotation.
+    },
+
+    memberTwo: function(arg) {}
+}