DevTools: [JsDocValidator] Avoid false-positive function receiver-related errors

The RequiredThisAnnotationChecker has been merged into FunctionReceiverChecker,
so it can now make use of the knowledge whether a nested function
references |this| or not. As such, the receiver checks now rely
on the fact of the |this| usage rather than the presence of the @this annotation.

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

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

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

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

-b5a448d248ee8e610b5559db732412030879a045401a93d6aa9ea638a9ae89aa src
-81609682686690d48af5d13b3ab005f4a1adb4d90ee53ac2264a5ba9509f186f jsdoc-validator.jar
+897c41ac961d3b29241cbc669a7b8de9bab29576011ba14d5221074236ef6eaf src
+c2d110cf773c37260c650b98086345d5a5796baec0552758d84d403c86306edb jsdoc-validator.jar

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

@@ -7,8 +7,6 @@ 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";

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

@@ -30,7 +30,6 @@ public class ContextTrackingValidationCheck extends ValidationCheck {
         super.setContext(context);
         state = new ContextTrackingState(context);
         registerClient(new ProtoFollowsExtendsChecker());
-        registerClient(new RequiredThisAnnotationChecker());
         registerClient(new ReturnAnnotationChecker());
         registerClient(new FunctionReceiverChecker());
     }

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

@@ -30,6 +30,7 @@ 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<SymbolicArgument>> symbolicArgumentsByName = new HashMap<>();
+    private final Set<FunctionRecord> functionsRequiringThisAnnotation = new HashSet<>();
 
     @Override
     void enterNode(AstNode node) {
@@ -37,21 +38,14 @@ public final class FunctionReceiverChecker extends ContextTrackingChecker {
         case Token.CALL:
             handleCall((FunctionCall) node);
             break;
-        case Token.FUNCTION:
-            FunctionRecord function = getState().getCurrentFunctionRecord();
-            if (function == null) {
-                break;
-            }
-            if (function.isTopLevelFunction()) {
-                symbolicArgumentsByName.clear();
-            } else {
-                AstNode nameNode = AstUtil.getFunctionNameNode((FunctionNode) node);
-                if (nameNode == null) {
-                    break;
-                }
-                nestedFunctionsByName.put(getContext().getNodeText(nameNode), function);
-            }
+        case Token.FUNCTION: {
+            handleFunction((FunctionNode) node);
+            break;
+        }
+        case Token.THIS: {
+            handleThis();
             break;
+        }
         default:
             break;
         }
@@ -85,6 +79,33 @@ public final class FunctionReceiverChecker extends ContextTrackingChecker {
                 .add(new CallSite(hasReceiver, functionCall));
     }
 
+
+    private void handleFunction(FunctionNode node) {
+        FunctionRecord function = getState().getCurrentFunctionRecord();
+        if (function == null) {
+            return;
+        }
+        if (function.isTopLevelFunction()) {
+            symbolicArgumentsByName.clear();
+        } else {
+            AstNode nameNode = AstUtil.getFunctionNameNode(node);
+            if (nameNode == null) {
+                return;
+            }
+            nestedFunctionsByName.put(getContext().getNodeText(nameNode), function);
+        }
+    }
+
+    private void handleThis() {
+        FunctionRecord function = getState().getCurrentFunctionRecord();
+        if (function == null) {
+            return;
+        }
+        if (!function.isTopLevelFunction() && !function.isConstructor) {
+            functionsRequiringThisAnnotation.add(function);
+        }
+    }
+
     private List<String> argumentsForCall(List<AstNode> argumentNodes) {
         int argumentCount = argumentNodes.size();
         List<String> arguments = new ArrayList<>(argumentCount);
@@ -145,8 +166,15 @@ public final class FunctionReceiverChecker extends ContextTrackingChecker {
             return;
         }
 
-        FunctionRecord function = getState().getCurrentFunctionRecord();
-        if (function == null || !function.isTopLevelFunction()) {
+        ContextTrackingState state = getState();
+        FunctionRecord function = state.getCurrentFunctionRecord();
+        if (function == null) {
+            return;
+        }
+        checkThisAnnotation(function);
+
+        // The nested function checks are only run when leaving a top-level function.
+        if (!function.isTopLevelFunction()) {
             return;
         }
 
@@ -160,21 +188,48 @@ public final class FunctionReceiverChecker extends ContextTrackingChecker {
         symbolicArgumentsByName.clear();
     }
 
+    private void checkThisAnnotation(FunctionRecord function) {
+        AstNode functionNameNode = AstUtil.getFunctionNameNode(function.functionNode);
+        if (functionNameNode == null) {
+            return;
+        }
+        boolean hasThisAnnotation = hasAnnotationTag(function.functionNode, "this");
+        if (hasThisAnnotation == functionReferencesThis(function)) {
+            return;
+        }
+        if (hasThisAnnotation) {
+            if (!function.isTopLevelFunction()) {
+                reportErrorAtNodeStart(
+                        functionNameNode,
+                        "@this annotation found for function not referencing 'this'");
+            }
+            return;
+        } else {
+            reportErrorAtNodeStart(
+                    functionNameNode,
+                    "@this annotation is required for functions referencing 'this'");
+        }
+    }
+
+    private boolean functionReferencesThis(FunctionRecord function) {
+        return functionsRequiringThisAnnotation.contains(function);
+    }
+
     private void processFunctionCallSites(FunctionRecord function, Set<CallSite> callSites) {
         if (callSites == null) {
             return;
         }
-        boolean hasThisAnnotation = hasAnnotationTag(function.functionNode, "this");
+        boolean functionReferencesThis = functionReferencesThis(function);
         for (CallSite callSite : callSites) {
-            if (hasThisAnnotation == callSite.hasReceiver || function.isConstructor) {
+            if (functionReferencesThis == callSite.hasReceiver || function.isConstructor) {
                 continue;
             }
             if (callSite.hasReceiver) {
                 reportErrorAtNodeStart(callSite.callNode,
-                        "Receiver specified for a function not annotated with @this");
+                        "Receiver specified for a function not referencing 'this'");
             } else {
                 reportErrorAtNodeStart(callSite.callNode,
-                        "Receiver not specified for a function annotated with @this");
+                        "Receiver not specified for a function referencing 'this'");
             }
         }
     }
@@ -186,23 +241,23 @@ public final class FunctionReceiverChecker extends ContextTrackingChecker {
             return;
         }
 
-        boolean hasThisAnnotation = hasAnnotationTag(function.functionNode, "this");
+        boolean referencesThis = functionReferencesThis(function);
         for (SymbolicArgument argument : argumentUses) {
             if (argument.receiverPresence == CheckedReceiverPresence.IGNORE) {
                 continue;
             }
             boolean receiverProvided =
                     argument.receiverPresence == CheckedReceiverPresence.PRESENT;
-            if (hasThisAnnotation == receiverProvided) {
+            if (referencesThis == receiverProvided) {
                 continue;
             }
-            if (hasThisAnnotation) {
+            if (referencesThis) {
                 reportErrorAtNodeStart(argument.node,
-                        "Function annotated with @this used as argument without " +
+                        "Function referencing 'this' used as argument without " +
                          "a receiver. " + SUPPRESSION_HINT);
             } else {
                 reportErrorAtNodeStart(argument.node,
-                        "Function not annotated with @this used as argument with " +
+                        "Function not referencing 'this' used as argument with " +
                          "a receiver. " + SUPPRESSION_HINT);
             }
         }

third_party/WebKit/Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/RequiredThisAnnotationChecker.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.FunctionNode;
-
-import java.util.HashSet;
-import java.util.Set;
-
-public final class RequiredThisAnnotationChecker extends ContextTrackingChecker {
-
-    private final Set<FunctionRecord> functionsRequiringThisAnnotation = new HashSet<>();
-
-    @Override
-    void enterNode(AstNode node) {
-        if (node.getType() == Token.THIS) {
-            FunctionRecord function = getState().getCurrentFunctionRecord();
-            if (function == null) {
-                return;
-            }
-            if (!function.isTopLevelFunction() && !function.isConstructor) {
-                functionsRequiringThisAnnotation.add(function);
-            }
-            return;
-        }
-    }
-
-    @Override
-    void leaveNode(AstNode node) {
-        if (node.getType() != Token.FUNCTION) {
-            return;
-        }
-
-        ContextTrackingState state = getState();
-        FunctionRecord function = state.getCurrentFunctionRecord();
-        FunctionNode functionNode = (FunctionNode) node;
-        if (!functionsRequiringThisAnnotation.contains(function)) {
-            AstNode functionNameNode = AstUtil.getFunctionNameNode(functionNode);
-            if (functionNameNode != null && !function.isTopLevelFunction() &&
-                    hasAnnotationTag(functionNode, "this")) {
-                reportErrorAtNodeStart(
-                        functionNameNode,
-                        "@this annotation found for function not referencing 'this'");
-            }
-            return;
-        }
-        AstNode functionNameNode = AstUtil.getFunctionNameNode(functionNode);
-        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/this.js

@@ -264,6 +264,20 @@ ReceiverTest.prototype = {
             this.foo = 1;
         }
         object.callFunction(func, [], ignoredCallbackWithThis); // OK - ignored.
+
+        function callbackReferencingThisNotAnnotated()
+        {
+            this.foo = 2;
+        }
+        this.memberTwo(callbackReferencingThisNotAnnotated.bind(this)); // OK - No @this annotation, but references |this|.
+
+        /**
+         * @this {Object}
+         */
+        function callbackNotReferencingThisAnnotated()
+        {
+        }
+        this.memberTwo(callbackNotReferencingThisAnnotated); // OK - Has @this annotation, but does not reference |this|.
     },
 
     memberTwo: function(arg) {}