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

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

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

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169450

[sergeyv, 2014-03-18 12:31:10]

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
File
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java
(right):

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:25:
case Token.CALL:
Maybe,  we should refactor all these cases, if-s and casts someday . 

something like: add interface EnterNodeListener<T> and method
registerEnterListener<T>(Class<T>, EnterNodeListener<T>  listener); 
e.g: registerEnterListener(FunctionClass.class, this);

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:48:

extra line

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:65:
if (partCount > 1 && "bind".equals(targetParts[partCount - 1])) {
boolean hasBind = partCount > 1 && "bind".equals(targetParts[partCount - 1]);
if (hasBind && partCount == 3  ...)

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:74:
if ("this".equals(firstPart)) {
"this".equals(firstPart) || partCount > 2
and remove else later

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:77:
if (partCount == 2) {
hasReceiver |= (partCount == 2) && ("call".equals(targetParts[1]) ||
"apply".equals(targetParts[1])) && isReceiverSpecified(arguments);
hasReceiver |= hasBind && isReceiverSpecified(arguments);

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:85:
Set<CallSite> callSites = callSitesByFunctionName.get(firstPart);
this and next 4 line: lets move this to separate function and get rid of code
duplication in this function

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:123:
getState().getContext().reportErrorInNode(argumentNode, 0,
we have a lot of "getState().getContext().reportErrorInNode" in our code, lets
create shortcut for this ContextTrackingChecker. Btw, most of the time second
param is zero

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:137:
if (!record.isConstructor) {
can we move this check:
if (hasThisAnnotation == callSite.hasReceiver || record.isConstructor) {
         continue;
}
?

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:148:
private boolean hasThisAnnotation(FunctionNode node) {
code duplication, we have same function in RequiredThisAnnotationChecker. Btw it
should be static.

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
File
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/RequiredThisAnnotationChecker.java
(right):

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/RequiredThisAnnotationChecker.java:41:
if (!function.isTopLevelFunction()) {
no need in separate if here,  functionNameNode != null &&
hasThisAnnotation(functionNode)  && !function.isTopLevelFunction()

[apavlov, 2014-03-18 13:09:08]

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
File
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java
(right):

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:25:
case Token.CALL:
On 2014/03/18 12:31:10, sergeyv wrote:
> Maybe,  we should refactor all these cases, if-s and casts someday . 
> 
> something like: add interface EnterNodeListener<T> and method
> registerEnterListener<T>(Class<T>, EnterNodeListener<T>  listener); 
> e.g: registerEnterListener(FunctionClass.class, this);  

This kind of solution is going to have a performance impact, since type checks
are slower than integer comparisons

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:48:

On 2014/03/18 12:31:10, sergeyv wrote:
> extra line

Done.

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:65:
if (partCount > 1 && "bind".equals(targetParts[partCount - 1])) {
On 2014/03/18 12:31:10, sergeyv wrote:
> boolean hasBind = partCount > 1 && "bind".equals(targetParts[partCount - 1]);
> if (hasBind && partCount == 3  ...)

Done.

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:74:
if ("this".equals(firstPart)) {
On 2014/03/18 12:31:10, sergeyv wrote:
> "this".equals(firstPart) || partCount > 2
> and remove else later

Done.

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:77:
if (partCount == 2) {
On 2014/03/18 12:31:10, sergeyv wrote:
> hasReceiver |= (partCount == 2) && ("call".equals(targetParts[1]) ||
> "apply".equals(targetParts[1])) && isReceiverSpecified(arguments);
> hasReceiver |= hasBind && isReceiverSpecified(arguments);

Done.

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:85:
Set<CallSite> callSites = callSitesByFunctionName.get(firstPart);
On 2014/03/18 12:31:10, sergeyv wrote:
> this and next 4 line: lets move this to separate function and get rid of code
> duplication in this function

Done.

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:123:
getState().getContext().reportErrorInNode(argumentNode, 0,
On 2014/03/18 12:31:10, sergeyv wrote:
> we have a lot of "getState().getContext().reportErrorInNode" in our code, lets
> create shortcut for this ContextTrackingChecker. Btw, most of the time second
> param is zero

Done.

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:137:
if (!record.isConstructor) {
On 2014/03/18 12:31:10, sergeyv wrote:
> can we move this check:
> if (hasThisAnnotation == callSite.hasReceiver || record.isConstructor) {
>          continue;
> }
> ?

Done.

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/FunctionReceiverChecker.java:148:
private boolean hasThisAnnotation(FunctionNode node) {
On 2014/03/18 12:31:10, sergeyv wrote:
> code duplication, we have same function in RequiredThisAnnotationChecker. Btw
it
> should be static.

Done.

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
File
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/RequiredThisAnnotationChecker.java
(right):

https://codereview.chromium.org/202813004/diff/1/Source/devtools/scripts/jsdo...
Source/devtools/scripts/jsdoc-validator/src/org/chromium/devtools/jsdoc/checks/RequiredThisAnnotationChecker.java:41:
if (!function.isTopLevelFunction()) {
On 2014/03/18 12:31:10, sergeyv wrote:
> no need in separate if here,  functionNameNode != null &&
> hasThisAnnotation(functionNode)  && !function.isTopLevelFunction()

Done.

[apavlov, 2014-03-18 13:32:30]

Patch updated

[sergeyv, 2014-03-18 13:37:44]

lgtm

[vsevik, 2014-03-18 13:44:22]

lgtm

[apavlov, 2014-03-18 14:12:42]

Committed patchset #3 manually as r169450 (presubmit successful).