False positive for Empty collections should not be accessed or iterated

[vojtech.bisek]

Using SonarQube 8.1 (build 31237) Scanner 3.3.0.1492 SonarJS 6.2.0.12043 (javascript)

Code below is flagged with rule javascript:S4158 on array. Whether the array is empty during the call of foo.f() however in my analysis depends on whether foo.g() was called or not. Hence I think this is a false positive.

(function () {
  angular.module('yeomanApp')
  .controller('Controller', Controller);

  function Controller() {
    var array = [];

    var Foo = function Foo() {
    };
    Foo.prototype.f = f;
    Foo.prototype.g = g;
    this.foo = new Foo();

    function f() {
      return array.indexOf('x'); // triggers javascript:S4158
    }

    function g() {
      array = h();
    }

    function h() {
      return ['a', 'b'];
    }
  }
})();

Note that while this reproduction scenario based on the original implementation is the whole file, the Sonar analysis still runs in the context of the whole application. I don’t quite know how I would be able to isolate it if the reproduction was dependent on “something” in the rest of the application anyways, as the rule does not provide any reasoning on why the array couldn’t have been populated.

[Alexandre_Gigleux]

Hello,

There is no information in your reproducer to determine that f() will never be called without a prior call to h() populating the array.

Why would you say we should not raise an issue on array.indexOf('x')?

Thanks
Alex

[vojtech.bisek]

The rule says Review this usage of “array” as it can only be empty here. which is definitely a false statement.
It may be empty or it may not, and that’s the whole point of f(), as the array contains information that is depndent on the outcome of g() calls. I can’t know whether ‘x’ is in the array until I look inside. (In this minimal reproduction example I can, but obviously the result of h() is not static in the real application, neither is the 'x' I am insterested in)

The rule continues stating When a collection is empty it makes no sense to access or iterate it. Doing so anyway is surely an error; either population was accidentally omitted or the developer doesn’t understand the situation. Neither of which is the case, it’s just that the rule fails to detect that something can indeed modify the array. The rule stops complaining if I inline h() (not an option in real application, as that’s third party code). It also does not complain if the array and foo are global or just not wrapped in a controller (not an option given this is an angular application).
So really there’s something in the minimal reproduction case above that makes it too complex for the rule to reason about the possible state of the array, but that should not be a reason to flag it as can be only empty.

[vojtech.bisek]

To make it more obvious that the rule is not about “the array may or may not be empty”, following code does not trigger it:

    function f() {
      array = Math.random() > 0.5 ? ['x'] : [];
      return array.indexOf('x');
    }

[Lena (Elena Vilchik)]

Hi,

Thanks for reporting, I’ve created a ticket to handle this case https://github.com/SonarSource/SonarJS/issues/1974