Implicit-global rule flags globals read elsewhere

Rule javascript:S2703 (BLOCKER Code Smell) states:
“JavaScript variable scope can be particularly difficult to understand and get right. The situation gets even worse when you consider the accidental creation of global variables, which is what happens when you declare a variable inside a function or the for clause of a for-loop without using the let , const or var keywords.”

This is true, but sometimes a prior developer actually intends to set a global variable subsequently read elsewhere, making this a false positive. Completing the ‘fix’ of a local-scope declaration would actually break functionality.

Contrary to the “2min effort” nominally associated with this type of issue, actually fixing this might require hours of manually reviewing the thousands of occurrences of that token in the project to determine if any of them are reading from a globally-scoped variable with the same name, prior to either fixing the issue as suggested or closing it as a false positive. Efficiency & accuracy would be improved if Sonarqube could also scan the rest of the project to find all other uses of the same token in reference to a variable of global scope (e.g. not declared within a non-global scope of the read).

If there are any reads of the global other than in the same function/context where this issue is being flagged, Sonarqube should say where those are (they might also be bugs, especially if the same token is implicitly-global twice) and if there aren’t any, Sonarqube should say that too. Only when there’s confidence that the globally-scoped variable is never read is this a “2min effort.” Sonarqube could also likely determine whether a correct local declaration would be var (e.g. if read before declaration), let (e.g. if set after declaration), or const, and be more specific in its suggestion.

Sonarqube version: Version 8.9.5 (build 50698)

Hello Ben,

I understand your concern, but unfortunately it seems that the effort to provide what you are asking for is not justified. Using global variables is less and less common with EcmaScript modules available.

One thing which could theoretically help you is looking at the results of S3827 rule, it will should show you the usages (read) of variables which are not declared in the file.

Regards,
Elena

At the least, ‘2min effort’ should be reconsidered…for popular names it can be more like 2hrs. Globals are fortunately becoming less common, but making the suggested change requires at least first verifying that the target variable is not actually used as global in a legacy codebase (especially because Sonarqube has a bad habit of flagging issues in old code as issues in new code that MUST be resolved immediately).

I also agree with Ben, especially if creating an undeclared variable outside a function to be used in another module, it is unlikely the accidental creation is at play. In addition to the effort time, this rule should be divided into catching undeclared variables within functions or codeblocks verse outside these as it would require refactoring to using getter/setters or checking against export/import implementations for undeclared variables usage in other modules.