DevTools: [JsDocValidator] Fix checking of receivers specified as arguments

r169450 has resulted in multiple failures of existing code where functions
annotated with @this are passed as arguments along with their receivers.

The cases include:
- Array.prototype.forEach and other iteration-with-callback methods.
- RemoteObject.prototype.callFunction[JSON], whose functionDeclaration
  can be evaluated on any Object (but expect a certain type thereof).
- WebInspector.Object.prototype.{add,remove}EventListener, which accepts
  the callback receiver as its third argument.

This patch introduces receiver specification detection in these cases,
or resorts to IGNORE in ambiguous situations.

R=aandrey, sergeyv, vsevik
NOTRY=true

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

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

third_party/WebKit/Source/devtools/front_end/ElementsPanel.js

@@ -465,6 +465,7 @@ WebInspector.ElementsPanel.prototype = {
 
             /**
              * @return {!{offsetWidth: number, offsetHeight: number, naturalWidth: number, naturalHeight: number}}
+             * @suppressReceiverCheck
              * @this {!Element}
              */
             function dimensions()

third_party/WebKit/Source/devtools/front_end/ElementsTreeOutline.js

@@ -701,6 +701,7 @@ WebInspector.ElementsTreeOutline.prototype = {
 
             /**
              * @param {?string} pseudoType
+             * @suppressReceiverCheck
              * @this {!Element}
              */
             function toggleClassAndInjectStyleRule(pseudoType)
@@ -2396,6 +2397,7 @@ WebInspector.ElementsTreeElement.prototype = {
         function scrollIntoViewCallback(object)
         {
             /**
+             * @suppressReceiverCheck
              * @this {!Element}
              */
             function scrollIntoView()

third_party/WebKit/Source/devtools/front_end/ObjectPropertiesSection.js

@@ -740,6 +740,7 @@ WebInspector.ArrayGroupingTreeElement._populateRanges = function(treeElement, ob
     object.callFunctionJSON(packRanges, [{value: fromIndex}, {value: toIndex}, {value: WebInspector.ArrayGroupingTreeElement._bucketThreshold}, {value: WebInspector.ArrayGroupingTreeElement._sparseIterationThreshold}], callback);
 
     /**
+     * @suppressReceiverCheck
      * @this {Object}
      * @param {number=} fromIndex // must declare optional
      * @param {number=} toIndex // must declare optional
@@ -839,6 +840,7 @@ WebInspector.ArrayGroupingTreeElement._populateAsFragment = function(treeElement
     object.callFunction(buildArrayFragment, [{value: fromIndex}, {value: toIndex}, {value: WebInspector.ArrayGroupingTreeElement._sparseIterationThreshold}], processArrayFragment.bind(this));
 
     /**
+     * @suppressReceiverCheck
      * @this {Object}
      * @param {number=} fromIndex // must declare optional
      * @param {number=} toIndex // must declare optional
@@ -901,7 +903,10 @@ WebInspector.ArrayGroupingTreeElement._populateNonIndexProperties = function(tre
 {
     object.callFunction(buildObjectFragment, undefined, processObjectFragment.bind(this));
 
-    /** @this {Object} */
+    /**
+     * @suppressReceiverCheck
+     * @this {Object}
+     */
     function buildObjectFragment()
     {
         var result = Object.create(this.__proto__);

third_party/WebKit/Source/devtools/front_end/PropertiesSidebarPane.js

@@ -59,6 +59,7 @@ WebInspector.PropertiesSidebarPane.prototype = {
                 return;
 
             /**
+             * @suppressReceiverCheck
              * @this {*}
              */
             function protoList()

third_party/WebKit/Source/devtools/front_end/RemoteObject.js

@@ -284,6 +284,7 @@ WebInspector.RemoteObjectImpl.prototype = {
     {
         /**
          * @param {string} arrayStr
+         * @suppressReceiverCheck
          * @this {Object}
          */
         function remoteFunction(arrayStr)

third_party/WebKit/Source/devtools/front_end/RuntimeModel.js

@@ -254,6 +254,7 @@ WebInspector.RuntimeModel.prototype = {
 
             /**
              * @param {string} primitiveType
+             * @suppressReceiverCheck
              * @this {WebInspector.RuntimeModel}
              */
             function getCompletions(primitiveType)

third_party/WebKit/Source/devtools/scripts/compiler-runner/src/org/chromium/devtools/compiler/Runner.java

@@ -15,6 +15,7 @@ import java.io.IOException;
 import java.io.InputStreamReader;
 import java.io.PrintStream;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutorService;
@@ -177,6 +178,7 @@ public class Runner {
         protected CompilerOptions createOptions() {
             CompilerOptions options = super.createOptions();
             options.setIdeMode(true);
+            options.setExtraAnnotationNames(Collections.singletonList("suppressReceiverCheck"));
             return options;
         }
 

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

-61eae58d8ddc4e962f98143aa6b244663bf52fa311d4a78e94302dfeafa314b8 src
-025d06d67547086a28dea803763983ffbfe8d5069a3cd9bd5d61c6e475bfd38a jsdoc-validator.jar
+b5a448d248ee8e610b5559db732412030879a045401a93d6aa9ea638a9ae89aa src
+81609682686690d48af5d13b3ab005f4a1adb4d90ee53ac2264a5ba9509f186f jsdoc-validator.jar

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

@@ -90,10 +90,5 @@ 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

 package org.chromium.devtools.jsdoc.checks;
 
 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 org.chromium.devtools.jsdoc.ValidatorContext;
 
+import java.util.regex.Pattern;
+
 abstract class ContextTrackingChecker {
     private ContextTrackingState state;
 
@@ -23,7 +27,13 @@ abstract class ContextTrackingChecker {
         return state.getContext();
     }
 
-    void reportErrorAtNodeStart(AstNode node, String errorText) {
+    protected boolean hasAnnotationTag(FunctionNode node, String tagName) {
+        Comment comment = AstUtil.getJsDocNode(node);
+        return comment != null &&
+                Pattern.matches("(?s).*@" + tagName + "\\b.*", getContext().getNodeText(comment));
+    }
+
+    protected 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/FunctionReceiverChecker.java

@@ -14,9 +14,22 @@ import java.util.Set;
 
 public final class FunctionReceiverChecker extends ContextTrackingChecker {
 
+    private static final Set<String> FUNCTIONS_WITH_CALLBACK_RECEIVER_AS_SECOND_ARGUMENT =
+            new HashSet<>();
+    private static final String SUPPRESSION_HINT = "This check can be suppressed using "
+            + "@suppressReceiverCheck annotation on function declaration.";
+    static {
+        // Array.prototype methods.
+        FUNCTIONS_WITH_CALLBACK_RECEIVER_AS_SECOND_ARGUMENT.add("every");
+        FUNCTIONS_WITH_CALLBACK_RECEIVER_AS_SECOND_ARGUMENT.add("filter");
+        FUNCTIONS_WITH_CALLBACK_RECEIVER_AS_SECOND_ARGUMENT.add("forEach");
+        FUNCTIONS_WITH_CALLBACK_RECEIVER_AS_SECOND_ARGUMENT.add("map");
+        FUNCTIONS_WITH_CALLBACK_RECEIVER_AS_SECOND_ARGUMENT.add("some");
+    }
+
     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<>();
+    private final Map<String, Set<SymbolicArgument>> symbolicArgumentsByName = new HashMap<>();
 
     @Override
     void enterNode(AstNode node) {
@@ -30,7 +43,7 @@ public final class FunctionReceiverChecker extends ContextTrackingChecker {
                 break;
             }
             if (function.isTopLevelFunction()) {
-                argumentNodesByName.clear();
+                symbolicArgumentsByName.clear();
             } else {
                 AstNode nameNode = AstUtil.getFunctionNameNode((FunctionNode) node);
                 if (nameNode == null) {
@@ -45,18 +58,18 @@ public final class FunctionReceiverChecker extends ContextTrackingChecker {
     }
 
     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)))) {
+        String[] callParts = getContext().getNodeText(functionCall.getTarget()).split("\\.");
+        String firstPart = callParts[0];
+        List<AstNode> argumentNodes = functionCall.getArguments();
+        List<String> actualArguments = argumentsForCall(argumentNodes);
+        int partCount = callParts.length;
+        String functionName = callParts[partCount - 1];
+
+        saveSymbolicArguments(functionName, argumentNodes, actualArguments);
+
+        boolean isBindCall = partCount > 1 && "bind".equals(functionName);
+        if (isBindCall && partCount == 3 && "this".equals(firstPart) &&
+            !(actualArguments.size() > 0 && "this".equals(actualArguments.get(0)))) {
                 reportErrorAtNodeStart(functionCall,
                         "Member function can only be bound to 'this' as the receiver");
                 return;
@@ -64,14 +77,55 @@ public final class FunctionReceiverChecker extends ContextTrackingChecker {
         if (partCount > 2 || "this".equals(firstPart)) {
             return;
         }
-        boolean hasReceiver = hasBind && isReceiverSpecified(arguments);
+        boolean hasReceiver = isBindCall && isReceiverSpecified(actualArguments);
         hasReceiver |= (partCount == 2) &&
-                ("call".equals(targetParts[1]) || "apply".equals(targetParts[1])) &&
-                isReceiverSpecified(arguments);
+                ("call".equals(functionName) || "apply".equals(functionName)) &&
+                isReceiverSpecified(actualArguments);
         getOrCreateSetByKey(callSitesByFunctionName, firstPart)
                 .add(new CallSite(hasReceiver, functionCall));
     }
 
+    private List<String> argumentsForCall(List<AstNode> argumentNodes) {
+        int argumentCount = argumentNodes.size();
+        List<String> arguments = new ArrayList<>(argumentCount);
+        for (AstNode argumentNode : argumentNodes) {
+            arguments.add(getContext().getNodeText(argumentNode));
+        }
+        return arguments;
+    }
+
+    private void saveSymbolicArguments(
+            String functionName, List<AstNode> argumentNodes, List<String> arguments) {
+        int argumentCount = arguments.size();
+        CheckedReceiverPresence receiverPresence = CheckedReceiverPresence.MISSING;
+        if (FUNCTIONS_WITH_CALLBACK_RECEIVER_AS_SECOND_ARGUMENT.contains(functionName)) {
+            if (argumentCount >= 2) {
+                receiverPresence = CheckedReceiverPresence.PRESENT;
+            }
+        } else if ("addEventListener".equals(functionName) ||
+                "removeEventListener".equals(functionName)) {
+            String receiverArgument = argumentCount < 3 ? "" : arguments.get(2);
+            switch (receiverArgument) {
+            case "":
+            case "true":
+            case "false":
+                receiverPresence = CheckedReceiverPresence.MISSING;
+                break;
+            case "this":
+                receiverPresence = CheckedReceiverPresence.PRESENT;
+                break;
+            default:
+                receiverPresence = CheckedReceiverPresence.IGNORE;
+            }
+        }
+
+        for (int i = 0; i < argumentCount; ++i) {
+            String argumentText = arguments.get(i);
+            getOrCreateSetByKey(symbolicArgumentsByName, argumentText).add(
+                    new SymbolicArgument(receiverPresence, argumentNodes.get(i)));
+        }
+    }
+
     private static <K, T> Set<T> getOrCreateSetByKey(Map<K, Set<T>> map, K key) {
         Set<T> set = map.get(key);
         if (set == null) {
@@ -97,31 +151,22 @@ public final class FunctionReceiverChecker extends ContextTrackingChecker {
         }
 
         for (FunctionRecord record : nestedFunctionsByName.values()) {
-            processNestedFunction(record);
+            processFunctionUsesAsArgument(record, symbolicArgumentsByName.get(record.name));
+            processFunctionCallSites(record, callSitesByFunctionName.get(record.name));
         }
+
         nestedFunctionsByName.clear();
         callSitesByFunctionName.clear();
-        argumentNodesByName.clear();
+        symbolicArgumentsByName.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>)");
-            }
-        }
-
+    private void processFunctionCallSites(FunctionRecord function, Set<CallSite> callSites) {
         if (callSites == null) {
             return;
         }
+        boolean hasThisAnnotation = hasAnnotationTag(function.functionNode, "this");
         for (CallSite callSite : callSites) {
-            if (hasThisAnnotation == callSite.hasReceiver || record.isConstructor) {
+            if (hasThisAnnotation == callSite.hasReceiver || function.isConstructor) {
                 continue;
             }
             if (callSite.hasReceiver) {
@@ -134,6 +179,51 @@ public final class FunctionReceiverChecker extends ContextTrackingChecker {
         }
     }
 
+    private void processFunctionUsesAsArgument(
+            FunctionRecord function, Set<SymbolicArgument> argumentUses) {
+        if (argumentUses == null ||
+                hasAnnotationTag(function.functionNode, "suppressReceiverCheck")) {
+            return;
+        }
+
+        boolean hasThisAnnotation = hasAnnotationTag(function.functionNode, "this");
+        for (SymbolicArgument argument : argumentUses) {
+            if (argument.receiverPresence == CheckedReceiverPresence.IGNORE) {
+                continue;
+            }
+            boolean receiverProvided =
+                    argument.receiverPresence == CheckedReceiverPresence.PRESENT;
+            if (hasThisAnnotation == receiverProvided) {
+                continue;
+            }
+            if (hasThisAnnotation) {
+                reportErrorAtNodeStart(argument.node,
+                        "Function annotated with @this used as argument without " +
+                         "a receiver. " + SUPPRESSION_HINT);
+            } else {
+                reportErrorAtNodeStart(argument.node,
+                        "Function not annotated with @this used as argument with " +
+                         "a receiver. " + SUPPRESSION_HINT);
+            }
+        }
+    }
+
+    private static enum CheckedReceiverPresence {
+        PRESENT,
+        MISSING,
+        IGNORE
+    }
+
+    private static class SymbolicArgument {
+        CheckedReceiverPresence receiverPresence;
+        AstNode node;
+
+        public SymbolicArgument(CheckedReceiverPresence receiverPresence, AstNode node) {
+            this.receiverPresence = receiverPresence;
+            this.node = node;
+        }
+    }
+
     private static class CallSite {
         boolean hasReceiver;
         FunctionCall callNode;

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

@@ -37,7 +37,7 @@ public final class RequiredThisAnnotationChecker extends ContextTrackingChecker
         if (!functionsRequiringThisAnnotation.contains(function)) {
             AstNode functionNameNode = AstUtil.getFunctionNameNode(functionNode);
             if (functionNameNode != null && !function.isTopLevelFunction() &&
-                    AstUtil.hasThisAnnotation(functionNode, getContext())) {
+                    hasAnnotationTag(functionNode, "this")) {
                 reportErrorAtNodeStart(
                         functionNameNode,
                         "@this annotation found for function not referencing 'this'");
@@ -45,7 +45,7 @@ public final class RequiredThisAnnotationChecker extends ContextTrackingChecker
             return;
         }
         AstNode functionNameNode = AstUtil.getFunctionNameNode(functionNode);
-        if (functionNameNode != null && !AstUtil.hasThisAnnotation(functionNode, getContext())) {
+        if (functionNameNode != null && !hasAnnotationTag(functionNode, "this")) {
             reportErrorAtNodeStart(
                     functionNameNode,
                     "@this annotation is required for functions referencing 'this'");

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

@@ -234,7 +234,7 @@ function badFuncAnnotatedButNoReturnValue() // ERROR - no returned value.
         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>)
+/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 a receiver. This check can be suppressed using @suppressReceiverCheck annotation on function declaration.
         this.memberTwo(callbackWithThis); // ERROR - Used as argument with no bound receiver.
                        ^
 
@@ -250,4 +250,24 @@ function badFuncAnnotatedButNoReturnValue() // ERROR - no returned value.
         this.memberTwo(callbackNoThis.bind(foo)); // ERROR - Bound to a receiver but has no @this annotation.
                        ^
 
-Total errors: 63
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:237: ERROR - Function annotated with @this used as argument without a receiver. This check can be suppressed using @suppressReceiverCheck annotation on function declaration.
+        array.forEach(callbackWithThis); // ERROR - No receiver.
+                      ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:238: ERROR - Function not annotated with @this used as argument with a receiver. This check can be suppressed using @suppressReceiverCheck annotation on function declaration.
+        array.forEach(callbackNoThis, this); // ERROR - Receiver for callback with no @this annotation.
+                      ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:247: ERROR - Function not annotated with @this used as argument with a receiver. This check can be suppressed using @suppressReceiverCheck annotation on function declaration.
+        element.addEventListener("click", callbackNoThis, this); // ERROR.
+                                          ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:253: ERROR - Function annotated with @this used as argument without a receiver. This check can be suppressed using @suppressReceiverCheck annotation on function declaration.
+        element.addEventListener("click", callbackWithThis, true); // ERROR.
+                                          ^
+
+/usr/local/google/home/apavlov/dev/blink/src/third_party/WebKit/Source/devtools/scripts/jsdoc-validator/tests/this.js:254: ERROR - Function annotated with @this used as argument without a receiver. This check can be suppressed using @suppressReceiverCheck annotation on function declaration.
+        element.addEventListener("click", callbackWithThis, false); // ERROR.
+                                          ^
+
+Total errors: 68

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

@@ -228,6 +228,42 @@ ReceiverTest.prototype = {
         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.
+
+        // Callback receivers specified as arguments.
+
+        array.forEach(callbackWithThis, this);
+        array.forEach(callbackNoThis);
+
+        array.forEach(callbackWithThis); // ERROR - No receiver.
+        array.forEach(callbackNoThis, this); // ERROR - Receiver for callback with no @this annotation.
+
+        var isMultiline = false;
+
+        element.addEventListener("click", callbackNoThis);
+        element.addEventListener("click", callbackNoThis, true);
+        element.addEventListener("click", callbackNoThis, false);
+        element.addEventListener("click", callbackNoThis, isMultiline); // OK - ignored.
+
+        element.addEventListener("click", callbackNoThis, this); // ERROR.
+
+        element.addEventListener("click", callbackWithThis, this);
+        element.addEventListener("click", callbackWithThis, foo); // OK - ignored.
+        element.addEventListener("click", callbackWithThis, isMultiline); // OK - ignored.
+
+        element.addEventListener("click", callbackWithThis, true); // ERROR.
+        element.addEventListener("click", callbackWithThis, false); // ERROR.
+
+        // DevTools-specific.
+
+        /**
+         * @suppressReceiverCheck
+         * @this {Object}
+         */
+        function ignoredCallbackWithThis()
+        {
+            this.foo = 1;
+        }
+        object.callFunction(func, [], ignoredCallbackWithThis); // OK - ignored.
     },
 
     memberTwo: function(arg) {}