False positives when matching identifiers is not set

[agienger]

Hi,
jsinspect produces false positives for a check with the linked 2 files, when the -i parameter is NOT set. I used the node level 30, 40 Looks like for function arguments that are callback functions the function code is not checked.
best regards Armin

https://github.com/agienger/issue_files/blob/master/jsinspect/callBack1.js
https://github.com/agienger/issue_files/blob/master/jsinspect/callBack2.js

[danielstjules]

Thanks for the thorough case, and I appreciate the example project! :) What is myFunc? Did you rename it for the purpose of this example? Is it an AMD define? By changing the myFunc to define in both files, you correctly get:

$ jsinspect -t 30 .

  Match - 3 instances
  ./jsinspect/callBack2.js:71,82
  ./jsinspect/callBack2.js:92,103
  ./jsinspect/callBack2.js:105,116

- ./jsinspect/callBack2.js:71,82
+ ./jsinspect/callBack2.js:92,103
-  _checkWorklistTypeEmpty: function(sWorklistType) {
-   var bWorklistTypeEmpty = false,
-       oInput = this.getView().byId("sWorklistType");
-   if (sWorklistType === "") {
+  _checkWorklistVariantNameEmpty: function(sName) {
+   var bNameEmpty = false,
+       oInput = this.getView().byId("sInputId");
+   if (sName === "") {
        this._setValueStateForInputField(oInput, ui.core.ValueState.Error, this.getResourceBundle().getText(
            "TOOLTIP_VALUE_STATE_ERROR_EMPTY"));
-       bWorklistTypeEmpty = true;
-   } else {
+       bNameEmpty = true;
+   }else {
        this._setValueStateForInputField(oInput, ui.core.ValueState.None, "");
    }
-   return bWorklistTypeEmpty;
+   return bNameEmpty;
   },

- ./jsinspect/callBack2.js:71,82
+ ./jsinspect/callBack2.js:105,116
-  _checkWorklistTypeEmpty: function(sWorklistType) {
-   var bWorklistTypeEmpty = false,
-       oInput = this.getView().byId("sWorklistType");
-   if (sWorklistType === "") {
+  _checkWorklistVariantDescriptionEmpty: function(sDescription) {
+   var bDescriptionEmpty = false,
+       oInput = this.getView().byId("sInputDescription");
+   if (sDescription === "") {
        this._setValueStateForInputField(oInput, ui.core.ValueState.Error, this.getResourceBundle().getText(
            "TOOLTIP_VALUE_STATE_ERROR_EMPTY"));
-       bWorklistTypeEmpty = true;
+       bDescriptionEmpty = true;
    } else {
        this._setValueStateForInputField(oInput, ui.core.ValueState.None, "");
    }
-   return bWorklistTypeEmpty;
+   return bDescriptionEmpty;
   },

 1 match found across 2 files

[danielstjules]

Basically, jsinspect has some edge case logic for ignoring AMD/CommonJS require/define statements for things to work. However, the check I wrote is pretty naive, and expects an exact string literal.

[agienger]

I see, thanks a lot for the prompt answer. Yes myFunc is actually sap.ui.define (https://openui5.hana.ondemand.com/docs/api/symbols/sap.ui.html#.define) . We at SAP frontend development are using this heavily, but I still would like to use your tool for code checks. Is there a chance to include sap.ui.define in your ignore list?
best regards Armin

[danielstjules]

Ah, I see! A quick fix would be to check to see if it's called define or contains .define :) That'd work for a patch release, and I'll see if I can come up with a more thoughtful approach later on. Thanks!

[agienger]

This sounds good, thanks a lot!

[agienger]

Hi,
I'd like to know whether you are considering a patch for this?

[danielstjules]

Sorry, I do plan on fixing this - have just been caught up with some other stuff lately. The quick fix I had in mind didn't quite work, so I'll need a better solution.

[danielstjules]

Tagged and released 0.7.1 :)

[agienger]

thanks a lot for the fix. Now jsinspect workds well for these cases.
However I have discovered another case of false positives (using "extend", this ia a legacy code structure, however often used)
https://github.com/agienger/issue_files/blob/master/jsinspect/S2.controller.js
https://github.com/agienger/issue_files/blob/master/jsinspect/S3.controller.js

[danielstjules]

Thanks! I'll leave that to #27

Might be nice to have some config for this as well. Maybe skipping blocks that start with a given line? Since there's a pattern here, I'd like that better than having QA tools pollute lib code