Using a Linter Check in Instance Scanner

[garyopela]

First, let's define linter, according to codeguru.com: "A Linter is an automated tool that runs on static code to find formatting discrepancies, non-adherence to coding standards and conventions, and find logical errors in your program."

Linters don't inherently do anything inside of ServiceNow. Think of them as tools in your toolbox, though. It reminds me of when I first learned about computers. I thought, 'Man, a computer can do everything!' But then my teacher informed me that 'A computer can do nothing until we tell it what to do.' Well, that's kind of like a linter inside of ServiceNow. You tell the linter what to check for, and it checks for things for you.

There is a little documentation on linters out there: 

From the docs: https://docs.servicenow.com/bundle/rome-servicenow-platform/page/administer/health-scan/task/hc-lint...

From the dev site blogs, a great article written by @Mark Roethof : https://www.servicenow.com/community/developer-articles/getting-instance-scan-linter-check-working/t...

I wanted to add an example, and show the power of Linters. Before I found we had linter check, I spent way too much time on a MASSIVE block of regex to do the same thing. I started with the goal of trying to identify cases where variables were declared at too low of a scope. For instance, take the two following code blocks.

/*------------- Code Block 1 -------------*/
var gr = new GlideRecord('cmdb_ci');
gr.query();
while (gr.next()) {
	var someVar = gr.getValue('sys_id');
	//do something with someVar
}

/*------------- Code Block 2 -------------*/
var someVar = '';
var gr = new GlideRecord('cmdb_ci');
gr.query();
while (gr.next()) {
	someVar = gr.getValue('sys_id');
	//do something with someVar
}

Code block 1 declares a variable inside of the while loop, whereas code block 2 declares it above the loop.

**** PLEASE READ COMMENT BELOW FROM JOHN DURDEN ON DIFFERENCE BETWEEN USING VAR AND LET.****

In code block 1, if we had 1 million CIs we were looping through, we would be created 1 million new objects in memory, then disposing them 1 million times. This creates a lot of stress on the Garbage Collector, which is a very CPU intensive process.

In code block 2, we correct this by declaring the variable once and then reusing it 1 million times, triggering only one garbage collection activity versus 1 million.

So, to create an automated scan that checks for this, we can first separate a given script into code blocks, then inside of each code block, look to see if it is an iterating code block, such as a for or while loop. If it is an iterating loop, then we need to see if we are defining and using a variable in the same block. If we are, then that variable should have been defined at a higher scope so that it is reused in the block rather than redeclared.

First, let's start with my original code, not using a linter check.... Bear in mind, this code is just for reference. It's incredibly heavy-handed code, and the Linter basically does the same thing but is much lighter work on the system. It has a hard time with some code, but it still generates a lot of findings.

/**************** SCRIPT TO FIND BAD GARBAGE COLLECTION (GC) PRACTICES ***************/
/********************************MORE INFORMATION HERE:*******************************/
/*https://stackoverflow.com/questions/18364175/best-practices-for-reducing-garbage-collector-activity-in-javascript*/
/************************ HERE ARE SOME ACTIVITIES IT MENTIONS ***********************/
/*
	* When you create an object via new or via literal syntax [...], or {}.
	* When you concatenate strings.
	* When you enter a scope that contains function declarations.
	* When you perform an action that triggers an exception.
	* When you evaluate a function expression: (function (...) { ... }).
	* When you perform an operation that coerces to Object like Object(myNumber) or Number.prototype.toString.call(42)
	* When you call a builtin that does any of these under the hood, like Array.prototype.slice.
	* When you use arguments to reflect over the parameter list.
	* When you split a string or match with a regular expression.
	* Avoid doing those, and pool and reuse objects where possible.

Specifically, look out for opportunities to:

Pull inner functions that have no or few dependencies on closed-over state out into a higher, longer-lived scope. (Some code minifiers like Closure compiler can inline inner functions and might improve your GC performance.)
Avoid using strings to represent structured data or for dynamic addressing. Especially avoid repeatedly parsing using split or regular expression matches since each requires multiple object allocations. This frequently happens with keys into lookup tables and dynamic DOM node IDs. For example, lookupTable['foo-' + x] and document.getElementById('foo-' + x) both involve an allocation since there is a string concatenation. Often you can attach keys to long-lived objects instead of re-concatenating. Depending on the browsers you need to support, you might be able to use Map to use objects as keys directly.
Avoid catching exceptions on normal code-paths. Instead of try { op(x) } catch (e) { ... }, [use] if (!opCouldFailOn(x)) { op(x); } else { ... }.
When you can't avoid creating strings, e.g. to pass a message to a server, use a builtin like JSON.stringify which uses an internal native buffer to accumulate content instead of allocating multiple objects.
Avoid using callbacks for high-frequency events, and where you can, pass as a callback a long-lived function (see 1) that recreates state from the message content.
Avoid using arguments since functions that use that have to create an array-like object when called.
/************************ HERE ARE SOME ACTIVITIES IT MENTIONS ***********************/
/************************ END OF INFORMATION FROM STACKOVERFLOW **********************/

// this code primarily focus on the case where a variable is both defined, assigned, and used within a loop that can repeat. */
// Primiarly at first I am focusing on while and for loops, although more might be added soon. */
// This uses an array of tables (scrtipArray assigned below) to hold the tables and the respect tables' script field. Add more entries here to increase the scope of what this checks for. 

/* the script will loop through the scriptArray array, then for each entry, it will pass the table and the script field to the checkCode function. The checkCode function then will execute a query to the table. It will loop through each row returned, extract the script field, and then send that to the recursiveArrayBuilder function. This function is incredibly important, it does all of the work. it is a recursive function, meaning it calls itself over and over until a condition is satisfied that causes it to exit. The final output of this recursion is an array of code blocks that are while or for loops, then more regex looks to see if variables are both assigned and used in that same code block. */

//Author: Gary Opela (Junior) - servicenow - gary.opela@servicenow.com

var argObj = {
    logLvl: 0, //0 is no logging, 1 is logging error only, 2 is logging error and info, 3 is super verbose logging, use with caution!
    result: "",
    str: "",
    prefix: "GC_Checker2: ",
    i: 0,
    scriptBody: "",
    regex: /((switch|catch|if|while|for|else\s*if)\s*\([^{}]*\)\s*\{([^{}]*)})|((do|try|finally|else|else if|return)\s*\{([^{}]*)})|(\{};)|((function)\s*([^{}]*)\([^{}]*\)\s*\{([^{}]*)})|(\{\d})|(:\s*\{([^{}]*)})|(=\s*\{([^{}]*)})|(\(\{([^{}]*)}\))|(\{},)|(\/\{([^{}]*)})|(\$\{([^{}]*)})|(\[([^{}]*)(\{([^{}]*)},([^{}]*))*((\{([^{}]*)})*)([^{}]*)])|('\{}';)|(\{\*})|(\$\{)|(\{\{)|(}})/g,
    rplc: /(\/\*([\s\S]*?)\*\/)|(^\/\/.*)|(\n\/\/.*)|(\s+\/\/.*)/g,
    varRegx: /var\s[\S]+/g,
    fixFors: /for\s*\(\s*var/g,
    masterArray: [],
    tmpArray: [],
    scriptArray: [ //this defines which tables to check. it's 2d array. Each element is a pair
        ["sys_script", "script"], //of table and script field name. i added script field name
        ["sys_script_client", "script"], //as an [x,1] element so that we can handle things like
        ["sysauto_script", "script"], //widgets or other things that don't use a 'script' field
        ["sys_transform_script", "script"],
        ["sys_script_include", "script"]
    ]
};

for (argObj.i = 0; argObj.i < argObj.scriptArray.length; argObj.i++) {
    argObj.targetTable = argObj.scriptArray[argObj.i][0];
    argObj.targetScriptField = argObj.scriptArray[argObj.i][1];
    checkCode(argObj);
}

function checkCode(argObj) {
    logIt(argObj, "Checking table: " + argObj.targetTable + " and script field: " + argObj.targetScriptField, 3);
    var grCode = new GlideRecord(argObj.targetTable);
    grCode.addNotNullQuery(argObj.targetScriptField);
    grCode.query();
    while (grCode.next()) {
        try {
            argObj.str = grCode.getValue(argObj.targetScriptField).replace(argObj.rplc, '');
            recursiveArrayBuilder(argObj, grCode.getValue('sys_id'));
        } catch (error) {
            logIt(argObj, "Unable to parse Sys ID: " + argObj.targetTable + ": " + grCode.getValue('sys_id') + " using regex: " + argObj.rplc + " ERROR: " + error, 1);
        }
    }
}

function recursiveArrayBuilder(argObj, sysID) {
    argObj.tmpArray = []; //blank out the array just in case.
    if (gs.nil(argObj.str)) {
        logIt(argObj, "masterArray length is: " + argObj.masterArray.length, 3);

        for (argObj.i = 0; argObj.i < argObj.masterArray.length; argObj.i++) {
            argObj.masterArray[argObj.i] = argObj.masterArray[argObj.i].replace(argObj.fixFors, 'for (');
            argObj.result = argObj.masterArray[argObj.i].match(argObj.varRegx);

            if (argObj.result) {
                argObj.result.forEach(function printing(element, index) {
                    gs.info(argObj.prefix + " Finding in script " + sysID + " in table: " + argObj.targetTable + " Element: "+ element);
                });
            }
        }
        return;
    }
    argObj.result = argObj.str.match(argObj.regex);
    if (argObj.result) {
        argObj.result.forEach(function printing(element, index) {
            logIt(argObj, "element is: " + element, 3);
            if ((/^(while\s*\(|for\s*\()/).test(element)) {
                //only push to array if it's while or for loop
                //we don't really need to check non-iterative loops
                //we will miss ones where the variable is declared inside of an iterative loop
                //but it is used in the if statement that is inside of the iterative loop
                //this would still be bad, but we will miss it.
                logIt(argObj, "after restricting based on loops, element is: " + element, 3);
                argObj.masterArray.push("Table: " + argObj.targetTable + " SysID: " + sysID + ": " + element);
            }
            argObj.str = argObj.str.replace(element, '');
        });
    } else {
        if (argObj.str.indexOf("{") >= 0) {
            logIt(argObj, "ERROR with Table, unable to fully parse script; unexpected curly braces are left over: " + argObj.targetTable + " SysID: " + sysID + " str is: " + argObj.str, 1);
        }
        return;
    }

    if (argObj.str.match(argObj.regex)) {
        recursiveArrayBuilder(argObj);
    } else {
        if (argObj.str.indexOf("{") >= 0) {
            logIt(argObj, "Remainder with Table; unable to fully parse script; unexpected curly braces are left over: " + argObj.targetTable + " SysID: " + sysID + " str is: " + argObj.str, 1);
        }
        return;
    }
    return;
}

function logIt(argObj, msg, thisLogLvl) {
    //logLvl is the global log setting. It's managed above at the top where the argObj object's properties are initialized.
    //thisLogLvl is used for each individual message. 1 is for errors and 2 is for errors and info. 3 is super verbose
    switch (true) {
        case (argObj.logLvl == 1 && thisLogLvl == 1):
            gs.error(argObj.prefix + msg);
            break;
        case (argObj.logLvl == 1):
            break;
        case (argObj.logLvl == 2 && thisLogLvl == 1):
            gs.error(argObj.prefix + msg);
            break;
        case (argObj.logLvl == 2 && thisLogLvl == 2):
            gs.info(argObj.prefix + msg);
            break;
        case (argObj.logLvl == 3 && thisLogLvl == 1):
            gs.error(argObj.prefix + msg);
            break;
        case (argObj.logLvl == 3):
            gs.info(argObj.prefix + msg);
            break;
        default:
    }
}

 Phew, that was a fix script I wrote. It still runs very fast on my OOTB instance (takes like 10 - 15 seconds without verbose logging on), but is kinda a mess, isn't it? So, while I was researching this, I did stumble upon Linter checks. Linter checks are part of Instance Scan. To create a new Linter Check, you go to Instance Scan -> Checks, click the New button, then from the interceptor, select "Create a new Linter Check."

Then, inside the linter check you can put this javascript:

(function(engine) {
    engine.rootNode.visit(function(node) {
        try {
            if ((node.getParent().getParent().getTypeName() == "WHILE" || node.getParent().getParent().getTypeName() == "FOR") && node.getParent().getTypeName() == "BLOCK" && node.getTypeName() == "VAR") {
                engine.finding.setCurrentSource(current.getValue('sys_id'));
                engine.finding.incrementWithNode(node);
            }
        } catch (error) {
			//this throws error when getParent() fails because there isn't a parent.
			//in standard linters, the top node's parent is itself, however
			//it seems here that the top node's parent is null
			//and when you try to getType on it, it errors out
			//This is to handle that scenario.
            gs.error("LINTER CHECK: "  + error);
        }

    });
})(engine);

That's much shorter, isn't it? So, that does basically the same thing that the previous very long script did, but inside of the Instance Scan application. It seems to work pretty well, too. It generates findings for each of the occurrences where a variable is declared inside of an iterating block. I'm sure a creative person could grow this even more.

The point of this wasn't about coding best practices, nor was it about garbage collection. The point of this was to give an example Linter type check, I just wanted to give you the background so you could see the use case of a check versus a fix script or schedule job.

Anyways, hope that helps! If it does, give it a reaction or comment so I know that you guys find value in this type of post and then I will be sure to post more!

Thanks!

Junior (Gary) Opela