SonarJS 3.0: Being Lean and Mean in JavaScript

by carlo bottiglieri|

    All through 2016 SonarJS has become richer and more powerful thanks to new rules and its new data flow engine, to the point of being able to find pretty interesting stuff like this:

    That’s cool, isn’t it? Yet, there’s such a thing as being blinded by coolness and, as Pirelli was fond of saying, power is nothing without control. What good is pointing out a very nasty and hidden bug if you have long since stopped listening to what SonarJS has to tell you?

    There are two main reasons a developer stops listening to the analyzer:

    1. The analyzer is noisy, stacking issue on top of issue because you insist on having more than one statement per line.
    2. The analyzer says something that is really dumb, so, the developer presumes, the analyzer is dumb. Life is too short to listen to dumb tools.

    Unless we tackled both these points we risked having our oh-so-powerful analyzer be perceived like a “the end is nigh” lunatic.

    Kill the noise

    We don’t want to spam the developer with potentially true but ultimately irrelevant messages. But what is relevant?

    We do want to provide value out of the box, so all SonarSource analysers provide a default rule-set, called the “Sonar way” profile, that does represent what it means for us to write good <insert language here> code. This means that we don’t have the luxury of saying “the users will setup the profile with the rules they prefer”, we have to take a stance on which rules are activated by default.

    Guess what? Nobody knew that defining what is good JavaScript could be so complicated!

    We thus embarked on a deep review of our default “Sonar way” profile to see if we could indeed find a meaningful, useful, common ground. We knew we needed an external point of view and we were very lucky to find a very knowledgeable and critical one: Alexander Kamushkin.

    Alexander worked with us for a month and he did an amazing job, if somewhat painful to us, pointing out which rules provided the most value regardless of team culture and idioms, which could become idiom-neutral with some work, and which were by definition optional conventions.

    After the first few rounds of discussion he put everything in what we have come to refer to as “Alexander’s Report”, of which this is a very small excerpt:

    Of course this was not the end of it, we kept on refining these findings, prioritizing and adding some more all through the development of SonarJS 3.0 and we have more in the pipe for later on.

    We improved dozens of rules, splitted rules to separate the unarguable bug-generating cases from maintenance-related cases, added heuristics to kill false-positives, almost no rule previously part of “Sonar way” was left untouched. We also further evolved the data flow engine itself to make sure it was not making assumptions that might lead rules to be overconfident in reporting an issue.

    We now feel that the default profile of SonarJS 3.0 is a carefully trimmed set of high-value/low-noise rules useful in almost any JS development context.

    We also created a new profile: “Sonar way Recommended”. This profile includes all the rules found in “Sonar way”, plus rules which we have evolved to be high-value/low-noise for JS developments that mandate high code readability and long-term evolution.

    Things we learned

    The issue is in the eye of the beholder

    Take for instance one excellent rule: “Dead stores should be removed”. This rule says that if you set a value to a variable and then you set a new value without ever using the previous one you have probably made a mistake.

    let x = 3;
    x = 10;
    alert(“Value : “ + x);


    We can hardly be more confident that something is wrong here, you probably wanted to do something with that “3”. What if you are in the habit of initialising all your number variables to 0, or all your strings to empty string?

    function bye(p) {
    
      let answer = "";
    
      switch(p) {
        case "HELLO" : answer = "GOODBYE";
          break;
        case "HI" : answer = "BYE";
          break;
        default : answer = "HASTA LA VISTA BABY";
      }
    
      return answer;
    }


    Do we want to raise a dead-store issue on that first initialisation? If we did we could kind of excuse ourselves by saying that it is indeed a dead-store, but since the developer did that on purpose the analyser is at best perceived as pedantic.

    After all when raising issues we are not addressing machines but human beings. We want them to read and care about these issues, we cannot hide behind technical correctness, we must be correct and also try to guess when something is done on purpose and when it is a genuine mistake.

    It’s not a bug until it is

    Before SonarJS 3.0 every issue we detected which could potentially lead to a bug was classified as a bug.

    This was done out of a coherent approach at issue criticality and it does draw a clear distinction between potential mistakes and more readability or maintenance-related code smells.

    Still, there’s something very alarming in getting a report saying that your project contains 1.542 bugs.

    A classic example of this is NaN. If you don’t expect NaN to be a possible value you can introduce some very nasty bugs, because, let’s not forget that NaN == NaN is false!

    Still, nothing might happen, because you are careful in other ways and as such playing with NaNs is at worst suspicious, not a bug.

    Also, we found out that, as the analyser improved, many potentially dangerous things could be resolved into being either certainly bugs or certainly not bugs. There’s no need to scream about an undefined variable if we can track its value and only raise an issue when you try to access a property on that undefined variable.

    If you can’t analyse it, don’t make assumptions

    The data flow analysis engine is pretty good, but it still cannot analyse everything. We learnt that if we cannot follow the whole life of a variable’s value we are better off assuming no knowledge than partial knowledge.

    let x;
    if(p) x = 2;
    if(isPositive(x)) {
      return 10/x;
    }


    Should we warn you of a possible NaN? It depends if we were able to resolve the declaration of isPositive and go through its implementation. If we don’t know what happens within isPositive, even if we know that it is possible that x is undefined we can’t be sure that x can be undefined when 10/x is executed. It’s safer, to avoid raising an issue because of our partial understanding, to not presume we know anything about x.

    And more

    There would be much more to say, but for the time being suffice to know that SonarJS 3.0 inaugurates a focus on minimalistic usefulness, or, as Marko Ramius would say: we have engaged the silent drive.